[Payment Request] Implement PaymentRequest.canMakePayment()
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.
Comment on attachment 323092 [details] Patch Attachment 323092 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4789511 New failing tests: http/tests/paymentrequest/payment-request-canmakepayment-method.https.html
Created attachment 323098 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
(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 323252 [details] Patch
Comment on attachment 323252 [details] Patch Attachment 323252 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4807796 New failing tests: http/tests/paymentrequest/payment-request-abort-method.https.html http/tests/paymentrequest/payment-request-canmakepayment-method.https.html http/tests/paymentrequest/payment-request-show-method.https.html
Created attachment 323274 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
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>