Closed Bug 1357517 Opened 7 years ago Closed 7 years ago

Stop loading Preferences.jsm before first paint

Categories

(Firefox :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: florian, Assigned: marco)

References

(Blocks 2 open bugs)

Details

Attachments

(16 files, 6 obsolete files)

9.48 KB, patch
lina
: review+
marco
: checkin+
Details | Diff | Splinter Review
2.43 KB, patch
kmag
: review+
marco
: checkin+
Details | Diff | Splinter Review
5.09 KB, patch
jaws
: review+
marco
: checkin+
Details | Diff | Splinter Review
1.55 KB, patch
MattN
: review+
marco
: checkin+
Details | Diff | Splinter Review
1.54 KB, patch
bzbarsky
: review+
marco
: checkin+
Details | Diff | Splinter Review
2.51 KB, patch
MattN
: review+
marco
: checkin+
Details | Diff | Splinter Review
1.22 KB, patch
MattN
: review+
marco
: checkin+
Details | Diff | Splinter Review
7.38 KB, patch
MattN
: review+
marco
: checkin+
Details | Diff | Splinter Review
58.69 KB, patch
rhelmer
: review+
Details | Diff | Splinter Review
5.99 KB, patch
mchang
: review+
marco
: checkin+
Details | Diff | Splinter Review
3.30 KB, patch
ato
: review+
marco
: checkin+
Details | Diff | Splinter Review
49.44 KB, patch
Dexter
: review+
marco
: checkin+
Details | Diff | Splinter Review
44.42 KB, patch
marco
: review+
marco
: checkin+
Details | Diff | Splinter Review
36.86 KB, patch
Gijs
: review+
marco
: checkin+
Details | Diff | Splinter Review
53.93 KB, patch
Gijs
: review+
marco
: checkin+
Details | Diff | Splinter Review
1.37 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
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.
Blocks: 1350472
Depends on: 1359040
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.
Flags: needinfo?(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.
Flags: needinfo?(florian)
Depends on: 1359737
Depends on: 720419
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.
Attachment #8891486 - Flags: review?(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?
Attachment #8891486 - Flags: review?(bzbarsky) → review-
(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
Attachment #8891486 - Attachment is obsolete: true
Attachment #8891491 - Flags: review?(bzbarsky)
Attachment #8891488 - Flags: review?(kmaglione+bmo) → review+
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 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....
Attachment #8891491 - Flags: review?(bzbarsky) → review-
Used SpecialPowers.pushPrefEnv.
Assignee: nobody → mcastelluccio
Attachment #8891491 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8891492 - Flags: review?(jaws) → review+
Attachment #8891496 - Flags: review?(bzbarsky)
Comment on attachment 8891496 [details] [diff] [review]
Remove Preferences.jsm usage from browser_bug1047663.js

Yes, this is much better!
Attachment #8891496 - Flags: review?(bzbarsky) → review+
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?
Attachment #8891485 - Flags: review?(kit) → review+
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`.
Keywords: leave-open
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b3cced9169
Remove Preferences.jsm usage from workers test. r=bz
Attachment #8891494 - Flags: review?(MattN+bmo) → review+
Attachment #8891523 - Flags: review?(MattN+bmo) → review+
Attachment #8891525 - Flags: review?(MattN+bmo) → review+
Attachment #8891530 - Flags: review?(MattN+bmo)
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"],
…
Attachment #8891530 - Flags: review?(MattN+bmo)
Attachment #8891530 - Attachment is obsolete: true
Attachment #8891541 - Flags: review?(MattN+bmo)
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
Attachment #8891541 - Flags: review?(MattN+bmo) → review+
Pushed by mcastelluccio@mozilla.com:
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
This is needed in order to add Preferences.jsm to the browser_startup.js test, otherwise Marionette loads it really early.
Attachment #8891573 - Flags: review?(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 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.
Attachment #8891573 - Flags: review?(ato) → review+
Attachment #8891742 - Flags: review?(alessio.placitelli)
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3362528e5e25
Remove or delay Preferences.jsm usage from Marionette. r=ato
Attachment #8891485 - Flags: checkin+
Attachment #8891488 - Flags: checkin+
Attachment #8891492 - Flags: checkin+
Attachment #8891494 - Flags: checkin+
Attachment #8891496 - Flags: checkin+
Attachment #8891523 - Flags: checkin+
Attachment #8891525 - Flags: checkin+
Attachment #8891541 - Flags: checkin+
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6025b5b43ec6
Fix max-len eslint rule breakage in marionette.js. r=me
Patch updated to also remove Preferences.jsm from the newly introduced UpdatePing module.
Attachment #8891742 - Attachment is obsolete: true
Attachment #8891742 - Flags: review?(alessio.placitelli)
Attachment #8891834 - Flags: review?(alessio.placitelli)
Attachment #8891834 - Flags: review?(alessio.placitelli) → review+
Attachment #8891573 - Flags: checkin+
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.
Attachment #8891571 - Flags: review?(mconley) → review+
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a2f780f966
Remove Preferences.jsm usage from some Telemetry files. r=Dexter
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?
Attachment #8891572 - Flags: review?(mconley)
(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.
(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...
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.
Attachment #8891571 - Attachment is obsolete: true
Attachment #8891967 - Flags: review+
Fixed what you asked in comment 41 (except distribution.js) and also removed Preferences.jsm usage from Experiments.jsm.
Attachment #8891572 - Attachment is obsolete: true
Attachment #8891968 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8891968 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mcastelluccio@mozilla.com:
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
Pushed by mcastelluccio@mozilla.com:
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
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9079c02563e7
Remove unneeded ternary operators to fix eslint failure. r=me
Attachment #8891565 - Flags: review?(mchang) → review+
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.
Attachment #8892078 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8892078 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1385952
Blocks: 1385954
No longer blocks: 1350472
No longer depends on: 1359040, 1385416
Summary: Consider removing Preferences.jsm → Stop loading Preferences.jsm before first paint
Pushed by mcastelluccio@mozilla.com:
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 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.
Attachment #8892075 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa81cd2510db
Remove Preferences.jsm usage from graphics sanity test. r=mchang
Pushed by mcastelluccio@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68b1b2f00e3f
Remove Preferences.jsm usage from some browser/extensions/ modules. r=Gijs
Attachment #8891565 - Flags: checkin+
Attachment #8891834 - Flags: checkin+
Attachment #8891967 - Flags: checkin+
Attachment #8891968 - Flags: checkin+
Attachment #8892075 - Flags: checkin+
No longer depends on: 720419
Attachment #8891564 - Flags: review?(rhelmer) → review+
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
Pushed by mcastelluccio@mozilla.com:
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
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/33788fc85648
https://hg.mozilla.org/mozilla-central/rev/984c45066b41
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1386883
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.
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)
Flags: needinfo?(mcastelluccio)
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.
Flags: needinfo?(mcastelluccio)
Fixing the target milestone here. As can be seen on comment 57, the first pieces of this bug started landing on 56.
Target Milestone: Firefox 57 → Firefox 56
Depends on: 1448361
No longer depends on: 1448361
You need to log in before you can comment on or make changes to this bug.