Closed Bug 1229519 Opened 4 years ago Closed 4 years ago

Toolkit code should pass eslint wherever possible

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(9 files, 1 obsolete file)

40 bytes, text/x-review-board-request
mak
: review+
Details
40 bytes, text/x-review-board-request
rhelmer
: review+
Details
40 bytes, text/x-review-board-request
mconley
: review+
Details
40 bytes, text/x-review-board-request
mak
: review+
Details
40 bytes, text/x-review-board-request
mak
: review+
Details
40 bytes, text/x-review-board-request
Dolske
: review+
Details
40 bytes, text/x-review-board-request
gfritzsche
: review+
Details
40 bytes, text/x-review-board-request
MattN
: review+
Details
40 bytes, text/x-review-board-request
MattN
: review+
Details
No description provided.
Blocks: eslint
Bug 1229519: Fix toolkit/modules to pass eslint checks.
Attachment #8695376 - Flags: review?(felipc)
Bug 1229519: Fix toolkit/components/thumbnails to pass eslint checks.
Attachment #8695377 - Flags: review?(rhelmer)
Bug 1229519: Fix toolkit/components/contentprefs to pass eslint checks.
Attachment #8695378 - Flags: review?(mconley)
Bug 1229519: Fix download managers to pass eslint checks.
Attachment #8695379 - Flags: review?(gijskruitbosch+bugs)
Bug 1229519: Fix crash manager to pass eslint checks.
Attachment #8695380 - Flags: review?(mak77)
Bug 1229519: Fix toolkit/components/satchel to pass eslint checks.
Attachment #8695381 - Flags: review?(dolske)
Bug 1229519: Fix toolkit/components/telemetry to pass eslint checks.
Attachment #8695382 - Flags: review?(gfritzsche)
Bug 1229519: Fix toolkit/content to pass eslint checks.
Attachment #8695384 - Flags: review?(felipc)
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."
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.
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
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)
Attachment #8695379 - Flags: review?(gijskruitbosch+bugs) → review?(mak77)
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+
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)
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+
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.
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.
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
Resolution: FIXED → ---
Target Milestone: mozilla45 → ---
Duplicate of this bug: 1228986
Relanded the bad changeset without the changes to toolkit/identity that seemed to upset Mulet tests. Added that file to .eslintignore for now.
https://hg.mozilla.org/mozilla-central/rev/2489a4b3a2a5
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Depends on: 1230839
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 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)
Attachment #8696797 - Attachment is obsolete: true
Depends on: 1231839
Blocks: 1233524
You need to log in before you can comment on or make changes to this bug.