[webkit-reviews] review granted: [Bug 177850] [Payment Request] Add a payment method that supports Apple Pay : [Attachment 322796] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 5 04:36:46 PDT 2017


youenn fablet <youennf at gmail.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 177850: [Payment Request] Add a payment method that supports Apple Pay
https://bugs.webkit.org/show_bug.cgi?id=177850

Attachment 322796: Patch

https://bugs.webkit.org/attachment.cgi?id=322796&action=review




--- Comment #27 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 322796
  --> https://bugs.webkit.org/attachment.cgi?id=322796
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.


More information about the webkit-reviews mailing list