Closed Bug 1572630 Opened 2 years ago Closed 2 years ago

Port bug 1571567 - Enable the no-fallthrough eslint rule

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: darktrojan, Assigned: darktrojan)

Details

Attachments

(2 files)

No description provided.

Somebody please check these, I'm not totally convinced I've done the right thing in every case.

Attachment #9084214 - Flags: review?(mkmelin+mozilla)
Attachment #9084214 - Flags: review?(jorgk)
Comment on attachment 9084214 [details] [diff] [review]
1572630-no-fallthrough-1.diff

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

::: mail/base/content/mailCommands.js
@@ +181,2 @@
>        }
> +      break;

Can't see the context here :-( - Looking at it, this break should be a return.

::: mail/base/content/messageWindow.js
@@ +933,5 @@
>      switch (command) {
>        case "cmd_createFilterFromPopup":
>        case "cmd_createFilterFromMenu":
>          loadedFolder = gFolderDisplay.displayedFolder;
> +        return loadedFolder && loadedFolder.server.canHaveFilters;

You're fixing a bug here, right? This was falling through when it shouldn't have.

::: mail/base/modules/GlobalPopupNotifications.jsm
@@ -499,5 @@
>          if (this.isPanelOpen) {
>            for (let elt of this.panel.children) {
>              elt.notification.timeShown = this.window.performance.now();
>            }
> -          break;

Why remove that?

::: mail/components/customizableui/PanelMultiView.jsm
@@ +1685,2 @@
>        }
> +      // Fall-through...

Isn't it "Falls through"? Does the linter care?

::: mail/components/preferences/general.js
@@ +2193,4 @@
>        // This should never happen, but if preferredAction is set to some weird
>        // value, then fall back to the generic application icon.
> +
> +      // Explicit fall-through

Another variation on the string.

::: mailnews/db/gloda/modules/indexer.js
@@ +807,2 @@
>          // the batch wants to get re-scheduled, do so.
> +        // (intentional fall-through to re-scheduling logic)

And another one.
Comment on attachment 9084214 [details] [diff] [review]
1572630-no-fallthrough-1.diff

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

::: mail/base/content/mailCommands.js
@@ +181,2 @@
>        }
> +      break;

Yes, all of the others are returns.

::: mail/base/content/messageWindow.js
@@ +933,5 @@
>      switch (command) {
>        case "cmd_createFilterFromPopup":
>        case "cmd_createFilterFromMenu":
>          loadedFolder = gFolderDisplay.displayedFolder;
> +        return loadedFolder && loadedFolder.server.canHaveFilters;

It looks like a bug. I can't imagine why falling through would be right.

::: mail/base/modules/GlobalPopupNotifications.jsm
@@ -499,5 @@
>          if (this.isPanelOpen) {
>            for (let elt of this.panel.children) {
>              elt.notification.timeShown = this.window.performance.now();
>            }
> -          break;

That's what they did upstream. https://hg.mozilla.org/mozilla-central/diff/c0ba1b368b5880253f4c6a5b3deced3d7044cf19/toolkit/modules/PopupNotifications.jsm

::: mail/components/customizableui/PanelMultiView.jsm
@@ +1685,2 @@
>        }
> +      // Fall-through...

No it doesn't care as long as there's something close to "fall" and "through". I've just used existing comments in these places.

(In reply to Geoff Lankow (:darktrojan) from comment #3)

That's what they did upstream.
https://hg.mozilla.org/mozilla-central/diff/c0ba1b368b5880253f4c6a5b3deced3d7044cf19/toolkit/modules/PopupNotifications.jsm

Right:
https://hg.mozilla.org/mozilla-central/rev/c0ba1b368b5880253f4c6a5b3deced3d7044cf19#l13.12
I asked in bug 1571567 comment #21. That certainly changes the behaviour if this.isPanelOpen.

I'd give an r+ with the break restored and the break changed to a return where discussed.

Also see bug 1571567, comment #22. They had doubts whether removing the break was OK, but it got landed anyway after being marked "Done".

Comment on attachment 9084214 [details] [diff] [review]
1572630-no-fallthrough-1.diff

Going t o fix the patch now and land it.
Attachment #9084214 - Flags: review?(jorgk)

Put the break back and changed a break to a return.

Attachment #9084393 - Flags: review+
Comment on attachment 9084393 [details] [diff] [review]
1572630-no-fallthrough-1.diff (JK)

If you want, you can look at this one.
Attachment #9084393 - Flags: review?(mkmelin+mozilla)
Attachment #9084214 - Flags: review?(mkmelin+mozilla)

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0659dff2f7e8
Port bug 1571567: Enable the no-fallthrough eslint rule. r=jorgk

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Attachment #9084393 - Flags: review?(mkmelin+mozilla)
You need to log in before you can comment on or make changes to this bug.