1252807 - Fix eslint warnings in the Net panel

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

All eslint warnings/errors in the net monitor files in the netmonitor folder should be fixed.

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1252807-har.patch

First patch fixing eslint warnings in har dir
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b3e39513e06

Honza

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1252807-panel.patch

Fix warnings for controller, view and panel.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=761b887f3cf6

Honza

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push

just har changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cc92150c0d9
all changes: https://hg.mozilla.org/try/pushloghtml?changeset=5369e8b5b577

Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1252807-har.patch

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1252807-panel.patch

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5369e8b5b577

@jryans: does the try results look good to you?

Note that there are many failures, but I've been talking to :Tomcat and we can safely ignore the Win failure (running on AWS spot machines).

Honza

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5369e8b5b577
> 
> @jryans: does the try results look good to you?
> 
> Note that there are many failures, but I've been talking to :Tomcat and we
> can safely ignore the Win failure (running on AWS spot machines).

Looks good, the failures don't seem related to the change.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8726759 [details] [diff] [review]
bug1252807-panel.patch

Patrick, tests look fine, can you review please.
Thanks!
Honza

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8726758 [details] [diff] [review]
bug1252807-har.patch

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

All code/format changes look good.
I just have a few comments about a few eslint configurations that I'd like to re-review once you change.

