+++ This bug was initially created as a clone of Bug #1329182 +++ From bug 1333324 comment #19, ::: browser/base/content/browser-thumbnails.js:96 (Diff revision 1) > }, > > _capture: function Thumbnails_capture(aBrowser) { > // Only capture about:newtab top sites. > - if (this._topSiteURLs.indexOf(aBrowser.currentURI.spec) == -1) > + if (!aBrowser.currentURI || > + this._topSiteURLs.indexOf(aBrowser.currentURI.spec) == -1) Is foo.indexOf(bar) == -1 a candidate for a scripted rewrite to !foo.includes(bar)?

Comment on attachment 8837348 [details] Bug 1339461 - Implement new eslint rule to prefer array.includes over array.indexOf comparison with -1. https://reviewboard.mozilla.org/r/112502/#review113906 ::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/prefer-array-includes.js:61 (Diff revision 1) > + // if (expression.right.argument.value !== 1) { > + // console.log(9); > + // return; > + // } These comments are here for debugging purposes. The tests pass when running `node tools/lint/eslint/eslint-plugin-mozilla/tests/test-run-all.js`. However when I run `mach eslint browser/base/content/browser-addons.js` I don't get any failures. We should be getting a failure at line 72 though. Do you see any reason why we're not getting any failures in practice?

Comment on attachment 8837348 [details] Bug 1339461 - Implement new eslint rule to prefer array.includes over array.indexOf comparison with -1. https://reviewboard.mozilla.org/r/112502/#review113932 ::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/prefer-array-includes.js:3 (Diff revision 2) > +/** > + * @fileoverview Prefer using Array.includes() instead of comparing > + Array.indexOf() to -1. .indexOf and .includes both exist on all of the following objects: array, string, TypedArray. We should ensure the wording for the description of the eslint rule and the error it produces are ok for all of these cases. ::: tools/lint/eslint/eslint-plugin-mozilla/package.json:3 (Diff revision 2) > { > "name": "eslint-plugin-mozilla", > - "version": "0.2.18", > + "version": "0.2.19", We were already at version 0.2.20 after bug 1338585. ::: tools/lint/eslint/eslint-plugin-mozilla/tests/prefer-array-includes.js:26 (Diff revision 2) > + > +exports.runTest = function(ruleTester) { > + ruleTester.run("prefer-array-includes", rule, { > + valid: [ > + "array.indexOf('foo') == 3;", > + "array.indexOf('foo') > 0" Is indexOf == 0 a valid case? (I would like us to recommend .startsWith when it's a string, but I don't see a way to make a difference between an indexOf call in a string or in an array of strings :( ). ::: tools/lint/eslint/eslint-plugin-mozilla/tests/prefer-array-includes.js:29 (Diff revision 2) > + valid: [ > + "array.indexOf('foo') == 3;", > + "array.indexOf('foo') > 0" > + ], > + invalid: [ > + invalidCode("function a() { if(array.indexOf('foo') == -1) return; }", "=="), All of the following are invalid (and should be tested in my opinion) : .indexOf == -1 .indexOf === -1 .indexOf != -1 .indexOf !== -1 .indexOf >= 0 .indexOf < 0 I would include a test for: var blah = stuff.foo.indexOf(bar) == -1;

I think attachment 8831467 [details] is my most recent xpcshell script for automated changes.

Comment on attachment 8837348 [details] Bug 1339461 - Implement new eslint rule to prefer array.includes over array.indexOf comparison with -1. https://reviewboard.mozilla.org/r/112502/#review114188 Removing the review flag for now; comment 4 and comment 5 contain actionable comments.

Given https://bugzilla.mozilla.org/show_bug.cgi?id=1343902#c8 I almost wonder if we should audit some of these to see if we should switch them over to Set()...

Placing my WIP xpcshell script here in case someone else wants to take this bug in the mean time.

The script I used is available at https://bitbucket.org/fqueze/xpcshell-rewrites/commits/157f99c70d784e16f4f73548d1d76c1da5a7fd60 Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=c609a48916e440d96a3e24d5b4c5c855af371b37&filter-tier=1 is green except for: - dom/html/test/test_bug1823.html which I'll fix by hand in the next attachment. - wpt failures that were on the base changeset.

I had missed a few files in the previous run (some exclusion rules were too large in my script, and some occurences were caught by eslint :-)).

Please remove all changes to toolkit/components/reader/ . Those files are (a) from an upstream repo, and (b) have their own eslint file that should prevent them from causing errors here assuming exceptions are set correctly, and (c) need to work on non-ES.next environments like iOS (which is defined in the eslint file as well...) so we can't make this change there.

