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

Remove delegated state and make abort() return a promise #190

Closed
wants to merge 1 commit into from

Conversation

adrianba
Copy link
Contributor

@adrianba adrianba commented May 11, 2016

The current spec defines a delegated state but there is no way to observe a transition into this state. The state concept is used to help describe algorithms in the spec in a succinct way and isn't exposed to web pages.

This change removes the delegated state from the spec and replaces it with equivalent functionality within the abort() method. Previously the only impact of delegated was to prevent abort() from being called. Determining whether abort is possible might require asynchronous processing. For example, some implementations may support abort even if they have delegated to a payment app but may not be able to tell synchronously if this is possible.

With this change, the abort() method attempts to abort the request and returns a promise while it tries. If it is not possible to abort then the promise is rejected (instead of throwing). If abort completes successfully then the promise is resolved.

Fixes #7.

You can read this change in place here.

The current spec defines a `delegated` state but there is no way to
observe a change into this state. The state concept is used to help
describe algorithms in the spec in a succinct way and isn't exposed to
web pages.

This change removes the delegated state from the spec. Previously the
only impact of delegated was to prevent abort() from being called.
Determining whether abort is possible might require asynchronous
processing. For example, some implementations may support abort even if
they have delegated to a payment app but may not be able to tell
synchronously if this is possible.

With this change, the abort() method attempts to abort the request and
returns a promise while it tries. If it is not possible to abort then
the promise is rejected (instead of throwing). If abort completes
successfully then the promise is resolved.
@adrianhopebailie
Copy link
Collaborator

This change solves for #7 but it also removes the section titled "User agent delegates payment request algorithm".

So while the change is fine I am concerned that we now have a very big gap between steps 4 and 5 in the "user accepts the payment request algorithm" where the user agent is supposed to send a payment request to a payment app and get back a response, the details of which are used in step 7.

I am +1 to merge but don't support publishing this draft until we address that omission which I have captured as an editorial issue at #192

@adrianba
Copy link
Contributor Author

First of all, nothing in the spec invoked the removed algorithm. Secondly, all that algorithm did is update the state slot to be set to delegated, which then prevented abort from being called. The spec has all the same capabilities with the changes as without - it just uses an improved approach to determine when it is impossible to abort (because it can now be done asynchronously during the call). It's not true that this change introduces a big gap.

@adrianhopebailie
Copy link
Collaborator

It's not true that this change introduces a big gap.

It does remove a "placeholder" that I think was an important reminder that there is still some undefined behaviour. I don't think that should hold up the PR which is why I have logged #194

@adrianba
Copy link
Contributor Author

The only placeholder it should remove is #7, which is whether there should be a delegated state. This PR removes that placeholder and answers the question saying there should not. I moved the other language from this algorithm into the abort section.

@adrianhopebailie
Copy link
Collaborator

I considered the section "User agent delegates payment request algorithm" as a placeholder for describing how we will delegate the request to a payment app (irrespective of if we have a delegated state or not).

I have replaced that with an issue. I can submit a PR with a marker for that issue if you think that's necessary but I think the markers for #39 and #50 are sufficient?

@adrianhopebailie
Copy link
Collaborator

@adrianba - I put this on the agenda for the call on 19 May but I see no objections to merging. Rather than waste time on the call I'd recommend merging beforehand.

@adrianba
Copy link
Contributor Author

Merged as 17b2c94.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should we support a delegated state for PaymentRequest?
2 participants