Toolkit code should pass eslint wherever possible

RESOLVED FIXED in Firefox 45

Status

()

Toolkit
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(9 attachments, 1 obsolete attachment)

40 bytes, text/x-review-board-request
mak
: review+
Details | Review
40 bytes, text/x-review-board-request
rhelmer
: review+
Details | Review
40 bytes, text/x-review-board-request
mconley
: review+
Details | Review
40 bytes, text/x-review-board-request
mak
: review+
Details | Review
40 bytes, text/x-review-board-request
mak
: review+
Details | Review
40 bytes, text/x-review-board-request
Dolske
: review+
Details | Review
40 bytes, text/x-review-board-request
gfritzsche
: review+
Details | Review
40 bytes, text/x-review-board-request
MattN
: review+
Details | Review
40 bytes, text/x-review-board-request
MattN
: review+
Details | Review
Comment hidden (empty)
Comment hidden (off-topic)
(Assignee)

Updated

3 years ago
Blocks: 1229856
(Assignee)

Comment 3

3 years ago
Created attachment 8695376 [details]
MozReview Request: Bug 1229519: Fix toolkit/modules to pass eslint checks.

Bug 1229519: Fix toolkit/modules to pass eslint checks.
Attachment #8695376 - Flags: review?(felipc)
(Assignee)

Comment 4

3 years ago
Created attachment 8695377 [details]
MozReview Request: Bug 1229519: Fix toolkit/components/thumbnails to pass eslint checks.

Bug 1229519: Fix toolkit/components/thumbnails to pass eslint checks.
Attachment #8695377 - Flags: review?(rhelmer)
(Assignee)

Comment 5

3 years ago
Created attachment 8695378 [details]
MozReview Request: Bug 1229519: Fix toolkit/components/contentprefs to pass eslint checks.

Bug 1229519: Fix toolkit/components/contentprefs to pass eslint checks.
Attachment #8695378 - Flags: review?(mconley)
(Assignee)

Comment 6

3 years ago
Created attachment 8695379 [details]
MozReview Request: Bug 1229519: Fix download managers to pass eslint checks.

Bug 1229519: Fix download managers to pass eslint checks.
Attachment #8695379 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 7

3 years ago
Created attachment 8695380 [details]
MozReview Request: Bug 1229519: Fix crash manager to pass eslint checks.

Bug 1229519: Fix crash manager to pass eslint checks.
Attachment #8695380 - Flags: review?(mak77)
(Assignee)

Comment 8

3 years ago
Created attachment 8695381 [details]
MozReview Request: Bug 1229519: Fix toolkit/components/satchel to pass eslint checks.

Bug 1229519: Fix toolkit/components/satchel to pass eslint checks.
Attachment #8695381 - Flags: review?(dolske)
(Assignee)

Comment 9

3 years ago
Created attachment 8695382 [details]
MozReview Request: Bug 1229519: Fix toolkit/components/telemetry to pass eslint checks.

Bug 1229519: Fix toolkit/components/telemetry to pass eslint checks.
Attachment #8695382 - Flags: review?(gfritzsche)
(Assignee)

Comment 10

3 years ago
Created attachment 8695384 [details]
MozReview Request: Bug 1229519: Fix toolkit/content to pass eslint checks.

Bug 1229519: Fix toolkit/content to pass eslint checks.
Attachment #8695384 - Flags: review?(felipc)
(Assignee)

Comment 11

3 years ago
Created attachment 8695385 [details]
MozReview Request: Bug 1229519: Fix miscellaneous parts of toolkit to pass eslint checks.

Bug 1229519: Fix miscellaneous parts of toolkit to pass eslint checks.
Attachment #8695385 - Flags: review?(MattN+bmo)
Comment on attachment 8695382 [details]
MozReview Request: Bug 1229519: Fix toolkit/components/telemetry to pass eslint checks.

https://reviewboard.mozilla.org/r/27095/#review24485

Looks good, thanks!

::: toolkit/components/telemetry/TelemetryStorage.jsm:1237
(Diff revision 1)
> +      if (!e.becauseExists)
> +        throw e;

Wrap the body in {} please.

