Bug 199377 - [WPE] Draw other button types
Summary: [WPE] Draw other button types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-01 13:59 PDT by Joshua Watt
Modified: 2019-07-25 11:04 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.12 KB, patch)
2019-07-01 13:59 PDT, Joshua Watt
no flags Details | Formatted Diff | Diff
Patch (2.09 KB, patch)
2019-07-16 14:05 PDT, Joshua Watt
no flags Details | Formatted Diff | Diff
Patch (2.91 KB, patch)
2019-07-24 11:37 PDT, Joshua Watt
no flags Details | Formatted Diff | Diff
Patch (2.87 KB, patch)
2019-07-25 08:23 PDT, Joshua Watt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.