Closed Bug 1379892 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Web Payments, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ben.tian, Assigned: edenchuang)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1318987
No longer blocks: 1318987
Assignee: nobody → echuang
Adding ddn, just so we can check whether the docs are correct on this point.
Keywords: dev-doc-needed
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?
Attachment #8888476 - Flags: review?(amarchesini) → review+
(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.
Flags: needinfo?(amarchesini)
> 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.
Flags: needinfo?(amarchesini)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dd757326f72b Reject PaymentRequest.show() with AbortError DOMException if a request is showing. r=baku
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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!
(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
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: