1379892 - Reject PaymentRequest.show() with AbortError DOMException if the user agent's "payment request is showing" boolean is true

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement)

Description

[Ben Tian (inactive)]

Web platform test for PaymentRequest.show() fails:
https://w3c-test.org/payment-request/payment-request-show-method.https.html

If the user agent's "payment request is showing" boolean is true, then return a promise rejected with an "AbortError" DOMException.

We should reject with AbortError instead of NS_ERROR_FAILURE.

Comment 1

[Eden Chuang[:edenchuang]]

Attachment: Bug 1379892 - Reject PaymentRequest.show() with AbortError DOMException if a request is showing. r?baku

Comment 2

[Chris Mills [:cmills]]

Adding ddn, just so we can check whether the docs are correct on this point.

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8888476 [details] [diff] [review]
Bug 1379892 - Reject PaymentRequest.show() with AbortError DOMException if a request is showing. r?baku

Review of attachment 8888476 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/PaymentRequest.cpp
@@ +403,5 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
> +    if (rv == NS_ERROR_ABORT) {
> +      promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
> +    } else {
> +      promise->MaybeReject(NS_ERROR_FAILURE);

NS_ERROR_FAILURE is a wrong error. Can we use invalid state? or anything else? What does the spec say here?

::: dom/payments/PaymentRequestService.cpp
@@ +327,5 @@
>        }
>        break;
>      }
>      /*
>       *  TODO: Launch/inform payment UI here once the UI module is implemented.

Is this comment still valid?

Comment 4

[Eden Chuang[:edenchuang]]

(In reply to Andrea Marchesini [:baku] from comment #3)
> Comment on attachment 8888476 [details] [diff] [review]
> Bug 1379892 - Reject PaymentRequest.show() with AbortError DOMException if a
> request is showing. r?baku
> 
> Review of attachment 8888476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/payments/PaymentRequest.cpp
> @@ +403,5 @@
> >    if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    if (rv == NS_ERROR_ABORT) {
> > +      promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
> > +    } else {
> > +      promise->MaybeReject(NS_ERROR_FAILURE);
> 
> NS_ERROR_FAILURE is a wrong error. Can we use invalid state? or anything
> else? What does the spec say here?

When PaymentRequestManager::ShowPayment returns NS_ERROR_FAILURE, it can be IPC transfer error or some other internal error.
The spec doesn't describe the situations about internal errors. According to the spec, the state is valid, so returning invalid state error might not the best. I prefer returning NS_ERROR_DOM_NOT_ALLOWED_ERR or NS_ERROR_DOM_UNKNOWN_ERR. How do you think?

> 
> ::: dom/payments/PaymentRequestService.cpp
> @@ +327,5 @@
> >        }
> >        break;
> >      }
> >      /*
> >       *  TODO: Launch/inform payment UI here once the UI module is implemented.
> 
> Is this comment still valid?

Yes, and this comment will be remove after bug 1382092 is resolved.

Comment 5

[Andrea Marchesini [:baku]]

> the spec, the state is valid, so returning invalid state error might not the
> best. I prefer returning NS_ERROR_DOM_NOT_ALLOWED_ERR or
> NS_ERROR_DOM_UNKNOWN_ERR. How do you think?

Better. I prefer NS_ERROR_DOM_NOT_ALLOWED_ERR but this should be covered by spec.

Comment 6

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d359a4ed0e37f6710d85a6a840f8e9ab22865b82

Comment 7

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eb2199f34042bb4686791125a28a5bdf276321f

Comment 8

[Eden Chuang[:edenchuang]]

Attachment: Bug 1379892 - Reject PaymentRequest.show() with AbortError DOMException if a request is showing. r=baku

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd757326f72b
Reject PaymentRequest.show() with AbortError DOMException if a request is showing. r=baku

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dd757326f72b

Comment 11

[Chris Mills [:cmills]]

I've added details of exceptions to this page:

https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/show#Exceptions

Let me know if that looks OK. Thanks!

Comment 12

[Ben Tian (inactive)]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #11)
> https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/
> show#Exceptions
> 
> Let me know if that looks OK. Thanks!

Correction:

In |AbortError| description, "payment request is showing" boolean is true means _another_ payment has already been shown, not this payment. If this payment is showing, call to show() would be rejected with |InvalidStateError| since the request's state is 'interactive' (i.e., the payment request is being presented to the user), not 'created' [2].

[1] https://w3c.github.io/payment-request/#dfn-payment-request-is-showing
[2] step 3 in https://w3c.github.io/payment-request/#show()-method

Comment 13

[Chris Mills [:cmills]]

(In reply to Ben Tian [:btian] from comment #12)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #11)
> > https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/
> > show#Exceptions
> > 
> > Let me know if that looks OK. Thanks!
> 
> Correction:
> 
> In |AbortError| description, "payment request is showing" boolean is true
> means _another_ payment has already been shown, not this payment. If this
> payment is showing, call to show() would be rejected with
> |InvalidStateError| since the request's state is 'interactive' (i.e., the
> payment request is being presented to the user), not 'created' [2].
> 
> [1] https://w3c.github.io/payment-request/#dfn-payment-request-is-showing
> [2] step 3 in https://w3c.github.io/payment-request/#show()-method

Cool, thanks for the corrections. I've updated the AbortError description, and added an InvalidStateError description.