::: toolkit/components/telemetry/TelemetryStorage.jsm:1304
(Diff revision 1)
> +      if (!(e instanceof OS.File.Error) || !e.becauseNoSuchFile)
> +        throw e;

Wrap the body in {} please.
Attachment #8695382 - Flags: review?(gfritzsche) → review+
Comment on attachment 8695378 [details]
MozReview Request: Bug 1229519: Fix toolkit/components/contentprefs to pass eslint checks.

https://reviewboard.mozilla.org/r/27087/#review24501
Attachment #8695378 - Flags: review?(mconley) → review+
Comment on attachment 8695385 [details]
MozReview Request: Bug 1229519: Fix miscellaneous parts of toolkit to pass eslint checks.

https://reviewboard.mozilla.org/r/27099/#review24491

::: .eslintignore:169
(Diff revision 1)
> +# toolkit/exclusions

Nit: "toolkit exclusions"?

::: toolkit/components/microformats/Microformats.js:10
(Diff revision 1)
>    /* Custom iterator so that microformats can be enumerated as */
>    /* for (i in Microformats) */
> -  __iterator__: function () {
> +  __iterator__: function* () {
>      for (let i=0; i < this.list.length; i++) {
>        yield this.list[i];

I don't know if this will work but tests should catch that

::: toolkit/components/social/test/xpcshell/head.js:135
(Diff revision 1)
>      // val is an iterator => prepend it to the queue and start on it
>      // val is otherwise truthy => call next
> -    if (val) {
> +    if (value) {

Why change the variable name? The comments are now stale.

::: toolkit/profile/content/createProfileWizard.js:122
(Diff revision 1)
> -#ifndef XP_MACOSX
> +    if (AppConstants.platform == "macosx")
> -    finishText.firstChild.data = gProfileManagerBundle.getString("profileFinishText");
> +      finishText.firstChild.data = gProfileManagerBundle.getString("profileFinishText");
> -#else
> +    else
> -    finishText.firstChild.data = gProfileManagerBundle.getString("profileFinishTextMac");
> +      finishText.firstChild.data = gProfileManagerBundle.getString("profileFinishTextMac");
> -#endif

New code is supposed to brace single-line bodies…

I didn't say anything for the catch blocks since the lines were short but in this case line 123 wrapped in mozreview so it was harder to read without the braces.

::: toolkit/profile/content/profileSelection.js:7
(Diff revision 1)
>  Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/AppConstants.jsm");

Nit: I would sort these

::: toolkit/profile/content/profileSelection.js:135
(Diff revision 1)
>    switch( aEvent.keyCode ) 
>    {

May as well cleanup the trailing whitespace here as I hope we can have a rule for that in eslint soon.
Attachment #8695385 - Flags: review?(MattN+bmo) → review+
https://reviewboard.mozilla.org/r/27087/#review24503

::: toolkit/components/contentprefs/nsContentPrefService.js:1074
(Diff revision 1)
> +        if (e.result != Cr.NS_ERROR_FILE_CORRUPTED)
> +          throw e;

According to MattN, we're bracing one-liners now.

::: toolkit/components/contentprefs/tests/unit_cps2/AsyncRunner.jsm:49
(Diff revision 1)
> -      if (typeof(val) != "boolean")
> -        this._iteratorQueue.unshift(val);
> +      if (typeof(value) != "boolean")
> +        this._iteratorQueue.unshift(value);

Brace the one-liner.
There was a discussion about this around the time of "goto fail" and it was added to the style guide and I think that's why Georg also mentioned it before me:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures
"Always brace controlled statements, even a single-line consequent of an if else else. This is redundant typically, but it avoids dangling else bugs, so it's safer at scale than fine-tuning."
(Assignee)

Comment 18

3 years ago
I attempted to make the bracing match the surrounding code but happy to switch it to brace everything. At some point we'll consider turning the curly rule on for browser/toolkit and then there will be a reckoning.
Note I have a volunteer working on removing array comprehensions from Places in bug 1228976, so please don't touch that for now
(In reply to Matthew N. [:MattN] from comment #17)
> "Always brace controlled statements, even a single-line consequent of an if
> else else. This is redundant typically, but it avoids dangling else bugs, so
> it's safer at scale than fine-tuning."

IIRC that was directly imported from the cpp coding style but broad consensus was never reached about single line ifs. Afaict nobody cares about that rule, indeed I keep seeing patches not bracing single line ifs.
It's also unclear (to me) if the coding style means to brace single-line only if they are part of an if-else.
Either we intend the rule as "always brace if part of an if-else(if)" as de-facto we did until today, or at this point we enforce bracing everywhere through eslint.

Updated

3 years ago
Attachment #8695380 - Flags: review?(mak77) → review+
Comment on attachment 8695380 [details]
MozReview Request: Bug 1229519: Fix crash manager to pass eslint checks.

https://reviewboard.mozilla.org/r/27091/#review24515

::: toolkit/components/crashmonitor/CrashMonitor.jsm:193
(Diff revision 1)
> -      Task.spawn(function() {
> +      Task.spawn(function*() {

missing space after *
Attachment #8695381 - Flags: review?(dolske) → review+
Comment on attachment 8695381 [details]
MozReview Request: Bug 1229519: Fix toolkit/components/satchel to pass eslint checks.

https://reviewboard.mozilla.org/r/27093/#review24519
(Assignee)

Comment 23

3 years ago
Comment on attachment 8695376 [details]
MozReview Request: Bug 1229519: Fix toolkit/modules to pass eslint checks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27083/diff/1-2/
Attachment #8695376 - Flags: review?(felipc) → review?(mak77)
(Assignee)

Updated

3 years ago
Attachment #8695379 - Flags: review?(gijskruitbosch+bugs) → review?(mak77)
(Assignee)

Comment 24

3 years ago
Comment on attachment 8695379 [details]
MozReview Request: Bug 1229519: Fix download managers to pass eslint checks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27089/diff/1-2/
Comment on attachment 8695376 [details]
MozReview Request: Bug 1229519: Fix toolkit/modules to pass eslint checks.

https://reviewboard.mozilla.org/r/27083/#review24523

::: toolkit/modules/Promise-backend.js:239
(Diff revision 1)
> -    let keys = [key for (key of this._map.keys())];
> +    for (let key of [...this._map.keys()]) {

Array.from(this._map.keys())

::: toolkit/modules/RemotePageManager.jsm:468
(Diff revision 1)
> -    messageManager.sendAsyncMessage("RemotePage:Register", { urls: [u for (u of this.pages.keys())] })
> +    messageManager.sendAsyncMessage("RemotePage:Register", { urls: [...this.pages.keys()] })

Array.from

::: toolkit/modules/ZipUtils.jsm:86
(Diff revision 1)
> -      readFailed(e);
> +        readFailed(e);

please brace since part of an if-else

::: toolkit/modules/ZipUtils.jsm:118
(Diff revision 1)
> -    return Task.spawn(function() {
> +    return Task.spawn(function*() {

missing space after *
Attachment #8695376 - Flags: review?(mak77) → review+
Comment on attachment 8695379 [details]
MozReview Request: Bug 1229519: Fix download managers to pass eslint checks.

https://reviewboard.mozilla.org/r/27089/#review24525

::: toolkit/components/downloads/test/unit/test_app_rep_maclinux.js:213
(Diff revision 1)
> -add_task(function()
> +add_task(function*()

missing space after *

::: toolkit/components/downloads/test/unit/test_app_rep_windows.js:313
(Diff revision 1)
> -add_task(function()
> +add_task(function*()

space after *

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:1716
(Diff revision 1)
> -                ex.result == Cr.NS_ERROR_NOT_AVAILABLE) {
> +      if (!(ex instanceof Components.Exception) || ex.result != Cr.NS_ERROR_NOT_AVAILABLE) {

could indent as
if (cond1 ||
    cond2) {

::: toolkit/components/jsdownloads/src/DownloadList.jsm:236
(Diff revision 1)
> -    Task.spawn(function() {
> +    Task.spawn(function*() {

space after *

::: toolkit/components/jsdownloads/test/unit/head.js:535
(Diff revision 1)
> -  return Task.spawn(function() {
> +  return Task.spawn(function*() {

space after *

::: toolkit/mozapps/downloads/content/downloads.js:489
(Diff revision 1)
> -#ifndef XP_MACOSX
> +        if (AppConstants.platform == "macosx" &&

This was an ifndef, so it should be != "macosx"

::: toolkit/mozapps/downloads/nsHelperAppDlg.js:1009
(Diff revision 1)
> -#else
> +      else

please brace

There's an actual mistake here, apart from minor things, due to an ifndef conversion.
Attachment #8695379 - Flags: review?(mak77) → review+
Comment on attachment 8695377 [details]
MozReview Request: Bug 1229519: Fix toolkit/components/thumbnails to pass eslint checks.

https://reviewboard.mozilla.org/r/27085/#review24539
Attachment #8695377 - Flags: review?(rhelmer) → review+
(Assignee)

Comment 28

3 years ago
Comment on attachment 8695384 [details]
MozReview Request: Bug 1229519: Fix toolkit/content to pass eslint checks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27097/diff/1-2/
Attachment #8695384 - Flags: review?(felipc) → review?(MattN+bmo)
(Assignee)

Updated

3 years ago
Whiteboard: keep-open
Comment on attachment 8695384 [details]
MozReview Request: Bug 1229519: Fix toolkit/content to pass eslint checks.

https://reviewboard.mozilla.org/r/27097/#review24547

r=me though I'm disappointed that try didn't catch the negation issue…

::: toolkit/content/globalOverlay.js:8
(Diff revision 1)
> -#ifndef XP_MACOSX
> +  if (AppConstants.platform == "macosx") {

`#ifndef XP_MACOSX` should translate to `if (AppConstants.platform != "macosx") {`

::: toolkit/content/globalOverlay.js:28
(Diff revision 1)
> +  }
> +  else if (typeof(aPromptFunction) == "function" && !aPromptFunction()) {

y u no cuddle?
Attachment #8695384 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 32

3 years ago
https://reviewboard.mozilla.org/r/27097/#review24547

> `#ifndef XP_MACOSX` should translate to `if (AppConstants.platform != "macosx") {`

I doubt we have tests that verify the app shuts down when the last window is closed :(
Perhaps a test to see that the App doesn't shut down on OS X with no window would have also caught this? I'd have to dig in further to know for sure.
(Assignee)

Updated

3 years ago
Whiteboard: keep-open
> getCharsetInfo: function(charsets, sort=true) {
> -    let list = [{
> +    let list = Array.from(charsets, charset => ({
>        label: this._getCharsetLabel(charset),
>        accesskey: this._getCharsetAccessKey(charset),
>        name: "charset",
>        value: charset
> -    } for (charset of charsets)];
> +    }));
Nit: charsets is already an array, so charsets.map() would have worked.

Comment 37

3 years ago
The misc toolkit portion (5161ded671e0, not yet merged) was backed out in https://hg.mozilla.org/integration/fx-team/rev/f32a93d9abd5 (also not yet merged), so I'm reopening this.
Status: RESOLVED → REOPENED
status-firefox45: fixed → affected
Resolution: FIXED → ---
Target Milestone: mozilla45 → ---
(Assignee)

Comment 40

3 years ago
Relanded the bad changeset without the changes to toolkit/identity that seemed to upset Mulet tests. Added that file to .eslintignore for now.

Comment 41

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2489a4b3a2a5
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Updated

3 years ago
Depends on: 1230839

Comment 42

3 years ago
Created attachment 8696797 [details] [diff] [review]
Followup to make sure AppConstants is defined in globalOverlay.js

globalOverlay.js is used in some windows where AppConstants isn't already defined, so it should be imported into scope to be sure.
Attachment #8696797 - Flags: review?(MattN+bmo)

Comment 43

3 years ago
Comment on attachment 8696797 [details] [diff] [review]
Followup to make sure AppConstants is defined in globalOverlay.js

Review of attachment 8696797 [details] [diff] [review]:
-----------------------------------------------------------------

Ah sorry, I didn't see the followup bug which already does the same thing.
Attachment #8696797 - Flags: review?(MattN+bmo)

Updated

3 years ago
Attachment #8696797 - Attachment is obsolete: true

Updated

3 years ago
Depends on: 1231839

Updated

3 years ago
Blocks: 1233524
You need to log in before you can comment on or make changes to this bug.