Closed Bug 1311347 Opened 8 years ago Closed 7 years ago

Enable eslint of browser/components/sessionstore/

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
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)

Bug 1311347 - Enable eslint of browser/components/sessionstore/. Initial changes by Sourav, updated by Standard8.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1311347 - Enable eslint of browser/components/sessionstore/. Autofix changes.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1311347 - Enable eslint of browser/components/sessionstore/. Manual fixes.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
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).
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
(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
Mark, I've attached the patch file. Please have a look. Thanks!
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)
Mark, can you specify which test should I run to check the results, or should I run all tests using '$ ./mach test'? Thank You.
(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/
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;`
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.
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?
(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
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)
(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?
(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.
(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.
(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.
Attachment #8805522 - Attachment is obsolete: true
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 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 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+
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.
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.
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: