Bug 177850 - [Payment Request] Add a payment method that supports Apple Pay
Summary: [Payment Request] Add a payment method that supports Apple Pay
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks: 174796
  Show dependency treegraph
 
Reported: 2017-10-03 17:42 PDT by Andy Estes
Modified: 2017-10-05 11:47 PDT (History)
14 users (show)

See Also:


Attachments
Patch (155.15 KB, patch)
2017-10-03 18:00 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (155.25 KB, patch)
2017-10-03 19:04 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (155.41 KB, patch)
2017-10-03 21:12 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.32 MB, application/zip)
2017-10-03 22:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.08 MB, application/zip)
2017-10-04 00:04 PDT, Build Bot
no flags Details
Patch (156.27 KB, patch)
2017-10-04 09:52 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.35 MB, application/zip)
2017-10-04 13:35 PDT, Build Bot
no flags Details
Patch (159.30 KB, patch)
2017-10-04 13:56 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (159.38 KB, patch)
2017-10-04 14:01 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (159.23 KB, patch)
2017-10-04 23:36 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (159.32 KB, patch)
2017-10-05 00:02 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (159.06 KB, patch)
2017-10-05 10:54 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (159.11 KB, patch)
2017-10-05 11:06 PDT, Andy Estes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2017-10-03 17:42:14 PDT
[Payment Request] Add a payment method that supports Apple Pay
Comment 1 Andy Estes 2017-10-03 18:00:25 PDT Comment hidden (obsolete)
Comment 2 Build Bot 2017-10-03 18:02:21 PDT Comment hidden (obsolete)
Comment 3 Andy Estes 2017-10-03 19:04:33 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2017-10-03 19:08:27 PDT Comment hidden (obsolete)
Comment 5 Andy Estes 2017-10-03 21:12:33 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-10-03 21:13:58 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2017-10-03 22:35:26 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2017-10-03 22:35:27 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-10-04 00:04:29 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2017-10-04 00:04:31 PDT Comment hidden (obsolete)
Comment 11 Andy Estes 2017-10-04 09:52:25 PDT
Created attachment 322683 [details]
Patch
Comment 12 Build Bot 2017-10-04 09:54:30 PDT Comment hidden (obsolete)
Comment 13 youenn fablet 2017-10-04 10:06:25 PDT
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.
Comment 14 Andy Estes 2017-10-04 10:12:54 PDT
(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 15 Alex Christensen 2017-10-04 12:02:19 PDT
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?
Comment 16 Andy Estes 2017-10-04 12:12:44 PDT
(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.
Comment 17 Build Bot 2017-10-04 13:35:44 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2017-10-04 13:35:46 PDT Comment hidden (obsolete)
Comment 19 Andy Estes 2017-10-04 13:56:04 PDT Comment hidden (obsolete)
Comment 20 Andy Estes 2017-10-04 14:01:24 PDT
Created attachment 322725 [details]
Patch
Comment 21 Build Bot 2017-10-04 14:04:10 PDT Comment hidden (obsolete)
Comment 22 Andy Estes 2017-10-04 14:32:51 PDT Comment hidden (obsolete)
Comment 23 youenn fablet 2017-10-04 23:33:55 PDT
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?
Comment 24 Andy Estes 2017-10-04 23:36:57 PDT
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.
Comment 25 Andy Estes 2017-10-04 23:50:44 PDT
(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.
Comment 26 Andy Estes 2017-10-05 00:02:37 PDT
Created attachment 322796 [details]
Patch
Comment 27 youenn fablet 2017-10-05 04:36:46 PDT
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.
Comment 28 Andy Estes 2017-10-05 09:45:49 PDT
(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!
Comment 29 Andy Estes 2017-10-05 10:54:43 PDT Comment hidden (obsolete)
Comment 30 WebKit Commit Bot 2017-10-05 10:57:09 PDT Comment hidden (obsolete)
Comment 31 Andy Estes 2017-10-05 11:06:32 PDT
Created attachment 322860 [details]
Patch
Comment 32 WebKit Commit Bot 2017-10-05 11:47:21 PDT
Comment on attachment 322860 [details]
Patch

Clearing flags on attachment: 322860

Committed r222921: <http://trac.webkit.org/changeset/222921>
Comment 33 WebKit Commit Bot 2017-10-05 11:47:23 PDT
All reviewed patches have been landed.  Closing bug.