- From: Domenic Denicola <notifications@github.com>
- Date: Tue, 12 Jun 2018 08:28:37 -0700
- To: w3c/payment-request <payment-request@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/payment-request/pull/720/review/128016092@github.com>
domenic approved this pull request. Mostly looks good, lots of editorial changes... Feel free to merge after fixing if you want to get a move on, or I can take another pass. > + <ol class="algorithm"> + <li>Let <var>response</var> be the <a>context object</a>. + </li> + <li>Let <var>request</var> be + <var>response</var>.<a>[[\request]]</a>. + </li> + <li>If <var>response</var>.<a>[[\complete]]</a> is true, return <a>a + promise rejected with</a> an "<a>InvalidStateError</a>" + <a>DOMException</a>. + </li> + <li>If <var>response</var>.<a>[[\retrying]]</a> is true, return <a>a + promise rejected with</a> an "<a>InvalidStateError</a>" + <a>DOMException</a>. + </li> + <li>Set <var>response</var>.<a>[[\retrying]]</a> to true. + </li> You may be able to get away with consolidating [[retryPromise]] and [[retrying]] into just [[retryPromise]], which will be null if you are not retrying. This might also be a good change because then you'd make it clearer that the UA can release the reference to the promise, when you set it to null. > + interface is being shown, or no longer is by the time this + step is reached, then: + </p> + <ol> + <li>Close down the user interface. + </li> + <li>Set the <a>user agent</a>'s <a>payment request is + showing</a> boolean to false. + </li> + <li>Reject <var>retryPromise</var> with an + "<a>AbortError</a>" <a>DOMException</a>. + </li> + </ol> + <p> + Finally, when <var>retryPromise</var> settles, set + <var>response</var><a>[[\retrying]]</a> to false. There are a lot of missing periods in this PR. Search for `</var><a>[[` (should be `</var>.<a>[[`). > + </li> + <li>Set <var>request</var>.<a>[[\state]]</a> to "<a>interactive</a>". + </li> + <li>Let <var>retryPromise</var> be <a>a new promise</a>. + </li> + <li>Set <var>response</var>.<a>[[\retryPromise]]</a> to + <var>retryPromise</var>. + </li> + <li> + <a>In parallel</a>: + <ol> + <li>In the payments UI, indicate to the end-user that something + is wrong with the user-provided data of the payment response. + </li> + <li> + <p> The first paragraph here doesn't seem like a numbered step. I would put it as a NOTE inside the step 10, "return retryPromise". > + </li> + <li>If <var>response</var>.<a>[[\retrying]]</a> is true, return <a>a + promise rejected with</a> an "<a>InvalidStateError</a>" + <a>DOMException</a>. + </li> + <li>Set <var>response</var>.<a>[[\retrying]]</a> to true. + </li> + <li>Set <var>request</var>.<a>[[\state]]</a> to "<a>interactive</a>". + </li> + <li>Let <var>retryPromise</var> be <a>a new promise</a>. + </li> + <li>Set <var>response</var>.<a>[[\retryPromise]]</a> to + <var>retryPromise</var>. + </li> + <li> + <a>In parallel</a>: I don't think any of these steps should be done in parallel (on a background thread). Updating the UI needs to be done from the UI thread. And the other steps are basically just setting up callbacks on main-thread objects. > + <li>In the payments UI, indicate to the end-user that something + is wrong with the user-provided data of the payment response. + </li> + <li> + <p> + The <var>retryPromise</var> will later be resolved or + rejected by either the <a>user accepts the payment request + algorithm</a> or the <a>user aborts the payment request + algorithm</a>, which are triggered through interaction with + the user interface. + </p> + <p data-test="rejects_if_not_active.https.html"> + If <var>document</var> stops being <a data-cite= + "!HTML#fully-active">fully active</a> while the user + interface is being shown, or no longer is by the time this + step is reached, then: I think you also want a fully active check earlier in the algorithm (before you present the UI in the first place), as is done in show(). > + If <var>document</var> stops being <a data-cite= + "!HTML#fully-active">fully active</a> while the user + interface is being shown, or no longer is by the time this + step is reached, then: + </p> + <ol> + <li>Close down the user interface. + </li> + <li>Set the <a>user agent</a>'s <a>payment request is + showing</a> boolean to false. + </li> + <li>Reject <var>retryPromise</var> with an + "<a>AbortError</a>" <a>DOMException</a>. + </li> + </ol> + <p> This paragraph should be its own standalone step. > + </tr> + <tr> + <td> + <dfn>[[\request]]</dfn> + </td> + <td> + The <a>PaymentRequest</a> instance that instantiated this + <a>PaymentResponse</a>. + </td> + </tr> + <tr> + <td> + <dfn>[[\retryPromise]]</dfn> + </td> + <td> + When <a>[[\retrying]]</a> is true, a <a>Promise</a> resolves when A Promise **that** resolves... > </li> - <li>Set the <a data-lt="PaymentResponse.methodName">methodName</a> - attribute value of <var>response</var> to the <a>payment method - identifier</a> for the <a>payment method</a> that the user selected - to accept the payment. + <li>If <var>isRetry</var> if false, initialize the newly created + <var>response</var>: + <ol> + <li>Set <var>response</var>.<a>[[\request]]</a> to + <var>request</var>. + </li> + <li>Set <var>response</var>.<a>[[\retrying]]</a> to false. + </li> + <li>Set <var>response</var>.<a>[[\retryPromise]]</a> to + undefined. null, not undefined, is what you used elsewhere for the default here -- 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/payment-request/pull/720#pullrequestreview-128016092
Received on Tuesday, 12 June 2018 15:29:01 UTC