1357517 - Stop loading Preferences.jsm before first paint

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Florian Quèze [:florian]]

With bug 1338306 and bug 1345294 fixed, most of the reasons for using Preferences.jsm are now gone, and using this module adds some overhead, so we should consider removing it.

If there's no simple way to do it for the whole tree, we should start with removing it from the startup path, and removing it from content processes.

Comment 1

[:Gijs (out for now; he/him)]

Florian, with your scripts is it easy to find files that both:

- import Preferences.jsm in some way
- only use Preferences.get / .set

and blanket-patch those usecases to use Services.prefs.get/set*Pref(..., (...)) instead? That feels like it'll be 99% of the work here.

Comment 2

[Florian Quèze [:florian]]

(In reply to :Gijs from comment #1)
> Florian, with your scripts is it easy to find files that both:
> 
> - import Preferences.jsm in some way
> - only use Preferences.get / .set
> 
> and blanket-patch those usecases to use Services.prefs.get/set*Pref(...,
> (...)) instead? That feels like it'll be 99% of the work here.

I don't think it would be easy to script unfortunately. Issues I observed:
- some files do use observers, or reset, or has
- to convert get/set, we need to guess the type of the pref, and that requires guessing the type from the provided default value. I'm not sure that value always has the right type.

We may be able to use a script to prepare the work, but I think the output will need careful review.

Comment 3

[Marco Castelluccio [:marco]]

I have a dirty WIP patch that would move Preferences.jsm loading to be "before handling user events". I'll try to clean it a bit and post it for review.

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from some dom/push files

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from browser_bug1047663.js

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from XPCOMUtils.jsm

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8891486 [details] [diff] [review]
Remove Preferences.jsm usage from browser_bug1047663.js

>+  let rcwnEnabled = Services.prefs.getBoolPref("network.http.rcwn.enabled");

setBoolPref, no?  Was this tested?

Comment 8

[Marco Castelluccio [:marco]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 8891486 [details] [diff] [review]
> Remove Preferences.jsm usage from browser_bug1047663.js
> 
> >+  let rcwnEnabled = Services.prefs.getBoolPref("network.http.rcwn.enabled");
> 
> setBoolPref, no?  Was this tested?

Ooops. Yes, it was even green on try :S

Comment 9

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from browser_bug1047663.js

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8891486 [details] [diff] [review]
Remove Preferences.jsm usage from browser_bug1047663.js

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

::: dom/workers/test/browser_bug1047663.js
@@ +15,2 @@
>    registerCleanupFunction(()=>{
> +    Services.prefs.getBoolPref("network.http.rcwn.enabled", rcwnEnabled);

Can you please use `SpecialPowers.pushPrefEnv` for this?

Comment 11

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from LaterRun.jsm

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8891491 [details] [diff] [review]
Remove Preferences.jsm usage from browser_bug1047663.js

>+    Services.prefs.getBoolPref("network.http.rcwn.enabled", rcwnEnabled);

This part is still wrong....

Comment 13

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm import from password manager

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from browser_bug1047663.js

Used SpecialPowers.pushPrefEnv.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8891496 [details] [diff] [review]
Remove Preferences.jsm usage from browser_bug1047663.js

Yes, this is much better!

Comment 16

[Lina Butler [:lina]]

Comment on attachment 8891485 [details] [diff] [review]
Remove Preferences.jsm usage from some dom/push files

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

Thanks! Up to you if you want to pass a default argument, or leave this as is and have it throw.

::: dom/push/PushRecord.jsm
@@ +48,5 @@
>  PushRecord.prototype = {
>    setQuota(suggestedQuota) {
>      if (this.quotaApplies()) {
>        let quota = +suggestedQuota;
> +      this.quota = quota >= 0 ? quota : prefs.getIntPref("maxQuotaPerSubscription");

Should we maybe provide a default value as the second argument to `getIntPref`, just in case the pref isn't set or set to the wrong type?

Comment 17

[Lina Butler [:lina]]

Comment on attachment 8891485 [details] [diff] [review]
Remove Preferences.jsm usage from some dom/push files

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

::: dom/push/PushServiceHttp2.jsm
@@ +430,5 @@
>    },
>  
>    validServerURI: function(serverURI) {
>      if (serverURI.scheme == "http") {
> +      return !!prefs.getBoolPref("testing.allowInsecureServerURL");

This definitely needs to be `getBoolPref("testing.allowInsecureServerURL", false)`, though, since there's no default value for this pref in `all.js`.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b3cced9169
Remove Preferences.jsm usage from workers test. r=bz

Comment 19

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from addoncompat/Prefetcher.jsm

Comment 20

[Marco Castelluccio [:marco]]

Attachment: Avoid leaking Preferences property in viewsource test

Comment 21

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from some toolkit modules

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8891530 [details] [diff] [review]
Remove Preferences.jsm usage from some toolkit modules

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

::: toolkit/modules/Locale.jsm
@@ -3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  this.EXPORTED_SYMBOLS = ["Locale"];
>  
> -const { classes: Cc, interfaces: Ci, utils: Cu } = Components;

I would prefer you didn't change this. At least the front-end prefers to have the common 3/4 aliases in every file once we need one.

::: toolkit/modules/tests/xpcshell/test_client_id.js
@@ +83,5 @@
>  
>    // Assure that cached IDs are being checked for validity.
>    for (let invalidID of invalidIDs) {
>      await ClientID._reset();
> +    Services.prefs.setStringPref(PREF_CACHED_CLIENTID, invalidID);

Hmm… this is changing what the test is testing. It was previously sometimes going through setBoolPref or setIntPref for some of the invalidIDs but now you're always forcing this through setStringPref. Maybe you could have the invalidIDs array also contain tuples of values and pref method names to use. e.g.

const invalidIDs = [
  [-1, "setIntPref"],
…
  ["INVALID-UUID", "setStringPref"],
  [true, "setBoolPref"],
…

Comment 23

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from some toolkit modules

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 8891541 [details] [diff] [review]
Remove Preferences.jsm usage from some toolkit modules

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

Thanks

::: toolkit/modules/tests/xpcshell/test_client_id.js
@@ +95,4 @@
>      let cachedID = ClientID.getCachedClientID();
>      Assert.strictEqual(cachedID, null, "ClientID should ignore invalid cached IDs");
> +    Assert.ok(!Services.prefs.prefHasUserValue(PREF_CACHED_CLIENTID), "ClientID should reset invalid cached IDs");
> +    Assert.ok(Services.prefs.getPrefType(PREF_CACHED_CLIENTID) == Ci.nsIPrefBranch.PREF_INVALID, "ClientID should reset invalid cached IDs");

You should probably wrap between some arguments as I think these are over 80 or 100

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/302da7be5b9a
Remove Preferences.jsm usage from LaterRun.jsm. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/19e6fdd4a271
Remove Preferences.jsm import from password manager. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/11360bb6d7a1
Remove Preferences.jsm usage from addoncompat component. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d62cdcccad4
Avoid leaking Preferences property in viewsource test. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/68c4c9c19f3c
Remove Preferences.jsm usage from some dom/push/* files. r=kitcambridge
https://hg.mozilla.org/integration/mozilla-inbound/rev/65105a4351d1
Remove Preferences.jsm usage from XPCOMUtils.jsm. r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/e05ac296ea14
Remove Preferences.jsm usage from some toolkit modules. r=MattN

Comment 26

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from toolkit/mozapps/extensions/

Comment 27

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from GfxSanityTest.js

Comment 28

[Marco Castelluccio [:marco]]

Attachment: Remove or delay Preferences.jsm usage from some browser/base/content/* files

Comment 29

[Marco Castelluccio [:marco]]

Attachment: Remove or delay Preferences.jsm usage from some browser/components/* files

Comment 30

[Marco Castelluccio [:marco]]

Attachment: Remove or delay Preferences.jsm usage from Marionette

This is needed in order to add Preferences.jsm to the browser_startup.js test, otherwise Marionette loads it really early.

Comment 31

[Andreas Tolfsen ❲:ato❳]

The reason we have preferred Preferences.jsm in the remote
protocol—and this may be besides the point of this bug—is that
nsIPrefBranch, with its getBoolPref/getCharPref/getIntPref et al.,
appears needlessly verbose next to Preferences.{get,set} which can
deduce a preference’s type.

If you consider
https://bugzilla.mozilla.org/attachment.cgi?id=8891573&action=diff,
the type inference adds code duplication that could be avoided if we
were not using the C++/XPCOM interface directly.

Comment 32

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8891573 [details] [diff] [review]
Remove or delay Preferences.jsm usage from Marionette

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

::: testing/marionette/components/marionette.js
@@ +71,5 @@
>  
> +function getPrefVal(pref) {
> +  let prefType = Services.prefs.getPrefType(pref);
> +  let prefValue = undefined;
> +  if (prefType == Ci.nsIPrefBranch.PREF_STRING) {

To make this easier on the eye, you could make shorthands for PREF_STRING, PREF_BOOL et al.:

> const {PREF_STRING, PREF_BOOL, PREF_INT, PREF_INVALID} = Ci.nsIPrefBranch;

@@ +78,5 @@
> +    prefValue = Services.prefs.getBoolPref(pref);
> +  } else if (prefType == Ci.nsIPrefBranch.PREF_INT) {
> +    prefValue = Services.prefs.getIntPref(pref);
> +  } else if (prefType == Ci.nsIPrefBranch.PREF_INVALID) {
> +    prefValue = undefined;

I would prefer the use of a switch statement to reduce the amount of code here.

@@ +80,5 @@
> +    prefValue = Services.prefs.getIntPref(pref);
> +  } else if (prefType == Ci.nsIPrefBranch.PREF_INVALID) {
> +    prefValue = undefined;
> +  } else {
> +    throw new Error("Unexpected preference type (" + prefType + ") for " + pref + ".");

Use TypeError and a template string.  Also drop the final stop at the end.

Comment 33

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from some Telemetry files

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0b3cced9169
https://hg.mozilla.org/mozilla-central/rev/302da7be5b9a
https://hg.mozilla.org/mozilla-central/rev/19e6fdd4a271
https://hg.mozilla.org/mozilla-central/rev/11360bb6d7a1
https://hg.mozilla.org/mozilla-central/rev/8d62cdcccad4
https://hg.mozilla.org/mozilla-central/rev/68c4c9c19f3c
https://hg.mozilla.org/mozilla-central/rev/65105a4351d1
https://hg.mozilla.org/mozilla-central/rev/e05ac296ea14

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3362528e5e25
Remove or delay Preferences.jsm usage from Marionette. r=ato

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6025b5b43ec6
Fix max-len eslint rule breakage in marionette.js. r=me

Comment 37

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from some Telemetry files

Patch updated to also remove Preferences.jsm from the newly introduced UpdatePing module.

Comment 38

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

https://hg.mozilla.org/mozilla-central/rev/3362528e5e25
https://hg.mozilla.org/mozilla-central/rev/6025b5b43ec6

Comment 39

[:Gijs (out for now; he/him)]

Comment on attachment 8891571 [details] [diff] [review]
Remove or delay Preferences.jsm usage from some browser/base/content/* files

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

Stealing. :-)

r=me with the below fixed.

::: browser/base/content/abouthealthreport/abouthealth.js
@@ +19,4 @@
>      let iframe = document.getElementById("remote-report");
>      iframe.addEventListener("load", healthReportWrapper.initRemotePage);
>      iframe.src = this._getReportURI().spec;
> +    this.prefs.observe("uploadEnabled", this.updatePrefState, healthReportWrapper);

I think you can remove this, and do:

XPCOMUtils.defineLazyPreferenceGetter(this, /* unused */ "_isUploadEnabled", "datareporting.healthreport.uploadEnabled", false, () => this.updatePrefState());

and remove uninit() and callsites.

::: browser/base/content/browser-captivePortal.js
@@ +11,5 @@
>     * This constant is chosen to be large enough for a portal recheck to complete,
>     * and small enough that the delay in opening a tab isn't too noticeable.
>     * Please see comments for _delayedCaptivePortalDetected for more details.
>     */
> +  PORTAL_RECHECK_DELAY_MS: Services.prefs.getIntPref("captivedetect.portalRecheckDelayMS", 500),

Could use XPCOMUtils.defineLazyPreferenceGetter() to avoid the early read.

::: browser/base/content/sanitize.js
@@ +812,4 @@
>      // Reset the pref, so that if we crash before having a chance to
>      // sanitize on shutdown, we will do at the next startup.
>      // Flushing prefs has a cost, so do this only if necessary.
> +    Services.prefs.clearUserPref(Sanitizer.PREF_SANITIZE_DID_SHUTDOWN);

Given this code, the if() might as well just check .prefHasUserValue(), which though not strictly equivalent to Preferences.has() is more readable, and should work here (no point resetting to defaults if there is only a default value).

::: browser/extensions/shield-recipe-client/test/browser/browser_ClientEnvironment.js
@@ -1,4 @@
>  "use strict";
>  
> -Cu.import("resource://gre/modules/Services.jsm");
> -Cu.import("resource://gre/modules/Preferences.jsm");

The normandy / shield-recipe-client stuff will need upstreaming into the relevant github repo.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a2f780f966
Remove Preferences.jsm usage from some Telemetry files. r=Dexter

Comment 41

[:Gijs (out for now; he/him)]

Comment on attachment 8891572 [details] [diff] [review]
Remove or delay Preferences.jsm usage from some browser/components/* files

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

::: browser/components/distribution.js
@@ +14,5 @@
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
> +                                  "resource://gre/modules/Preferences.jsm");

Could just rewrite the actual uses here to use the prefbranch directly?

::: browser/components/extensions/test/browser/head.js
@@ +33,5 @@
>  PromiseTestUtils.whitelistRejectionsGlobally(/Receiving end does not exist/);
>  
>  const {AppConstants} = Cu.import("resource://gre/modules/AppConstants.jsm", {});
>  const {CustomizableUI} = Cu.import("resource:///modules/CustomizableUI.jsm", {});
> +const {Preferences} = Cu.import("resource://gre/modules/Preferences.jsm", {});

Why is this necessary?

::: browser/components/newtab/NewTabPrefsProvider.jsm
@@ +79,5 @@
>      for (let pref of gNewtabPagePrefs) {
> +      results[pref] = null;
> +
> +      if (Services.prefs.getPrefType(pref) != Ci.nsIPrefBranch.PREF_INVALID &&
> +          gPrefsMap.has(pref)) {

Why the check for gPrefsMap.has() ? Both of those lists look constant to me, and all the items in gNewtabPagePrefs are in gPrefsMap, right?

Comment 42

[Marco Castelluccio [:marco]]

(In reply to :Gijs from comment #41)
> Comment on attachment 8891572 [details] [diff] [review]
> Remove or delay Preferences.jsm usage from some browser/components/* files
> 
> Review of attachment 8891572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/distribution.js
> @@ +14,5 @@
> >  
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >  Cu.import("resource://gre/modules/Services.jsm");
> > +XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
> > +                                  "resource://gre/modules/Preferences.jsm");
> 
> Could just rewrite the actual uses here to use the prefbranch directly?

Do we know the types of the preferences here? Or do we need to implement some kind of type inference like Preferences.jsm does in Preferences.set?

> 
> ::: browser/components/extensions/test/browser/head.js
> @@ +33,5 @@
> >  PromiseTestUtils.whitelistRejectionsGlobally(/Receiving end does not exist/);
> >  
> >  const {AppConstants} = Cu.import("resource://gre/modules/AppConstants.jsm", {});
> >  const {CustomizableUI} = Cu.import("resource:///modules/CustomizableUI.jsm", {});
> > +const {Preferences} = Cu.import("resource://gre/modules/Preferences.jsm", {});
> 
> Why is this necessary?

The file is using Preferences.jsm but was not importing it, assuming Preferences would be available as a global.

> 
> ::: browser/components/newtab/NewTabPrefsProvider.jsm
> @@ +79,5 @@
> >      for (let pref of gNewtabPagePrefs) {
> > +      results[pref] = null;
> > +
> > +      if (Services.prefs.getPrefType(pref) != Ci.nsIPrefBranch.PREF_INVALID &&
> > +          gPrefsMap.has(pref)) {
> 
> Why the check for gPrefsMap.has() ? Both of those lists look constant to me,
> and all the items in gNewtabPagePrefs are in gPrefsMap, right?

Right, I can remove this check, assuming this will always be true.

Comment 43

[:Gijs (out for now; he/him)]

(In reply to Marco Castelluccio [:marco] from comment #42)
> (In reply to :Gijs from comment #41)
> > Comment on attachment 8891572 [details] [diff] [review]
> > Remove or delay Preferences.jsm usage from some browser/components/* files
> > 
> > Review of attachment 8891572 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/components/distribution.js
> > @@ +14,5 @@
> > >  
> > >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > >  Cu.import("resource://gre/modules/Services.jsm");
> > > +XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
> > > +                                  "resource://gre/modules/Preferences.jsm");
> > 
> > Could just rewrite the actual uses here to use the prefbranch directly?
> 
> Do we know the types of the preferences here? Or do we need to implement
> some kind of type inference like Preferences.jsm does in Preferences.set?

You would need the type inference, so perhaps that means it's not worth it...

Comment 44

[Marco Castelluccio [:marco]]

Attachment: Remove or delay Preferences.jsm usage from some browser/base/content/* files

Carrying r+.
There are also a couple of additional tests where I removed Preferences.jsm, but they are really simple fixes. Feel free to double check.

Comment 45

[Marco Castelluccio [:marco]]

Attachment: Remove or delay Preferences.jsm usage from some browser/components/* files

Fixed what you asked in comment 41 (except distribution.js) and also removed Preferences.jsm usage from Experiments.jsm.

Comment 46

[:Gijs (out for now; he/him)]

Comment on attachment 8891968 [details] [diff] [review]
Remove or delay Preferences.jsm usage from some browser/components/* files

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

::: browser/experiments/Experiments.jsm
@@ +402,3 @@
>      this._log.trace("enabled=" + gExperimentsEnabled + ", " + this.enabled);
>  
> +    Services.prefs.addObserver(PREF_BRANCH + PREF_LOGGING, configureLogging);

This used to be a weak observer via the indirection of Preferences.jsm, and now isn't. It looks like it's a singleton so I imagine it doesn't matter, but I wanted to note it in case there are leaks when you push this.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0029dcf7b3e
TelemetrySend crash annotations are controlled by TelemetryEnabled pref on Android, FhrUploadEnabled on other platforms. r=me

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10de190a839b
Remove or delay Preferences.jsm usage from some browser/base/content/* files. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f64ae9515a8
Remove or delay Preferences.jsm usage from some browser/components/* files. r=Gijs

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9079c02563e7
Remove unneeded ternary operators to fix eslint failure. r=me

Comment 50

[Marco Castelluccio [:marco]]

Attachment: Remove Preferences.jsm usage from some browser/extensions/ modules

Comment 51

[Marco Castelluccio [:marco]]

Attachment: Add Preferences.jsm to browser_startup.js blacklist until "before handling user events" phase

Once all the patches above land, Preferences.jsm is loaded in the "before handling user events" phase, so we can add it to the "before first paint" blacklist.

I'll morph this bug into getting Preferences.jsm out of the startup path, then I'll open a new bug to move it further away (maybe in "before becoming idle") and another bug to consider deprecating the module.

Comment 52

[:Gijs (out for now; he/him)]

Comment on attachment 8892078 [details] [diff] [review]
Add Preferences.jsm to browser_startup.js blacklist until "before handling user events" phase

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

rs=me

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45821738caeb
Disable pref observer TelemetrySend test on platforms where the crash reporter component is not available (like asan). r=me

Comment 54

[:Gijs (out for now; he/him)]

Comment on attachment 8892075 [details] [diff] [review]
Remove Preferences.jsm usage from some browser/extensions/ modules

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

::: browser/extensions/e10srollout/bootstrap.js
@@ +231,5 @@
>    }
>  
> +  let value = 0.0;
> +  if (prefType == Ci.nsIPrefBranch.PREF_INT) {
> +    value = Services.prefs.getIntPref(pref) / 100;

Nit: please keep the comment ("convert old integer value")

::: browser/extensions/shield-recipe-client/lib/PreferenceExperiments.jsm
@@ +241,5 @@
>        lastSeen: new Date().toJSON(),
>        preferenceName,
>        preferenceValue,
>        preferenceType,
> +      previousPreferenceValue: getPref(preferences, preferenceName, preferenceType, undefined),

Because there's an arg defined it'll get the value undefined without you explicitly passing it in here, so I would just omit it here.

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa81cd2510db
Remove Preferences.jsm usage from graphics sanity test. r=mchang

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68b1b2f00e3f
Remove Preferences.jsm usage from some browser/extensions/ modules. r=Gijs

Comment 57

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b1a2f780f966
https://hg.mozilla.org/mozilla-central/rev/e0029dcf7b3e
https://hg.mozilla.org/mozilla-central/rev/10de190a839b
https://hg.mozilla.org/mozilla-central/rev/0f64ae9515a8
https://hg.mozilla.org/mozilla-central/rev/9079c02563e7

Comment 58

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

https://hg.mozilla.org/mozilla-central/rev/45821738caeb
https://hg.mozilla.org/mozilla-central/rev/fa81cd2510db
https://hg.mozilla.org/mozilla-central/rev/68b1b2f00e3f

Comment 59

[[github robot]]

Commits pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/f30b605a0cf733e3024de29f534a4ca0462eb653
chore(bootstrap): Backport Preferences -> Services.prefs change from bug 1357517

https://github.com/mozilla/activity-stream/commit/8d800d6172eaba7e9075dc314e7f127a4ff1ba75
Merge pull request #3067 from Mardak/gh3065-backport

chore(bootstrap): Backport Preferences -> Services.prefs change from bug 1357517

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33788fc85648
Add Preferences.jsm to browser_startup.js blacklist until "before handling user events" phase. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/984c45066b41
Remove Preferences.jsm usage from some toolkit/mozapps/* files. r=rhelmer

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/33788fc85648
https://hg.mozilla.org/mozilla-central/rev/984c45066b41

Comment 62

[[github robot]]

Commits pushed to master at https://github.com/mozilla/normandy

https://github.com/mozilla/normandy/commit/912bcaed88db508eb85d3845051a971b4b450792
Bug 1357517 - Remove Preferences.jsm usage from some modules. r=Gijs

https://github.com/mozilla/normandy/commit/ff1b680c19f03653a1c758b724f69541d3822b10
Merge pull request #971 from mythmon/backport-1357517

Bug 1357517 - Remove Preferences.jsm usage from some modules.

Comment 63

[:Felipe Gomes (needinfo for replies!)]

Hmm I just noticed, while testing the clicktoplay-rollout addon, that there's a difference in behavior in undefined values. While it's all in JS, not passing an argument, or passing `undefined`, is the same. But it looks like in the XPCOM boundary, there's a difference in an explicit or implicit `undefined`.

Preferences.get("foo", undefined)       -> returns undefined
Preferences.get("foo")                  -> returns undefined

Services.prefs.getStringPref("foo", undefined) -> returns null
Services.prefs.getBoolPref("foo", undefined)   -> returns false
Services.prefs.getIntPref("foo", undefined)    -> returns 0
Services.prefs.getStringPref("foo")            -> *throws*
Services.prefs.getBoolPref("foo")              -> *throws*
Services.prefs.getIntPref("foo")               -> *throws*


So they are all different and should be looked more closely, but the last cases are the worst ones, as it causes the code to throw. For example, it caused this line to fail: https://hg.mozilla.org/mozilla-central/rev/68b1b2f00e3f#l2.32


(Note: this is not very important for the clicktoplay-rollout addon, as I'm removing it on bug 1390706, but I think there are other cases that changed in this bug that needs to be double checked)

Comment 64

[Marco Castelluccio [:marco]]

Sorry for the delay, I was on PTO.

I did try not to alter the behavior by adding default values when needed or making sure that the calls were in a try block. I could have missed something though.

I've taken another look at all the patches, I believe the only one where I missed it is the clicktoplay rollout addon.

Comment 65

[:Felipe Gomes (needinfo for replies!)]

Fixing the target milestone here. As can be seen on comment 57, the first pieces of this bug started landing on 56.