(In reply to :Gijs (lower availability until Jan 27) from comment #13) > Please remove all changes to toolkit/components/reader/ . Those files are > (a) from an upstream repo, and (b) have their own eslint file that should > prevent them from causing errors here assuming exceptions are set correctly, > and (c) need to work on non-ES.next environments like iOS (which is defined > in the eslint file as well...) so we can't make this change there. According to the current ESLint exclusions, and a brief look at the github repo, it is only toolkit/components/reader/Readability.js and toolkit/components/reader/JSDOMParser.js that are imported. So it seems we should still be able to apply this to the other files in the directory & sub-directories? e.g. test/head.js (which is in Florian's patch).

(In reply to Mark Banner (:standard8) from comment #14) > (In reply to :Gijs (lower availability until Jan 27) from comment #13) > > Please remove all changes to toolkit/components/reader/ . Those files are > > (a) from an upstream repo, and (b) have their own eslint file that should > > prevent them from causing errors here assuming exceptions are set correctly, > > and (c) need to work on non-ES.next environments like iOS (which is defined > > in the eslint file as well...) so we can't make this change there. > > According to the current ESLint exclusions, and a brief look at the github > repo, it is only toolkit/components/reader/Readability.js and > toolkit/components/reader/JSDOMParser.js that are imported. So it seems we > should still be able to apply this to the other files in the directory & > sub-directories? e.g. test/head.js (which is in Florian's patch). Yes, sorry for being imprecise. That said, JSDOMParser.js and Readability.js are also both in the patch...

Ran the script again with devtools/client/debugger/new/debugger.js, toolkit/components/reader/Readability.js and toolkit/components/reader/JSDOMParser.js added to the exclusion list.

Fixed the eslint error in browser_sort_in_library.js New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d8e8606e6c6906cb26ba2e35b6dbedc2e166b79

Pushed by florian@queze.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a5ad1dbbf2 script-generated patch to convert foo.indexOf(...) == -1 to foo.includes(), r=Mossop. https://hg.mozilla.org/integration/mozilla-inbound/rev/5f88a7c7be35 add an eslint rule to detect when indexOf should be replaced with includes, r=Standard8.

https://hg.mozilla.org/mozilla-central/rev/d5a5ad1dbbf2 https://hg.mozilla.org/mozilla-central/rev/5f88a7c7be35

Thanks for excluding devtools/client/debugger/new/debugger.js. For info we have three other webpack bundles in the same folder that should be excluded from refactors: - devtools/client/debugger/new/parser-worker.js - devtools/client/debugger/new/pretty-print-worker.js - devtools/client/debugger/new/search-worker.js (or handled upstream in https://github.com/devtools-html/debugger.html) Is there a file we can contribute to that lists generated artifacts in m-c? Maybe we should decide on a common suffix for generated files (.bundle.js ?) to avoid conflicts.

(In reply to Julian Descottes [:jdescottes][:julian] from comment #20) > Is there a file we can contribute to that lists generated artifacts in m-c? > Maybe we should decide on a common suffix for generated files (.bundle.js ?) > to avoid conflicts. Having a common suffix for generated files like this would make them easy to skip for automated rewrite scripts, so I think this is a good idea. Even better would be to not have these huge generated files at all (they are difficult to look at and take a long time to parse).

(In reply to Julian Descottes [:jdescottes][:julian] from comment #20) > Is there a file we can contribute to that lists generated artifacts in m-c? > Maybe we should decide on a common suffix for generated files (.bundle.js ?) > to avoid conflicts. As a starting point, adding generated files to .eslintignore is a good start - though, if they are our generated files, then I'd expect the sources to be linted. Note also, in bug 1366714 and bug 1433522 we're discussing having various moz.build settings/flags which would probably allow scripts to more easily ignore these types of files. As Florian said, it'd be nice to not have them autogenerated, but I understand there's good reasons for that (if node is one of the things, I believe that's currently being discussed).

Thanks for answering! As soon as we can use node in the build (which is being discussed but not coming soon if I understood correctly), we'll be happy to move to having the real sources in m-c for the debugger project. All the debugger bundles are eslintignored as part of a wide rule: devtools/client/debugger/**. So even though I said "conflicts" in my previous comment, that was inappropriate. It is not a big issue for us if the bundles are automatically refactored, the changes will be overridden by the next release that updates those bundles. The only "conflict" we had with this patch was for an "asset" file which is not bundled but simply synchronized between m-c and github.