-
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
PaymentDetails "total" member should be required #320
Comments
Just noticed this as well. AFAICT Blink always throws TypeError when it's missing, so offloading this to generated bindings would be nice. |
|
@adrianba, I see it's used also in https://w3c.github.io/browser-payment-api/#dom-paymentrequestupdateevent-updatewith, is that the place? If so, the because the dictionary has no required members it'd have to be made optional in the constructor, even though it'd always throw when emitted. ( |
Yes. We discussed this with @marcoscaceres and he was going to think about refactoring. |
It's probably a better idea to just use two dictionary types in the two places that want different behavior. Failing that, you should fix the constructor to at least properly define the behavior it wants. And note that updateWith as specced is fairly nonsensical; I still need to report an issue on that... |
Splitting the dictionaries SGTM, that would also get rid of the forbidden |
Note that the shared fields could be a shared parent dictionary if desired. |
Splitting the dictionaries would have the normative change of making |
That seems fine to me, having a dictionary member that does nothing except throw is present is a bit odd, so unless it's expected to be a common mistake just ignoring it probably works? |
Closed by 3c3a100 |
Right now there is prose in https://w3c.github.io/browser-payment-api/#constructor step 4 that attempts (not quite correctly) to enforce that. Please just mark it as required in the IDL instead.
The text was updated successfully, but these errors were encountered: