Re: [w3ctag/spec-reviews] PaymentRequest.canMakeActivePayment() (#146)

OK, so a few somewhat blunt comments here:

First, it's a bit unusual to ask for TAG review of something that doesn't have a specification.  That said, it's not something I would want to discourage, since a group might have a particular architectural issue they want feedback on that seems likely to be important or controversial.  However, although I'm an outsider to the Blink project, it does seem (to my understanding of it) somewhat contrary to the intent of [Blink's launch process](http://www.chromium.org/blink#launch-process)'s rationale for requiring asking for TAG review; it [says](http://www.chromium.org/blink#new-features), for example:
> In practice, we strive to ensure that the features we ship by default have **open standards**.

and it feels to me that part of the purpose of asking for TAG review is not only to request review that the API fits with the way Web APIs are designed, but also that it fits with the way that they're specified, so that they have a good path towards being implemented interoperably by multiple implementations.

Second, it feels in other ways that you're trying to cram this through without listening to feedback.  The  [first](https://github.com/w3c/browser-payment-api/issues/310#issuecomment-260663230) and [second comments](https://github.com/w3c/browser-payment-api/issues/310#issuecomment-261130645) by others in https://github.com/w3c/browser-payment-api/issues/310 and my [initial reaction](https://github.com/w3ctag/spec-reviews/issues/146#issuecomment-261425830) here were all saying that the name doesn't make sense, and you don't seem to have responded to that concern other than to explain why other proposals are also misleading.  However, it does look like the method has been renamed in response to [a different concern](https://github.com/w3c/browser-payment-api/issues/310#issuecomment-263732733), although the explainer hasn't been updated, and, to be honest, it's not clear how much of the explainer even still applies following that change.  (That is, does the API still meet the rationale for adding it, after that change?)

-----

Further comments, looking at the [still unmerged pull request](https://github.com/w3c/browser-payment-api/pull/316/files) (although I don't know if it's possible to link to the *current state* of that pull request, or if the current state will even be preserved in the future to make these comments make sense):

> In order to prevent the page from probing different payment methods supported by user, <a>canMakePayment</a> 
> can only be called once per top-level domain. Multiple calls to <a>canMakePayment</a> will result in
> rejection of the promise with QuotaExceededError. To reduce privacy risks, implementations MAY limit calls 
> to <a>canMakePayment</a> for a certain period of time before allowing top-level origin to call
> <a>canMakePayment</a> again. However <a>canMakePayment</a> can be called multiple times with same 
> set of <a data-lt="PaymentMethodData.supportedMethods">supportedMethods</a> per top-level origin.

This text is quite unclear; it doesn't clearly explain how the passing of time and calls from different TLDs or origins interact to lead to QuotaExceededError.  It seems like it should be a clearer English summary of what the algorithm below it leads to.

> 5. Store <var>canMakePaymentPromise</var> in <em>request</em>@[[\canMakePaymentPromise]].

It's not clear what purpose this serves, since this property doesn't appear to be read from, and since multiple calls to canMakePayment() will overwrite this value.

> (within point 7) Let <var>cachedSupportedMethods</var> be <a>supportedMethods</a> sequences from each <a>PaymentMethodData</a> from previous call to <a>PaymentRequest</a>.  Let <var>cachedResponse</var> be response from previous call to <a>canMakePayment</a>. Let <var>canMakePaymentQuotaReached</var> be boolean value of previous call to <a>canMakePayment</a> for the <a>topLevelOrigin</a>.

This seems oddly less precise than the rest of the algorithm, which does things by storing variables in places.  Is it the previous call made by the same origin?  Which step in this algorithm does that call need to have reached for it to count?  Is previous defined by the order of call or the order of promise resolution?

> 9.  If cachedSupportedMethods is preset and matches supportMethods, then resolve canMakePaymentPromise with cachedResponse. Else, set cachedSupportedMethods to supportedMethods.

It seems like the "Else" part of this should come after step 10.  Otherwise if you have three calls in succession, with the first having one set of methods, and the second and third having another, then the first call will return a real result, the second call will be rejected with QuotaExceededError, and the third call will return the cached response from the first call (which has different methods, but which were incorrectly stored in the second call).

(This is one of the disadvantages of writing specs so algorithmically:  it's harder to detect bugs in the specifications.)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3ctag/spec-reviews/issues/146#issuecomment-264328851