As part of rolling out ESLint across the tree, we should enable it for the chrome/ 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 `xpcom/**` line. - Run eslint: ./mach eslint xpcom - In the output there will be two files which fail due to a parsing error (unexpected token if), to fix them, look at the files, and change instances of: try { ... } catch (e if (e instanceof Ci.nsIException && e.result == Cr.NS_ERROR_FAILURE)) { } into: try { ... } catch (e) { if (!(e instanceof Ci.nsIException && e.result == Cr.NS_ERROR_FAILURE)) { throw e; } } - Commit those changes. - Run eslint: ./mach eslint --fix xpcom 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. - Create a second commit with these automatic fixes. - 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 xpcom` to check you've fixed them). - Create a third commit of the manual 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.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e639eb53343426b1b6ec736b93f456376e37c36

Comment on attachment 8913849 [details] Bug 1403959, part 1 - Remove non-standard conditional catch clause. https://reviewboard.mozilla.org/r/185244/#review190708 Can you file a bug on the JS engine for removing this exception filtering syntax? Maybe they're already aware of it, maybe not, but it seems like the sort of thing they'd like to get rid of.

Comment on attachment 8913850 [details] Bug 1403959, part 2 - Automatically generated eslint fixes. https://reviewboard.mozilla.org/r/185246/#review190710 rs=me

Comment on attachment 8913851 [details] Bug 1403959, part 3 - Manually fix some xpcom/ eslint failures and enable it. https://reviewboard.mozilla.org/r/185248/#review190712 Some comments below, but feel free to tell me why I'm wrong. ::: xpcom/tests/unit/test_seek_multiplex.js:20 (Diff revision 1) > - var BinaryInputStream = Components.Constructor("@mozilla.org/binaryinputstream;1", > + Components.Constructor("@mozilla.org/binaryinputstream;1", > - "nsIBinaryInputStream"); > + "nsIBinaryInputStream"); > - var BinaryOutputStream = Components.Constructor("@mozilla.org/binaryoutputstream;1", > + Components.Constructor("@mozilla.org/binaryoutputstream;1", > - "nsIBinaryOutputStream", > + "nsIBinaryOutputStream", > - "setOutputStream"); > + "setOutputStream"); Why are we even doing these? It doesn't look to me like the constructors are actually used. ::: xpcom/tests/unit/test_seek_multiplex.js:144 (Diff revision 1) > - var BinaryInputStream = Components.Constructor("@mozilla.org/binaryinputstream;1", > + Components.Constructor("@mozilla.org/binaryinputstream;1", > - "nsIBinaryInputStream"); > + "nsIBinaryInputStream"); > - var BinaryOutputStream = Components.Constructor("@mozilla.org/binaryoutputstream;1", > + Components.Constructor("@mozilla.org/binaryoutputstream;1", > - "nsIBinaryOutputStream", > + "nsIBinaryOutputStream", > - "setOutputStream"); > + "setOutputStream"); Likewise for these. ::: xpcom/tests/unit/test_storagestream.js:27 (Diff revision 1) > function test1() { > var ss = Cc["@mozilla.org/storagestream;1"] > .createInstance(Ci.nsIStorageStream); > ss.init(1024, 1024, null); > > - var out = ss.getOutputStream(0); > + ss.getOutputStream(0); For the instances in this file, is it possible to just tell eslint to ignored the unused variables? Seems pretty weird to call the getters/readers and not do anything with the result.

(In reply to Nathan Froyd [:froydnj] from comment #5) > Can you file a bug on the JS engine for removing this exception filtering > syntax? Maybe they're already aware of it, maybe not, but it seems like the > sort of thing they'd like to get rid of. Good idea. I filed bug 1405098. (In reply to Nathan Froyd [:froydnj] from comment #7) > Why are we even doing these? It doesn't look to me like the constructors > are actually used. I figured it was easier to delete the declaration than figure out whether the constructor had some side effect. I can figure that out. IIRC, these were in since the initial landing of the test, and also unused there. > > - var out = ss.getOutputStream(0); > > + ss.getOutputStream(0); > > For the instances in this file, is it possible to just tell eslint to > ignored the unused variables? Seems pretty weird to call the > getters/readers and not do anything with the result. Again, I wasn't sure if there was some side effect here that mattered, or I would have just deleted the call entirely. Having an unused declaration plus an annotation that it is unused seems weirder to me than ignoring the return value, but I can do it that way if you prefer.

(In reply to Andrew McCreight (PTO-ish Oct 1 - 12) [:mccr8] from comment #8) > (In reply to Nathan Froyd [:froydnj] from comment #5) > > Can you file a bug on the JS engine for removing this exception filtering > > syntax? Maybe they're already aware of it, maybe not, but it seems like the > > sort of thing they'd like to get rid of. > > Good idea. I filed bug 1405098. Thanks. > (In reply to Nathan Froyd [:froydnj] from comment #7) > > Why are we even doing these? It doesn't look to me like the constructors > > are actually used. > > I figured it was easier to delete the declaration than figure out whether > the constructor had some side effect. I can figure that out. IIRC, these > were in since the initial landing of the test, and also unused there. Eyeballing them suggests that they are unused, but maybe there's some action at a distance...? > > > - var out = ss.getOutputStream(0); > > > + ss.getOutputStream(0); > > > > For the instances in this file, is it possible to just tell eslint to > > ignored the unused variables? Seems pretty weird to call the > > getters/readers and not do anything with the result. > > Again, I wasn't sure if there was some side effect here that mattered, or I > would have just deleted the call entirely. Having an unused declaration plus > an annotation that it is unused seems weirder to me than ignoring the return > value, but I can do it that way if you prefer. I don't know about the side effects either, I was assuming the call was done to make sure we didn't throw. Your guess is as good as mine is here.

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6fa8c33befbb part 1 - Remove non-standard conditional catch clause. r=froydnj https://hg.mozilla.org/integration/autoland/rev/6a43cd7df85e part 2 - Automatically generated eslint fixes. r=froydnj https://hg.mozilla.org/integration/autoland/rev/4bdb274daa63 part 3 - Manually fix some xpcom/ eslint failures and enable it. r=froydnj

https://hg.mozilla.org/mozilla-central/rev/6fa8c33befbb https://hg.mozilla.org/mozilla-central/rev/6a43cd7df85e https://hg.mozilla.org/mozilla-central/rev/4bdb274daa63