.
<rdar://problem/87736727>
<rdar://problem/88590628>
Created attachment 451143 [details] Patch
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
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 (☞ﾟヮﾟ)☞
Created attachment 451312 [details] [fast-cq] Patch
Created attachment 451580 [details] [fast-cq] Patch
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].