659680 - Show compatibility warning if a variable called "top" is used in chrome code

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Developer Pages, defect, P2)

Description

[Jorge Villalobos [:jorgev] (he/him)]

As explained in bug 654137, window.top is now read-only. This means that if you have an undefined variable called "top", your add-on will break. Ideally this check should only detect when you try to redefine window.top in some way. Otherwise we should at least check for all assignments done to a variable named "top" in chrome code.

This is only a warning. It should appear in the compatibility message sent to authors, but should not prevent a compatibility bump from happening.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

Message:

The "top" global variable is reserved and cannot be assigned any values starting with Firefox 6. Review your code for any uses of the "top" global, and refer to <LINKED_BUG> for more information.

Krupa:

Depending on how this is implemented, the flag may show up for All-In-One Gestures:
https://addons.mozilla.org/en-US/firefox/addon/all-in-one-gestures/

If the implementation picks up all uses of a variable called "top", this add-on (and a million others) will be flagged. If it looks for assignments only to undeclared "top" variables, then there could be no matches at all.

Comment 2

[Matt Basta [:basta]]

Does this apply to JS modules?

Comment 3

[Matt Basta [:basta]]

This is done for now:

https://github.com/mattbasta/amo-validator/commit/bb5818d857856be99f3d4e436e7ecbb1f116586d

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to comment #2)
> Does this apply to JS modules?

No, it would be best to discard JS modules if possible.

Comment 5

[Kumar McMillan [:kumar]]

(In reply to comment #0)
> This is only a warning. It should appear in the compatibility message sent
> to authors, but should not prevent a compatibility bump from happening.

As of bug 658161 we no longer show warnings on the compatibility results page.  An author will only be notified when there are compatibility errors, so it wouldn't make sense to show warnings.  

We need to fix something.  We can either make this an error (the easy fix) or start to display compatibility warnings and figure out some way to also notify authors of warnings (the hard fix)

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

Wasn't bug 658161 about removing messages that are unrelated to the compatibility push?

Making this and other warnings into errors would produce a very large number of false positives, which is not something we want.

Comment 7

[Kumar McMillan [:kumar]]

bug 661259 and bug 661261 are required to integrate this new compatibility warning.

Comment 8

[Jorge Villalobos [:jorgev] (he/him)]

Bug 659681 and bug 660349 are also warnings.

Comment 9

[Matt Basta [:basta]]

Merged:

https://github.com/mozilla/amo-validator/commit/7778eec40ff8923f8f515d17b702b17690706a8a