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 `chrome/**` line. - Run eslint: ./mach eslint --fix chrome 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 commit of the work so far. - 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 chrome` to check you've fixed them). - Create a second 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.

Hey! I'am a new contributor and I've been looking for a good first bug. This issue looks like a good place to get started. Can I be assigned this bug?

Thank you for picking it up, I've assigned it to you.

Hi! I just pushed the changes to mozreview! It took me a while completely understand what was going on.I was a little overwhelmed when I saw there were 163 errors once I enabled eslint for the chrome directory but it wasn't difficult at all once I got down to it.

I just realized that the push to mozreview was aborted because I "cannot submit reviews referencing multiple bugs'. Do you know why I am getting this error?

Never mind I figured it out,I needed to reference my Bug Number in my commit messages! On MozReview it says that my request lacks reviewers because I wasn't sure if I was supposed to specify Mark as a reviewer when making the push.

(In reply to Mithilan Sivanesan from comment #3) > I just pushed the changes to mozreview! It took me a while completely > understand what was going on.I was a little overwhelmed when I saw there > were 163 errors once I enabled eslint for the chrome directory but it wasn't > difficult at all once I got down to it. Thanks, I'll take a look in a minute. Did you use --fix? That should have resolved most of those errors automatically, unless I'd misjudged it.

Comment on attachment 8921337 [details] Bug 1403956 - Enable ESLint from chrome/ directory. https://reviewboard.mozilla.org/r/192352/#review197618 ::: commit-message-19b32:1 (Diff revision 1) > +Bug 1403956 removed chrome directory from .eslintignore file This changeset can be merged into the main one, no need to keep these ones separate.

Comment on attachment 8921338 [details] Bug 1403956 resolved all no-undef, no-redeclare and no-unused-vars eslint errors https://reviewboard.mozilla.org/r/192354/#review197622 Thank you very much for working on the patch. There are a few issues that were my fault as I'd forgotten to mention what to do when I first submitted the bug. It shouldn't be hard to change them though, and most of the hard work appears to be done already. I'm ok to review this, since it appears to be classed as part of toolkit and is test-only anyway. Next time you push a patch, just add " r?Standard" in the commit message and it will request review from me automatically. ::: commit-message-3f306:1 (Diff revision 1) > +Bug 1403956 resolved all no-undef, no-redeclare and no-unused-vars eslint errors For the commit message, I'd go with something like: Bug 1403956 - Enable ESLint from chrome/ directory. r?Standard8 That's descriptive enough to tell people what is going on in the patch and where it is targeted to apply to. ::: .eslintrc.js:10 (Diff revision 1) > // tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js to > // allow external repositories that use the plugin to pick them up as well. > "extends": [ > - "plugin:mozilla/recommended" > + "plugin:mozilla/recommended", > + "plugin:mozilla/chrome-test", > + "plugin:mozilla/xpcshell-test" Sorry, I forgot to mention this when I filed the bug - you should only need the xpcshell-test entry, but it should also go into a new .eslintrc.js file in the chrome/test/ sub-directory, here's an existing example: http://searchfox.org/mozilla-central/source/browser/components/places/tests/unit/.eslintrc.js ::: .eslintrc.js:23 (Diff revision 1) > // turn off processing of the html plugin for .xml files. > "settings": { > "html/xml-extensions": [ ".xhtml" ] > }, > + "rules": { > + "no-native-reassign": ["error", {"exceptions": ["run_test"]}] This shouldn't be necessary. I suspect if you relocate the extends definitions I suggested it will go away. ::: browser/components/about/test/unit/test_getURIFlags.js:10 (Diff revision 1) > const contract = "@mozilla.org/network/protocol/about;1?what=newtab"; > const am = Cc[contract].getService(Ci.nsIAboutModule); > const uri = Services.io.newURI("about:newtab"); > > +/*eslint no-native-reassign: "error"*/ > +/*eslint-env browser*/ As above, neither of these comments should need to be here, it should be getting everything inherited in the right places. ::: chrome/test/unit/test_bug564667.js:50 (Diff revision 1) > * the manifest files > * > * @param type The type of overlay: overlay|style > */ > function test_no_overlays(chromeURL, target, type) { > - var type = type || "overlay"; > + type = type || "overlay"; We can improve on this here. You can drop this `type = type || "overlay";` line and change the function definition to: function test_no_overlays(chromeURL, target, type = "overlay") { ::: chrome/test/unit/test_data_protocol_registration.js:15 (Diff revision 1) > > -function run_test() > +function run_test() { > -{ > const uuidGenerator = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator); > > Components.utils.import("resource://testing-common/AppInfo.jsm", this); Rather than adding the `globals newAppInfo` line above, please can you change the import line to: let newAppInfo = Components.utils.import("resource://testing-common/AppInfo.jsm", {}).newAppInfo; That's then an explicit description of what is being imported. Please also do this in the other files where you're currently specifying newAppInfo as a global. ::: chrome/test/unit_ipc/test_resolve_uris_ipc.js:4 (Diff revision 1) > // > // Run test script in content process instead of chrome (xpcshell's default) > // > - > +/* globals do_run_test */ Can you move the comment to just inside the run_test() please - just before the load() line. That is where the global is imported from in this case, so I prefer to keep the comment & loading line close together.

Once the issues are resolved, Do I make another commit and push to MozReview again?

(In reply to Mithilan Sivanesan from comment #11) > Once the issues are resolved, Do I make another commit and push to MozReview > again? Please fold them all into one commit with the commit message I suggested. Then push to MozReview again :-)

I've resolved all the errors except one. After I've relocated the extends definition to a new .eslintrc.js file in the chrome/test subdirectory, I'am still getting a no-native-reassign error for the run_test() function. Adding a rule exception resolves the error but I'am not sure if this is the best way to go about it.Any ideas?

(In reply to Mithilan Sivanesan from comment #13) > I've resolved all the errors except one. After I've relocated the extends > definition to a new .eslintrc.js file in the chrome/test subdirectory, I'am > still getting a no-native-reassign error for the run_test() function. Adding > a rule exception resolves the error but I'am not sure if this is the best > way to go about it.Any ideas? I just took a look, it seems this is a one-off special case due to how chrome/test/unit_ipc/test_resolve_uris_ipc.js works. I think you can just add: // eslint-disable-next-line no-native-reassign just above where it assigns to run_test in test_resolve_uris.js.

Comment on attachment 8921910 [details] Bug 1403956 - Enable ESLint from chrome/ directory. https://reviewboard.mozilla.org/r/192942/#review198496 Thank you for the new update. For some reason, it is showing as multiple patches still. You might need to check what you're pushing with `hg outgoing`. If you need to merge the commits, there's some hints here for how to do it: https://stackoverflow.com/posts/1559922/revisions ::: browser/.eslintrc.js:5 (Diff revision 1) > "use strict"; > > module.exports = { > + "extends": [ > + "plugin:mozilla/xpcshell-test", This change shouldn't be here. I'm also not seeing the new chrome/test/.eslintrc.js file, did you forget to `hg add` it?

Comment on attachment 8921337 [details] Bug 1403956 - Enable ESLint from chrome/ directory. https://reviewboard.mozilla.org/r/192352/#review198728 Thank you, this is much better. There's a few small tweaks just to finish this off. Could you also rebase on top of the latest mozilla-central? There's been a small change to .eslintignore and we may or may not be able to get mozreview to land the patch without updating - so might as well rebase to be safe and avoid another update cycle. Just rebase & add the changes and keep the one changeset please. ::: .eslintrc.js:8 (Diff revision 2) > module.exports = { > // New rules and configurations should generally be added in > // tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js to > // allow external repositories that use the plugin to pick them up as well. > "extends": [ > - "plugin:mozilla/recommended" > + "plugin:mozilla/recommended", Can you drop the comma you added to this line please? We don't need it for this change and prefer to keep the blame minimal. ::: .eslintrc.js:13 (Diff revision 2) > - "plugin:mozilla/recommended" > + "plugin:mozilla/recommended", > ], > "plugins": [ > "mozilla" > ], > + Also undo the addition of this line. ::: browser/components/about/test/unit/.eslintrc.js:5 (Diff revision 2) > "use strict"; > > module.exports = { > "extends": [ > - "plugin:mozilla/browser-test" > + "plugin:mozilla/browser-test", Please also drop the comma here.

Will do! I really appreciate your patience and help through all this :-)

Comment on attachment 8921337 [details] Bug 1403956 - Enable ESLint from chrome/ directory. https://reviewboard.mozilla.org/r/192352/#review198964 Great, many thanks for the updates. This looks good to go.

Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3c2982568a78 Enable ESLint from chrome/ directory. r=standard8

(In reply to Pulsebot from comment #22) > Pushed by mbanner@mozilla.com: > https://hg.mozilla.org/integration/autoland/rev/3c2982568a78 > Enable ESLint from chrome/ directory. r=standard8 Mithilan: as you'll see from above, this has been landed on our integration branch. If the tests all pass (which they should do), then this will probably be merged to mozilla-central (the main branch) sometime in the next 24 hours, at which point the bug will be marked as fixed. Thank you for you work on this.