-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Define PaymentResponse.prototype.retry() method #720
Conversation
Ok, implemented this and it seems to work for me locally. |
(again noting, this doesn't include passing the actual errors yet - that's part 2.) |
@domenic, this is ready for review when you have time. Let me know how you want to handler all the parts. I tried to limit the changes to make is easier to review. |
index.html
Outdated
<var>request</var>.<a>[[\details]]</a>.<a data-lt= | ||
"PaymentDetailsInit.id">id</a>. | ||
</li> | ||
<li>Set the <a data-lt= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zkoch, question for you: upon a retry, will it be possible for a user to change payment handlers. For instance, can a user switch from "basic-card" to "https://bob.pay"? Seems like they probably should be able to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My view is yes, they should be able to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move it down, so it's not part of the initialization.
Seeking additional implementation commitment for |
@ianbjacobs, @adrianhopebailie, could you evaluate if there is any impact on payment handler? I've not given it any thought. If there is, could you add details to the "Impact on Payment Handler spec?" Same applies for the other pull requests relating to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Also, as a nit on the commit message, it's PaymentResponse.prototype.retry() or paymentResponse.retry(), not PaymentResponse.retry().
index.html
Outdated
<a>DOMException</a>. | ||
</li> | ||
<li>Set <var>response</var>.<a>[[\retrying]]</a> to true. | ||
</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! I'll make this change.
index.html
Outdated
</ol> | ||
<p> | ||
Finally, when <var>retryPromise</var> settles, set | ||
<var>response</var><a>[[\retrying]]</a> to false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of missing periods in this PR. Search for </var><a>[[
(should be </var>.<a>[[
).
index.html
Outdated
is wrong with the user-provided data of the payment response. | ||
</li> | ||
<li> | ||
<p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first paragraph here doesn't seem like a numbered step. I would put it as a NOTE inside the step 10, "return retryPromise".
index.html
Outdated
<var>retryPromise</var>. | ||
</li> | ||
<li> | ||
<a>In parallel</a>: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
index.html
Outdated
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. But we still need this for when the UI is being presented (as happens in show()
step 19). Adding both checks.
index.html
Outdated
"<a>AbortError</a>" <a>DOMException</a>. | ||
</li> | ||
</ol> | ||
<p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph should be its own standalone step.
index.html
Outdated
<dfn>[[\retryPromise]]</dfn> | ||
</td> | ||
<td> | ||
When <a>[[\retrying]]</a> is true, a <a>Promise</a> resolves when |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Promise that resolves...
index.html
Outdated
<li>Set <var>response</var>.<a>[[\retrying]]</a> to false. | ||
</li> | ||
<li>Set <var>response</var>.<a>[[\retryPromise]]</a> to | ||
undefined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null, not undefined, is what you used elsewhere for the default here
8d2345e
to
0b629e9
Compare
Sets foundation for the method, which is then expanded upon by other pull requests.
0b629e9
to
9b1e54a
Compare
Added some developer docs: |
WebKit tracking bug: https://bugs.webkit.org/show_bug.cgi?id=190985 |
part 1 of #705
The following tasks have been completed:
Implementation commitment:
Impact on Payment Handler spec?
Unknown.
Example
This pull request gets us here... the user doesn't yet know what's actually wrong with the payment, but at least they know something is wrong.
Preview | Diff