Closed Bug 1252807 Opened 4 years ago Closed 4 years ago

Fix eslint warnings in the Net panel

Categories

(DevTools :: Netmonitor, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

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

Honza
Attached patch bug1252807-har.patch (obsolete) — Splinter Review
First patch fixing eslint warnings in har dir
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b3e39513e06

Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Attached patch bug1252807-har.patch (obsolete) — Splinter Review
Attachment #8725663 - Attachment is obsolete: true
Attached patch bug1252807-panel.patch (obsolete) — Splinter Review
Attachment #8725734 - Attachment is obsolete: true
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
Flags: needinfo?(jryans)
(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.
Flags: needinfo?(jryans)
Attachment #8726758 - Flags: review?(pbrosset)
Comment on attachment 8726759 [details] [diff] [review]
bug1252807-panel.patch

Patrick, tests look fine, can you review please.
Thanks!
Honza
Attachment #8726759 - Flags: review?(pbrosset)
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.
Attachment #8726758 - Flags: review?(pbrosset)
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.
Attachment #8726759 - Flags: review?(pbrosset)
Attached patch bug1252807-har.patch (obsolete) — Splinter Review
(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
Attachment #8726758 - Attachment is obsolete: true
Attachment #8728958 - Flags: review?(pbrosset)
Attached patch bug1252807-panel.patch (obsolete) — Splinter Review
(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.
Attachment #8728976 - Flags: review?(pbrosset)
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.
Attachment #8728958 - Flags: review?(pbrosset) → review+
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.
Attachment #8728976 - Flags: review?(pbrosset) → review+
(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
Attachment #8726759 - Attachment is obsolete: true
Attachment #8728958 - Attachment is obsolete: true
Attachment #8728989 - Flags: review+
(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
Attachment #8728976 - Attachment is obsolete: true
Attachment #8728991 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ec3039221fa8
https://hg.mozilla.org/mozilla-central/rev/af1e95751534
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.