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

Various fixes to the PaymentRequest constructor #374

Merged
merged 1 commit into from
Jan 4, 2017

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Dec 13, 2016


This is on top of #368 and #370 and can only be merged after them.

@marcoscaceres
Copy link
Member

@domenic, this one also now needs rebase. So. Close.

@domenic
Copy link
Collaborator Author

domenic commented Dec 13, 2016

Just hit refresh :)

@marcoscaceres
Copy link
Member

ah 👍

@adrianba
Copy link
Contributor

Is there a need to reorder the checks to align with Blink? I think this might make our implementation out of compliance with the spec. Need to check on the impact of this but would probably prefer we don't change ordering if we don't need to.

@domenic
Copy link
Collaborator Author

domenic commented Dec 14, 2016

In general some ordering fixes would be good. E.g. the current spec splits up checking methodData into two locations of the algorithm, interspersing checks and processing of other arguments between them, which is just strange (and requires looping over the sequence twice).

If there's a different ordering to the top-level "sections" desired, that'd be fine too. But the grouping here makes more sense than the current spec.

In other words, this PR's ordering is:

  • allowed to use check
  • process payment methods
  • process the total
  • process displayItems
  • process shipping options
  • process payment details modifiers
  • process error

Any reordering of these seems reasonable, although I think the allowed to use check should be first. But the current spec, which splits up several of these steps into two or three separate substeps which are then sprinkled around the algorithm, seems bad.

@marcoscaceres
Copy link
Member

@domenic can you rebase?

I also think @domenic refactoring is an improvement here - if only from an editorial perspective. We can do any additional reordering after that.

- Fixes w3c#373: align ordering of steps with Blink's implementation, except check for allowed-to-use first since that makes more sense
- Fixes w3c#334: properly get the total.amount.value instead of pretending total is a string
- Fixes w3c#333 and fixes w3c#335: do not assume various dictionaries are present
- Fixes part of w3c#321: use proper dictionary terminology
- Ensures request.[[details]].shippingOptions/modifiers is always a sequence, instead of sometimes left as not present
- Fixes various algorithm structure and typographic issues
@domenic
Copy link
Collaborator Author

domenic commented Dec 23, 2016

@marcoscaceres done

@marcoscaceres
Copy link
Member

@adrianba, want your ok to merge this (and that Edge is willing to make the necessary changes to implementation, if any).

Copy link
Contributor

@adrianba adrianba left a comment

Choose a reason for hiding this comment

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

I can live with this.

@marcoscaceres marcoscaceres merged commit d02d96c into w3c:gh-pages Jan 4, 2017
@domenic domenic deleted the constructor-fix branch January 5, 2017 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment