1059620 - Convert XPIProvider to use Preferences.jsm

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

XPIProvider.jsm uses it's own custom wrapper object for dealing with preferences via a nicer API than the raw nsIPrefBranch interface. However, now days we have Preferences.jsm - a generic module that does the same. IIRC, much of the API is basically the same.

We should convert XPIProvider.jsm to use Preferences.jsm, which will let us remove a bunch of code, and make it easier to hack on in the future.

Comment 1

[Michael Pruett]

Attachment: Patch

Here are the results from the try server:

https://tbpl.mozilla.org/?tree=Try&rev=5846191c6d9f

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

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

Thanks, Micheal :)

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +204,4 @@
>  
>  var gGlobalScope = this;
>  
> +var gPrefs = new Preferences();

Constructing a new Preferences object is only really useful when you want to use non-default settings, such as specifying a branch prefix. ie: 
  let prefs = new Preferences("browser.tabs.");
  let value = prefs.get("remote", false);

(Or getting the default branch like you've done later in this patch.)

Since we're not going that, we can (and should) just use the Preferences object directly:
  let value = Preferences.get("browser.tabs.remote", false);

@@ +494,3 @@
>      return Services.locale.getLocaleComponentForUserAgent();
> +  try {
> +    let locale = Prefs.getComplexValue(PREF_SELECTED_LOCALE, Ci.nsIPrefLocalizedString);

Missed one. Incidentally, looks like Preferences.jsm can't do this at the moment :\ I've filed bug 1061360 for this - want to pick that up too?

@@ +497,5 @@
> +    if (locale)
> +      return locale;
> +  }
> +  catch (e) {
> +  }

Nit: No line break between { and } when the catch is intentionally empty - to make it obvious that it is intentional.

Comment 3

[Michael Pruett]

Attachment: Patch

Thanks, Blair. I have updated the patch with the changes you suggested. I'm now using Preferences.get() rather than creating a global Preferences instance. I've also used Preferences.get() with the new type parameter added in bug 1061360 for accessing localized strings.

Here are the results from the try server:

https://tbpl.mozilla.org/?tree=Try&rev=f32778092946

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

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

This makes a nice improvement, thanks :)

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +6430,5 @@
>          try {
>            let pref = PREF_EM_EXTENSION_FORMAT + aAddon.id + "." + aProp;
> +          let value = Preferences.get(pref, null, Ci.nsIPrefLocalizedString);
> +		  if (value)
> +		  	result = value;

Nit: Looks like you have a stray tab character in these lines, messing up the indenting.

Comment 5

[Michael Pruett]

Attachment: bug-1059620-fix.patch

Comment 6

[Michael Pruett]

Attachment: Patch

I have fixed the inconsistent indentation and have updated the patch with the most recent changes to XPIProvider.jsm.

Comment 7

[:Irving Reid (No longer working on Firefox)]

Joint try run with a couple of my patches that depend on this one: https://tbpl.mozilla.org/?tree=Try&rev=dfc290f6d73c

Comment 8

[:Irving Reid (No longer working on Firefox)]

https://hg.mozilla.org/integration/fx-team/rev/98879631e235

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98879631e235