Created attachment 322683 [details] Patch

Comment on attachment 322683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322683&action=review > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.h:51 > + Document& m_document; Since this is asynchronous, I am not sure Document& might still be there at the time it will be used. It may be best to pass it as a parameter, make ApplePayPaymentHandler a ContextDestructionObserver or some other way. > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:360 > auto scope = DECLARE_THROW_SCOPE(scriptExecutionContext()->vm()); PaymentRequest::finishShowing is called asynchronously before the patch and I guess the current patch might end up with the same case. Since PaymentRequest is an ActiveDOMObject, it might be stopped between show() is finished and finishShowing is not yet called. If stopped, this method should exit early. Similar principle should be done with ApplePayPaymentHandler in the current patch.

(In reply to youenn fablet from comment #13) > Comment on attachment 322683 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322683&action=review > > > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.h:51 > > + Document& m_document; > > Since this is asynchronous, I am not sure Document& might still be there at > the time it will be used. > It may be best to pass it as a parameter, make ApplePayPaymentHandler a > ContextDestructionObserver or some other way. There's no asynchrony. ApplePayPaymentHandlers do not outlive the call to PaymentRequest::show(). > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:360 > > auto scope = DECLARE_THROW_SCOPE(scriptExecutionContext()->vm()); > > PaymentRequest::finishShowing is called asynchronously before the patch and > I guess the current patch might end up with the same case. > Since PaymentRequest is an ActiveDOMObject, it might be stopped between > show() is finished and finishShowing is not yet called. > If stopped, this method should exit early. > Similar principle should be done with ApplePayPaymentHandler in the current > patch. finishShowing() was removed by this patch. Everything is done directly inside show().

Comment on attachment 322683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322683&action=review > Source/WebCore/Modules/applepay/PaymentRequestValidator.mm:93 > + if (amount > 100000000) Where does this number come from?

(In reply to Alex Christensen from comment #15) > Comment on attachment 322683 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322683&action=review > > > Source/WebCore/Modules/applepay/PaymentRequestValidator.mm:93 > > + if (amount > 100000000) > > Where does this number come from? Unknown, but this check has existed since the beginning of Apple Pay.

Created attachment 322725 [details] Patch

Comment on attachment 322725 [details] Patch Didn't have time for a full review. Here are some random points. View in context: https://bugs.webkit.org/attachment.cgi?id=322725&action=review > Source/WebCore/ChangeLog:24 > + Test: http/tests/ssl/applepay/PaymentRequest.html If a test has the .https prefix, it is served through https. Can we rename it to http/tests/ssl/applepay/PaymentRequest.https.html? That way, we might be able to move away from ssl folder. > Source/WebCore/Modules/applepay/ApplePayContactField.cpp:33 > +ExceptionOr<ApplePaySessionPaymentRequest::ContactFields> convertAndValidate(unsigned version, Vector<ApplePayContactField>&& contactFields) Does contactFields need to take a &&? > Source/WebCore/Modules/applepay/ApplePayMerchantCapability.cpp:33 > +ExceptionOr<ApplePaySessionPaymentRequest::MerchantCapabilities> convertAndValidate(Vector<ApplePayMerchantCapability>&& merchantCapabilities) Ditto here for &&. > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:55 > + : m_document { document } It is still not very clear to me why m_document is needed here. Is it for future developments? As it is, it seems that this is only used inside convertData to get an ExecState. Can convertData take an ExecState& directly? Also, we create a vector of PaymentHandler in show(), but we only pick the first one. What is the processing model here?

Created attachment 322789 [details] Patch I've removed the Document reference from ApplePayPaymentHandler to address Youenn's comments. I only needed it to get an ExecState in convertData(), but I can just pass one in instead.

(In reply to youenn fablet from comment #23) > Comment on attachment 322725 [details] > Patch > > Didn't have time for a full review. > Here are some random points. Thanks for your comments! > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322725&action=review > > > Source/WebCore/ChangeLog:24 > > + Test: http/tests/ssl/applepay/PaymentRequest.html > > If a test has the .https prefix, it is served through https. > Can we rename it to http/tests/ssl/applepay/PaymentRequest.https.html? > That way, we might be able to move away from ssl folder. Ok, I'll rename this. > > > Source/WebCore/Modules/applepay/ApplePayContactField.cpp:33 > > +ExceptionOr<ApplePaySessionPaymentRequest::ContactFields> convertAndValidate(unsigned version, Vector<ApplePayContactField>&& contactFields) > > Does contactFields need to take a &&? > > > Source/WebCore/Modules/applepay/ApplePayMerchantCapability.cpp:33 > > +ExceptionOr<ApplePaySessionPaymentRequest::MerchantCapabilities> convertAndValidate(Vector<ApplePayMerchantCapability>&& merchantCapabilities) > > Ditto here for &&. I'll change to a const &. > > > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:55 > > + : m_document { document } > > It is still not very clear to me why m_document is needed here. > Is it for future developments? > > As it is, it seems that this is only used inside convertData to get an > ExecState. > Can convertData take an ExecState& directly? Yeah, that makes more sense. I did that in the patch I just uploaded (before I saw your comments, oops!). I did plan to use it in a follow-on in order for ApplePayPaymentHandler::show() to get to the PaymentCoordinator stored on the main frame, but I can just pass a Document to show() instead. > > Also, we create a vector of PaymentHandler in show(), but we only pick the > first one. > What is the processing model here? According to the spec, the model is that we should allow the user to choose between the supported payment methods. But right now we only support Apple Pay, so for now we just need to show the first one. I could probably get rid of the Vector and just store the first non-null PaymentHandler. I still need to loop through all the payment methods, though, since the spec says we should JSON-parse all the method data and throw any exceptions.

Created attachment 322796 [details] Patch

Comment on attachment 322796 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322796&action=review > Source/WebCore/Modules/applepay/ApplePayPaymentRequest.h:38 > +struct ApplePayPaymentRequest : public ApplePayRequest { Can probably remove public here. > Source/WebCore/Modules/applepay/ApplePayRequestBase.cpp:33 > +static ExceptionOr<Vector<String>> convertAndValidate(unsigned version, Vector<String>&& supportedNetworks) Why not having something like std::optional<Exception> validate(const Vector<String>&, unsigned version)? That would remove the WTFMove uses. > Source/WebCore/Modules/applepay/ApplePayRequestBase.cpp:46 > +ExceptionOr<ApplePaySessionPaymentRequest> convertAndValidate(unsigned version, ApplePayRequestBase& request) Since we are moving some of the internal members of request, would it make sense to ask for passing an ApplePayRequestBase&& to improve clarity? > Source/WebCore/Modules/applepay/ApplePayRequestBase.h:34 > +#include <wtf/text/WTFString.h> We might not need Vector.h and WTFString.h > Source/WebCore/Modules/applepay/ApplePaySession.cpp:215 > + ApplePaySessionPaymentRequest result = convertedRequest.releaseReturnValue(); use auto? > Source/WebCore/Modules/applepay/PaymentRequestValidator.mm:42 > +static ExceptionOr<void> validateShippingMethod(const ApplePaySessionPaymentRequest::ShippingMethod&); How about using std::optional instead? That would allow writing something like: exception = validateXX(); if (exception) return exception.value(); > Source/WebCore/Modules/applepay/PaymentRequestValidator.mm:44 > + Remove this line? > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:41 > +#include "ScriptExecutionContext.h" Probably not all includes are needed, for instance ScriptExecutionContext.h. > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:74 > + return result; You can probably write this function as follows: return WTF::map(lineItems, convert); > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:111 > +ExceptionOr<void> ApplePayPaymentHandler::convertData(JSC::ExecState& execState, JSC::JSValue&& data) How about passing m_paymentRequest as a parameter here as well? > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:116 > + return Exception { ExistingExceptionError }; I didn't know about that, I guess it is forwarding the error, nice. > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.h:47 > + ApplePayPaymentHandler(PaymentRequest&); If we keep it like that, we probably want to use explicit. > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.h:50 > + void show() override; final here and there. > Source/WebCore/Modules/paymentrequest/PaymentHandler.h:39 > +class Document; not needed anymore. > Source/WebCore/Modules/paymentrequest/PaymentHandler.h:44 > + virtual ~PaymentHandler(); = default? > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:358 > + std::unique_ptr<PaymentHandler> selectedPaymentHandler; Since selectedPaymentHandler is not kept further than this method, should we try not allocating it in the heap? > LayoutTests/http/tests/ssl/applepay/PaymentRequest.https.html:5 > +<script src="../../resources/js-test-pre.js"></script> If we move this test in the future, it might be simpler to use path starting with / Ditto for js-test-post.js > LayoutTests/http/tests/ssl/applepay/PaymentRequest.https.html:10 > +description("Test basic creation of a PaymentRequest object with an Apple Pay payment method."); Nice tests! I guess I am very biased here but I would probably have used testharness.js. Probably not a big deal since these tests might never go to web-platform-test.

(In reply to youenn fablet from comment #27) > Comment on attachment 322796 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322796&action=review > > > Source/WebCore/Modules/applepay/ApplePayRequestBase.cpp:33 > > +static ExceptionOr<Vector<String>> convertAndValidate(unsigned version, Vector<String>&& supportedNetworks) > > Why not having something like std::optional<Exception> validate(const > Vector<String>&, unsigned version)? > That would remove the WTFMove uses. I got rid of the WTFMove and changed the return type to ExceptionOr<void>. > > > Source/WebCore/Modules/applepay/ApplePayRequestBase.cpp:46 > > +ExceptionOr<ApplePaySessionPaymentRequest> convertAndValidate(unsigned version, ApplePayRequestBase& request) > > Since we are moving some of the internal members of request, would it make > sense to ask for passing an ApplePayRequestBase&& to improve clarity? No, because then I'd have to copy or WTFMove the subclasses of ApplePayRequestBase at the call sites, but I need them to remain as lvalues so I can convert the subclass fields. > > > Source/WebCore/Modules/applepay/PaymentRequestValidator.mm:42 > > +static ExceptionOr<void> validateShippingMethod(const ApplePaySessionPaymentRequest::ShippingMethod&); > > How about using std::optional instead? > That would allow writing something like: > exception = validateXX(); > if (exception) > return exception.value(); I don't really have a preference between ExceptionOr<void> and std::optional<Exception>, but I just left this as-is for consistency with the other convert/validate functions. > > > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:74 > > + return result; > > You can probably write this function as follows: > return WTF::map(lineItems, convert); This didn't work. I didn't look into it too closely though :( > > > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:111 > > +ExceptionOr<void> ApplePayPaymentHandler::convertData(JSC::ExecState& execState, JSC::JSValue&& data) > > How about passing m_paymentRequest as a parameter here as well? I'll need m_paymentRequest to be a member variable in a follow-on, so I'll leave this as-is. > > > Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:358 > > + std::unique_ptr<PaymentHandler> selectedPaymentHandler; > > Since selectedPaymentHandler is not kept further than this method, should we > try not allocating it in the heap? I'll need to keep it around in a follow-on, so I'll keep it heap-allocated. I addressed all your other comments. Thanks for the review!

Created attachment 322860 [details] Patch

Comment on attachment 322860 [details] Patch Clearing flags on attachment: 322860 Committed r222921: <http://trac.webkit.org/changeset/222921>

All reviewed patches have been landed. Closing bug.