Closed
Bug 1229224
Opened 8 years ago
Closed 8 years ago
Make all mozilla plugins use the same methods for finding global variables and add a way to get globals from browser.js
Categories
(Testing :: General, defect)
Testing
General
Tracking
(firefox46 fixed)
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: mossop, Assigned: mossop)
References
Details
Attachments
(3 files, 1 obsolete file)
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•8 years ago
|
||
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 2•8 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 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•8 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•8 years ago
|
Attachment #8693881 -
Flags: review?(pbrosset) → review?(mratcliffe)
Assignee | ||
Comment 4•8 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
Comment 5•8 years ago
|
||
See also bug 1229911. This patch will need a small tweak if that happens to go in first.
Assignee | ||
Comment 6•8 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•8 years ago
|
Attachment #8693881 -
Attachment is obsolete: true
Assignee | ||
Updated•8 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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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 10•8 years ago
|
||
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+
Comment 11•8 years ago
|
||
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•8 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 */
Comment 13•8 years ago
|
||
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+
Comment 16•8 years ago
|
||
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 19•8 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•