1553510 - _maybeToggleBookmarkToolbarVisibility initializes places early when there's no need

Status: RESOLVED FIXED

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

Description

[:Gijs (he/him)]

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.

Comment 1

[Alex Thayer [:alexical] (she/her)]

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?

Comment 2

[Florian Quèze [:florian]]

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'.

Comment 3

[Alex Thayer [:alexical] (she/her)]

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

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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?

Comment 5

[:Gijs (he/him)]

(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)

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1553510 - Don't compute whether or not to show the Bookmarks Toolbar for new profiles until Places has finished initting. r?MattN

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.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d08b80220a5f377ab11c649a873f99d69e2a186b

Comment 9

[Mike Conley (:mconley) (:⚙️)]

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

Comment 10

[Mike Kaply [:mkaply]]

Answered in the phab request.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/df2ba9ebafc9