1229224 - Make all mozilla plugins use the same methods for finding global variables and add a way to get globals from browser.js

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Dave Townsend [:mossop]]

I found the components-imports not to be quite good enough for the various ways we define properties so I've forked it to something a little more complex. It checks that the scope you're importing to is the global for most cases (the only kind of variable that need to be declared I think).

Also fixed the way that helpers declares variables in scope.

Comment 1

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1229224: Add eslint plugin for various ways to declare properties on the global object and enable globally. r?miker

Bug 1229224: Add eslint plugin for various ways to declare properties on the global object and enable globally. r?pbro

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8693881 [details]
MozReview Request: Bug 1229224: Add eslint plugin for various ways to declare properties on the global object and enable globally. r?miker

I think Mike should review this instead (I haven't found a way to edit the reviewer in mozreview, so I'm doing it here only).

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 8693881 [details]
MozReview Request: Bug 1229224: Add eslint plugin for various ways to declare properties on the global object and enable globally. r?miker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26611/diff/1-2/

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 8693881 [details]
MozReview Request: Bug 1229224: Add eslint plugin for various ways to declare properties on the global object and enable globally. r?miker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26611/diff/2-3/

Comment 5

[Tom Tromey :tromey]

See also bug 1229911.
This patch will need a small tweak if that happens to go in first.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8693881 [details]
MozReview Request: Bug 1229224: Add eslint plugin for various ways to declare properties on the global object and enable globally. r?miker

Want to tinker with this some more

Comment 7

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1229224: Fix linting for some browser files that tests depend on. r=felipe

Review commit: https://reviewboard.mozilla.org/r/28543/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28543/

Comment 8

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1229224: Support more forms of defining globals and make anywhere we import scripts use them too. r=miker

Review commit: https://reviewboard.mozilla.org/r/28545/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28545/

Comment 9

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1229224: Add an eslint plugin for importing all browser.js globals for browser-chrome tests. r=miker

Review commit: https://reviewboard.mozilla.org/r/28547/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28547/

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8700055 [details]
MozReview Request: Bug 1229224: Fix linting for some browser files that tests depend on. r=felipe

https://reviewboard.mozilla.org/r/28543/#review25375

::: browser/base/content/browser-social.js:1363
(Diff revision 1)
> -    [m.parentNode.removeChild(m) for (m of menus)];
> +      m.parentNode.removeChild(m);

I was probably doing something dumb at the time, but I tried fixing this file and made this change before, and there was a test that was failing it, and I couldn't figure out why.. So, just a heads up. I think it was browser_social_marks.js but I'm not 100% sure.

Anyway, now that this file is fixed, can you remove it from the exclusion list in .eslintignore?

Comment 11

[:Felipe Gomes (needinfo for replies!)]

https://reviewboard.mozilla.org/r/28547/#review25381

It would be nice if this rule could also be activated with a comment tag (like import-globals-from does), so that we can use it for browser.js and these listed scripts themselves too, and not only for tests.  Or maybe you're already thinking of a different way to deal with the undeclared vars on those files?

Comment 12

[Dave Townsend [:mossop]]

https://reviewboard.mozilla.org/r/28547/#review25381

You can activate any eslint rule with a comment tag already, /* eslint mozilla/import-browserjs-globals: 2 */

Comment 13

[:Felipe Gomes (needinfo for replies!)]

ah, neat!

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8700056 [details]
MozReview Request: Bug 1229224: Support more forms of defining globals and make anywhere we import scripts use them too. r=miker

https://reviewboard.mozilla.org/r/28545/#review25487

Awesome, I really like estraverse... nice find!

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8700057 [details]
MozReview Request: Bug 1229224: Add an eslint plugin for importing all browser.js globals for browser-chrome tests. r=miker

https://reviewboard.mozilla.org/r/28547/#review25489

::: testing/eslint-plugin-mozilla/lib/index.js:34
(Diff revision 1)
>      "import-headjs-globals": 0,

Nit: You should probably include `"import-browserjs-globals": 0,` here as this is where we set defaults.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c656b3dad435
https://hg.mozilla.org/integration/fx-team/rev/b60b2371b0c0
https://hg.mozilla.org/integration/fx-team/rev/7417b352c32d

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1d259b3dbc70

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/11d71b79bc12

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c656b3dad435
https://hg.mozilla.org/mozilla-central/rev/b60b2371b0c0
https://hg.mozilla.org/mozilla-central/rev/7417b352c32d
https://hg.mozilla.org/mozilla-central/rev/1d259b3dbc70
https://hg.mozilla.org/mozilla-central/rev/11d71b79bc12