Just removed a few lines of dead code and made existing code conform to our eslint setup.

Review commit: https://reviewboard.mozilla.org/r/35133/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35133/

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31c31c671b8e

The failed jobs are unrelated, known intermittents.

Comment on attachment 8719821 [details] MozReview Request: Bug 1248563 - eslint cleanup of storage inspector code r=pbrosset https://reviewboard.mozilla.org/r/35133/#review31917 Mostly good to go. My most important comment is about not disabling eslint entirely in head.js but instead coniguring the unused rule (which is what we're doing for other head.js files). No need to re-request review after the changed have been done. One question: with these changes, can the storage inspector be un-ignored from .eslintignore, or are there other things left to clean up? If so, can you file another bug (good-first, mentored) to finish the cleanup? ::: devtools/client/shared/widgets/TableWidget.js:858 (Diff revision 1) > - return; > + return true; > } > > if (event.button == 0 && event.originalTarget == this.header) { > return this.table.sortBy(this.id); Why does this event handler callback even return a value at all? `sortBy` doesn't seem to return anything useful anyway. ESLint complains about multiple returns when they don't seem to be of the same type. But this would pass ESLint rules and, I think, be correct: ``` if (event.originalTarget == this.column) { return; } if (event.button == 0 && event.originalTarget == this.header) { this.table.sortBy(this.id) } ``` ::: devtools/client/shared/widgets/TreeWidget.js:487 (Diff revision 1) > + items[this.level]; nit: maybe put each condition on its own line? ``` let label = items[this.level].label || items[this.level].id || items[this.level]; ``` ::: devtools/client/storage/panel.js:1 (Diff revision 1) > -/* -*- Mode: Javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* -*- Mode: Javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ // eslint-disable-line I'm not a fan of adding yet another comment at the end of this already long line. Can't this emacs config line be wrapped on 2 lines? If not, I would advise changing our .eslintrc instead by adding another regex part in our max-len ignorePattern regex: `"max-len": [1, 80, 2, {"ignoreUrls": true, "ignorePattern": "\\s*require\\s*\\(|^\\s*loader\\.lazy|-\\*- Mode"}],` ::: devtools/client/storage/test/head.js:7 (Diff revision 1) > +/* eslint-disable */ I'm guessing you've disabled this whole block because eslint complains about unused globals, right? If so, there's a better solution than just disabling eslint altogether. Add this comment to the top of this head.js file: ``` /* eslint no-unused-vars: [2, {"vars": "local"}] */ ``` This makes sure eslint doesn't complain about unused globals variables in this file (but will still complain about unused local variables). ::: devtools/client/storage/test/head.js:8 (Diff revision 1) > var { console } = Cu.import("resource://gre/modules/Console.jsm", {}); I think you don't need to import console in tests, it's already defined by the test runner. ::: devtools/client/storage/test/head.js:90 (Diff revision 1) > -function* openTabAndSetupStorage(url) { > +function* openTabAndSetupStorage(url) { // eslint-disable-line This can then be removed. ::: devtools/client/storage/test/head.js:200 (Diff revision 1) > -function* finishTests() { > +function* finishTests() { // eslint-disable-line And this. ::: devtools/client/storage/test/head.js:295 (Diff revision 1) > - function fetchError(aProp) { > + function fetchError(prop) { // eslint-disable-line And this. ::: devtools/client/storage/test/head.js:328 (Diff revision 1) > -function findVariableViewProperties(aRules, aParsed) { > +function findVariableViewProperties(ruleArray, parsed) { // eslint-disable-line And this. ::: devtools/client/storage/test/head.js:488 (Diff revision 1) > -function* selectTreeItem(ids) { > +function* selectTreeItem(ids) { // eslint-disable-line And this. ::: devtools/client/storage/test/head.js:509 (Diff revision 1) > -function* selectTableItem(id) { > +function* selectTableItem(id) { // eslint-disable-line And this. ::: devtools/client/storage/test/head.js:526 (Diff revision 1) > -function once(target, eventName, useCapture=false) { > +function once(target, eventName, useCapture = false) { // eslint-disable-line And this! ::: devtools/server/actors/storage.js:1808 (Diff revision 1) > - this.childActorPool.set(store, new actor(this)); > + // Let's use an upper-case constructor to avoid eslint errors. > + let Actor = actor; > + this.childActorPool.set(store, new Actor(this)); I find this a little easier to read (and no need for the eslint comment): ``` for (let [store, ActorConstructor] of storageTypePool) { this.childActorPool.set(store, new ActorConstructor(this)); } ```

Comment on attachment 8719821 [details] MozReview Request: Bug 1248563 - eslint cleanup of storage inspector code r=pbrosset Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35133/diff/1-2/

https://hg.mozilla.org/integration/fx-team/rev/8b188df8101e985e7c5f0dbdbdcdd78e0893a400 Bug 1248563 - eslint cleanup of storage inspector code r=pbrosset