910172 - Refactor XPIProvider.importPermissions out to a reusable JSM

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

XPIProvider.importPermissions() is useful for other components too, so would be useful to refactor that out to a reusable JSM.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: 910172.diff

Needs tests yet, but...

Comment 2

[Dave Townsend [:mossop]]

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?

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Now with a test and documentation (and a bug fix).

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 814022 [details] [diff] [review]
Patch v1

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

r+, I don't see the test though

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.00001

Gah, that had me worried. Wrong version of the file? I dunno. This one has the test.

Comment 7

[Dave Townsend [:mossop]]

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.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd6752c496fd

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

And a bustage fix landed for bug 925279:
https://hg.mozilla.org/mozilla-central/rev/40c7c53fe9b0

Comment 11

[Mihai Morar, (:MihaiMorar)]

I confirm the fix is verified on Latest Nightly 28.
BuildID: 20131105030206