Re: [w3c/browser-payment-api] Detecting Payment Method Availability (#316)

marcoscaceres requested changes on this pull request.



> @@ -523,6 +524,60 @@
           </li>
         </ol>
       </section>
+      <section>  
+        <h2>
+          <code>canMakeActivePayment()</code> method
+        </h2>
+        <p>
+          The <dfn>canMakeActivePayment</dfn> method is called when the page wants to know if the user has a payment method available to use for payment before calling <a data-lt="PaymentRequest.show">show</a>. The <a>canMakeActivePayment</a> method returns a <a>Promise</a> that will be resolved when the <a>user agent</a> has determined if at least one method is available from <a>supportedMethods</a> data. In order to prevent the page from probing different payment methods supported by user, <a>canMakeActivePayment</a> can only be called once per top-level domain. Multiple calls to <a>canMakeActivePayment</a> will result in cached response from previous call. To reduce privacy risks, user agents MAY limit calls to <a>canMakeActivePayment</a> for a certain time before invalidating the cached response per top-level domain. Developers can call <a>canMakeActivePayment</a> multiple times with same set of <a>supportedMethods</a> per top-level domain.  

Nit: we need to link things properly here, but we need to get the text right first. 

> @@ -523,6 +524,60 @@
           </li>
         </ol>
       </section>
+      <section>  
+        <h2>
+          <code>canMakeActivePayment()</code> method
+        </h2>
+        <p>
+          The <dfn>canMakeActivePayment</dfn> method is called when the page wants to know if the user has a payment method available to use for payment before calling <a data-lt="PaymentRequest.show">show</a>. The <a>canMakeActivePayment</a> method returns a <a>Promise</a> that will be resolved when the <a>user agent</a> has determined if at least one method is available from <a>supportedMethods</a> data. In order to prevent the page from probing different payment methods supported by user, <a>canMakeActivePayment</a> can only be called once per top-level domain. Multiple calls to <a>canMakeActivePayment</a> will result in cached response from previous call. To reduce privacy risks, user agents MAY limit calls to <a>canMakeActivePayment</a> for a certain time before invalidating the cached response per top-level domain. Developers can call <a>canMakeActivePayment</a> multiple times with same set of <a>supportedMethods</a> per top-level domain.  
+        </p>
+        <p>
+          The <a>canMakeActivePayment</a> method MUST act as follows:
+        </p>
+        <ol>
+          <li>
+            Let <var>request</var> be the <a>PaymentRequest</a> object on which the method is called.

Note: I know elsewhere the algos are written to use "request", but being a method, you can just refer to "this" (being the object bound to the method).  

> +          <li>
+            Let <var>request</var> be the <a>PaymentRequest</a> object on which the method is called.
+          </li>
+          <li>If the value of <var>request</var>@[[\state]] is not "created", then
+            <a>throw</a> an <a>InvalidStateError</a>.</li>
+          <li>
+            Set the value of <em>request</em>@[[\state]] to "interactive".
+          </li>
+          <li>
+            Let <var>acceptPromise</var> be a new <a>Promise</a>.
+          </li>
+          <li>
+            Store <var>acceptPromise</var> in <em>request</em>@[[\acceptPromise]].
+          </li>
+          <li>
+            Return <var>acceptPromise</var> and asynchronously perform the remaining steps.

Note, we should use HTML's "in parallel" definition instead of asynchronously.  

> +          <li>If the value of <var>request</var>@[[\state]] is not "created", then
+            <a>throw</a> an <a>InvalidStateError</a>.</li>
+          <li>
+            Set the value of <em>request</em>@[[\state]] to "interactive".
+          </li>
+          <li>
+            Let <var>acceptPromise</var> be a new <a>Promise</a>.
+          </li>
+          <li>
+            Store <var>acceptPromise</var> in <em>request</em>@[[\acceptPromise]].
+          </li>
+          <li>
+            Return <var>acceptPromise</var> and asynchronously perform the remaining steps.
+          </li>
+          <li>
+            Let <var>topLevelDomain</var> be the URL of the top level page. Let <var>cachedResponse</var> be cached result of previous call to <a>canMakeActivePayment</a>. If is <var>cachedResponse</var> present, resolve <var>acceptPromise</var> promise with <var>cachedResponse</var>.

This doesn't seem to follow the web's security model. I expect discussions of "origin", not "domain" here. 

> +            Set the value of <em>request</em>@[[\state]] to "interactive".
+          </li>
+          <li>
+            Let <var>acceptPromise</var> be a new <a>Promise</a>.
+          </li>
+          <li>
+            Store <var>acceptPromise</var> in <em>request</em>@[[\acceptPromise]].
+          </li>
+          <li>
+            Return <var>acceptPromise</var> and asynchronously perform the remaining steps.
+          </li>
+          <li>
+            Let <var>topLevelDomain</var> be the URL of the top level page. Let <var>cachedResponse</var> be cached result of previous call to <a>canMakeActivePayment</a>. If is <var>cachedResponse</var> present, resolve <var>acceptPromise</var> promise with <var>cachedResponse</var>.
+          </li>
+          <li>
+            Let <var>cacheInvalidateTimer</var> be the time <a>user agent</a> recorded for previous call to <a>canMakeActivePayment</a> for <var>topLevelDomain</var>. Set <a>DateTime</a> to <var>cacheInvalidateTimer</var> if it's not set. 

cacheInvalidateTimer should have been initialized before being used (at least initially set to null). Also, it's not clear where this variable lives.   

> @@ -523,6 +524,60 @@
           </li>
         </ol>
       </section>
+      <section>  
+        <h2>
+          <code>canMakeActivePayment()</code> method
+        </h2>
+        <p>
+          The <dfn>canMakeActivePayment</dfn> method is called when the page wants to know if the user has a payment method available to use for payment before calling <a data-lt="PaymentRequest.show">show</a>. The <a>canMakeActivePayment</a> method returns a <a>Promise</a> that will be resolved when the <a>user agent</a> has determined if at least one method is available from <a>supportedMethods</a> data. In order to prevent the page from probing different payment methods supported by user, <a>canMakeActivePayment</a> can only be called once per top-level domain. Multiple calls to <a>canMakeActivePayment</a> will result in cached response from previous call. To reduce privacy risks, user agents MAY limit calls to <a>canMakeActivePayment</a> for a certain time before invalidating the cached response per top-level domain. Developers can call <a>canMakeActivePayment</a> multiple times with same set of <a>supportedMethods</a> per top-level domain.  
+        </p>
+        <p>
+          The <a>canMakeActivePayment</a> method MUST act as follows:
+        </p>
+        <ol>
+          <li>
+            Let <var>request</var> be the <a>PaymentRequest</a> object on which the method is called.
+          </li>
+          <li>If the value of <var>request</var>@[[\state]] is not "created", then

Reject the promise with the InvalidStateError

> @@ -523,6 +524,60 @@
           </li>
         </ol>
       </section>
+      <section>  
+        <h2>
+          <code>canMakeActivePayment()</code> method
+        </h2>
+        <p>
+          The <dfn>canMakeActivePayment</dfn> method is called when the page wants to know if the user has a payment method available to use for payment before calling <a data-lt="PaymentRequest.show">show</a>. The <a>canMakeActivePayment</a> method returns a <a>Promise</a> that will be resolved when the <a>user agent</a> has determined if at least one method is available from <a>supportedMethods</a> data. In order to prevent the page from probing different payment methods supported by user, <a>canMakeActivePayment</a> can only be called once per top-level domain. Multiple calls to <a>canMakeActivePayment</a> will result in cached response from previous call. To reduce privacy risks, user agents MAY limit calls to <a>canMakeActivePayment</a> for a certain time before invalidating the cached response per top-level domain. Developers can call <a>canMakeActivePayment</a> multiple times with same set of <a>supportedMethods</a> per top-level domain.  
+        </p>
+        <p>
+          The <a>canMakeActivePayment</a> method MUST act as follows:
+        </p>
+        <ol>
+          <li>
+            Let <var>request</var> be the <a>PaymentRequest</a> object on which the method is called.
+          </li>
+          <li>If the value of <var>request</var>@[[\state]] is not "created", then

To be clear, you can't have a promise-returning method throw an exception. It must always return a promise. You can return a rejected Promise with an InvalidStateError here and stop.  

> +            Store <var>acceptPromise</var> in <em>request</em>@[[\acceptPromise]].
+          </li>
+          <li>
+            Return <var>acceptPromise</var> and asynchronously perform the remaining steps.
+          </li>
+          <li>
+            Let <var>topLevelDomain</var> be the URL of the top level page. Let <var>cachedResponse</var> be cached result of previous call to <a>canMakeActivePayment</a>. If is <var>cachedResponse</var> present, resolve <var>acceptPromise</var> promise with <var>cachedResponse</var>.
+          </li>
+          <li>
+            Let <var>cacheInvalidateTimer</var> be the time <a>user agent</a> recorded for previous call to <a>canMakeActivePayment</a> for <var>topLevelDomain</var>. Set <a>DateTime</a> to <var>cacheInvalidateTimer</var> if it's not set. 
+          </li>
+          <li>
+            If <var>cacheInvalidateTimer</var> is set and is still active, resolve <var>acceptPromise</var> promise with <var>cached</var> response.
+          </li>
+          <li>
+            Let <var>supportedMethods</var> be the union of all the <a>supportedMethods</a> sequences from each

You might want to actually perform the union algorithmically here and derive the value into a `<var>`.  

> +          <li>
+            Let <var>topLevelDomain</var> be the URL of the top level page. Let <var>cachedResponse</var> be cached result of previous call to <a>canMakeActivePayment</a>. If is <var>cachedResponse</var> present, resolve <var>acceptPromise</var> promise with <var>cachedResponse</var>.
+          </li>
+          <li>
+            Let <var>cacheInvalidateTimer</var> be the time <a>user agent</a> recorded for previous call to <a>canMakeActivePayment</a> for <var>topLevelDomain</var>. Set <a>DateTime</a> to <var>cacheInvalidateTimer</var> if it's not set. 
+          </li>
+          <li>
+            If <var>cacheInvalidateTimer</var> is set and is still active, resolve <var>acceptPromise</var> promise with <var>cached</var> response.
+          </li>
+          <li>
+            Let <var>supportedMethods</var> be the union of all the <a>supportedMethods</a> sequences from each
+            <a>PaymentMethodData</a> in the <var>request</var>@[[\methodData]] sequence.
+          </li>
+          <li>
+            Let <var>acceptedMethods</var> be <var>supportedMethods</var> with all identifiers removed that the
+            <a>user agent</a> does not accept and method supports active payment instrument for payment.

There is something not quite write with the second clause of this sentence. Can you please check your grammar here. 

> +          <li>
+            Let <var>cacheInvalidateTimer</var> be the time <a>user agent</a> recorded for previous call to <a>canMakeActivePayment</a> for <var>topLevelDomain</var>. Set <a>DateTime</a> to <var>cacheInvalidateTimer</var> if it's not set. 
+          </li>
+          <li>
+            If <var>cacheInvalidateTimer</var> is set and is still active, resolve <var>acceptPromise</var> promise with <var>cached</var> response.
+          </li>
+          <li>
+            Let <var>supportedMethods</var> be the union of all the <a>supportedMethods</a> sequences from each
+            <a>PaymentMethodData</a> in the <var>request</var>@[[\methodData]] sequence.
+          </li>
+          <li>
+            Let <var>acceptedMethods</var> be <var>supportedMethods</var> with all identifiers removed that the
+            <a>user agent</a> does not accept and method supports active payment instrument for payment.
+          </li>
+          <li>
+            If the length of <var>acceptedMethods</var> is zero, then resolve <var>acceptPromise</var> with

I changed this to resolve with false. Rejections can only happen for errors, not for expected values.  

> +          <li>
+            Let <var>cacheInvalidateTimer</var> be the time <a>user agent</a> recorded for previous call to <a>canMakeActivePayment</a> for <var>topLevelDomain</var>. Set <a>DateTime</a> to <var>cacheInvalidateTimer</var> if it's not set. 
+          </li>
+          <li>
+            If <var>cacheInvalidateTimer</var> is set and is still active, resolve <var>acceptPromise</var> promise with <var>cached</var> response.
+          </li>
+          <li>
+            Let <var>supportedMethods</var> be the union of all the <a>supportedMethods</a> sequences from each
+            <a>PaymentMethodData</a> in the <var>request</var>@[[\methodData]] sequence.
+          </li>
+          <li>
+            Let <var>acceptedMethods</var> be <var>supportedMethods</var> with all identifiers removed that the
+            <a>user agent</a> does not accept and method supports active payment instrument for payment.
+          </li>
+          <li>
+            If the length of <var>acceptedMethods</var> is zero, then resolve <var>acceptPromise</var> with

(i.e., only reject with actual exceptions)

> +            If <var>cacheInvalidateTimer</var> is set and is still active, resolve <var>acceptPromise</var> promise with <var>cached</var> response.
+          </li>
+          <li>
+            Let <var>supportedMethods</var> be the union of all the <a>supportedMethods</a> sequences from each
+            <a>PaymentMethodData</a> in the <var>request</var>@[[\methodData]] sequence.
+          </li>
+          <li>
+            Let <var>acceptedMethods</var> be <var>supportedMethods</var> with all identifiers removed that the
+            <a>user agent</a> does not accept and method supports active payment instrument for payment.
+          </li>
+          <li>
+            If the length of <var>acceptedMethods</var> is zero, then resolve <var>acceptPromise</var> with
+            <code>false</code>, otherwise resolve <var>acceptPromise</var> with <code>true</code>.
+          </li>
+          <li>
+            Cache the response in <var>cachedResponse</var> and set <var>cacheInvalidateTimer</var> to certain <a>DateTime</a> for the <var>topLevelDomain</var>.

Nit: I don't think DateTime is defined in the spec. 

-- 
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/316#pullrequestreview-8962273