1369460 - AsyncPrefs.jsm should be imported lazily from nsBrowserGlue.js

Status: RESOLVED FIXED

(Toolkit :: General, enhancement, P1)

Description

[Florian Quèze [:florian]]

Like bug 1358921 did for lots of modules imported into nsBrowserGlue.js, AsyncPrefs.jsm can be imported lazily the first time a relevant message is received.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8885863 [details] [diff] [review]
Patch

Also flagging snorp (per rnewman's suggestion in #mobile) for the mobile/ changes.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8885863 [details] [diff] [review]
Patch

Review of attachment 8885863 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/components/geckoview/GeckoViewPrompt.js
@@ +41,5 @@
>        }
>        case "profile-after-change": {
>          // ContentPrefServiceParent is needed for e10s file picker.
>          ContentPrefServiceParent.init();
> +        // AsyncPrefs is needed for reader mode.

Is reader mode the only thing that uses this? I would prefer to only initialize it once reader mode needs it. Putting it here means it's in the critical path for startup, but I guess it was in browser.js anyway.

Comment 4

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

Comment on attachment 8885863 [details] [diff] [review]
Patch

Review of attachment 8885863 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: browser/base/content/test/performance/browser_startup.js
@@ +103,5 @@
>    // and loaded lazily when used for the first time by the user should
>    // be blacklisted here.
>    "before becoming idle": {blacklist: {
>      modules: new Set([
> +      "resource://gre/modules/AsyncPrefs.jsm",

Hooray!

::: browser/components/nsBrowserGlue.js
@@ -141,5 @@
>      // PLEASE KEEP THIS LIST IN SYNC WITH THE LISTENERS ADDED IN ContentPrefServiceParent.init
>      "ContentPrefs:FunctionCall": ["ContentPrefServiceParent"],
>      "ContentPrefs:AddObserverForName": ["ContentPrefServiceParent"],
>      "ContentPrefs:RemoveObserverForName": ["ContentPrefServiceParent"],
> -    // PLEASE KEEP THIS LIST IN SYNC WITH THE LISTENERS ADDED IN ContentPrefServiceParent.init

A warning was placed above and below this block. Perhaps we should keep (and follow) that tactic to avoid screwing this ordering up down the line?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25e361f978f5
AsyncPrefs.jsm should be imported lazily from nsBrowserGlue.js, r=mconley,snorp.

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/25e361f978f5

Comment 7

[Marco Mucci [:MarcoM]]

Hi Florian, should I add bug 1369460 to the Performance MVP?

Comment 8

[Florian Quèze [:florian]]

(In reply to Marco Mucci [:MarcoM] from comment #7)
> Hi Florian, should I add bug 1369460 to the Performance MVP?

Yes