1242689 - Fix eslint errors in devtools/client/*.js

Status: RESOLVED FIXED

Description

[Patrick Brosset <:pbro>]

devtools/client/definitions.js
devtools/client/devtools-clhandler.js
devtools/client/main.js

Comment 1

[Patrick Brosset <:pbro>]

Created attachment 8711842 [details]
MozReview Request: Bug 1242689 - Fix eslint errors in devtools/client/*.js and un-ignore the files; r=tromey

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

Comment 2

[Patrick Brosset <:pbro>]

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 3

[Tom Tromey :tromey]

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).

Comment 4

[Patrick Brosset <:pbro>]

(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.

Comment 5

[Patrick Brosset <:pbro>]

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.

Comment 6

[Pulsebot]

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

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cdf3a74d6bc0