WebKit support for Apple Pay Buttons with custom corner radii
Created attachment 390563 [details] Patch
Comment on attachment 390563 [details] 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".
Created attachment 390712 [details] Patch
Comment on attachment 390712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390712&action=review > Source/WebCore/PAL/pal/cocoa/PassKitSoftLink.h:50 > SOFT_LINK_FUNCTION_FOR_HEADER(PAL, PassKit, PKDrawApplePayButton, void, (CGContextRef context, CGRect drawRect, CGFloat scale, PKPaymentButtonType type, PKPaymentButtonStyle style, NSString *languageCode), (context, drawRect, scale, type, style, languageCode)) We can remove this now. > Source/WebCore/PAL/pal/cocoa/PassKitSoftLink.mm:54 > SOFT_LINK_FUNCTION_FOR_SOURCE(PAL, PassKit, PKDrawApplePayButton, void, (CGContextRef context, CGRect drawRect, CGFloat scale, PKPaymentButtonType type, PKPaymentButtonStyle style, NSString *languageCode), (context, drawRect, scale, type, style, languageCode)) Ditto. > Source/WebCore/PAL/pal/spi/cocoa/PassKitSPI.h:366 > extern "C" > void PKDrawApplePayButton(_Nonnull CGContextRef, CGRect drawRect, CGFloat scale, PKPaymentButtonType, PKPaymentButtonStyle, NSString * _Nullable languageCode); Ditto. > Source/WebCore/rendering/RenderThemeCocoa.mm:103 > +static float getMaxBorderRadius(const RenderStyle& renderStyle) > +{ > + return renderStyle.hasBorderRadius() ? std::max({renderStyle.borderTopLeftRadius().height.value(), renderStyle.borderTopLeftRadius().width.value(), renderStyle.borderTopRightRadius().height.value(), renderStyle.borderTopRightRadius().width.value(), renderStyle.borderBottomLeftRadius().height.value(), renderStyle.borderBottomLeftRadius().width.value(), renderStyle.borderBottomRightRadius().height.value(), renderStyle.borderBottomRightRadius().width.value()}) : PAL::get_PassKit_PKApplePayButtonDefaultCornerRadius(); > +} We should return a CGFloat, and we typically don't prefix functions like this with "get". A suggestion for even easier reading: static CGFloat largestCornerRadius(const RenderStyle& style) { if (!renderStyle.hasBorderRadius()) return PAL::get_PassKit_PKApplePayButtonDefaultCornerRadius(); return std::max<CGFloat>({ style.borderTopLeftRadius().height.value(), style.borderTopLeftRadius().width.value(), style.borderTopRightRadius().height.value(), style.borderTopRightRadius().width.value(), style.borderBottomLeftRadius().height.value(), style.borderBottomLeftRadius().width.value(), style.borderBottomRightRadius().height.value(), style.borderBottomRightRadius().width.value() }); } > LayoutTests/http/tests/ssl/applepay/ApplePayButtonV4.html:46 > + for (let style of ["white-outline", "black"]) { Did you mean to remove the "white" style? Maybe we should render all the buttons over a neither-white-nor-black background so that it's easy to see the corner radius in all cases.
Created attachment 390779 [details] Patch
Comment on attachment 390779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390779&action=review One last comment, but otherwise the patch looks great. You can upload a new patch with my named filled in to the "Reviewed by" lines in the ChangeLog. You don't need to set the review bit, but if you set the cq? bit I'll cq+ it. Thanks Nikos! > LayoutTests/http/tests/ssl/applepay/ApplePayButton.html:55 > + if((type == "plain" || type == "set-up")){ Why only these two types?
Created attachment 390799 [details] Patch
Comment on attachment 390799 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390799&action=review > LayoutTests/http/tests/ssl/applepay/ApplePayButton.html:10 > + float: left; Ah, I get why you excluded some types now. Good fix.
The commit-queue encountered the following flaky tests while processing attachment 390799 [details]: imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events.html bug 207790 (author: graouts@apple.com) The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 390799 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) http/tests/security/contentSecurityPolicy/report-status-code-zero-when-using-https.html bug 197297 (author: dbates@webkit.org) The commit-queue is continuing to process your patch.
Comment on attachment 390799 [details] Patch Clearing flags on attachment: 390799 Committed r256648: <https://trac.webkit.org/changeset/256648>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59474051>
Comment on attachment 390799 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390799&action=review > LayoutTests/ChangeLog:18 > + * platform/mac-highsierra/http/tests/ssl/applepay/ApplePayButton-expected.png: > + * platform/mac-highsierra/http/tests/ssl/applepay/ApplePayButton-expected.txt: > + * platform/mac/http/tests/ssl/applepay/ApplePayButton-expected.png: > + * platform/mac/http/tests/ssl/applepay/ApplePayButton-expected.txt: > + * platform/mac/http/tests/ssl/applepay/ApplePayButtonV4-expected.png: > + * platform/mac/http/tests/ssl/applepay/ApplePayButtonV4-expected.txt: We don't run pixel tests, so these tests are useless. You need to concoct some reference tests with transforms and marks to detect the kind of corner rendering.