Created attachment 323092 [details] Patch

Comment on attachment 323092 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323092&action=review > Source/WebCore/Modules/applepay/PaymentCoordinator.h:82 > + PaymentCoordinatorClient* m_testingClient { nullptr }; Would it be possible for Internals to create a PaymentsCoordinator with a mock client and replace the default PaymentsCoordinator when being used in WTR. Or are you planning to support testing clients with regular browsers? > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:175 > + completionHandler(paymentCoordinator(document).canMakePayments()); Why not completionHandler(false)? > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:433 > + handler->canMakePayment(downcast<Document>(*scriptExecutionContext()), [promise = WTFMove(promise)](bool canMakePayment) mutable { Can we make canMakePayment take a Document& with CallWith=Document in the IDL? I would probably also do the same for the other methods using scriptExecutionContext() with CallWith=ScriptExecutionContext. Should canMakePayment completion handler take an ExceptionOr<bool>? > Source/WebCore/testing/MockPaymentCoordinator.cpp:40 > + mockPaymentCoordinator->ref(); // Balanced by deref() in paymentCoordinatorDestroyed() That seems a bit strange. Cannot we store/access the MockPaymentCoordinator as an Internals attribute? > Source/WebCore/testing/MockPaymentCoordinator.cpp:66 > + completionHandler(true); For better mocking, should this be done asynchronously? > Source/WebCore/testing/MockPaymentCoordinator.cpp:71 > + completionHandler(true); Ditto.

(In reply to youenn fablet from comment #2) > Comment on attachment 323092 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323092&action=review > > > Source/WebCore/Modules/applepay/PaymentCoordinator.h:82 > > + PaymentCoordinatorClient* m_testingClient { nullptr }; > > Would it be possible for Internals to create a PaymentsCoordinator with a > mock client and replace the default PaymentsCoordinator when being used in > WTR. > Or are you planning to support testing clients with regular browsers? I like this idea. I ended up implementing it in another patch since it turns out I needed a mock there too (see r223076). > > > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:175 > > + completionHandler(paymentCoordinator(document).canMakePayments()); > > Why not completionHandler(false)? Calling canMakePayments() is the correct fallback when applePayCapabilityDisclosureAllowed() is false. > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:433 > > + handler->canMakePayment(downcast<Document>(*scriptExecutionContext()), [promise = WTFMove(promise)](bool canMakePayment) mutable { > > Can we make canMakePayment take a Document& with CallWith=Document in the > IDL? > I would probably also do the same for the other methods using > scriptExecutionContext() with CallWith=ScriptExecutionContext. Ok sure. > > Should canMakePayment completion handler take an ExceptionOr<bool>? I don't think so. There are no exceptions to be thrown by the completion handler. > > > Source/WebCore/testing/MockPaymentCoordinator.cpp:40 > > + mockPaymentCoordinator->ref(); // Balanced by deref() in paymentCoordinatorDestroyed() > > That seems a bit strange. > Cannot we store/access the MockPaymentCoordinator as an Internals attribute? Yes, this probably makes sense. For now I didn't expose the mock at all since I don't (yet) need to configure it from script. > > > Source/WebCore/testing/MockPaymentCoordinator.cpp:66 > > + completionHandler(true); > > For better mocking, should this be done asynchronously? I did this in r223076.

Created attachment 323348 [details] Patch

Comment on attachment 323348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323348&action=review > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:401 > + if (exception.hasException()) > + m_showPromise->reject(exception.releaseException()); Oops, I need to return after rejecting if there is an exception.

Comment on attachment 323348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323348&action=review > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:343 > +static ExceptionOr<JSC::JSValue> parse(ScriptExecutionContext& context, const String& string) This routine is nice and might be useful elsewhere now or in the future. > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:405 > setPendingActivity(this); I usually like to have setPendingActivity/unsetPendingActivity close one to the other. That is made a bit more easy with a call to setPendingActivity and then a lambda with an unsetPendingActivity inside it. > LayoutTests/http/tests/paymentrequest/payment-request-canmakepayment-method.https.html:33 > +promise_test(async t => { This mix of async/await with promise_test looks good! > LayoutTests/http/tests/paymentrequest/payment-request-canmakepayment-method.https.html:48 > + "if it throws, then it must be a NotAllowedError." It seems strange that we are not always expecting either an exception or no exception.

Created attachment 323360 [details] Patch

(In reply to youenn fablet from comment #11) > Comment on attachment 323348 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323348&action=review > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:405 > > setPendingActivity(this); > > I usually like to have setPendingActivity/unsetPendingActivity close one to > the other. > That is made a bit more easy with a call to setPendingActivity and then a > lambda with an unsetPendingActivity inside it. For now I moved stop() directly below show() and added a comment. > > > LayoutTests/http/tests/paymentrequest/payment-request-canmakepayment-method.https.html:48 > > + "if it throws, then it must be a NotAllowedError." > > It seems strange that we are not always expecting either an exception or no > exception. The reason for this check is that the user agent is allowed to abort a request if canMakePayment() is called too many times (to prevent fingerprinting/abuse). So in some implementations, the second call might abort the request resulting in a NotAllowedError. We don't implement any rate-limiting yet. Thanks for the review!

Comment on attachment 323360 [details] Patch Clearing flags on attachment: 323360 Committed r223160: <http://trac.webkit.org/changeset/223160>

All reviewed patches have been landed. Closing bug.

<rdar://problem/34924234>