[webkit-reviews] review granted: [Bug 237177] [GPU Process] dont load Apple Pay button/logo PDFs in the WebProcess : [Attachment 453653] [fast-cq] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 237177: [GPU Process] dont load Apple Pay button/logo PDFs in the
WebProcess
https://bugs.webkit.org/show_bug.cgi?id=237177

Attachment 453653: [fast-cq] Patch

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




--- Comment #5 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 453653
  --> https://bugs.webkit.org/attachment.cgi?id=453653
[fast-cq] Patch

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

> Source/WebCore/Modules/applepay/ApplePayButtonSystemImage.mm:92
> +    CGContextSetShouldSmoothFonts(platformContext, true);

Necessary?

> Source/WebCore/Modules/applepay/ApplePayLogoSystemImage.mm:46
> +#if PLATFORM(MAC)
> +	   passKitBundle = [NSBundle bundleWithURL:[NSURL
fileURLWithPath:@"/System/Library/PrivateFrameworks/PassKit.framework"
isDirectory:YES]];
> +#else
> +	   dlopen("/System/Library/Frameworks/PassKit.framework/PassKit",
RTLD_NOW);
> +	   passKitBundle = [NSBundle
bundleForClass:NSClassFromString(@"PKPaymentAuthorizationViewController")];

Not new code, but this is quite something.

> Source/WebCore/Modules/applepay/ApplePayLogoSystemImage.mm:65
> +static CGPDFPageRef applePayLogoWhite()

Would be nicer to return RetainPtr<CGPDFPageRef>

> Source/WebCore/Modules/applepay/ApplePayLogoSystemImage.mm:67
> +    static CGPDFPageRef logoPage;

Might be clearer for these to be NeverDestoyed<RetainPtr<>> since they are
basically leaked.

> Source/WebCore/Modules/credentialmanagement/BasicCredential.h:38
> +class Document;
> +template<typename IDLType> class DOMPromiseDeferred;
> +struct IDLBoolean;

I don't think these should be in this patch.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:642
> +void GraphicsContext::drawSystemImage(const Ref<SystemImage>& systemImage,
const FloatRect& destination)

Please rename "destination" to "destinationRect" everywhere ("destination"
could be taken to refer to the destination context).

> Source/WebCore/platform/graphics/GraphicsContext.cpp:877
> +    float scale;
> +    float translationX = 0;
> +    float translationY = 0;
> +    if (srcSize.aspectRatio() > dstSize.aspectRatio()) {
> +	   scale = dstSize.width() / srcSize.width();
> +	   translationY = (dstSize.height() - scale * srcSize.height()) / 2;
> +    } else {
> +	   scale = dstSize.height() / srcSize.height();
> +	   translationX = (dstSize.width() - scale * srcSize.width()) / 2;
> +    }

This math belongs in GeometryUtilities.cpp (and can probably share logic with
existing functions there).

> Source/WebCore/platform/graphics/GraphicsContext.h:432
> +    virtual void drawSystemImage(const Ref<SystemImage>&, const FloatRect&);

Just pass SystemImage&