Add AbortPaymentEvent. #207

rsolomakhin · 2017-08-28T19:57:18Z

rsolomakhin · 2017-08-29T14:24:04Z

Pease don't merge until I've reported on implementation experience.

Spec change: w3c/payment-handler#207 Bug: 736745 Change-Id: Ifb78aa332080044180e2e836ac0709ba3f727567 Reviewed-on: https://chromium-review.googlesource.com/636504 Reviewed-by: Rouslan Solomakhin <[email protected]> Commit-Queue: Ganggui Tang <[email protected]> Cr-Commit-Position: refs/heads/master@{#498114}

marcoscaceres · 2017-08-30T09:56:54Z

Made a "do not merge" label... to celebrate :)

romandev · 2017-08-30T14:46:07Z

If CanMakePayment event is risky, AbortPayment event might also be risky?
Because it can be triggered from merchant's abort() call without user interaction.
The event doesn't take any information but it's not important.
Might take some informations such as urls by using clients.matchAll with includeUncontrolled option.

rsolomakhin · 2017-08-30T15:03:29Z

The user agent fires the AbortPayment event only in the currently invoked payment app. From user perspective, that's after the user has selected a payment app and clicked [PAY], while the user is waiting for the payment app to complete. From implementation perspective, that's after PaymentRequest event has been fired, but before PaymentRequest.updateWith() promise has been resolved. At this point, the user has given explicit consent for the merchant website's information to be passed into the payment app to facilitate the payment. Hence, the risk of AbortPayment event is low.

marcoscaceres

This is a good start. I think we can get Payment Request to do some of the work tho.

marcoscaceres · 2017-11-28T09:00:51Z

index.html

-      </pre>
+        </pre>
+        <p>
+          The <a>paymentManager</a> attribute exposes payment handler


Shouldn't this be a dfn?

There's a for paymentManager eight lines above. I'm not super familiar with spec format, but shouldn't there be a single tag per term in a spec?

marcoscaceres · 2017-11-28T09:02:45Z

index.html

+      <h2>
+        Aborting an invocation
+      </h2>
+      <p>


This paragraph is redundant. This should be in an algorithm somewhere (probably is, just haven't gotten to it).

marcoscaceres · 2017-11-28T09:03:54Z

index.html

+        the <a>user agent</a> fires <a>AbortPaymentEvent</a> in the <a>payment
+        handler</a> that is currently handling <a>PaymentRequestEvent</a>.
+        There is no expectation that the <a>payment handler</a> would roll back
+        a transaction or issue a refund. (This is out of scope.) The <a>payment


Note, this whole talk of "refunds" and other "money talk" is not ideal. We should only talk about how we manipulate things in the API - but not go into this kind of detail. We can describe "here is how you do a refund" in tutorial material.

marcoscaceres · 2017-11-28T09:06:12Z

index.html

+      <p>
+        Implementations may impose a timeout for developers to respond to the
+        <a>AbortPaymentEvent</a>. If the timeout expires, then the
+        implementation will behave as if <a data-lt=


Nit: there is no need to use data-lt :) Just add data-link-for="AbortPaymentEvent" in a parent element (either the paragraph or the parent <section>).

That's a neat trick, thank you!

marcoscaceres · 2017-11-28T09:07:18Z

index.html

+            <dfn>onabortpayment</dfn> attribute
+          </h2>
+          <p>
+            The <a>onabortpayment</a> attribute is an <a>event handler</a>


Nit: <a>event handler</a> should be <a>event handler attribute</a> and linked to HTML correctly.

marcoscaceres · 2017-11-28T09:16:38Z

index.html

+          <a>PaymentRequestEvent</a> that is being processed.
+          </li>
+          <li>If <var>registration</var> is not found, reject the
+          <a>Promise</a> that was created by <a data-cite=


So, I don't think we can do this... we need to hook back into Payment Request here - but we can't reach across threads and reject the promise?

marcoscaceres · 2017-11-28T09:17:51Z

index.html

