Skip to content

Update Add-on Compatibility prefs and show warning when it gets enabled #14

Closed
brianking opened this Issue Feb 9, 2012 · 14 comments

5 participants

@brianking
Mozilla member

I am not sure if NTT is still doing this, or how, but with Compatible By Default (CBD) it should probably not be doing it anymore. This is why the ACR stopped doing it.

Basically, the traditional compatibility prefs and CBD do not play nice together. The dirty details are in this bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=698653

ACR related bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=708931

@tonymec
@whimboo
whimboo commented Feb 15, 2012

I can still see the force add-on compatibility menu entry in the main menu. So I would say that we still support overriding the compatibility prefs.

I also have mixed feelings and have expressed those to Brian and Dave Townsend. While I think the compatibility check removal in ACR was correct because this extension is used by even normal users, we probably should keep the feature in NTT. As Tony mentioned it would allow us to even run extensions which haven't been updated for a while, also those with binary components.

@brianking
Mozilla member

So I looked at the code and conclude that NTT is setting a different pref than ACR was. ACR set 'extensions.checkCompatibility.nightly', and NTT sets 'nightly.disableCheckCompatibility'. To be honest, I am not sure what the latter does and how it differs from the former.

Previously with 'extensions.checkCompatibility.nightly' you could install fresh from AMO add-ons not compatible. A good example (or maybe not because it has binary components) right now of one not updated for Firefox 4+ is:

https://addons.mozilla.org/addon/kidzui/

But even with 'nightly.disableCheckCompatibility' set to true, Firefox doesn't allow it to be installed.

Tested with Mozilla/5.0 (Windows NT 6.0; rv:11.0) Gecko/20100101 Firefox/11.0

So what I think 'nightly.disableCheckCompatibility' does is allow add-ons already installed but disabled to work again.

I think it is fine to leave it then, but would like clarification first on 'nightly.disableCheckCompatibility' does as I can't find documentation on it. I'll ping Blair.

@xabolcs
xabolcs commented Feb 15, 2012

Hi there,

as You can see in nttAddonCompatibilityService.setCompatPrefs() it is a little bit outdated:
it's maxversion is Firefox 7. So if somebody clicks on the "Force Addon Compatibility" nowadays the nothing special will happen with current Toolkit Applications.
Just the nightly.disableCheckCompatibility indicator flag will be toggled.

IMHO if somebody would like to touch nttAddonCompatibilityService.js (read: revamp NTT's force-compatiblity feature) then he/she should consider about Kris Maglione's work in his addon in addition - how to set only the necessary and sufficient subset of extensions.checkCompatibility.* preferences.

Anyway, +1 to keep this compatibility feature in NTT.

@Unfocused
Mozilla member

nightly.disableCheckCompatibility isn't used by (and has never been used by) the Add-ons Manager. As with all nightly.* prefs, it's only used by NTT. From a quick look at the code, it's what NTT checks to determine whether it should set the extensions.checkCompatibility.* prefs (see nttAddonCompatibilityService).

@brianking
Mozilla member

Doh! Obvious I am new to this extension.

Maybe what we should do when we flip the prefs is a present a warning that a) old add-ons may not work and b) may cause Firefox to behave badly because of clashes with CBD.

What does everyone think?

@whimboo
whimboo commented Mar 21, 2012

Sounds good. I'm happy to fold this into 3.3

@brianking brianking added a commit to brianking/nightlytt that referenced this issue Mar 22, 2012
@brianking brianking Issue #14: Update Add-on Compatibility prefs fa963bb
@brianking brianking added a commit to brianking/nightlytt that referenced this issue Mar 26, 2012
@brianking brianking Issue #14 : More compat updates 6385223
@whimboo
whimboo commented Mar 26, 2012

@brianking can you please let me merge pull requests the next time? The review process wasn't fully done but the merge already happened and the request has been closed. That shouldn't have happened. Also please don't commit directly as given by your last update. Any code change needs a review and the reviewer should check in or say that it's good to be checked in first. Thanks.

@whimboo
whimboo commented Mar 26, 2012

Hm, actually I don't see that anything has landed on mozilla/master. What happened with the last merge and why has the pull request #48 been closed. I'm a bit confused now.

@xabolcs
xabolcs commented Mar 27, 2012

whimboo commented

Hm, actually I don't see that anything has landed on mozilla/master. What happened with the last merge and why has the pull request #48 been closed. I'm a bit confused now.

See my comment in #15:

  • In short: I rebased my commits on top of mozilla/master therefore the original commit range became empty.

  • In long: when somebody requires a pull GitHub stores it's base commit (parent of first commit) and the pull request will consits of that commit descendants.
    If the requester rebase that commits on to the mozilla/master (which could be moved forward in the meantime), the original, saved "pointer" of the pull request will point to old commit, where nothing will be find. Therefore pull request will be empty.

It would be interesting how to collaspe and not to pollute: if requester rebases the request, it would became empty.
It would be good to know git is able to collapse merge commits too.

@whimboo
whimboo commented Mar 27, 2012

Ah wait. Now I see. So you should never ever work on a branch you rebase to and which also exists in the original repository. Looks like a new pull request has to be opened by Brian with the changes he did on pull #48.

I for myself will make sure to check the pull details exactly in the future to see such an upcoming situation early enough. Thanks for making it clear.

@xabolcs
xabolcs commented Mar 27, 2012

... So you should never ever work on a branch you rebase to and which also exists in the original repository. ...

Not clearly understand, but surely. :)
IMHO You should ask some other githubber at mozilla to clarify this. :) One of the Addon-SDK team will be a good choice.
If You got answers please share it to me (at least)!

@brianking
Mozilla member

Yes, I messed up. Prompted by a comment by @xabolcs somewhere yesterday, I researched a bit more about having multiple pull requests open at the same time and the way to do it is to have branches on your fork. I had already committed to my master. So what I did is deleted my fork and started fresh. github removing issue 48 was a bit confusing, sorry about that.

I'm going to put together the changesets now again, this time on branches.

@brianking brianking added a commit to brianking/nightlytt that referenced this issue Mar 27, 2012
@brianking brianking Issue #14 : Compat updates 3cb9499
@whimboo
whimboo commented Mar 28, 2012

Code has been merged.

@whimboo whimboo closed this Mar 28, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.