UrlbarProviderSearchTips.jsm triggers main thread I/O to updates.xml to fetch the updateCount
Categories
(Firefox :: Address Bar, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox74 | --- | disabled |
firefox75 | --- | fix-optional |
firefox76 | --- | fix-optional |
People
(Reporter: florian, Assigned: daisuke, NeedInfo)
References
(Regression)
Details
(Keywords: perf, regression, Whiteboard: [fxperf:p3][snt-scrubbed][search-performance])
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
The lastBrowserUpdateDate
function in UrlbarProviderSearchTips.jsm is async so it looks like it was meant to not block the main thread, but calling updateManager.updateCount at https://searchfox.org/mozilla-central/rev/13b081a62d3f3e3e3120f95564529257b0bf451c/browser/components/urlbar/UrlbarProviderSearchTips.jsm#464 triggers main thread I/O to updates.xml. See this profile: https://perfht.ml/2vIUtI3
This code is from bug 1606909 and just got enabled in bug 1617029.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Updated•4 years ago
|
Comment 1•2 years ago
|
||
It'd be good to first investigate why we did await lastBrowserUpdateDate()
in the first place, and then decide if we can still keep showing a search tip if the _updatesCache
is null
/ unavailable.
Updated•2 years ago
|
Comment 2•1 year ago
|
||
Reading the discussion in Bug 1595328, the purpose of gathering when a user last updated was to avoid inundating user with help messages when they restarted their browser, per Verdi's comment:
The intent is to not annoy people or add to all of the other things we're asking people to look at, do, or consider.
It is either 24 hours since the browser last applied the update (the browser might not have been restarted) or when the user profile was first used (if not available, then its the created date).
We allow the enabling of the search tips provider if at least one of the search tips hasn't reached its maximum threshold of views. And checking if search tips should appear on a page occurs via onLocationChange, which could be when a user first opens the browser and views a New Tab.
I wonder if one compromise we can make is to query the update service if the updates are cached (e.g. a new method called hasUpdatesCached
, and if it doesn't, we assume we shouldn't show a search tip, but in a callback passed to idleDispatchToMainThread
we'll call getUpdateCount
, and then if search tips provider is queried again (via another onLocationEvent), the cache might be updated.
I think the main drawback of that approach is a user will likely not see the Onboarding Search Tip without having triggered onLocationChange
, so they probably won't see it immediately if they re-open the browser.
Thoughts :adw? This feels more like it needs UX input and should be taken out of the dragon slayer month.
Assignee | ||
Comment 3•1 year ago
|
||
I also tried to think about this.
If calling UpdatePing.handleUpdateSuccess() in BrowserContentHandler means "this session was started with a new update", I wonder if we can add something flag to UpdateManager
etc here? In UrlbarProviderSearchTips
, if the flag is true, refer to Services.startup.getStartupInfo().start
, if this does not pass 24 hours, will not show the tips. Then, it seems that we can avoid accessing update.xml
and file access via TelemetryArchive.promiseArchivedPingList()
. There may be issues, so I'll try to make a prototype once.
Assignee | ||
Comment 4•1 year ago
|
||
Updated•1 year ago
|
Description
•