1240183 - make styleeditor eslint-clean

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Description

[Tom Tromey :tromey]

Remove all the eslint warnings from the styleeditor.

I gave this a try today; so taking it.

It's not totally possible to do this yet.  There's at least bug 1240167.

Comment 1

[Tom Tromey :tromey]

Attachment: make styleeditor eslint-clean

Comment 2

[Tom Tromey :tromey]

Comment on attachment 8711059 [details] [diff] [review]
make styleeditor eslint-clean

This comes as close as is reasonably possible to making the style editor eslint-clean.

Most of the patch is just the usual sort of trivia.

The various /* globals */ comments are needed due to bug 1240068
(which won't be fixed).

The "minMaxPattern" stuff in StyleEditorUI.jsm is a bit ugly perhaps.

The tests were the hardest part to clean.  In fact, I didn't totally succeed,
there are still two warnings from head.js:

/home/tromey/firefox-git/gecko/devtools/client/styleeditor/test/head.js
  23:6  error  "console" is read only  no-undef
  24:5  error  "promise" is read only  no-undef


The problem here is that some style editor tests do this:

Services.scriptloader.loadSubScript("chrome://mochitests/content/browser/devtools/client/inspector/shared/test/head.js", this);


If we make "console" and "promise" const, like eslint wants, then this load tries
to modify these and fails.

I tried several things to get around this, all without success:

* Make the style editor's head.js use shared-head.js.  This fails in a different way,
  when the subscript load of the inspector head.js tries to load shared-head.js a second time.
  I think loading shared-head.js via a subscript load is problematic.

* Make the style editor's head.js always load the inspector head.js.
  I forget now why this failed, but it also seems somewhat unclean.

* Load the inspector's head.js into a new object.  This, bizarrely, causes timeouts in
  some tests.  I didn't debug this.  Also this is ugly because it requires passing
  in a bunch of globals to the subscript load, like:

  let result = {
    waitForExplicitFinish,
    gTestPath,
    registerCleanupFunction,
    info,
    waitForFocus,
    EventUtils
  };
  Services.scriptloader.loadSubScript("chrome://mochitests/content/browser/devtools/client/inspector/shared/test/head.js", result);

  ... and this list is created just by trying things and seeing what breaks.


Note finally that this patch copies a bit of code from the inspector, in particular
some frame script stuff.  This removes some warnings about CPOWs.  Sharing this code
would be nice but, I think, runs into the same sorts of issues as the above.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8711059 [details] [diff] [review]
make styleeditor eslint-clean

Review of attachment 8711059 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for cleaning this up.
I'd like to avoid using /* globals */ instructions as much as possible. Please take a look at my comments below about StyleEditorUtil.jsm and see if import-from-globals could be used instead.

::: devtools/client/styleeditor/StyleEditorUI.jsm
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* globals wire, text, showFilePicker, _ */

So I'm wondering about something, but I might just not know enough about *.jsm files.
If StyleEditorUtil.jsm wasn't adding its globals to 'this', but defining them globally instead, then we'd be able to use the import-globals-from eslint plugin instead of having to maintain this list of globals manually.

::: devtools/client/styleeditor/StyleEditorUtil.jsm
@@ +33,5 @@
>   * @param ...rest
>   *        Optional arguments to format in the string.
>   * @return string
>   */
> +this._ = function _(name) {

As commented earlier, I do think that it's possible to replace this._ with const _ and therefore have import-from-globals work.

::: devtools/client/styleeditor/styleeditor-panel.js
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* globals _ */

omg, this is awful. We should just rename this _ to something that actually makes sense.

@@ +21,1 @@
>  Cu.import("resource://devtools/client/styleeditor/StyleEditorUtil.jsm");

Isn't _ the only global symbol used here? This import-globals-from comment doesn't seem required.

::: devtools/client/styleeditor/test/browser_styleeditor_cmd_edit.html
@@ +4,5 @@
>       http://creativecommons.org/publicdomain/zero/1.0/ -->
>  <head>
>    <meta charset="utf-8">
>    <title>Resources</title>
>    <script type="text/javascript" id="script1">

You're using let instead of var in this script, don't you require
type="application/javascript;version=1.8"
on the <script> tag for this to work?

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Tom Tromey :tromey from comment #2)
> The tests were the hardest part to clean.  In fact, I didn't totally succeed,
> there are still two warnings from head.js:
> 
> /home/tromey/firefox-git/gecko/devtools/client/styleeditor/test/head.js
>   23:6  error  "console" is read only  no-undef
>   24:5  error  "promise" is read only  no-undef
Have you tried simply removing these 2 lines?
I don't think you need to import these at all.
At least the error about "promise" comes from this file:
testing\mochitest\browser.eslintrc
It declares "promise" as being a global already (and that file is inherited from in devtools\.eslintrc.mochitests which, in turn, is inherited from in devtools\client\styleeditor\test\.eslintrc).
So the no-undef rule complains about re-defining "promise" since it's already a global.

I think it also complains about "console" for a similar reason.

Comment 5

[Tom Tromey :tromey]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #4)
> (In reply to Tom Tromey :tromey from comment #2)
> > The tests were the hardest part to clean.  In fact, I didn't totally succeed,
> > there are still two warnings from head.js:
> > 
> > /home/tromey/firefox-git/gecko/devtools/client/styleeditor/test/head.js
> >   23:6  error  "console" is read only  no-undef
> >   24:5  error  "promise" is read only  no-undef
> Have you tried simply removing these 2 lines?
> I don't think you need to import these at all.
> At least the error about "promise" comes from this file:
> testing\mochitest\browser.eslintrc
> It declares "promise" as being a global already (and that file is inherited
> from in devtools\.eslintrc.mochitests which, in turn, is inherited from in
> devtools\client\styleeditor\test\.eslintrc).
> So the no-undef rule complains about re-defining "promise" since it's
> already a global.

Somehow I missed this :(

But removing these doesn't work at least for promise:

883 INFO TEST-UNEXPECTED-FAIL | devtools/client/styleeditor/test/browser_styleeditor_sv_resize.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/styleeditor/test/head.js:43 - ReferenceError: promise is not defined
Stack trace:
    addTab@chrome://mochitests/content/browser/devtools/client/styleeditor/test/head.js:43:7
    openStyleEditorForURL<@chrome://mochitests/content/browser/devtools/client/styleeditor/test/head.js:102:19
    @chrome://mochitests/content/browser/devtools/client/styleeditor/test/browser_styleeditor_sv_resize.js:14:31
    Tester_execTest@chrome://mochikit/content/browser-test.js:803:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:723:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:735:59
    Tester_execTest@chrome://mochikit/content/browser-test.js:803:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:723:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:735:59


So perhaps browser.eslintrc is mistaken on this point.
I will try to find out.

Comment 6

[Tom Tromey :tromey]

My belief is that this line is incorrect:

https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/browser.eslintrc#29

... because I think testing/mochitest/browser-test.js defines "Promise", not "promise".

Dave, does this seem correct to you?
If so I will fix up browser.eslintrc.

Comment 7

[Dave Townsend [:mossop]]

(In reply to Tom Tromey :tromey from comment #6)
> My belief is that this line is incorrect:
> 
> https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/browser.
> eslintrc#29
> 
> ... because I think testing/mochitest/browser-test.js defines "Promise", not
> "promise".
> 
> Dave, does this seem correct to you?
> If so I will fix up browser.eslintrc.

eslint should already define Promise for the environment so we probably just don't need that line. I think I just copied it from devtool's original definitions. Does anything fail if you get rid of it? If not then by all means drop it.

Comment 8

[Tom Tromey :tromey]

Attachment: make styleeditor eslint-clean

Address review comments.

Comment 9

[Tom Tromey :tromey]

Comment on attachment 8711871 [details] [diff] [review]
make styleeditor eslint-clean

I tried to address all the review comments.

/* globals */ is now only used in one place.  In one spot I changed how a Cu.import was done in order to simplify this; in particular look for

-Cu.import("resource://gre/modules/osfile.jsm");
+const { TextDecoder, OS } = Cu.import("resource://gre/modules/osfile.jsm", {});

I changed "this.mumble" to just plain old "function mumble".

The .html patch worked fine previously, but I added the version= bits just for clarity.

I fixed the promise and console things, including a fix for browser.eslintrc.

I did not rename "_".  I can do that if you want; though TBH the name was always clear to me, as this is a common name for the string-translation function.

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8711871 [details] [diff] [review]
make styleeditor eslint-clean

Review of attachment 8711871 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/styleeditor/StyleEditorUtil.jsm
@@ +36,5 @@
>   * @param ...rest
>   *        Optional arguments to format in the string.
>   * @return string
>   */
> +function _(name) {

So I learned today that most JSM modules define their globals on `this` because that's the only way to make them work on b2g. This JSM doesn't need however, so we're fine. I'm suspecting that it used `this` in order to be consistent with the rest of the code.
We don't need to care about this now, since we want to eventually change this to a normal js file.

Comment 11

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6711c2c87fbc

Comment 12

[Pulsebot]

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7428a32a3346