Re: [w3c/browser-payment-api] feat: put request id in details dictionary (#426)

domenic requested changes on this pull request.

Processing model looks good, just a few nits and the issue of where to store it.

> @@ -344,6 +342,17 @@
           </li>
           <li>Let <var>serializedMethodData</var> be an empty list.
           </li>
+          <li>Establish the request's id:
+            <ol>
+              <li>If <var>details.id</var> is present, set <a>[[\requestId]]</a>

Elsewhere (e.g. for details.shippingOptions) we've used the approach of normalizing the details.whatever, instead of creating a separate slot. I think that'd work best here too, so no separate internal slot for the ID. A separate slot leads to potential confusion where algorithms could accidentally reach into both places and get different values.

> @@ -591,6 +602,15 @@
       </section>
       <section data-dfn-for="PaymentRequest" data-link-for="PaymentRequest">
         <h2>
+          <dfn>id</dfn> attribute
+        </h2>
+        <p>
+          When getting, the <a>id</a> attribute reflects the value of
+          the payment request's <a>[[\requestId]]</a> internal slot.

"Reflects" is a term of art that isn't quite appropriate here. You want "When getting, returns this PaymentRequest's [[requestId]] internal slot" (or, per the above advice, "this PaymentRequest's [[details]].id")

> @@ -1033,6 +1062,17 @@
       </p>
       <dl>
         <dt>
+          <dfn>id</dfn>
+        </dt>
+        <dd>
+          <a>id</a> is a free-form identifier for this payment request.
+          <p class="note">
+            If a <a>id</a> member is not present, then the UA will generate a

s/a/an; maybe link "user agent" instead of using "UA"

> @@ -1033,6 +1062,17 @@
       </p>
       <dl>
         <dt>
+          <dfn>id</dfn>
+        </dt>
+        <dd>
+          <a>id</a> is a free-form identifier for this payment request.
+          <p class="note">
+            If a <a>id</a> member is not present, then the UA will generate a
+            unique identifier for the payment request during <a data-lt=
+            "PaymentRequest.PaymentRequest()">construction</a>.

This is a broken link according to ReSpec.

> @@ -204,7 +204,6 @@
           Promise&lt;void&gt; abort();
           Promise&lt;boolean&gt; canMakePayment();
 
-          readonly attribute DOMString? paymentRequestId;

We should not treat `null` as a special case. It is no more special than `false` or `0` (all of them are "things that are vaguely negative, but are not the default-triggering special value of `undefined`").

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/browser-payment-api/pull/426#pullrequestreview-23566576