Bug 236254 – Permit simultaneous Apple Pay and script injection

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

[Devin Rousso]

.

[Devin Rousso]

<rdar://problem/87736727>

[Radar WebKit Bug Importer]

<rdar://problem/88590628>

[Devin Rousso]

Created attachment 451143 [details]
Patch

[Devin Rousso]

<rdar://problem/87736727>

[Kate Cheney]

Comment on attachment 451143 [details]
Patch

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

code change looks good, only concern is about dropping the weakThis in PaymentCoordinator::canMakePaymentsWithActiveCard

> Source/WebCore/Modules/applepay/PaymentCoordinator.cpp:69
> +bool PaymentCoordinator::canMakePayments(Document&)

Why keep Document around?

> Source/WebCore/Modules/applepay/PaymentCoordinator.cpp:78
> +    m_client.canMakePaymentsWithActiveCard(merchantIdentifier, document.domain(), [this, completionHandler = WTFMove(completionHandler)](bool canMakePayments) {

Seems like we shouldn't drop weakThis, unless I'm missing something

> Source/WebCore/Modules/applepay/PaymentCoordinatorClient.h:69
>      virtual String userAgentScriptsBlockedErrorMessage() const { return { }; }

I think this can be removed too

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ApplePay.mm:-243
> -TEST(ApplePay, ActiveSessionBlocksUserAgentScripts)

Is there a reason we got rid of the ActiveSession test instead if changing it to ActiveSessionDoesNotBlockUserAgentScripts? Might be useful to include in the ChangeLog if so.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/apple-pay-availability-existing-object.html:17
> +        window.wkPaymentRequest ??= new PaymentRequest([applePayMethod()], applePayDetails);

getting fancy with some nullish coalescing operators :P

[Devin Rousso]

Comment on attachment 451143 [details]
Patch

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

>> Source/WebCore/Modules/applepay/PaymentCoordinator.cpp:69
>> +bool PaymentCoordinator::canMakePayments(Document&)
> 
> Why keep Document around?

I figured just in case it was needed in the future (just like how `supportsVersion` has it).  I'll remove it.

>> Source/WebCore/Modules/applepay/PaymentCoordinator.cpp:78
>> +    m_client.canMakePaymentsWithActiveCard(merchantIdentifier, document.domain(), [this, completionHandler = WTFMove(completionHandler)](bool canMakePayments) {
> 
> Seems like we shouldn't drop weakThis, unless I'm missing something

Eh, yeah probably.  The only reason we capture `this` is cause `PAYMENT_COORDINATOR_RELEASE_LOG` uses it, and it just logs the raw pointer value (i.e. location).  Nothing actually dereferences `this` inside here.  But probably couldn't hurt to keep it :)

>> Source/WebCore/Modules/applepay/PaymentCoordinatorClient.h:69
>>      virtual String userAgentScriptsBlockedErrorMessage() const { return { }; }
> 
> I think this can be removed too

Oh oops good catch!

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ApplePay.mm:-243
>> -TEST(ApplePay, ActiveSessionBlocksUserAgentScripts)
> 
> Is there a reason we got rid of the ActiveSession test instead if changing it to ActiveSessionDoesNotBlockUserAgentScripts? Might be useful to include in the ChangeLog if so.

`apple-pay-active-session.html` only simulated that an active session actually took place (via `Internals.prototype.setApplePayIsActive`).  The rest of the tests actually create an active session, so there's not really a benefit to keeping it (it would basically just be a copy of the other tests).

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/apple-pay-availability-existing-object.html:17
>> +        window.wkPaymentRequest ??= new PaymentRequest([applePayMethod()], applePayDetails);
> 
> getting fancy with some nullish coalescing operators :P

(☞ﾟヮﾟ)☞

[Devin Rousso]

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

[Devin Rousso]

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

[EWS]

Committed r289578 (247095@main): <https://commits.webkit.org/247095@main>

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