Refactor XPIProvider.importPermissions out to a reusable JSM

VERIFIED FIXED in mozilla27

Status

()

defect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Unfocused, Assigned: Unfocused)

Tracking

Trunk
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

XPIProvider.importPermissions() is useful for other components too, so would be useful to refactor that out to a reusable JSM.
Posted patch 910172.diff (obsolete) — Splinter Review
Needs tests yet, but...
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Attachment #796575 - Flags: feedback?(dtownsend+bugmail)
Comment on attachment 796575 [details] [diff] [review]
910172.diff

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

::: toolkit/modules/PermissionsUtils.jsm
@@ +48,5 @@
> +                     Services.perms.DENY_ACTION);
> +
> +    gImportedPrefBranches.add(aPrefBranch);
> +	}
> +};

Indenting is all weird here

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +3096,5 @@
>     * manager for the user to change later.
>     */
>    importPermissions: function XPI_importPermissions() {
> +    PermissionsUtils.importFromPrefs(PREF_XPI_PERMISSIONS_BRANCH,
> +                                     XPI_PERMISSION);

At the moment we do this early in startup to make sure the prefs have been imported before they are needed. With this shared code we could potentially only do this immediately before querying the pref service. Worth it?
Attachment #796575 - Flags: feedback?(dtownsend+bugmail) → feedback+
(In reply to Dave Townsend (:Mossop) from comment #2)
> At the moment we do this early in startup to make sure the prefs have been
> imported before they are needed. With this shared code we could potentially
> only do this immediately before querying the pref service. Worth it?

Yep, bug 697314, which Irving and I hope to fix soonish. In addition to code-reuse, this is a tiny stepping stone toward both that and lazy-loading as much code as possible.
Posted patch Patch v1 (obsolete) — Splinter Review
Now with a test and documentation (and a bug fix).
Attachment #796575 - Attachment is obsolete: true
Attachment #814022 - Flags: review?(dtownsend+bugmail)
Comment on attachment 814022 [details] [diff] [review]
Patch v1

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

r+, I don't see the test though
Attachment #814022 - Flags: review?(dtownsend+bugmail) → review+
Gah, that had me worried. Wrong version of the file? I dunno. This one has the test.
Attachment #814022 - Attachment is obsolete: true
Attachment #814762 - Flags: review?(dtownsend+bugmail)
Comment on attachment 814762 [details] [diff] [review]
Patch v1.00001

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

Throw a test in where there is a leading or trailing comma in the pref and it's good to go.
Attachment #814762 - Flags: review?(dtownsend+bugmail) → review+
https://hg.mozilla.org/mozilla-central/rev/cd6752c496fd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 925279
I confirm the fix is verified on Latest Nightly 28.
BuildID: 20131105030206
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.