1242135 - Annotate intentional switch fallthrough in widget/gtk/

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: gtk_Wimplicit-fallthrough.patch

clang's -Wimplicit-fallthrough warnings (not yet enabled in mozilla-central) warn about switch cases that fall through without a break or return statement. MOZ_FALLTHROUGH (bug 1215411) is an annotation to suppress -Wimplicit-fallthrough warnings where the fallthrough is intentional.

widget/gtk/nsNativeThemeGTK.cpp:1032:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
widget/gtk/nsNativeThemeGTK.cpp:1058:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
widget/gtk/nsNativeThemeGTK.cpp:1274:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
widget/gtk/nsNativeThemeGTK.cpp:1718:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]

Comment 1

[Karl Tomlinson (:karlt)]

Comment on attachment 8711303 [details] [diff] [review]
gtk_Wimplicit-fallthrough.patch

>@@ -1023,16 +1023,17 @@ nsNativeThemeGTK::GetExtraSizeForWidget(
>         gint top, left, bottom, right;
>         moz_gtk_button_get_default_overflow(&top, &left, &bottom, &right);
>         aExtra->top = top;
>         aExtra->right = right;
>         aExtra->bottom = bottom;
>         aExtra->left = left;
>         break;
>       }
>+      MOZ_FALLTHROUGH; // to get button's outline size
>     }
>   case NS_THEME_FOCUS_OUTLINE:

This fallthrough is actually a bug as it was meant to fall through to the default case.
It is a regression from one or both of these changesets:
https://hg.mozilla.org/mozilla-central/rev/13c6e0ecf8a7#l2.108
https://hg.mozilla.org/mozilla-central/rev/3ad397becd2b#l4.60

NS_THEME_FOCUS_OUTLINE is actually something special.  If there is a need for
extra size for button focus then it should be calculated differently, but I
don't expect there is a need.

Can you return false instead of MOZ_FALLTHROUGH here, please?
If that somehow causes test failures, then please change the comment to FIXME
or similar.

Comment 2

[Chris Peterson [:cpeterson]]

Thanks. I will re-test without the fallthrough. I suspected the NS_THEME_FOCUS_OUTLINE fallthrough might have beeen unintentional (a bug!) but I could also imagine some special case where non-default themed buttons needed to include their outline size. :)

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b28431d80522

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b28431d80522