Re: [w3c/payment-handler] Add CanMakePaymentEvent and AbortPaymentEvent. (#170)

romandev commented on this pull request.

Overall looks good to me! Thank you for this change.
I think there might need a little bit of improvements in order to reduce duplicated parts but we can revisit it later.
I'm also working on implementing this change in Blink. So, I'd like to merge this patch as soon as possible if there is no objection.

I found that there are some respec error and warnings in your patches
Could you fix them if you don't mind?
I left some inline comments.

Even if you fix them, you can still face the following warning.
"No <dfn> for operation respondWith in PaymentRequestEvent."
If you fix the PaymentRequestEvent in the same way, it will work well.

> @@ -1251,6 +1244,163 @@
         </p>
       </section>
     </section>
+    <section id="canmakepayment">
+      <h2>
+        Can make payment
+      </h2>
+      <p>
+        If the <a>payment handler</a> supports <a>CanMakePaymentEvent</a>, the
+        <a>user agent</a> uses it to help with filtering of the available
+        payment handlers.
+      </p>
+      <p>
+        Implementations may impose a timeout for developers to respond to the
+        <a>CanMakePaymentEvent</a>. If the timeout expires, then the
+        implementation will behave as if <dfn>respondWith()</dfn> was called

I think it should be `<a data-lt="CanMakePaymentEvent.respondWith()">respondWith()</a>`. You are defining it below.

> +        interface CanMakePaymentEvent : ExtendableEvent {
+          readonly attribute USVString topLevelOrigin;
+          readonly attribute USVString paymentRequestOrigin;
+          readonly attribute FrozenArray&lt;PaymentMethodData&gt; methodData;
+          readonly attribute FrozenArray&lt;PaymentDetailsModifier&gt; modifiers;
+          void respondWith(Promise&lt;boolean&gt;canMakePaymentResponse);
+        };
+        </pre>
+          <p>
+            The <dfn>topLevelOrigin</dfn>, <dfn>paymentRequestOrigin</dfn>,
+            <dfn>methodData</dfn>, and <dfn>modifiers</dfn> members share their
+            definitions with those defined for <a>PaymentRequestEvent</a>.
+          </p>
+        <section>
+          <h2>
+            <dfn>respondWith</dfn>() method

Should be `<dfn>respondWith()</dfn>`.
The `<dfn>respondWith</dfn>` is already defined by IDL. So, this statement can cause duplication definition error.

FYI,
`<a data-lt="CanMakePaymentEvent.respondWith"></a>` makes a link which can jump to IDL definition.
(line 1303 in your patch)

`<a data-lt="CanMakePaymentEvent.respondWith()"></a>` makes a link which can jump here.

> +      <h2>
+        Aborting an invocation
+      </h2>
+      <p>
+        If the <a>payee</a> uses [[!payment-request]] method <a>abort()</a>, 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
+        handler</a> should respond with a boolean that indicates whether the
+        abort was successful.
+      </p>
+      <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 <dfn>respondWith()</dfn> was called

Should be `<a data-lt="AbortPaymentEvent.respondWith()">respondWith()</a>`

> +        <h2>
+          The <dfn>AbortPaymentEvent</dfn>
+        </h2>
+        <p>
+          The <a>AbortPaymentEvent</a> is used to attempt to abort a payment
+          request in progress.
+        </p>
+        <pre class="idl">
+        [Constructor(DOMString type, ExtendableEventInit eventInitDict), Exposed=ServiceWorker]
+        interface AbortPaymentEvent : ExtendableEvent {
+          void respondWith(Promise&lt;boolean&gt;paymentAbortedResponse);
+        };
+        </pre>
+        <section>
+          <h2>
+            <dfn>respondWith</dfn>() method

Should be `<dfn>respondWith()</dfn>`.

-- 
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/payment-handler/pull/170#pullrequestreview-49161043