Closed Bug 715787 Opened 9 years ago Closed 9 years ago

Refactor common provider-agnostic prefs to be properties of AddonManager

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: Unfocused, Assigned: Unfocused)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #586336 - Flags: review?(dtownsend)
I think this won't work right on aurora, see bug 669541 and the code that we fixed it with there
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
Attachment #586336 - Flags: review?(dtownsend) → feedback+
Attached patch Patch v2Splinter Review
*sigh* I need to stop getting distracted mid-bug.
Attachment #586336 - Attachment is obsolete: true
Attachment #591717 - Flags: review?(dtownsend)
Attachment #591717 - Flags: review?(dtownsend) → review+
Comment on attachment 591717 [details] [diff] [review]
Patch v2

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

Wait, this still hasn't addressed comment 2
Attachment #591717 - Flags: review+ → review-
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 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.
Attachment #591717 - Flags: review- → review+
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc64c895511
Flags: in-testsuite+
Flags: in-litmus-
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/bbc64c895511
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.