perf(qml): Drop the file selectors#20696
Open
alexjba wants to merge 1 commit intoperf/async-messengerfrom
Open
perf(qml): Drop the file selectors#20696alexjba wants to merge 1 commit intoperf/async-messengerfrom
alexjba wants to merge 1 commit intoperf/async-messengerfrom
Conversation
Member
Jenkins BuildsClick to see older builds (9)
|
There was a problem hiding this comment.
Pull request overview
This PR removes QML file selector usage and switches the Browser WebView adapter selection to an explicit Loader.setSource approach, aiming to reduce QML compilation/startup CPU overhead (especially on mobile warm start).
Changes:
- Replace file-selector-based Browser adapter overrides with runtime adapter loading via
Loader.setSource. - Add a mobile-specific
MobileWebViewAdapterbacked byStatusQ.CustomWebViewand expose it in the adapters module. - Remove the
+mobileand+noWebEngineselector directories/usages, including Storybook and Nim-side selector configuration.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/app/AppLayouts/Browser/adapters/qmldir | Exports MobileWebViewAdapter from the adapters module. |
| ui/app/AppLayouts/Browser/adapters/ProfileManager.qml | Drops static QtWebEngine import; adds mobile guards and uses numeric enum values for script injection configuration. |
| ui/app/AppLayouts/Browser/adapters/MobileWebViewAdapter.qml | New mobile adapter using MobileWebViewBackend with history/find/snapshot and web actions plumbing. |
| ui/app/AppLayouts/Browser/adapters/LazyWebViewAdapter.qml | Switches from selector-resolved sourceComponent to runtime setSource with per-platform adapter path and bound properties. |
| ui/app/AppLayouts/Browser/adapters/+mobile/qmldir | Removed (no longer using file selectors). |
| ui/app/AppLayouts/Browser/adapters/+mobile/ProfileManager.qml | Removed (mobile behavior moved into shared ProfileManager.qml). |
| ui/app/AppLayouts/Browser/+noWebEngine/BrowserLayout.qml | Removed (no longer using file selectors). |
| storybook/qmlfilesserver_main.cpp | Removes Storybook file-selector setup (+noWebEngine). |
| src/nim_status_client.nim | Removes Nim-side QT_FILE_SELECTORS manipulation for mobile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
22
to
26
| name: name, | ||
| sourceUrl: path, | ||
| injectionPoint: WebEngineScript.DocumentCreation, | ||
| worldId: WebEngineScript.MainWorld, | ||
| injectionPoint: 2, //WebEngineScript.DocumentCreation | ||
| worldId: 0, //WebEngineScript.MainWorld | ||
| runsOnSubFrames: runOnSubFrames |
There was a problem hiding this comment.
Hard-coding injectionPoint: 2 and worldId: 0 is brittle and obscures intent (these are QtWebEngine enum values). Consider defining named constants in this file (with a reference to the QtWebEngine docs), or retrieving the enum values via a small Qt.createQmlObject helper so the mapping stays correct across Qt upgrades.
alexjba
force-pushed
the
perf/drop-file-selectors
branch
from
fe8ffea to
16bc15b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does the PR do
Dropping the qml file selectors (even the default ones)
Needs status-im/dotherside#108
The QML file selectors are on top of the CPU offenders on start-up, on the qml compilation time. Most of the work done by the file selectors is on the qml thread, but it flows into the main thread as well.
By removing the qml file selectors I'm getting more than 3 sec CPU drop on the QQmlThread warm-start-up. This will also speed-up the main thread (probably less waits). The qml compilation time is reduced and assets are loaded faster.
The Browser was the only component still using the file selectors. I've replaced the file selectors with
Loader.setSourceAffected areas
QML engine configuration
Browser
How to test
run
make mobile-profile(make sure you have a device connected)After app start -> attach the qml profiler to allow the app to start -> login -> close the app -> wait for the profiler tcp connection to close
Start the app again -> attach android studio CPU profiler -> attach the qml profiler
Inspect the CPU time.
Risk
Medium for browser