I have started working on a patch to get `toolkit/mozapps/extensions/` to pass eslint, and to remove this directory from the top-level `.eslintignore`. The main issues I am seeing are not too surprising: * need .eslintrc with proper rules enabled * remove C preprocessor instructions (e.g. move #ifdefs to AppConstants) * use of features not on standards track (Array comprehensions) Losing array comprehensions makes me pretty sad. If we really want it we could write a rule or I think the babel-eslint parser supports it, but we're probably better off just avoiding them for now.

(In reply to Robert Helmer [:rhelmer] from comment #0) > * need .eslintrc with proper rules enabled > * remove C preprocessor instructions (e.g. move #ifdefs to AppConstants) > * use of features not on standards track (Array comprehensions) * yield used without function*

Awesome, I was going to look into it myself next week. Bug 1226386 removes almost all of the preprocessing but yeah we need to get rid of the array comprehensions and stuff.

bug 1228792 - quote non-numerals r?mossop

bug 1228792 - use function* for generators r?mossop

bug 1228792 - use standard version of catch r?mossop

bug 1228792 - remove addons manager from eslint ignore r?mossop

bug 1228792 - move addons manager preprocessor instructions to AppConstants.jsm r?mossop

bug 1228792 - remove use of array comprehensions r?mossop

(In reply to Dave Townsend [:mossop] from comment #2) > Awesome, I was going to look into it myself next week. Bug 1226386 removes > almost all of the preprocessing but yeah we need to get rid of the array > comprehensions and stuff. Yes I was thinking this :) I went ahead and made all the changes necessary to pass ESLint here (broken up into separate commits), but let's go ahead with landing bug 1226386 first and I'll deal with any conflicts after.

Comment on attachment 8693392 [details] MozReview Request: bug 1228792 - remove use of array comprehensions r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26435/diff/1-2/

Comment on attachment 8693388 [details] MozReview Request: bug 1228792 - use function* for generators r?mossop https://reviewboard.mozilla.org/r/26427/#review23987

Comment on attachment 8693389 [details] MozReview Request: bug 1228792 - use standard version of catch r?mossop https://reviewboard.mozilla.org/r/26429/#review23993 ::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:1615 (Diff revision 1) > - } catch (e if e instanceof OS.File.Error && e.becauseNoSuchFile) { > + } catch (e) { You should be able to do this: } catch (e) { if (e instanceof ...) { } else { logger.error("Malformed" ... } } ::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm:653 (Diff revision 1) > + if (! e instanceof SyntaxError) JS operator precedence makes ! more important than instanceof so you have to wrap the instanceof check in brackets. ::: toolkit/mozapps/extensions/internal/PluginProvider.jsm:467 (Diff revision 1) > + if (! e.result && e.result != Components.results.NS_ERROR_FAILURE) This should be || not && ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1806 (Diff revision 1) > + if (! e instanceof OS.File.Error && ! e.becauseNoSuchFile) Same issues as above ::: toolkit/mozapps/extensions/nsBlocklistService.js:102 (Diff revision 1) > + ! ex.result == Cr.NS_NOINTERFACE) This needs to be: (!(ex instanceof Components.Exception) || ex.result != Cr.NS_NO_INTERFACE) ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1078 (Diff revision 1) > + if (! ex instanceof OS.File.Error) Ditto ::: toolkit/mozapps/extensions/test/xpcshell/test_mapURIToAddonID.js:271 (Diff revision 1) > - catch (ex if ex.result) { > + catch (ex) { See previous comment about not needing to nest catches.

Comment on attachment 8693390 [details] MozReview Request: bug 1228792 - remove addons manager from eslint ignore r?mossop https://reviewboard.mozilla.org/r/26431/#review24009

Comment on attachment 8693391 [details] MozReview Request: bug 1228792 - move addons manager preprocessor instructions to AppConstants.jsm r?mossop https://reviewboard.mozilla.org/r/26433/#review24013 I'll wait to review this after rebasing on top of bug 1226386

Comment on attachment 8693392 [details] MozReview Request: bug 1228792 - remove use of array comprehensions r?mossop https://reviewboard.mozilla.org/r/26435/#review24015 Using array.filter and array.map can reduce the ammount of additional code here. I've commented on a few to show examples but I think most of this can be done in the same way. Where you've actually got an iterator and not a real array then Array.from can be used to convert. ::: toolkit/mozapps/extensions/AddonManager.jsm:1303 (Diff revision 2) > - port.sendAsyncMessage("PluginList", [filterProperties(p) for (p of aPlugins)]); > + let plugins = []; You can just do: aPlugins.map(filterProperties) ::: toolkit/mozapps/extensions/AddonManager.jsm:2437 (Diff revision 2) > - let promises = [for (p of this.providers) promiseCallProvider(p, "getAddonByID", aID)]; > + let promises = []; this.providers.map(p => promiseCallProvider(p, "getAddonByID", aID)) ::: toolkit/mozapps/extensions/content/update.js:216 (Diff revision 2) > - gUpdateWizard.addons = [a for (a of fetchedAddons) if (a)]; > + gUpdateWizard.addons = []; fetchedAddons.filter(a => a) ::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:640 (Diff revision 2) > - let ids = [a.id for (a of allAddons)]; > + let ids = []; let ids = allAddons.map(a => a.id) ::: toolkit/mozapps/extensions/internal/AddonRepository_SQLiteMigrator.jsm:63 (Diff revision 2) > - let resultArray = [addon for ([,addon] of Iterator(results))]; > + let resultArray = []; Iterator is going to go away soon too: let resultArray = results.values()

Comment on attachment 8693387 [details] MozReview Request: bug 1228792 - remove use of array comprehensions r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26425/diff/1-2/

Comment on attachment 8693387 [details] MozReview Request: bug 1228792 - remove use of array comprehensions r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26425/diff/2-3/

bug 1228792 - use function* for generators r?mossop

bug 1228792 - use standard version of catch r?mossop

bug 1228792 - remove addons manager from eslint ignore r?mossop

bug 1228792 - move addons manager preprocessor instructions to AppConstants.jsm r?mossop

bug 1228792 - remove use of array comprehensions r?mossop

Comment on attachment 8693864 [details] MozReview Request: bug 1228792 - remove addons manager from eslint ignore r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26587/diff/1-2/

bug 1228792 - remove addons manager from eslint ignore r?mossop

bug 1228792 - move addons manager preprocessor instructions to AppConstants.jsm r?mossop

bug 1228792 - remove use of array comprehensions r?mossop

Comment on attachment 8693864 [details] MozReview Request: bug 1228792 - remove addons manager from eslint ignore r?mossop https://reviewboard.mozilla.org/r/26587/#review24097 ::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:1614 (Diff revision 2) > - } catch (e if e instanceof OS.File.Error && e.becauseNoSuchFile) { > + if (e instanceof OS.File.Error && e.becauseNoSuchFile) { Perhaps I typed badly, you still need a catch block around the if/else block here. ::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm:653 (Diff revision 2) > + if (! e (instanceof SyntaxError)) Brackets need to extend to surround the e too! ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1806 (Diff revision 2) > + if (! e (instanceof OS.File.Error) || ! e.becauseNoSuchFile) Brace must surround e ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1078 (Diff revision 2) > + if (!ex (instanceof OS.File.Error)) Brace must surround ex

https://reviewboard.mozilla.org/r/26587/#review24097 > Perhaps I typed badly, you still need a catch block around the if/else block here. Nope, what you said made sense - I just dropped the line accidentally.

https://reviewboard.mozilla.org/r/26587/#review24097 > Brackets need to extend to surround the e too! sigh

Comment on attachment 8693387 [details] MozReview Request: bug 1228792 - remove use of array comprehensions r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26425/diff/3-4/

Comment on attachment 8693863 [details] MozReview Request: bug 1228792 - move addons manager preprocessor instructions to AppConstants.jsm r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26585/diff/1-2/

Comment on attachment 8693864 [details] MozReview Request: bug 1228792 - remove addons manager from eslint ignore r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26587/diff/2-3/

Comment on attachment 8693868 [details] MozReview Request: bug 1228792 - use standard version of catch r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26595/diff/1-2/

Comment on attachment 8693869 [details] MozReview Request: bug 1228792 - use function* for generators r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26597/diff/1-2/

Comment on attachment 8693870 [details] MozReview Request: bug 1228792 - remove leading 0, be explicit about octals r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26599/diff/1-2/

Comment on attachment 8693869 [details] MozReview Request: bug 1228792 - use function* for generators r?mossop I just rebased against fx-team, this one isn't quite ready yet.

Comment on attachment 8693870 [details] MozReview Request: bug 1228792 - remove leading 0, be explicit about octals r?mossop I just rebased against fx-team, this one isn't quite ready yet.

Comment on attachment 8693870 [details] MozReview Request: bug 1228792 - remove leading 0, be explicit about octals r?mossop (discussed in IRC) Going to use the new ES6 octal notation for the chmod settings, and remove the leading 0 for the Date parts, to make eslint happy hear but make our intent clearer.

(In reply to Dave Townsend [:mossop] from comment #27) > Comment on attachment 8693864 [details] > > ::: toolkit/mozapps/extensions/internal/AddonUpdateChecker.jsm:653 > (Diff revision 2) > > + if (! e (instanceof SyntaxError)) > > Brackets need to extend to surround the e too! > > ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1806 > (Diff revision 2) > > + if (! e (instanceof OS.File.Error) || ! e.becauseNoSuchFile) > > Brace must surround e > > ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1078 > (Diff revision 2) > > + if (!ex (instanceof OS.File.Error)) > > Brace must surround ex This was a search/replace fail.

Comment on attachment 8693864 [details] MozReview Request: bug 1228792 - remove addons manager from eslint ignore r?mossop https://reviewboard.mozilla.org/r/26587/#review24163

Comment on attachment 8693868 [details] MozReview Request: bug 1228792 - use standard version of catch r?mossop https://reviewboard.mozilla.org/r/26595/#review24165 ::: toolkit/mozapps/extensions/internal/PluginProvider.jsm:505 (Diff revision 2) > + if (! e.result || e.result != Components.results.NS_ERROR_FAILURE) Nit: No space after the ! ::: toolkit/mozapps/extensions/nsBlocklistService.js:101 (Diff revision 2) > + if (! (ex instanceof Components.Exception) || Nit: No space after the !

Comment on attachment 8693869 [details] MozReview Request: bug 1228792 - use function* for generators r?mossop https://reviewboard.mozilla.org/r/26597/#review24167

Comment on attachment 8693387 [details] MozReview Request: bug 1228792 - remove use of array comprehensions r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26425/diff/4-5/

Comment on attachment 8693864 [details] MozReview Request: bug 1228792 - remove addons manager from eslint ignore r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26587/diff/3-4/

Comment on attachment 8693868 [details] MozReview Request: bug 1228792 - use standard version of catch r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26595/diff/2-3/

Comment on attachment 8693869 [details] MozReview Request: bug 1228792 - use function* for generators r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26597/diff/2-3/

Comment on attachment 8693870 [details] MozReview Request: bug 1228792 - remove leading 0, be explicit about octals r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26599/diff/2-3/

Comment on attachment 8693387 [details] MozReview Request: bug 1228792 - remove use of array comprehensions r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26425/diff/5-6/

Comment on attachment 8693864 [details] MozReview Request: bug 1228792 - remove addons manager from eslint ignore r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26587/diff/4-5/

Comment on attachment 8693868 [details] MozReview Request: bug 1228792 - use standard version of catch r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26595/diff/3-4/

Comment on attachment 8693869 [details] MozReview Request: bug 1228792 - use function* for generators r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26597/diff/3-4/

Comment on attachment 8693870 [details] MozReview Request: bug 1228792 - remove leading 0, be explicit about octals r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26599/diff/3-4/

Comment on attachment 8693870 [details] MozReview Request: bug 1228792 - remove leading 0, be explicit about octals r?mossop https://reviewboard.mozilla.org/r/26599/#review24393 ::: toolkit/mozapps/extensions/test/browser/browser_sorting.js:32 (Diff revision 4) > - size: 0265, > + size: "0265", Just use 265 here ::: toolkit/mozapps/extensions/test/browser/browser_sorting.js:38 (Diff revision 4) > - name: "\u010Cesk\u00FD slovn\u00EDk", // Český slovník > + name: "\u010Cesk\u00FD slovn\u00EDk", // ÄeskÃœ slovnà Looks like your editor clobbered the international characters here?

Comment on attachment 8693387 [details] MozReview Request: bug 1228792 - remove use of array comprehensions r?mossop https://reviewboard.mozilla.org/r/26425/#review24399 ::: toolkit/mozapps/extensions/content/selectAddons.js:129 (Diff revision 6) > - var addons = [gAddons[id] for (id in gAddons)]; > + let addons = gAddons.map(a => a.id); This isn't right. I think you just want Object.values(gAddons) ::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:627 (Diff revision 6) > - allAddons = [a for (a of allAddons) if (a.id != AddonManager.hotfixID)]; > + allAddons = allAddons.filter(a => a != AddonManager.hotfixID); Looks like there should be an a.id in here ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3620 (Diff revision 6) > - + [for (b of bootstrapDescriptors) b] + ")"); > + + Array.from(bootstrapDescriptors).map(a => a) + ")"); This map call doesn't do anything ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:417 (Diff revision 6) > - return [for (addon of addonDB.values()) if (aFilter(addon)) addon]; > + return Array.from(addonDB.values()).filter(a => aFilter(a)); You can just use .filter(aFilter) here

Comment on attachment 8693387 [details] MozReview Request: bug 1228792 - remove use of array comprehensions r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26425/diff/6-7/

Comment on attachment 8693864 [details] MozReview Request: bug 1228792 - remove addons manager from eslint ignore r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26587/diff/5-6/

Comment on attachment 8693868 [details] MozReview Request: bug 1228792 - use standard version of catch r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26595/diff/4-5/

Comment on attachment 8693869 [details] MozReview Request: bug 1228792 - use function* for generators r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26597/diff/4-5/

Comment on attachment 8693870 [details] MozReview Request: bug 1228792 - remove leading 0, be explicit about octals r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26599/diff/4-5/

Tests pass for me locally, also pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=acd639049aba

A handful of the try tests found a problem with the array comprehension patch, re-pushed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42c7c03cfff0

Comment on attachment 8693387 [details] MozReview Request: bug 1228792 - remove use of array comprehensions r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26425/diff/7-8/

bug 1228792 - remove addons manager from eslint ignore r?mossop

bug 1228792 - use standard version of catch r?mossop

bug 1228792 - use function* for generators r?mossop

bug 1228792 - remove leading 0, be explicit about octals r?mossop

Carrying forward previous r+ from mossop, waiting on new try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42c7c03cfff0

Comment on attachment 8693387 [details] MozReview Request: bug 1228792 - remove use of array comprehensions r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26425/diff/8-9/

Comment on attachment 8695331 [details] MozReview Request: bug 1228792 - remove addons manager from eslint ignore r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27069/diff/1-2/

Comment on attachment 8695332 [details] MozReview Request: bug 1228792 - use standard version of catch r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27071/diff/1-2/

Comment on attachment 8695333 [details] MozReview Request: bug 1228792 - use function* for generators r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27073/diff/1-2/

Comment on attachment 8695334 [details] MozReview Request: bug 1228792 - remove leading 0, be explicit about octals r?mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27075/diff/1-2/

Carrying forward r+ from mossop, got a good try run for this set: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53c9d4821a55 The test timeouts I see there don't seem to be related to this patch, and have open intermittent bugs.

https://hg.mozilla.org/integration/fx-team/rev/a405a78a5229 https://hg.mozilla.org/integration/fx-team/rev/07267b96720a https://hg.mozilla.org/integration/fx-team/rev/9abfe93bfd41 https://hg.mozilla.org/integration/fx-team/rev/5ada25e1f319 https://hg.mozilla.org/integration/fx-team/rev/e5c4090f0c4d

https://hg.mozilla.org/mozilla-central/rev/a405a78a5229 https://hg.mozilla.org/mozilla-central/rev/07267b96720a https://hg.mozilla.org/mozilla-central/rev/9abfe93bfd41 https://hg.mozilla.org/mozilla-central/rev/5ada25e1f319 https://hg.mozilla.org/mozilla-central/rev/e5c4090f0c4d