[webkit-reviews] review granted: [Bug 190985] [Payment Request] Implement PaymentResponse.retry() : [Attachment 353234] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 190985: [Payment Request] Implement PaymentResponse.retry()
https://bugs.webkit.org/show_bug.cgi?id=190985

Attachment 353234: Patch

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




--- Comment #4 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 353234
  --> https://bugs.webkit.org/attachment.cgi?id=353234
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353234&action=review

Looks sane.

> Source/WebCore/ChangeLog:9
> +	   Implemented the retry() method on PaymentResponse as specified in
the Payment Request API
> +	   W3C Editor's Draft of 24 October 2018.

Link please.

> Source/WebCore/Modules/applepay/PaymentCoordinator.h:53
> +    PaymentCoordinatorClient& client() const { return m_client; }

It has come up before on webkit-dev that functions that are const should return
const data and functions thats are non-const can return non-const data. The
reasoning goes like this: a const function that returns non-const data is
effectively non-const because the caller can mutate the returned non-const data
out from under it. Is it possible to pick a signature from among this
dichotomy?

> Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:537
> +    auto authorizationResult = PaymentAuthorizationResult {
PaymentAuthorizationStatus::Failure, WTFMove(errors) };

Is this use of auto preferred? I do not see the benefit of using "auto" here
because we have to name the type. It is shorter and seems more idiomatic to
write:

PaymentAuthorizationResult authorizationResult {
PaymentAuthorizationStatus::Failure, WTFMove(errors) };

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:480
> +void PaymentRequest::abort(AbortPromise&& promise)

This function does not take ownership of promise; => we should not pass
AbortPromise as an rvalue reference. It seems sufficient to pass promise as an
lvalue reference.

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:716
> +    m_response->setShippingOption(m_options.requestShipping ?
m_shippingOption : String());
> +    m_response->setPayerName(m_options.requestPayerName ? payerName :
String());
> +    m_response->setPayerEmail(m_options.requestPayerEmail ? payerEmail :
String());
> +    m_response->setPayerPhone(m_options.requestPayerPhone ? payerPhone :
String());

Very minor. OK as-is. String() => String { }

> Source/WebCore/Modules/paymentrequest/PaymentResponse.cpp:58
>  void PaymentResponse::complete(std::optional<PaymentComplete>&& result,
DOMPromiseDeferred<void>&& promise)

I know this signature was not changed. This function should not take
DOMPromiseDeferred<void>&&. It should take const DOMPromiseDeferred<void>&
because there is no ownership transfer involved in this function. You do not
need to fix this up in this patch.

> Source/WebCore/testing/Internals.cpp:551
> +	   auto mockPaymentCoordinator = new
MockPaymentCoordinator(*frame->page());

Eww, a raw pointer. Who deallocates this?

> LayoutTests/ChangeLog:12
> +	   *
http/tests/paymentrequest/payment-response-rejects-if-not-active.https-expected
.txt: Added.
> +	   *
http/tests/paymentrequest/payment-response-rejects-if-not-active.https.html:
Added.
> +	   *
http/tests/paymentrequest/payment-response-retry-method.https-expected.txt:
Added.
> +	   *
http/tests/paymentrequest/payment-response-retry-method.https.html: Added.
> +

Are these imported and modified Web Platform Tests (WPT)? If so, please add a
remark in this ChangeLog to explain what revision of the WPT repo these tests
are from and explain what modifications were needed. If these are verbatim Web
Platform Tests then they are not being imported into the appropriate place.
They should be under LayoutTests/imported/.... If you wrote these tests and
plan to contribute them to WPT then they should be under
LayoutTests/http/wpt/.... If you wrote these test snd plan to contribute them
to WPT then I have some stylistic comments and we should discuss further.

>
LayoutTests/http/tests/paymentrequest/payment-response-rejects-if-not-active.ht
tps-expected.txt:4
> +
> +
> +
> +

What is up with all this whitespace?

>
LayoutTests/http/tests/paymentrequest/payment-response-retry-method.https-expec
ted.txt:10
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +

Ditto.