1245496 - Make about:debugging pass eslint

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect)

Description

[Jan Keromnes [:janx]]

Steps to reproduce:
1. Remove the line "devtools/client/aboutdebugging/**" in mozilla-central/.eslintignore
2. Run `./mach eslint --setup`
3. Run `./mach eslint devtools/client/aboutdebugging/`

Expected result:
4. There should be no errors in the code.

Actual result:
4. There are 22 errors in the code.

It would be nice to write a patch that fixes all these errors and removes the line from step 1, in order to enable automated eslint tests for about:debugging.

However, we could also do this step by step, and start by fixing a few errors at first.

I can provide help all the way from writing and uploading a patch to bugzilla, to code review and merging patches into Firefox.

Comment 1

[Jan Keromnes [:janx]]

Attachment: Full ESLint error log as of 2016-02-03

Comment 2

[Jan Keromnes [:janx]]

Attachment: Full ESLint error log as of 2016-02-03

Note: Sometimes, the right way to deal with a problem is to make ESLint ignore it.

If you're unsure about what to do, please use the "Need more information" checkbox on me.

Comment 3

[Joe Whitfield-Seed]

Attachment: fix eslint errors in about:debugging

Comment 4

[Joe Whitfield-Seed]

[first timer] I have worked through this (hope I got the hg stuff right). The patch fixes errors and warnings except 3 that I don't know how/if to deal with:

> warning content is a possible Cross Process Object Wrapper (CPOW)

xpcshell tests pass but I have not had time to run mochitests yet. Should I run those locally? or can they run on the try server?

Could I get some direction and/or feedback? Thanks!

Comment 5

[Jan Keromnes [:janx]]

Hi Joe, thanks a lot for looking into this! I will now review your patch and submit it to try.

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8715545 [details] [diff] [review]
fix eslint errors in about:debugging

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

The patch looks good and works great! (Even with `./mach mochitest devtools/client/aboutdebugging/test/`).

I took the liberty of sending it to try for you: https://treeherder.mozilla.org/#/jobs?repo=try&revision=168ee7bf131e

Please address all my comments, then re-upload your new patch, and set the "review" flag on "?" to myself.

(In reply to Joe Whitfield-Seed from comment #4)
> [first timer] I have worked through this (hope I got the hg stuff right).

You got everything right, congratulations on uploading your first patch!

> The patch fixes errors and warnings except 3 that I don't know how/if to
> deal with:
> 
> > warning content is a possible Cross Process Object Wrapper (CPOW)

I believe these CPOW warnings are false positives, because the infringing use of "content.*" lives in a frameScript that doesn't actually run inside the test, but gets loaded into the targeted content page. You can simply tell ESLint to ignore them by adding "/* eslint-disable no-cpows-in-tests */" to the relevant files.

> Bug 1245496 - fix eslint errors in about:debugging

Nit: Please format the patch title with "r=janx" at the end, like so:
Bug 1245496 - Fix eslint errors in about:debugging. r=janx

::: devtools/client/aboutdebugging/test/addons/unpacked/bootstrap.js
@@ +1,3 @@
> +/* eslint-env browser */
> +/* exported startup, shutdown, install, uninstall */
> +"use strict";

Nit: Please separate the comments from the "use strict" with an empty line. Also, please add the "Public Domain" comment at the top (we have it in all our test files, but it looks like we forgot it here).

::: devtools/client/aboutdebugging/test/browser_service_workers.js
@@ +1,4 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/* global sendAsyncMessage */

Nit: Please separate this comment from "use strict" with an empty line.

::: devtools/client/aboutdebugging/test/browser_service_workers_timeout.js
@@ +1,4 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/* global sendAsyncMessage */

Nit: Please separate this comment from "use strict" with an empty line.

::: devtools/client/aboutdebugging/test/head.js
@@ +40,5 @@
>    info("Closing about:debugging");
>    return removeTab(tab);
>  }
>  
> +function addTab(url, targetWindow = window) {

Nit: Cool usage of default parameters! However, I believe this doesn't do exactly the same thing as before: For example, if the second parameter is `null`, previously we would have `targetWindow === window`, but with this code we now have `targetWindow === null` (I don't actually know if there parameter can ever be `null` though).

Please convince me that the second parameter can never be `null` or `false` or otherwise falsy, or else please reinstate the `targetWindow = aWindow || window` OR-assignment (I suppose the problem was with the "aWindow" notation, in that case you can just call it "win" instead).

@@ +58,5 @@
>      }, true);
>    });
>  }
>  
> +function removeTab(tab, targetWindow = window) {

Nit: Same comment as above about the default parameter.

Comment 7

[Joe Whitfield-Seed]

Attachment: fix eslint errors in about:debugging

Comment 8

[Joe Whitfield-Seed]

Comment on attachment 8715983 [details] [diff] [review]
fix eslint errors in about:debugging

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

Thanks for your comments!

> Please convince me that the second parameter can never be `null` or `false` or otherwise falsy, or else please reinstate the `targetWindow = aWindow || window` OR-assignment (I suppose the problem was with the "aWindow" notation, in that case you can just call it "win" instead).

The second parameter is not passed to either function in the `aboutdebugging` tests, but that pattern is used in several other test cases. I've used `win || window` for consistency.

Comment 9

[Jan Keromnes [:janx]]

Comment on attachment 8715983 [details] [diff] [review]
fix eslint errors in about:debugging

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

Nice, the new patch is perfect, thanks!

Comment 10

[Jan Keromnes [:janx]]

I'm using the `checkin-needed` keyword to notify the Sheriffs that this patch is ready to be merged.

The try push looks encouraging: https://treeherder.mozilla.org/#/jobs?repo=try&revision=168ee7bf131e&selectedJob=16318682

(There were a few orange jobs, but they all looked unrelated to your patch, so I've re-triggered them).

Comment 11

[Jan Keromnes [:janx]]

New try FYI: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31d41547bad5

Comment 12

[Pulsebot]

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

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 14

[Jan Keromnes [:janx]]

Joe, your patch has successfully been merged into mozilla-central, the soure repository of Firefox -- congratulations! And thank you for fixing all these problems.

Your patch will be part of the Firefox 47 cycle (currently in Nightly), which will become Developer Edition (Aurora) next month, then Beta 6 weeks later, before being released to every Firefox user around the world in about 4 months from now.