Closed Bug 1403961 Opened 3 years ago Closed 3 years ago

Enable ESLint for ipc/

Categories

(Core :: IPC, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: standard8, Assigned: mccr8, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(2 files)

As part of rolling out ESLint across the tree, we should enable it for the ipc/ directory.

I'm happy to mentor this bug. There's background on our eslint setups here:

https://developer.mozilla.org/docs/ESLint

Here's some approximate steps:

- In .eslintignore, remove the `ipc/**` line.
- Run eslint:

./mach eslint --fix ipc

This should fix most/all of the issues.

- Inspect the diff to make sure that the indentation of the lines surrounding the changes look ok.

- For the remaining issues, you'll need to fix them by hand. I think most of these will be no-undef, there are hints on how to fix those here:

https://developer.mozilla.org/en-US/docs/ESLint#no-undef

(you can just run `./mach eslint ipc` to check you've fixed them).

- Create commit of the changes and push it to mozreview:

http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html

If you're not sure of how to fix something, please look at the rules documentation http://eslint.org/docs/rules/ or just ask here.
Assignee: nobody → continuation
This is the second part of the diff that seems to have broken MozReview:

diff --git a/ipc/testshell/tests/test_ipcshell_child.js b/ipc/testshell/tests/test_ipcshell_child.js
index d9c9fb6..2a0566b 100644
--- a/ipc/testshell/tests/test_ipcshell_child.js
+++ b/ipc/testshell/tests/test_ipcshell_child.js
@@ -3,7 +3,6 @@ var Ci = Components.interfaces;
 
 const runtime = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime);
 
-if (typeof(run_test) == "undefined") {
-  run_test = function() {
-    do_check_eq(runtime.processType, Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT);
-  }^M}
+function run_test() {
+  do_check_eq(runtime.processType, Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT);
+}

I guess it didn't like the ^M, whatever that is.
(In reply to Andrew McCreight (PTO-ish Oct 1 - 12) [:mccr8] from comment #4)
> -  }^M}
> +function run_test() {
> +  do_check_eq(runtime.processType, Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT);
> +}
> 
> I guess it didn't like the ^M, whatever that is.

The ^M is the representation of the carriage return - there were mixed line-endings in the child file.
Comment on attachment 8913843 [details]
Bug 1403961, part 1 - Don't load test_ipcshell_child.js from test_ipcshell.js.

https://reviewboard.mozilla.org/r/185230/#review190490

::: ipc/testshell/tests/test_ipcshell.js
(Diff revision 1)
>                do_check_eq(cmp, result);
>                do_test_finished(); 
>            });
>      })
>  }
> -load('test_ipcshell_child.js');

One alternative here would be to do:

/* import-globals-from test_ipcshell_child.js */

ESLint will then detect that these are loaded from the child script.
(In reply to Mark Banner (:standard8) from comment #6)
> One alternative here would be to do:

The first fix I had was to add an annotation along those lines, but I decided I might as well take the opportunity to clean up a garbled test. (test_ipcshell_child.js used to do more, so maybe it used to make sense.)
Comment on attachment 8913843 [details]
Bug 1403961, part 1 - Don't load test_ipcshell_child.js from test_ipcshell.js.

https://reviewboard.mozilla.org/r/185230/#review190752
Attachment #8913843 - Flags: review?(wmccloskey) → review+
Comment on attachment 8913844 [details]
Bug 1403961, part 2 - Fix and enable eslint for ipc/.

https://reviewboard.mozilla.org/r/185232/#review190754
Attachment #8913844 - Flags: review?(wmccloskey) → review+
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6201576b6471
part 1 - Don't load test_ipcshell_child.js from test_ipcshell.js. r=billm
https://hg.mozilla.org/integration/autoland/rev/cab06f0f811a
part 2 - Fix and enable eslint for ipc/. r=billm
You need to log in before you can comment on or make changes to this bug.