Last Comment Bug 693901 - Add preference to globally (re)enable strict compatibility checks
: Add preference to globally (re)enable strict compatibility checks
Status: VERIFIED FIXED
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Blair McBride [:Unfocused] (UNAVAILABLE)
:
: Andy McKay [:andym]
Mentors:
Depends on: 704165 702920 779471
Blocks: 692664
  Show dependency treegraph
 
Reported: 2011-10-11 20:30 PDT by Blair McBride [:Unfocused] (UNAVAILABLE)
Modified: 2012-08-01 06:04 PDT (History)
7 users (show)
blair: in‑testsuite+
blair: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (186.40 KB, patch)
2011-10-17 22:33 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
dtownsend: review-
Details | Diff | Splinter Review
Patch v2 (174.72 KB, patch)
2011-10-19 21:52 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
dtownsend: review+
Details | Diff | Splinter Review
Patch v2.1 (177.97 KB, patch)
2011-10-31 01:36 PDT, Blair McBride [:Unfocused] (UNAVAILABLE)
no flags Details | Diff | Splinter Review

Description Blair McBride [:Unfocused] (UNAVAILABLE) 2011-10-11 20:30:56 PDT
Bug 693897 will allow individual addons to opt-in to strict compatibility checking, but there are cases where strict compatibility checking should be able to be enabled globally (ie, fall back to old not-compatibile-by-default behaviour):

* Other toolkit apps may want to have strict compatibility checking
* Mission critical situations. If you're sending Firefox to the aliens on Europa as a peace offering, you don't want it breaking because of an incompatible addon. They might not take kindly to that.
* Will act as a kill-switch for this functionality if something breaks (saves a backout of the feature).

Suggested name: extensions.strictCompatibilityChecking
Comment 1 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-10-12 04:49:13 PDT
My patch for this breaks a third of all Addon Manager xpcshell tests. This may take awhile...
Comment 2 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-10-17 22:33:50 PDT
Created attachment 567683 [details] [diff] [review]
Patch v1

My strategy for fixing the (OMG SO MANY) tests was:
* If it tests something that determines compatibility, make a copy - one for compatible-by-default, one for strict compatibility. This was much easier than testing both in one test.
* Otherwise, the test just wants an incompatible addon and doesn't care why it's incompatible - so enable strict compatibility for that test.

I also made the updater tests run with strict compatiblity, as some of them expect certain addons to be incompatible. Same with one of the browser tests.

The tests I made copies of already test everything to do with actually determining whats compatible and what's not, and how that affects appDisabled (etc). That just left testing the pref changes.
Comment 3 Dave Townsend [:mossop] 2011-10-19 11:29:16 PDT
Comment on attachment 567683 [details] [diff] [review]
Patch v1

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

I'm surprised to see no changes to AddonUpdateChecker.jsm here, or will those come elsewhere?

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +378,5 @@
>  
> +    try {
> +      gStrictCompatibility = Services.prefs.getBoolPref(PREF_EM_STRICT_COMPATIBILITY);
> +    } catch (e) {}
> +    Services.prefs.addObserver(PREF_EM_STRICT_COMPATIBILITY, this, false);

I think the default should be true so apps have to explicitly opt into this behaviour

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +6857,5 @@
> +    // Only extensions can be compatible by default; themes always use strict
> +    // compatibility checking.
> +    if (this.type == "extension" && !AddonManager.strictCompatibility)
> +      return true;
> +

I'm not sure we remembered to add it to the spec but we still want to test against the minVersion, i.e. an extension for Firefox 21 shouldn't install in Firefox 10.
Comment 4 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-10-19 21:52:00 PDT
(In reply to Dave Townsend (:Mossop) from comment #3)
> I'm surprised to see no changes to AddonUpdateChecker.jsm here, or will
> those come elsewhere?

Was going to do that in bug 527141.

> I think the default should be true so apps have to explicitly opt into this
> behaviour

Hm, yea, makes sense.

> I'm not sure we remembered to add it to the spec but we still want to test
> against the minVersion, i.e. an extension for Firefox 21 shouldn't install
> in Firefox 10.

Almost forgot about that (it wasn't in the spec). Filed bug 695977 and amended the spec.
Comment 5 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-10-19 21:52:59 PDT
Created attachment 568299 [details] [diff] [review]
Patch v2
Comment 6 Dave Townsend [:mossop] 2011-10-24 11:51:29 PDT
Comment on attachment 568299 [details] [diff] [review]
Patch v2

Would it be worth grabbing one of hte project branches for this work? Looks like larch might be free
Comment 7 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-10-30 20:22:22 PDT
(In reply to Dave Townsend (:Mossop) from comment #6)
> Would it be worth grabbing one of hte project branches for this work? Looks
> like larch might be free

Instead of that, where I'd still need to deal with bitrot, what do you say to having this land with the pref toggled? (ie, land with the entire feature disabled)
Comment 8 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-10-31 01:36:20 PDT
Created attachment 570629 [details] [diff] [review]
Patch v2.1

Just some minor test fixes.
Comment 9 Dave Townsend [:mossop] 2011-10-31 11:09:59 PDT
(In reply to Blair McBride (:Unfocused) from comment #7)
> (In reply to Dave Townsend (:Mossop) from comment #6)
> > Would it be worth grabbing one of hte project branches for this work? Looks
> > like larch might be free
> 
> Instead of that, where I'd still need to deal with bitrot, what do you say
> to having this land with the pref toggled? (ie, land with the entire feature
> disabled)

I am in full support of this plan
Comment 10 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-10-31 18:39:40 PDT
(In reply to Dave Townsend (:Mossop) from comment #9)
> > Instead of that, where I'd still need to deal with bitrot, what do you say
> > to having this land with the pref toggled? (ie, land with the entire feature
> > disabled)
> 
> I am in full support of this plan

Filed bug 698653 to flip the pref when we're ready.
Comment 11 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-10-31 22:54:53 PDT
https://hg.mozilla.org/integration/fx-team/rev/41c52a9e1337
Comment 12 Tim Taubert [:ttaubert] 2011-11-01 05:32:56 PDT
https://hg.mozilla.org/mozilla-central/rev/1fb831781a15
Comment 13 Siddhartha Dugar [:sdrocking] 2011-11-13 11:18:09 PST
Does this not affect compatibility checking for themes?
Comment 14 Dão Gottwald [:dao] 2011-11-13 12:10:07 PST
(In reply to sdrocking from comment #13)
> Does this not affect compatibility checking for themes?

Correct. Themes created for a particular Firefox version are almost certainly broken in the subsequent release.
Comment 15 Virgil Dicu [:virgil] [QA] 2011-11-29 04:17:27 PST
Setting this to verified.

The pref is present and functional in about:config. Currently enabled on Nightly and disabled on Aurora.

Two dependencies remaining for this.
Comment 16 Eric Shepherd [:sheppy] 2011-12-19 10:28:22 PST
Documented:

https://developer.mozilla.org/en/Install_Manifests#strictCompatibility

And mentioned on Firefox 10 for developers.

Note You need to log in before you can comment on or make changes to this bug.