1572630 - Port bug 1571567 - Enable the no-fallthrough eslint rule

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1572630-no-fallthrough-1.diff

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

Comment 2

[Jorg K (CEST = GMT+2)]

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 3

[Geoff Lankow (:darktrojan)]

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.

Comment 4

[Jorg K (CEST = GMT+2)]

(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 5

[Jorg K (CEST = GMT+2)]

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

Going t o fix the patch now and land it.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1572630-no-fallthrough-1.diff (JK)

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

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9084393 [details] [diff] [review]
1572630-no-fallthrough-1.diff (JK)

If you want, you can look at this one.

Comment 8

[Pulsebot]

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