Closed
Bug 1311347
Opened 8 years ago
Closed 7 years ago
Enable eslint of browser/components/sessionstore/
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: standard8, Assigned: jordan, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [lang=js])
Attachments
(3 files, 1 obsolete file)
To help with preventing errors in coding, we should enable eslint in browser/components/sessionstore/ To enable checking of files, remove the `browser/components/sessionstore/**` entry from the top-level `.eslintignore`. Then you can run eslint on the directory with ./mach eslint browser/components/sessionstore/ (note: eslint needs a one-time setup of `./mach eslint --setup`). The errors listed in the output are the ones to be fixed. Information about the eslint rules can be found here: http://eslint.org/docs/rules/ If you are uncertain of how to fix a rule, please feel free to ask here or on irc (https://wiki.mozilla.org/Irc) in #fx-team (I'm on as Standard8).
Assignee | ||
Comment 1•8 years ago
|
||
Mark, help! eslint browser/components/sessionstore/test/unit/data/sessionstore_valid.js shows error "Parsing error: Unexpected token :" No clue what this file is about, & what should I do here
Reporter | ||
Comment 2•8 years ago
|
||
(In reply to Sourav Garg [:jordan] from comment #1) > Mark, help! > eslint browser/components/sessionstore/test/unit/data/sessionstore_valid.js > shows error "Parsing error: Unexpected token :" > No clue what this file is about, & what should I do here So these are really json files (although one is going to be invalid anyway). However, I don't think we want to change the .js extension to .json as session store does seem to use .js for its files. Therefore I suggest ignoring these files, adding to .eslintignore something like: # Test files that are really json not js, and don't need to be linted. browser/components/sessionstore/test/unit/data/sessionstore_valid.js browser/components/sessionstore/test/unit/data/sessionstore_invalid.js
Assignee: nobody → souravgarg833
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•8 years ago
|
||
Mark, I've attached the patch file. Please have a look. Thanks!
Reporter | ||
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8805522 [details] Bug 1311347 - Enable eslint of browser/components/sessionstore/; https://reviewboard.mozilla.org/r/89282/#review88492 Thank you for the patch. When I run it through tests, some of the tests are failing - see the details below. I'm not sure why that is, I've poked around at a couple of things that I thought might cause it, but I haven't found it yet. Can you try taking a look please? Maybe undo some of your changes until you find what caused it? The following tests failed: 33 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_broadcast.js | Uncaught exception - [object Object] 34 INFO TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_broadcast.js | Uncaught exception - [object Object] ::: browser/components/sessionstore/SessionStore.jsm:2664 (Diff revision 1) > // Don't continue if the tab was closed before TabStateFlusher.flush resolves. > if (tab.linkedBrowser) { > let tabState = TabState.collect(tab); > return { index: tabState.index - 1, entries: tabState.entries } > } > + return null; This needs to be indented by 2 more spaces.
Attachment #8805522 -
Flags: review?(standard8)
Assignee | ||
Comment 6•8 years ago
|
||
Mark, can you specify which test should I run to check the results, or should I run all tests using '$ ./mach test'? Thank You.
Reporter | ||
Comment 7•8 years ago
|
||
(In reply to Sourav Garg [:jordan] from comment #6) > Mark, can you specify which test should I run to check the results, or > should I run all tests using '$ ./mach test'? Thank You. You can pass either a filename, or a subdirectory. I think in this case I was doing: ./mach test browser/components/sessionstore/
Reporter | ||
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8805522 [details] Bug 1311347 - Enable eslint of browser/components/sessionstore/; https://reviewboard.mozilla.org/r/89282/#review89566 Hi Sourav, whilst working on another bug, I learnt a few things about the old `catch (ex if ex...)` format and the generator functions. If you make the additional changes below (as well as the ones I mentioned previously), then I believe it should start passing again & should be ready or close to being able to land. Sorry I didn't get round to looking at this in more detail earlier. ::: browser/components/sessionstore/ContentRestore.jsm:250 (Diff revision 1) > - // Ignore page load errors, but return false to signal that the load never > + // Ignore page load errors, but return false to signal that the load never > - // happened. > + // happened. > - return false; > + return false; > - } > + } > + } > + return null; Here you need to re-throw ex in the catch, then you shouldn't need to return null, i.e. `throw ex` The `ex if ex` won't catch other exceptions, they would still get thrown, hence the need to re-throw. ::: browser/components/sessionstore/SessionFile.jsm:256 (Diff revision 1) > - corrupted = true; > + corrupted = true; > - } catch (ex if ex instanceof SyntaxError) { > + } else if (ex instanceof SyntaxError) { > - console.error("Corrupt session file (invalid JSON found) ", ex, ex.stack); > + console.error("Corrupt session file (invalid JSON found) ", ex, ex.stack); > - // File is corrupted, try next file > + // File is corrupted, try next file > - corrupted = true; > + corrupted = true; > + } This needs to be made into a ``` } else { throw ex; } ``` ::: browser/components/sessionstore/SessionMigration.jsm:73 (Diff revision 1) > - readState: function(aPath) { > - return Task.spawn(function() { > + readState: function* (aPath) { > + return Task.spawn(function* () { > let bytes = yield OS.File.read(aPath); > let text = gDecoder.decode(bytes); > let state = JSON.parse(text); > throw new Task.Result(state); This needs changing to `return state;` The old Task.Result was a work around for using non-generator functions. Now it is a generator function, it can simply return the result. ::: browser/components/sessionstore/content/content-sessionStore.js:836 (Diff revision 1) > - }; > + }; > - sendAsyncMessage("SessionStore:error", { > + sendAsyncMessage("SessionStore:error", { > - telemetry > + telemetry > - }); > + }); > - } > + } > + } This should also get an ``` } else { throw ex; } ``` Also, the whole block needs to be un-indented by 2 spaces. ::: browser/components/sessionstore/test/browser_broadcast.js:129 (Diff revision 1) > browser.loadURI(url); > yield promiseBrowserLoaded(browser); > yield modifySessionStorage(browser, {test: INITIAL_VALUE}); > } > > throw new Task.Result(tab); This Task.Result throw also needs to be converted to a return: `return tab;`
Assignee | ||
Comment 9•8 years ago
|
||
Thanks Mark, I'm not able to spend much time on bugs these days, as my end-semester exams are going on. I'll get back to work in 15 days.
Assignee | ||
Comment 10•8 years ago
|
||
hi Mark, On executing '$ ./mach test browser/components/sessionstore/' on this patch without any further change it is passing all 6 test, with Test Summary Passed: 6 Failed: 0 Todo: 0 Retried: 0 Is not that enough? What am I missing?
Reporter | ||
Comment 11•8 years ago
|
||
(In reply to Sourav Garg [:jordan] from comment #10) > On executing '$ ./mach test browser/components/sessionstore/' on this patch > without any further change it is passing all 6 test, with Test Summary > Passed: 6 > Failed: 0 > Todo: 0 > Retried: 0 > > Is not that enough? What am I missing? I suspect the real failures are being hidden, as typically there's a few test runs that happen, try using: ./mach mochitest browser/components/sessionstore/ or: ./mach mochitest browser/components/sessionstore/test/browser_broadcast.js ./mach mochitest browser/components/sessionstore/test/browser_parentProcessRestoreHash.js
Reporter | ||
Comment 12•8 years ago
|
||
Note: due to other eslint bugs landing, there's also more failures now. We can either land the patch with the fixes above, and not remove the .ignore, or maybe fix the new errors in a separate commit (I suspect a lot of them are fixable using ./mach eslint --fix)
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to Mark Banner (:standard8, limited time in Dec) from comment #12) > Note: due to other eslint bugs landing, there's also more failures now. We > can either land the patch with the fixes above, and not remove the .ignore, > or maybe fix the new errors in a separate commit (I suspect a lot of them > are fixable using ./mach eslint --fix) So, should I just re-add './mach eslint browser/components/sessionstore/' to .eslintignore or add more fixes in above commit?
Reporter | ||
Comment 14•8 years ago
|
||
(In reply to Sourav Garg [:jordan] from comment #13) > (In reply to Mark Banner (:standard8, limited time in Dec) from comment #12) > > Note: due to other eslint bugs landing, there's also more failures now. We > > can either land the patch with the fixes above, and not remove the .ignore, > > or maybe fix the new errors in a separate commit (I suspect a lot of them > > are fixable using ./mach eslint --fix) > > So, should I just re-add './mach eslint browser/components/sessionstore/' to > .eslintignore or add more fixes in above commit? If you're happy fixing them, then just add the fixes in the commit. That will work nicely for me.
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to Mark Banner (:standard8, afk, back 3rd Jan) from comment #12) > Note: due to other eslint bugs landing, there's also more failures now. We Hi Mark, './mach eslint browser/components/sessionstore/' outputs "✖ 0 problems (0 errors, 0 warnings)" after pulling this patch. I wonder which new eslint bugs are you talking about.
Reporter | ||
Comment 16•8 years ago
|
||
(In reply to Sourav Garg [:jordan] from comment #15) > (In reply to Mark Banner (:standard8, afk, back 3rd Jan) from comment #12) > > Note: due to other eslint bugs landing, there's also more failures now. We > > Hi Mark, './mach eslint browser/components/sessionstore/' outputs "✖ 0 > problems (0 errors, 0 warnings)" after pulling this patch. I wonder which > new eslint bugs are you talking about. Did you pull the latest, and rebase the patch on top of that? I see about 424 errors with the patch applied to the latest tree. Also, note that when I applied it, the .eslintignore changes didn't apply successfully (the rest did), so that could be an issue for you as well.
Updated•7 years ago
|
Blocks: ss-feature
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Updated•7 years ago
|
Attachment #8805522 -
Attachment is obsolete: true
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8846717 [details] Bug 1311347 - Enable eslint of browser/components/sessionstore/. Initial changes by Sourav, updated by Standard8. https://reviewboard.mozilla.org/r/119726/#review121636 ::: browser/components/sessionstore/ContentRestore.jsm:253 (Diff revision 1) > - // Ignore page load errors, but return false to signal that the load never > + // Ignore page load errors, but return false to signal that the load never > - // happened. > + // happened. > - return false; > + return false; > - } > + } > + } > + return null; The indent here doesn't look right. ::: browser/components/sessionstore/SessionStore.jsm:2861 (Diff revision 1) > // Don't continue if the tab was closed before TabStateFlusher.flush resolves. > if (tab.linkedBrowser) { > let tabState = TabState.collect(tab); > return { index: tabState.index - 1, entries: tabState.entries } > } > + return null; The indent here doesn't look right. ::: browser/components/sessionstore/content/aboutSessionRestore.js:245 (Diff revision 1) > - item.parent.checked = item.parent.tabs.every(isChecked) ? true : > - item.parent.tabs.some(isChecked) ? 0 : false; > + item.parent.checked = false; > + if (item.parent.tabs.every(isChecked)) { > + item.parent.checked = true; > + } else if (item.parent.tabs.some(isChecked)) { > + item.parent.checked = 0; > + } Can we use a temp here so that we don't flip from true->false->true when it's not necessary? > let state = false; > if (item.parent.tabs.every(isChecked)) { > state = true; > } else if (item.parent.tabs.some(isChecked)) { > state = 0; > } > item.parent.checked = state; ::: browser/components/sessionstore/nsSessionStartup.js:176 (Diff revision 1) > CrashMonitor.previousCheckpoints.then(checkpoints => { > if (checkpoints) { > // If the previous session finished writing the final state, we'll > // assume there was no crash. > this._previousSessionCrashed = !checkpoints["sessionstore-final-state-write-complete"]; > - > + } The indent doesn't look right here. ::: browser/components/sessionstore/nsSessionStartup.js:177 (Diff revision 1) > - // If the Crash Monitor could not load a checkpoints file it will > + // If the Crash Monitor could not load a checkpoints file it will > - // provide null. This could occur on the first run after updating to > + // provide null. This could occur on the first run after updating to > - // a version including the Crash Monitor, or if the checkpoints file > + // a version including the Crash Monitor, or if the checkpoints file > - // was removed, or on first startup with this profile, or after Firefox Reset. > + // was removed, or on first startup with this profile, or after Firefox Reset. > - > + else if (noFilesFound) { Can we move this in to the above block, and then combine the closing bracket with the else-if here? I don't particularly like the else-if not connecting with the above `if` via curly brackets.
Attachment #8846717 -
Flags: review?(jaws) → review+
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8846718 [details] Bug 1311347 - Enable eslint of browser/components/sessionstore/. Autofix changes. https://reviewboard.mozilla.org/r/119728/#review121648
Attachment #8846718 -
Flags: review?(jaws) → review+
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8846719 [details] Bug 1311347 - Enable eslint of browser/components/sessionstore/. Manual fixes. https://reviewboard.mozilla.org/r/119730/#review121650
Attachment #8846719 -
Flags: review?(jaws) → review+
Comment 23•7 years ago
|
||
Feel free to make the changes requested in comment 20 as a follow-up commit on top of the full patchset so you don't have to rebase on top of the changes.
Reporter | ||
Comment 24•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8846717 [details] Bug 1311347 - Enable eslint of browser/components/sessionstore/. Initial changes by Sourav, updated by Standard8. https://reviewboard.mozilla.org/r/119726/#review121636 > Can we move this in to the above block, and then combine the closing bracket with the else-if here? > > I don't particularly like the else-if not connecting with the above `if` via curly brackets. This was already fixed in the last patch of the set, but I've just reviewed it again and moved the comment slightly so that it reads better.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 28•7 years ago
|
||
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eb4b44b2eb02 Enable eslint of browser/components/sessionstore/. Initial changes by Sourav, updated by Standard8. r=jaws https://hg.mozilla.org/integration/autoland/rev/3947cd1d75d8 Enable eslint of browser/components/sessionstore/. Autofix changes. r=jaws https://hg.mozilla.org/integration/autoland/rev/c0a0ddf7cb98 Enable eslint of browser/components/sessionstore/. Manual fixes. r=jaws
Comment 29•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eb4b44b2eb02 https://hg.mozilla.org/mozilla-central/rev/3947cd1d75d8 https://hg.mozilla.org/mozilla-central/rev/c0a0ddf7cb98
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in
before you can comment on or make changes to this bug.
Description
•