Bug 199377

Summary: [WPE] Draw other button types
Product: WebKit Reporter: Joshua Watt <JPEW.hacker>
Component: WPE WebKitAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cgarcia, commit-queue, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Joshua Watt 2019-07-01 13:59:13 PDT
[WPE] Draw other button types
Comment 1 Joshua Watt 2019-07-01 13:59:28 PDT
Created attachment 373259 [details]
Patch
Comment 2 Michael Catanzaro 2019-07-16 07:59:48 PDT
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;
Comment 3 Carlos Garcia Campos 2019-07-16 08:10:43 PDT
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.
Comment 4 Michael Catanzaro 2019-07-16 08:40:35 PDT
OK, as you prefer
Comment 5 Joshua Watt 2019-07-16 14:05:53 PDT
Created attachment 374239 [details]
Patch
Comment 6 Joshua Watt 2019-07-16 14:07:03 PDT
Sorry I forgot the changelog. This is my first WebKit bug, the tooling scripts for uploading changes are still a little clunky to me.
Comment 7 Michael Catanzaro 2019-07-16 16:25:59 PDT
(In reply to Joshua Watt from comment #5)
> Created attachment 374239 [details]
> Patch

This new patch is actually still missing the ChangeLog. :)
Comment 8 Joshua Watt 2019-07-24 11:37:59 PDT
Created attachment 374790 [details]
Patch
Comment 9 Michael Catanzaro 2019-07-24 12:04:16 PDT
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.
Comment 10 Joshua Watt 2019-07-25 08:23:16 PDT
Created attachment 374889 [details]
Patch
Comment 11 Michael Catanzaro 2019-07-25 09:37:44 PDT
Comment on attachment 374889 [details]
Patch

Request cq? when you're ready for commit.
Comment 12 WebKit Commit Bot 2019-07-25 11:04:22 PDT
Comment on attachment 374889 [details]
Patch

Clearing flags on attachment: 374889

Committed r247827: <https://trac.webkit.org/changeset/247827>
Comment 13 WebKit Commit Bot 2019-07-25 11:04:24 PDT
All reviewed patches have been landed.  Closing bug.