715787 - Refactor common provider-agnostic prefs to be properties of AddonManager

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

There are various prefs the addons manager code uses quite a bit all over the place, and they're all doing the same sanity checks and managing their own observers. The check compatibility pref is particularly messy, since it depends on the application channel and version. It would be useful to consolidate these into properties of the AddonManager object.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Type listeners (onTypeAdded/onTypeRemoved) could also be merged into the new manager listeners... but that would break that API. Though I couldn't find any consumers other than the code in /toolkit/mozapps/extensions. If we do end up merging, I'd want to do that separately so it can easily be undone.

Comment 2

[Dave Townsend [:mossop]]

I think this won't work right on aurora, see bug 669541 and the code that we fixed it with there

Comment 3

[Dave Townsend [:mossop]]

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

Along the right lines but would like comment 2 to be addressed before I do a full review

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

*sigh* I need to stop getting distracted mid-bug.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 591717 [details] [diff] [review]
Patch v2

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

Wait, this still hasn't addressed comment 2

Comment 6

[Dave Townsend [:mossop]]

See here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/head_addons.js#405

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

On non-nightly branches, I made that pref be computed in AddonManagerInternal.startup() - AFAICT, that fixes any issues (and I tested using a build configured as Aurora). Or am I missing something?

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 591717 [details] [diff] [review]
Patch v2

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

No, I'm missing something, a brain. Can we remove that code from head_addons.js now then?

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +497,5 @@
>                                  (appChanged === undefined ? 0 : -1));
>      }
>  
> +#ifdef MOZ_COMPATIBILITY_NIGHTLY
> +    PREF_EM_CHECK_COMPATIBILITY = PREF_EM_CHECK_COMPATIBILITY_BASE + ".nightly";

This isn't necessary is it? It's computed above.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dave Townsend (:Mossop) from comment #8)
> Can we remove that code from
> head_addons.js now then?

I was going to leave it in - we don't lose anything by it, and it increases the test surface. Will update the comment though.

> > +#ifdef MOZ_COMPATIBILITY_NIGHTLY
> > +    PREF_EM_CHECK_COMPATIBILITY = PREF_EM_CHECK_COMPATIBILITY_BASE + ".nightly";
> 
> This isn't necessary is it? It's computed above.

Oops, yes, was sure I had already removed that.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc64c895511

Comment 11

[Marco Bonardo [:mak]]

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