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

RESOLVED FIXED in Firefox 46

Status

Testing
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created 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

Bug 1229224: Add eslint plugin for various ways to declare properties on the global object and enable globally. r?pbro
Attachment #8693881 - Flags: review?(pbrosset)
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).
Attachment #8693881 - Flags: review?(pbrosset) → review?(mratcliffe)
(Assignee)

Comment 3

2 years ago
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/
Attachment #8693881 - Attachment description: MozReview Request: Bug 1229224: Add eslint plugin for various ways to declare properties on the global object and enable globally. r?pbro → MozReview Request: Bug 1229224: Add eslint plugin for various ways to declare properties on the global object and enable globally. r?mratcliffe
Attachment #8693881 - Flags: review?(mratcliffe) → review?(pbrosset)
(Assignee)

Updated

2 years ago
Attachment #8693881 - Flags: review?(pbrosset) → review?(mratcliffe)
(Assignee)

Comment 4

2 years ago
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/
Attachment #8693881 - Attachment description: MozReview Request: Bug 1229224: Add eslint plugin for various ways to declare properties on the global object and enable globally. r?mratcliffe → MozReview Request: Bug 1229224: Add eslint plugin for various ways to declare properties on the global object and enable globally. r?miker
(Assignee)

Updated

2 years ago
Blocks: 1229856

Comment 5

2 years ago
See also bug 1229911.
This patch will need a small tweak if that happens to go in first.
(Assignee)

Comment 6

2 years ago
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
Attachment #8693881 - Flags: review?(mratcliffe)
(Assignee)

Updated

2 years ago
Attachment #8693881 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Blocks: 1229142
(Assignee)

Updated

2 years ago
Summary: Add eslint plugin to support Object.defineProperty etc. → Make all mozilla plugins use the same methods for finding global variables and add a way to get globals from browser.js
(Assignee)

Comment 7

2 years ago
Created attachment 8700055 [details]
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/
Attachment #8700055 - Flags: review?(felipc)
(Assignee)

Comment 8

2 years ago
Created attachment 8700056 [details]
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/
Attachment #8700056 - Flags: review?(mratcliffe)
(Assignee)

Comment 9

2 years ago
Created attachment 8700057 [details]
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/
Attachment #8700057 - Flags: review?(mratcliffe)
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?
Attachment #8700055 - Flags: review?(felipc) → review+
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?
(Assignee)

Comment 12

2 years ago
https://reviewboard.mozilla.org/r/28547/#review25381

You can activate any eslint rule with a comment tag already, /* eslint mozilla/import-browserjs-globals: 2 */
ah, neat!
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!
Attachment #8700056 - Flags: review?(mratcliffe) → review+
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.
Attachment #8700057 - Flags: review?(mratcliffe) → review+
You need to log in before you can comment on or make changes to this bug.