Bug 237177 – [GPU Process] dont load Apple Pay button/logo PDFs in the WebProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Devin Rousso]

.

[Devin Rousso]

Created attachment 453170 [details]
[fast-cq] Patch

[Simon Fraser (smfr)]

Comment on attachment 453170 [details]
[fast-cq] Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.h:456
> +#if ENABLE(APPLE_PAY)
> +    virtual void paintApplePayButton(ApplePayButtonType, ApplePayButtonStyle, const String& locale, float largestCornerRadius, const FloatRect&) = 0;
> +    virtual void paintApplePayLogo(ApplePayLogoStyle, const FloatRect&) = 0;
> +#endif

I don't think GraphicsContext should know about Apple Pay. Can we make this more generic (paintNamedImage) and do mapping of button types to more generic strings and back? Then you don't need all the ApplePay specific display list stuff.

[Devin Rousso]

Created attachment 453651 [details]
[fast-cq] Patch

[Devin Rousso]

Created attachment 453653 [details]
[fast-cq] Patch

[Simon Fraser (smfr)]

Comment on attachment 453653 [details]
[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&

[Devin Rousso]

Comment on attachment 453653 [details]
[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?

I think this is the equivalent of calling `context.setShouldSmoothFonts(true);`.  I suppose I could've left it as
```
GraphicsContextStateSaver stateSaver(context);
	 	
context.setShouldSmoothFonts(true);
context.scale(FloatSize(1, -1));
```
instead of using `platformContext()`.

>> Source/WebCore/Modules/credentialmanagement/BasicCredential.h:38
>> +struct IDLBoolean;
> 
> I don't think these should be in this patch.

Hmm previously this patch didnt build without these, but I guess it's been fixed since then?  I'll remove.

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:877
>> +    }
> 
> This math belongs in GeometryUtilities.cpp (and can probably share logic with existing functions there).

Are you suggesting I create something like a `struct ScaledPoint` that contains both a `FloatPoint translation` and `float scale` (or maybe just use an `AffineTransform`) and then have that be returned by a new method named something like `calculateScaleToFit`?

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

I tried doing that before, but I ran into issues when trying to `decode` the given `SystemImage` in the GPUProcess, because I think the `std::optional<SystemImage>` only has space allocated for the `SystemImage` type, not the subclass.  I ended up having `SystemImage` inherit from `RefCounted` so that we could just use a pointer (and `Ref`) to avoid the sizing issue.

Or were you perhaps suggesting that I could have this be `SystemImage&` here and then just `Ref { systemImage }` only when I'm about to send it via IPC (and `.get()` on the other end)?

[Simon Fraser (smfr)]

(In reply to Devin Rousso from comment #6)
> Comment on attachment 453653 [details]
> [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?
> 
> I think this is the equivalent of calling
> `context.setShouldSmoothFonts(true);`.  I suppose I could've left it as
> ```
> GraphicsContextStateSaver stateSaver(context);
> 	 	
> context.setShouldSmoothFonts(true);
> context.scale(FloatSize(1, -1));
> ```
> instead of using `platformContext()`.

The question is really about why font smoothing is special when drawing apply pay buttons. Isn't smoothing on by default? Is this only relevant when CSS turns that off?

> >> Source/WebCore/platform/graphics/GraphicsContext.cpp:877
> >> +    }
> > 
> > This math belongs in GeometryUtilities.cpp (and can probably share logic with existing functions there).
> 
> Are you suggesting I create something like a `struct ScaledPoint` that
> contains both a `FloatPoint translation` and `float scale` (or maybe just
> use an `AffineTransform`) and then have that be returned by a new method
> named something like `calculateScaleToFit`?

I think this is an aspect-ratio-preserving fit, right? So call largestRectWithAspectRatioInsideRect() and then compute your translate etc from the result. Just don't duplicate aspect-ratio sizing math.

> >> Source/WebCore/platform/graphics/GraphicsContext.h:432
> >> +    virtual void drawSystemImage(const Ref<SystemImage>&, const FloatRect&);
> > 
> > Just pass SystemImage&
> 
> I tried doing that before, but I ran into issues when trying to `decode` the
> given `SystemImage` in the GPUProcess, because I think the
> `std::optional<SystemImage>` only has space allocated for the `SystemImage`
> type, not the subclass.  I ended up having `SystemImage` inherit from
> `RefCounted` so that we could just use a pointer (and `Ref`) to avoid the
> sizing issue.
> 
> Or were you perhaps suggesting that I could have this be `SystemImage&` here
> and then just `Ref { systemImage }` only when I'm about to send it via IPC
> (and `.get()` on the other end)?

I'm not sure why the signature of drawSystemImage() affects IPC code; you can still downcast<> a reference.

[Devin Rousso]

Created attachment 453762 [details]
[fast-cq] Patch

[Devin Rousso]

Created attachment 453769 [details]
[fast-cq] Patch

[Radar WebKit Bug Importer]

<rdar://problem/89792781>

[EWS]

Committed r290813 (248049@main): <https://commits.webkit.org/248049@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453769 [details].

[Jon Lee]

*** Bug 236925 has been marked as a duplicate of this bug. ***