AsyncPrefs.jsm should be imported lazily from nsBrowserGlue.js

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-performance])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Posted patch PatchSplinter Review
Attachment #8885863 - Flags: review?(mconley)
(Assignee)

Updated

2 years ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Comment on attachment 8885863 [details] [diff] [review]
Patch

Also flagging snorp (per rnewman's suggestion in #mobile) for the mobile/ changes.
Attachment #8885863 - Flags: review?(snorp)
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.
Attachment #8885863 - Flags: review?(snorp) → review+
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?
Attachment #8885863 - Flags: review?(mconley) → review+

Comment 5

2 years ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25e361f978f5
AsyncPrefs.jsm should be imported lazily from nsBrowserGlue.js, r=mconley,snorp.
https://hg.mozilla.org/mozilla-central/rev/25e361f978f5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Hi Florian, should I add bug 1369460 to the Performance MVP?
Flags: needinfo?(florian)
(Assignee)

Comment 8

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

Yes
Flags: needinfo?(florian) → qe-verify-
Whiteboard: [photon-performance]
Iteration: --- → 56.3 - Jul 24
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.