[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