Bug 199377 – [WPE] Draw other button types

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Joshua Watt]

[WPE] Draw other button types

[Joshua Watt]

Created attachment 373259 [details]
Patch

[Michael Catanzaro]

Comment on attachment 373259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373259&action=review

Could you please add a ChangeLog entry (Tools/Scripts/prepare-ChangeLog -b 199377)?

Then the ChangeLog can explain more directly why you're changing what you're changing here.

> Source/WebCore/platform/wpe/ThemeWPE.cpp:71
> +    case DefaultButtonPart:
> +    case ButtonPart:

Please use fallthrough annotations:

case DefaultButtonPart:
    FALLTHROUGH;
case ButtonPart:
    FALLTHROUGH;
case SquareButtonPart:

> Source/WebCore/platform/wpe/ThemeWPE.cpp:142
> +    float roundness = 2;
> +
> +    if (part == SquareButtonPart)
> +        roundness = 0;

It'd be nicer to write this in one line IMO:

float roundness = (part == SquareButtonPart) ? 0 : 2;

[Carlos Garcia Campos]

Comment on attachment 373259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373259&action=review

>> Source/WebCore/platform/wpe/ThemeWPE.cpp:71
>> +    case ButtonPart:
> 
> Please use fallthrough annotations:
> 
> case DefaultButtonPart:
>     FALLTHROUGH;
> case ButtonPart:
>     FALLTHROUGH;
> case SquareButtonPart:

I don't think we want FALLTHROUGH in those cases, because it's obvious. FALLTHROUGH is useful when you actually have code in the case and want to fallthrough to the next case, to make it explicit that you haven't forgotten the break.

[Michael Catanzaro]

OK, as you prefer

[Joshua Watt]

Created attachment 374239 [details]
Patch

[Joshua Watt]

Sorry I forgot the changelog. This is my first WebKit bug, the tooling scripts for uploading changes are still a little clunky to me.

[Michael Catanzaro]

(In reply to Joshua Watt from comment #5)
> Created attachment 374239 [details]
> Patch

This new patch is actually still missing the ChangeLog. :)

[Joshua Watt]

Created attachment 374790 [details]
Patch

[Michael Catanzaro]

Comment on attachment 374790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374790&action=review

> Source/WebCore/ChangeLog:8
> +        [WPE] Draw other button types

Don't duplicate the title here.

[Joshua Watt]

Created attachment 374889 [details]
Patch

[Michael Catanzaro]

Comment on attachment 374889 [details]
Patch

Request cq? when you're ready for commit.

[WebKit Commit Bot]

Comment on attachment 374889 [details]
Patch

Clearing flags on attachment: 374889

Committed r247827: <https://trac.webkit.org/changeset/247827>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.