Closed Bug 1553510 Opened 1 year ago Closed 8 months ago

_maybeToggleBookmarkToolbarVisibility initializes places early when there's no need

Categories

(Firefox :: Bookmarks & History, defect, P3)

68 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 72
Tracking Status
firefox72 --- fixed

People

(Reporter: Gijs, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p2])

Attachments

(1 file)

https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/browser/components/BrowserGlue.jsm#2297

This gets run on a clean profile only, but unless migration ran, enterprise policies ran, or the user copied files into the dir, we shouldn't have any bookmarks anyway. Profile: https://perfht.ml/2X7VIsA

We could maybe use Cm.isServiceInstantiatedByContractID to check if the bookmarks service has been created, and if not, delay doing this work until an idle task after the window has been shown. Given that this only affects new profiles, it seems more important to bring the window up ASAP.

Whiteboard: [fxperf]
Blocks: 1546460

Does this just kick the can down the road, or do we know this is all it would take to delay initializing the bookmarks service until an arbitrarily later time?

We have test coverage that ensures the bookmarks service isn't started before handling user events: https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/browser/base/content/test/performance/browser_startup.js#100

At https://searchfox.org/mozilla-central/rev/f8b11433159cbc9cc80500b3e579d767473fa539/browser/base/content/test/performance/browser.ini#8-10 a pref is set so that we skip the migration code path when running the browser_startup.js test.

(In reply to Doug Thayer [:dthayer] from comment #1)

do we know this is all it would take to delay initializing the bookmarks service until an arbitrarily later time?

Per the above two points, I'm fairly confident the answer is 'yes'.

[fxperf:p2] then, as it could have a substantial impact on first profile startup for not a ton of work.

Whiteboard: [fxperf] → [fxperf:p2]

I don't recall if this was also for the Profile Reset case, where we will copy over places.sqlite, and we want to show the toolbar. Do we restore toolbar status on Profile Reset?

Priority: -- → P3

(In reply to Marco Bonardo [::mak] from comment #4)

I don't recall if this was also for the Profile Reset case, where we will copy over places.sqlite, and we want to show the toolbar. Do we restore toolbar status on Profile Reset?

We don't, but it seems OK to do so lazily in that one case (or, if we're really concerned about that, to add code to check for this as part of the migration inside fx profile reset)

(In reply to :Gijs (he/him) from comment #0)

We could maybe use Cm.isServiceInstantiatedByContractID to check if the bookmarks service has been created, and if not, delay doing this work until an idle task after the window has been shown. Given that this only affects new profiles, it seems more important to bring the window up ASAP.

If we're willing to do the work of updating the visibility of the bookmarks toolbar after the UI has painted in the common case, maybe we're willing to do it even if the bookmarks service has been created.

So maybe we can just unilaterally delay _maybeToggleBookmarkToolbarVisibility from running until, say, Places has already initted.

Assignee: nobody → mconley

This causes BrowserGlue to wait until Places has notified that it's initted before checking
to see whether or not the Bookmarks Toolbar should be shown.

Also, if it turns out that the Bookmarks Toolbar is shown, we now use CustomizableUI to do
this, which means that the Bookmarks Toolbar will be made visible on all windows after the
check is run - not just new windows.

ni?ing mkaply for MattN's question in https://phabricator.services.mozilla.com/D51701#1569604

Flags: needinfo?(mozilla)

Answered in the phab request.

Flags: needinfo?(mozilla)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df2ba9ebafc9
Don't compute whether or not to show the Bookmarks Toolbar for new profiles until Places has finished initting. r=MattN
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72
Regressions: 1601014
You need to log in before you can comment on or make changes to this bug.