[webkit-reviews] review denied: [Bug 207664] WebKit support for Apple Pay Buttons with custom corner radii : [Attachment 390563] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 13 16:38:50 PST 2020


Andy Estes <aestes at apple.com> has denied Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 207664: WebKit support for Apple Pay Buttons with custom corner radii
https://bugs.webkit.org/show_bug.cgi?id=207664

Attachment 390563: Patch

https://bugs.webkit.org/attachment.cgi?id=390563&action=review




--- Comment #2 from Andy Estes <aestes at apple.com> ---
Comment on attachment 390563
  --> https://bugs.webkit.org/attachment.cgi?id=390563
Patch

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

> Source/WebCore/rendering/RenderThemeCocoa.mm:112
> +	  
PKDrawApplePayButtonWithCornerRadius(paintInfo.context().platformContext(),
CGRectMake(paintRect.x(), -paintRect.maxY(), paintRect.width(),
paintRect.height()), 1.0, std::max({topLeft.height.value(),
topLeft.width.value(), topRight.height.value(), topRight.width.value(),
bottomLeft.height.value(), bottomLeft.width.value(),
bottomRight.height.value(), bottomRight.width.value()}),
toPKPaymentButtonType(renderer.style().applePayButtonType()),
toPKPaymentButtonStyle(renderer.style().applePayButtonStyle()),
renderer.style().locale());

Pretty hard to read all this as one statement. Can you compute the corner
radius separately? Maybe a static helper function that takes a `const
RenderStyle&` and returns the (maybe default) corner radius?

> Source/WebCore/rendering/RenderThemeCocoa.mm:116
>      PKDrawApplePayButton(paintInfo.context().platformContext(),
CGRectMake(paintRect.x(), -paintRect.maxY(), paintRect.width(),
paintRect.height()), 1.0,
toPKPaymentButtonType(renderer.style().applePayButtonType()),
toPKPaymentButtonStyle(renderer.style().applePayButtonStyle()),
renderer.style().locale());

I would call PKDrawApplePayButtonWithCornerRadius whether or not there is a
`border-radius`. If there's no border radius, you can use
`PKDrawApplePayButtonWithCornerRadius` as a default (see
SOFT_LINK_CONSTANT_FOR_{HEADER,SOURCE} for how to load it).

> LayoutTests/http/tests/ssl/applepay/ApplePayButton.html:39
> +	   .borderRadiusLarge{

Missing a space before the {.

> LayoutTests/http/tests/ssl/applepay/ApplePayButton.html:43
> +	       border-top-left-radius: 20px;
> +	       border-top-right-radius: 25px;
> +	       border-bottom-left-radius: 10px;
> +	       border-bottom-right-radius: 5px;

How about cases where the width and height differ?

> LayoutTests/http/tests/ssl/applepay/ApplePayButton.html:50
> +	   for (let style of ["white","white-outline", "black"]) {

Missing a space before "white-outline".


More information about the webkit-reviews mailing list