Bug 190985 – [Payment Request] Implement PaymentResponse.retry()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[Payment Request] Implement PaymentResponse.retry()

[Andy Estes]

Created attachment 353233 [details]
Patch

[Andy Estes]

Created attachment 353234 [details]
Patch

[EWS Watchlist]

Attachment 353234 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/paymentrequest/PaymentResponse.cpp:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 353234 [details]
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.https-expected.txt:4
> +
> +
> +
> +

What is up with all this whitespace?

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

Ditto.

[Andy Estes]

(In reply to Daniel Bates from comment #4)
> Comment on attachment 353234 [details]
> 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.

Added.

> 
> > 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?

Fixed by removing the const qualification on this and Internals::mockPaymentCoordinator().

> 
> > 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) };

Nope, your way is clearly better. Fixed.

> 
> > 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.

I'm not sure I agree with this, so maybe we should discuss it further offline. It doesn't take ownership of the promise, but its callers exclusively pass rvalues, so that seems like an OK reason to use an rvalue 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 { }

Fixed.

> 
> > 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.

See above.

> 
> > Source/WebCore/testing/Internals.cpp:551
> > +        auto mockPaymentCoordinator = new MockPaymentCoordinator(*frame->page());
> 
> Eww, a raw pointer. Who deallocates this?

PaymentCoordinatorClients (of which MockPaymentCoordinator is one) are supposed to delete themselves when PaymentCoordinator calls paymentCoordinatorDestroyed() :(

It's gross and we should fix it, but not here.

> 
> > 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.

These are manual web platform tests that I converted to automated tests using window.internals.mockPaymentCoordinator. There is no plan to upstream them as-is because of their dependency on window.internals. I'll add a ChangeLog comment as you suggested.

> 
> > LayoutTests/http/tests/paymentrequest/payment-response-rejects-if-not-active.https-expected.txt:4
> > +
> > +
> > +
> > +
> 
> What is up with all this whitespace?
> 
> > LayoutTests/http/tests/paymentrequest/payment-response-retry-method.https-expected.txt:10
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> 
> Ditto.

Fixed.

Thanks for the review!

[Andy Estes]

Created attachment 353380 [details]
Patch

[EWS Watchlist]

Attachment 353380 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/paymentrequest/PaymentResponse.cpp:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Committed r237597: <https://trac.webkit.org/changeset/237597>

[Radar WebKit Bug Importer]

<rdar://problem/45676628>

[Radar WebKit Bug Importer]

<rdar://problem/45676588>