Closed Bug 1242689 Opened 5 years ago Closed 5 years ago
Fix eslint errors in devtools/client/*.js
MozReview Request: Bug 1242689 - Fix eslint errors in devtools/client/*.js and un-ignore the files; r=tromey
58 bytes, text/x-review-board-request
devtools/client/definitions.js devtools/client/devtools-clhandler.js devtools/client/main.js
Review commit: https://reviewboard.mozilla.org/r/32351/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/32351/
Attachment #8711842 - Flags: review?(ttromey)
Partial try push with every job but ESLint canceled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69523d27e5f9&selectedJob=15910657 So, yay, ESLint still passes!
Comment on attachment 8711842 [details] MozReview Request: Bug 1242689 - Fix eslint errors in devtools/client/*.js and un-ignore the files; r=tromey https://reviewboard.mozilla.org/r/32351/#review29013 Looks good. ::: devtools/client/devtools-clhandler.js:4 (Diff revision 1) > +/* globals BrowserToolboxProcess */ I don't mind this use of globals but I wonder if the import-browserjs-globals rule could be used somehow instead (assuming it is in fact appropriate to use).
(In reply to Tom Tromey :tromey from comment #3) > Comment on attachment 8711842 [details] > MozReview Request: Bug 1242689 - Fix eslint errors in devtools/client/*.js > and un-ignore the files; r=tromey > > https://reviewboard.mozilla.org/r/32351/#review29013 > > Looks good. > > ::: devtools/client/devtools-clhandler.js:4 > (Diff revision 1) > > +/* globals BrowserToolboxProcess */ > > I don't mind this use of globals but I wonder if the > import-browserjs-globals rule could be used somehow instead (assuming it is > in fact appropriate to use). I thought about that too. I'll need to dive into how this module is loaded better though. I don't see where this global comes from. Thanks for the review. I'll try to fix this and land the patch then.
Somehow, I had completely miss this import line in devtools-clhandler.js: Cu.import("resource://devtools/client/framework/ToolboxProcess.jsm"); So, that's where the BrowserToolboxProcess global comes from. So, in theory, we could use /* import-globals-from */ instead of /* globals */ but the problem is that this jsm module defines BrowserToolboxProcess with `this.BrowserToolboxProcess`, so it's not defined as a global, but as a property of `this`. This means that the import-globals-from plugin doesn't register it as a global. After discussing with :ochameau, I learned that most jsm modules do this so they can run on b2g (where the scope jsms are loaded in is shared). So I can't really change ToolboxProcess.jsm and remove the `this` part. So, we can either keep the /* globals */ comment or evolve the import-globals-from plugin so it registers properties stored on the top-level `this` in jsms as globals too. Doing this belongs to a separate bug anyway, so I'm going to go ahead and land this change and file a separate bug for this: bug 1242893.
You need to log in before you can comment on or make changes to this bug.