[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