+          </li>
+          <li>Invoke the <a>handle functional event</a> algorithm with a
+          <a>ServiceWorkerRegistration</a> of <var>registration</var> and <var>
+            callbackSteps</var> set to the following steps:


Maybe we want some kinda of task here? This looks like a task.

marcoscaceres · 2017-11-28T09:19:12Z

index.html

+          <li>Invoke the <a>handle functional event</a> algorithm with a
+          <a>ServiceWorkerRegistration</a> of <var>registration</var> and <var>
+            callbackSteps</var> set to the following steps:
+            <ol>


Yeah, we shouldn't need to do any of this... Payment Request should do this for us. We just need to tell Payment Request that we have aborted, and it should do all this, no?

marcoscaceres · 2017-11-28T09:22:00Z

index.html

+              </li>
+              <li>Dispatch <var>e</var> to <var>global</var>.
+              </li>
+              <li>Wait for all of the promises in the <a>extend lifetime


This feels out of place to me. Shouldn't we get this for free from .waitUntil() without needing to say anything? Hopefully this whole task can go away.

marcoscaceres · 2017-11-28T09:28:59Z

index.html

+              response, reject the <a>Promise</a> that was created by
+              <a data-cite=
+              "!payment-request#abort-method">PaymentRequest.abort()</a> with a
+              <a>DOMException</a> whose value "<a>OperationError</a>".


You can remove this whole thing. Payment Request doesn't know anything about "OperationError"s and WebIDL will just coerce anything you send into .respondeWith() into a boolean.

...anyway, I'm kinda sure this whole thing needs to go away. Fingers crossed!

rsolomakhin · 2018-09-19T15:35:35Z

Let me close this pull request and submit a new one.

Add AbortPaymentEvent.

2de4b15

rsolomakhin requested a review from ianbjacobs August 28, 2017 19:57

ianbjacobs mentioned this pull request Aug 29, 2017

Support for Abort() being delegated to Payment Handler #117

Open

marcoscaceres added the DO NOT MERGE label Aug 30, 2017

Nit: wording and xrefs

4b83039

marcoscaceres requested changes Nov 28, 2017

View reviewed changes

ExE-Boss mentioned this pull request Apr 10, 2018

Update Payment API interfaces. mdn/browser-compat-data#1726

Closed

rsolomakhin closed this Sep 19, 2018

rsolomakhin deleted the abort-payment branch January 20, 2023 15:19

skyclouds2001 mentioned this pull request Dec 26, 2023

mark ServiceWorkerGlobalScope: abortpayment event as non-standard mdn/browser-compat-data#21746

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AbortPaymentEvent. #207

Add AbortPaymentEvent. #207

rsolomakhin commented Aug 28, 2017 •

edited by pr-preview bot

Loading

rsolomakhin commented Aug 29, 2017

marcoscaceres commented Aug 30, 2017

romandev commented Aug 30, 2017

rsolomakhin commented Aug 30, 2017

marcoscaceres left a comment

marcoscaceres Nov 28, 2017

rsolomakhin Dec 3, 2017

marcoscaceres Nov 28, 2017

marcoscaceres Nov 28, 2017

marcoscaceres Nov 28, 2017

rsolomakhin Dec 3, 2017

marcoscaceres Nov 28, 2017

marcoscaceres Nov 28, 2017

marcoscaceres Nov 28, 2017

marcoscaceres Nov 28, 2017

marcoscaceres Nov 28, 2017

marcoscaceres Nov 28, 2017

marcoscaceres Nov 28, 2017

rsolomakhin commented Sep 19, 2018

Add AbortPaymentEvent. #207

Add AbortPaymentEvent. #207

Conversation

rsolomakhin commented Aug 28, 2017 • edited by pr-preview bot Loading

rsolomakhin commented Aug 29, 2017

marcoscaceres commented Aug 30, 2017

romandev commented Aug 30, 2017

rsolomakhin commented Aug 30, 2017

marcoscaceres left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsolomakhin commented Sep 19, 2018

rsolomakhin commented Aug 28, 2017 •

edited by pr-preview bot

Loading