538234 - Should use extensions.checkCompatibility.3.6pre for 3.6pre versions

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Nickolay_Ponomarev]

Since bug 521905 Firefox checks the "extensions.checkCompatibility.<version>" pref to determine if it should allow installing/enabling incompatible add-ons.

The algorithm for determining the <version> part is only described by providing a few simple examples:
http://www.oxymoronical.com/blog/2009/11/Changing-the-checkCompatibility-preference
http://kb.mozillazine.org/Extensions.checkCompatibility#Background

and yields somewhat unexpected results on 3.6pre nightlies: the pref with the suffix "3.6p" is checked. This makes the add-on compatibility reporter not work, in particular (since it doesn't set such pref).

The regexp that is applied to get the <version> in the pref name out of app version (like "3.6b5"):
http://mxr.mozilla.org/mozilla-central/search?string=gBranchVersion

1) It doesn't make any sense to me (I can't understand the intention from this code).
2) Please document the possible values for the <version> part.
3) Perhaps the 3.6pre strings should resolve to a different <version> part?
4) Are there other non-trivial app versions where this doesn't work as desired?

Comment 1

[Dave Townsend [:mossop]]

(In reply to comment #0)
> Since bug 521905 Firefox checks the "extensions.checkCompatibility.<version>"
> pref to determine if it should allow installing/enabling incompatible add-ons.
> 
> The algorithm for determining the <version> part is only described by providing
> a few simple examples:
> http://www.oxymoronical.com/blog/2009/11/Changing-the-checkCompatibility-preference
> http://kb.mozillazine.org/Extensions.checkCompatibility#Background
> 
> and yields somewhat unexpected results on 3.6pre nightlies: the pref with the
> suffix "3.6p" is checked. This makes the add-on compatibility reporter not
> work, in particular (since it doesn't set such pref).

That is unfortunate, we should file a bug to get the ACR to handle this case.

> 1) It doesn't make any sense to me (I can't understand the intention from this
> code).

From the bug: it just uses 2 version parts including whether the second part has any alpha, beta or anything else in it.

So basically the major and minor versions plus some indicator of the stability of the branch.

> 2) Please document the possible values for the <version> part.

This is probably a full list unless we changed versioning schemes: X.Ya, X.Yb, X.Yp, X.Y

> 3) Perhaps the 3.6pre strings should resolve to a different <version> part?

While I never considered the 3.6pre version when this was coded I don't think that using 3.6p as the pref for this case is contrary to the original intention of the change. Pre is certainly an indicator of the stability of the branch at that point. One could argue that we should just ignore pre and call it 3.6 at that point since we only go there after spinning RC, but I'm not sure whether it is worth the effort.

> 4) Are there other non-trivial app versions where this doesn't work as desired?

Is it possible for other applications to use their own versioning system where this doesn't yield ideal results? Yes certainly. But as I believed when I implemented it this way, the majority of apps will work just fine with this.

Comment 2

[Dave Townsend [:mossop]]

So there are a couple of options for what we can do here.

1. Do nothing.
2. Ignore pre parts (maybe even only include a or b so we'd have a list of X.Ya, X.Yb or X.Y only).
3. Include the full string part so we'd have X.Ya, X.Yb, X.Ypre, X.Y.

I'm not particularly pulled either way though bsmedberg prefers option 3.

Comment 3

[Dave Townsend [:mossop]]

Beltzner agrees with bsmedberg so we're going to use any letters following the numeric part of the minor version in the pref.

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This makes the pref be the major version part and the first sequence of numbers and first sequence of letters in the minor version part.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 420459 [details] [diff] [review]
patch rev 1

Benjamin, want to confirm that this regexp matches what you expect?

Comment 6

[Benjamin Smedberg]

Comment on attachment 420459 [details] [diff] [review]
patch rev 1

technically I think it should be [0-9]+[^0-9]* but I doubt it matters in practice. r=me either way.

Comment 7

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/0f26f1f2e55b

Comment 8

[Nickolay_Ponomarev]

Thanks! I updated the kb.mozillazine docs.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 420459 [details] [diff] [review]
patch rev 1

Not critical for release, but would be useful to fix the pref on current nightly branch builds.

Comment 10

[Alice0775 White]

No I do not think so,
We have already used 3.6p
3.6pre causes confusion

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Let's take it; Stuart, I don't think this will have any affect on mobile, but wanted to cc you for the heads-up.

a192=beltzner, but don't put it on the relbranches, obviously

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 420459 [details] [diff] [review]
patch rev 1

Pushed to branch per beltzner's a+ above: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/05a4cc93f14b

Comment 13

[Marco Bonardo [:mak]]

looks like you forgot to mark final-fixed... correct me if i'm wrong.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Fixed a long time ago and I haven't discovered any problem so far. Marking as verified fixed.