Closed
Bug 1059620
Opened 10 years ago
Closed 10 years ago
Convert XPIProvider to use Preferences.jsm
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: Unfocused, Assigned: michael, Mentored)
References
Details
(Whiteboard: [good second bug][lang=js])
Attachments
(1 file, 3 obsolete files)
18.12 KB,
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Here are the results from the try server:
https://tbpl.mozilla.org/?tree=Try&rev=5846191c6d9f
Attachment #8481112 -
Flags: review?(bmcbride)
Reporter | ||
Comment 2•10 years ago
|
||
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.
Attachment #8481112 -
Flags: review?(bmcbride) → review-
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → michael
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
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
Attachment #8481112 -
Attachment is obsolete: true
Attachment #8486984 -
Flags: review?(bmcbride)
Reporter | ||
Comment 4•10 years ago
|
||
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.
Attachment #8486984 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
I have fixed the inconsistent indentation and have updated the patch with the most recent changes to XPIProvider.jsm.
Attachment #8486984 -
Attachment is obsolete: true
Attachment #8489024 -
Attachment is obsolete: true
Attachment #8489026 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8489026 -
Flags: review+ → review?(bmcbride)
Reporter | ||
Updated•10 years ago
|
Attachment #8489026 -
Flags: review?(bmcbride) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 7•10 years ago
|
||
Joint try run with a couple of my patches that depend on this one: https://tbpl.mozilla.org/?tree=Try&rev=dfc290f6d73c
Comment 8•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [good second bug][lang=js] → [good second bug][lang=js][fixed-in-fx-team]
Comment 9•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good second bug][lang=js][fixed-in-fx-team] → [good second bug][lang=js]
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•