::: devtools/client/netmonitor/.eslintrc
@@ +1,1 @@
> +{

I don't see why this new .eslintrc is required.
First of, the root devtools/.eslintrc already defines the react plugin globally, so you don't need to re-define it here.
Secondly, the globals you added are all test related, so if they were needed, they should go in a .eslintrc file inside the test folder, *but*, they aren't needed in fact, here's why:
there's already an .eslintrc file in the test folder. This one just extends from devtools/.eslintrc.mochitests, which itself extends from testing/mochitest/browser.eslintrc.
And if you look at that file, you can see that it already defines all of these globals and more.

So this new file should just be removed.

::: devtools/client/netmonitor/har/test/browser_net_har_copy_all_as_har.js
@@ +2,5 @@
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +"use strict";
> +
> +/* import-globals-from ./head.js */

We already have a custom eslint plugin that does this for you, and it's ON by default. So you should not have to do this at all.

@@ +3,5 @@
>  
> +"use strict";
> +
> +/* import-globals-from ./head.js */
> +/* import-globals-from ../../test/head.js */

Also, please move this inside devtools/client/netmonitor/har/test/head.js
Our global importing mechanism is recursive, and so what's going to happen is that when eslint runs on this test, it will first load all globals from the local head.js, and then it will see the import-globals-from instruction inside head.js and recursively find globals from that other head.js, etc...

@@ +4,5 @@
> +"use strict";
> +
> +/* import-globals-from ./head.js */
> +/* import-globals-from ../../test/head.js */
> +/* eslint-disable mozilla/no-cpows-in-tests */

Not a big deal, but I would prefer to add
// eslint-disable-line
at the end of the lines where 'content' is used, rather than disabling this rule altogether.

::: devtools/client/netmonitor/har/test/browser_net_har_post_data.js
@@ +3,5 @@
>  
> +"use strict";
> +
> +/* import-globals-from ./head.js */
> +/* import-globals-from ../../test/head.js */

Same comments as in the previous test.

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8726759 [details] [diff] [review]
bug1252807-panel.patch

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

::: devtools/client/netmonitor/netmonitor-controller.js
@@ -6,5 @@
>  "use strict";
>  
> -var { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> -
> -const NET_STRINGS_URI = "chrome://devtools/locale/netmonitor.properties";

I'm guessing that you moved these (as well as various prefs and imports below) to netmonitor-view.js because they are used there only?
I don't know the netmonitor at all, but just a suggestion: if it makes sense to keep them in netmonitor-controller.js, then the import-globals-from comment in netmonitor-view.js should help.
What I'm saying is, I don't think the sole reason for moving this code should be to make eslint happy. If it makes more sense to keep them here, then we have mechanisms for this to work.

::: devtools/client/netmonitor/netmonitor-view.js
@@ +21,5 @@
> +const {ToolSidebar} = require("devtools/client/framework/sidebar");
> +const {Tooltip} = require("devtools/client/shared/widgets/Tooltip");
> +const DevToolsUtils = require("devtools/shared/DevToolsUtils");
> +
> +Cu.import("resource://devtools/client/shared/widgets/ViewHelpers.jsm");

If you also add this:
/* import-globals-from ../shared/widgets/ViewHelpers.jsm */
then you won't need to define all of these globals in /* globals ... */ above:
Heritage, ViewHelpers, WidgetMethods, setNamedTimeout, ...

Also, as said previously, our eslint globals importing mechanism is recursive, so even if this comment was inside netmonitor-controller.js, this would work.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1252807-har.patch

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #9)
> Comment on attachment 8726758 [details] [diff] [review]
> bug1252807-har.patch
> 
> Review of attachment 8726758 [details] [diff] [review]:
> -----------------------------------------------------------------
All fixed, thanks Patrick!

So, I created .eslintrc file in netmonitor/har/test dir, which is the
same as in netmonitor/test dir

Honza

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1252807-panel.patch

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #10)
> Comment on attachment 8726759 [details] [diff] [review]
> bug1252807-panel.patch
> 
> Review of attachment 8726759 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/netmonitor/netmonitor-controller.js
> @@ -6,5 @@
> >  "use strict";
> >  
> > -var { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> > -
> > -const NET_STRINGS_URI = "chrome://devtools/locale/netmonitor.properties";
> 
> I'm guessing that you moved these (as well as various prefs and imports
> below) to netmonitor-view.js because they are used there only?
Yes

> I don't know the netmonitor at all, but just a suggestion: if it makes sense
> to keep them in netmonitor-controller.js, then the import-globals-from
> comment in netmonitor-view.js should help.
> What I'm saying is, I don't think the sole reason for moving this code
> should be to make eslint happy. If it makes more sense to keep them here,
> then we have mechanisms for this to work.
I see. One of the reasons was that I can't import globals from nemonitor-view.js since it already imports globals from the netmonitor-controller.js (that would cause infinite loop).

> 
> ::: devtools/client/netmonitor/netmonitor-view.js
> @@ +21,5 @@
> > +const {ToolSidebar} = require("devtools/client/framework/sidebar");
> > +const {Tooltip} = require("devtools/client/shared/widgets/Tooltip");
> > +const DevToolsUtils = require("devtools/shared/DevToolsUtils");
> > +
> > +Cu.import("resource://devtools/client/shared/widgets/ViewHelpers.jsm");
> 
> If you also add this:
> /* import-globals-from ../shared/widgets/ViewHelpers.jsm */
Nice, done.

Some globals (like setTimeout, etc.) still defined.

Thanks for the tips Patrick!

Honza

> then you won't need to define all of these globals in /* globals ... */
> above:
> Heritage, ViewHelpers, WidgetMethods, setNamedTimeout, ...
> 
> Also, as said previously, our eslint globals importing mechanism is
> recursive, so even if this comment was inside netmonitor-controller.js, this
> would work.

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a23af0e0429

Honza

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8728958 [details] [diff] [review]
bug1252807-har.patch

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

::: devtools/client/netmonitor/har/test/browser_net_har_copy_all_as_har.js
@@ +8,5 @@
>   */
>  add_task(function*() {
> +  // The first 'tab' isn't necessary so, don't create a var for it
> +  // to avoid eslint warning.
> +  let [ {}, debuggee, monitor ] = yield initNetMonitor(SIMPLE_URL);

This works well too:

let [, debuggee, monitor] = yield initNetMonitor(SIMPLE_URL);

instead of using an empty object.

::: devtools/client/netmonitor/har/test/browser_net_har_post_data.js
@@ +8,5 @@
>   */
>  add_task(function*() {
> +  // The first 'tab' isn't necessary so, don't create a var for it
> +  // to avoid eslint warning.
> +  let [ {}, debuggee, monitor ] = yield initNetMonitor(

Same comment here.

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8728976 [details] [diff] [review]
bug1252807-panel.patch

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

::: devtools/client/netmonitor/netmonitor-view.js
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +/* import-globals-from ./netmonitor-controller.js */
> +/* import-globals-from ../shared/widgets/ViewHelpers.jsm */
> +/* globals gNetwork setInterval, setTimeout, clearInterval,
> +   clearTimeout btoa */

Please choose to use either space or comma to separate these globals, but not a mix of both.

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1252807-har.patch

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #14)
> Comment on attachment 8728958 [details] [diff] [review]
> bug1252807-har.patch
> 
> Review of attachment 8728958 [details] [diff] [review]:
> -----------------------------------------------------------------
Fixed

Honza

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1252807-panel.patch

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #15)
> Comment on attachment 8728976 [details] [diff] [review]
> bug1252807-panel.patch
> 
> Review of attachment 8728976 [details] [diff] [review]:
> Please choose to use either space or comma to separate these globals, but
> not a mix of both.
Fixed

Honza

Comment 18

[Pulsebot]

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec3039221fa8
https://hg.mozilla.org/mozilla-central/rev/af1e95751534