Closed Bug 1059620 Opened 10 years ago Closed 10 years ago

Convert XPIProvider to use Preferences.jsm

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: Unfocused, Assigned: michael, Mentored)

References

Details

(Whiteboard: [good second bug][lang=js])

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Patch (obsolete) — Splinter Review
Here are the results from the try server: https://tbpl.mozilla.org/?tree=Try&rev=5846191c6d9f
Attachment #8481112 - Flags: review?(bmcbride)
Depends on: 1061360
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-
Assignee: nobody → michael
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
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)
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+
Attached patch bug-1059620-fix.patch (obsolete) — Splinter Review
Attached patch PatchSplinter Review
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+
Attachment #8489026 - Flags: review+ → review?(bmcbride)
Attachment #8489026 - Flags: review?(bmcbride) → review+
Keywords: checkin-needed
Joint try run with a couple of my patches that depend on this one: https://tbpl.mozilla.org/?tree=Try&rev=dfc290f6d73c
Keywords: checkin-needed
Whiteboard: [good second bug][lang=js] → [good second bug][lang=js][fixed-in-fx-team]
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.

Attachment

General

Creator:
Created:
Updated:
Size: