Skip to content
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

Retry request abort the update #723

Merged
merged 6 commits into from Jun 26, 2018

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Jun 12, 2018

part 3 of #705 - aborting the update when in retry state

The following tasks have been completed:

Implementation commitment:

  • Safari (link to issue)
  • Chrome (link to issue)
  • Firefox (link to issue)
  • Edge (public signal)

Impact on Payment Handler spec?


Preview | Diff

@marcoscaceres marcoscaceres force-pushed the retry_request_abort_the_update branch 3 times, most recently from 345c887 to 5f7a7f8 Compare June 22, 2018 20:22
@marcoscaceres marcoscaceres force-pushed the retry_request_abort_the_update branch 2 times, most recently from 899ec95 to 5301abc Compare June 25, 2018 20:47
@marcoscaceres
Copy link
Member Author

@domenic, little one for you :)

Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused as to why this algorithm sets complete to true but the "user aborts the payment request algorithm" does not.

index.html Outdated
@@ -4545,7 +4545,20 @@ <h2>
<li>Set <var>request</var>.<a>[[\state]]</a> to
"<a>closed</a>".
</li>
<li>Reject the promise
<li>If <var>request</var><a>[[\response]]</a> is not null,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing dot

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh... keep doing this. Need to add a linter rule for ReSpec ☺️

index.html Outdated
then:
<ol>
<li>Let <var>response</var> be
<var>request</var><a>[[\response]]</a>.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing dot. Also maybe you should do this before the "if request.[[response]] is not null" step (and then just check if response is null)

index.html Outdated
<li>Let <var>response</var> be
<var>request</var><a>[[\response]]</a>.
</li>
<li>Set <var>response</var><a>[[\complete]]</a> to true.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing dot

index.html Outdated
</li>
<li>Set <var>response</var><a>[[\complete]]</a> to true.
</li>
<li>Reject <var>response</var><a>[[\retryPromise]]</a> with
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing dot

index.html Outdated
</li>
<li>Set <var>response</var><a>[[\complete]]</a> to true.
</li>
<li>Reject <var>response</var><a>[[\retryPromise]]</a> with
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this guaranteed to always be non-null? If so maybe add an assert.

</li>
</ol>
</li>
<li>Otherwise, reject
<var>request</var>.<a>[[\acceptPromise]]</a> with
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The note at the bottom seems like it needs updating to mention the retryPromise/retry() in addition to the acceptPromise/show().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a few clarifications throughout the spec about this.

@marcoscaceres
Copy link
Member Author

I'm a bit confused as to why this algorithm sets complete to true but the "user aborts the payment request algorithm" does not.

Yeah, should probably do the same in "user aborts the payment request algorithm". Having a look.

@marcoscaceres marcoscaceres force-pushed the retry_request_abort_the_update branch from 5301abc to fe8bc97 Compare June 25, 2018 21:56
@marcoscaceres marcoscaceres force-pushed the retry_request_abort_the_update branch from fe8bc97 to 0d9db1c Compare June 25, 2018 22:13
@marcoscaceres
Copy link
Member Author

... now to test all this goodness 💪

Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with wording tweaks to the note

index.html Outdated
Consequently, the <a>PaymentRequest</a> moves to a "<a>closed</a>"
state. The error is signaled to the developer through the rejection
of the <a>[[\acceptPromise]]</a>, i.e., the promise returned by
<a data-lt="PaymentRequest.show">show()</a>.
</p>
<p data-link-for="PaymentResponse">
Similarly, aborting the update occurs during a <a>retry()</a> will
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence doesn't read right... "aborting the update occurs during a retry() will cause"?

index.html Outdated
Similarly, aborting the update occurs during a <a>retry()</a> will
cause the <a>[[\retryPromise]]</a> to reject, and the corresponding
<a>PaymentRequest</a>'s <a>[[\complete]]</a> internal slot will be
set to true (i.e., meaning it can no longer be used).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"i.e." and "meaning" seem redundant; pick one or the other.

@marcoscaceres
Copy link
Member Author

Added test web-platform-tests/wpt@9f479ea

@marcoscaceres marcoscaceres merged commit b417638 into gh-pages Jun 26, 2018
@marcoscaceres marcoscaceres deleted the retry_request_abort_the_update branch June 26, 2018 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants