Closed
Bug 1368949
Opened 7 years ago
Closed 6 years ago
Dictionaries inside dictionary should be optional, not always have a default value
Categories
(Core :: DOM: Bindings (WebIDL), enhancement, P1)
Core
DOM: Bindings (WebIDL)
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: alchen, Assigned: bzbarsky)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [webpayments-reserve])
Attachments
(2 files, 1 obsolete file)
28.91 KB,
application/zip
|
Details | |
11.04 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
For PaymentDetailsModifier.total in PaymentRequest.webidl, it should be a optional. http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/dom/webidl/PaymentRequest.webidl#36 PaymentDetailsModifier.total is another dictionary type(PaymentCurrencyAmount) with two required members. http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/dom/webidl/PaymentRequest.webidl#16 There are two symptoms for "PaymentDetailsModifier.total" 1. We cannot create a PaymentRequest without "PaymentDetailsModifier.total". new PaymentRequest( [ { supportedMethods: ["basic-card"], }, ], { total: { label: "", amount: { currency: "USD", value: "1.00", }, }, modifiers: [ { supportedMethods: ["basic-card"], data: { some: "data", }, }, ], } ); 2. We can only access the PaymentDetailsModifier.total as required member. http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/dom/payments/PaymentRequest.cpp#122
Reporter | ||
Comment 1•7 years ago
|
||
(In reply to Alphan Chen [:alchen] from comment #0) > We cannot create a PaymentRequest without "PaymentDetailsModifier.total". We can create PaymentRequest successfully with "PaymentDetailsModifier.total". > > new PaymentRequest( > > [ > { > supportedMethods: ["basic-card"], > }, > ], > { > total: { > label: "", > amount: { > currency: "USD", > value: "1.00", > }, > }, > modifiers: [ > { > supportedMethods: ["basic-card"], total: { label: "", amount: { currency: "USD", value: "33", }, }, > data: { > some: "data", > }, > }, > ], > } > ); >
Reporter | ||
Comment 2•7 years ago
|
||
From MDN, https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Dictionary_types Note that optional dictionary arguments are always considered to have a default value of null, so dictionary arguments are never wrapped in Optional<>.
Reporter | ||
Comment 3•7 years ago
|
||
(In reply to Alphan Chen [:alchen] from comment #0) > For PaymentDetailsModifier.total in PaymentRequest.webidl, it should be a > optional. > We cannot create a PaymentRequest without "PaymentDetailsModifier.total". The error message is "Missing required 'amount' member of PaymentItem." dictionary PaymentItem { required DOMString label; required PaymentCurrencyAmount amount; boolean pending = false; }; dictionary PaymentDetailsModifier { required sequence<DOMString> supportedMethods; PaymentItem total; sequence<PaymentItem> additionalDisplayItems; object data; }; > > new PaymentRequest( > > [ > { > supportedMethods: ["basic-card"], > }, > ], > { > total: { > label: "", > amount: { > currency: "USD", > value: "1.00", > }, > }, > modifiers: [ > { > supportedMethods: ["basic-card"], > data: { > some: "data", > }, > }, > ], > } > ); >
Reporter | ||
Comment 4•7 years ago
|
||
STR 1. set "dom.payments.request.enabled" as true in about:config 2. Run "Test for PaymentRequest Constructor.html" in dictionaryInDictionary.zip
Comment 5•7 years ago
|
||
Set P3 for now. Once we finish the major feature implementation of payment requests, we will be back to triage defects detected during testing.
Priority: -- → P3
Updated•7 years ago
|
Blocks: paymentrequest-wpt
Comment 6•7 years ago
|
||
(In reply to Hsin-Yi Tsai (55 Regression Engineering support) [:hsinyi] from comment #5) > Set P3 for now. Once we finish the major feature implementation of payment > requests, we will be back to triage defects detected during testing. Alphan/Ben, I guess it's time to re-evaluate the priority of this. Is this something we need to fix by end of 57?
Flags: needinfo?(btian)
Flags: needinfo?(alchen)
Comment 7•7 years ago
|
||
This bug won't block intrgration with front-end but fails 2 wpt test cases in https://w3c-test.org/payment-request/payment-request-constructor.https.html Keep P3 but will investigate the cause first. Assign to myself.
Assignee: nobody → btian
Flags: needinfo?(btian)
Flags: needinfo?(alchen)
Comment 8•7 years ago
|
||
De-assign myself for not actively working on this bug.
Assignee: btian → nobody
Comment 9•6 years ago
|
||
This looks like a binding generator bug to me for optional dictionary members that are themselves dictionaries with required members. I find no support in https://heycam.github.io/webidl/#required-dictionary-member for looking at nested required keywords (only ancestors) when parsing JS into a dictionary (combined with https://heycam.github.io/webidl/#es-dictionary "Otherwise, if value is undefined and member is a required dictionary member, then throw a TypeError."). We ran into the same problem today in #media with MediaConfiguration [1] where it gives us an error that VideoConfiguration.bitrate is missing for {audio: {contentType: 'audio/webm; codecs=opus'}}; [1] https://wicg.github.io/media-capabilities/#dictdef-mediaconfiguration
Component: DOM → DOM: Bindings (WebIDL)
Flags: needinfo?(bzbarsky)
Updated•6 years ago
|
Priority: P3 → P2
Assignee | ||
Comment 10•6 years ago
|
||
See https://github.com/heycam/webidl/issues/76 which has been open on this for a while. We implement the thing we have been trying to get the spec changed to... In terms of spec design, it doesn't really make sense to me to have an optional dictionary with required members. Either you need the information in those members (in which case the dictionary is needed) or you can do without it, in which case they're not actually required.
Flags: needinfo?(bzbarsky)
Comment 11•6 years ago
|
||
I don't see the link between having the dictionary present, and that the dictionary attributes being required. Using https://wicg.github.io/media-capabilities/#mediaconfiguration as an example: we want to find out if a particular audio, video or audio+video is supported. then with this particular dictionary we can do things like: https://wicg.github.io/media-capabilities/#media-capabilities-interface 2. If configuration.video is present and is not a valid video configuration, return a Promise rejected with a TypeError. 3. If configuration.audio is present and is not a valid audio configuration, return a Promise rejected with a TypeError. At present, with our current bindings, we can't handle such code flow.
Comment 12•6 years ago
|
||
It sounds like specs are trying to say IFF the dictionary member dictionary (sic) is present, THEN the members of that member are required.
Assignee | ||
Comment 13•6 years ago
|
||
Please see the IDL spec issue. Dictionary-typed arguments can't ever be "not present". The question is whether that should be possible for dictionary-typed dictionary members. It's not clear that the asymmetry between arguments and dictionary members is desirable.
Assignee | ||
Updated•6 years ago
|
Summary: Dictionaries inside dictionary should be optional → Dictionaries inside dictionary should be optional, not always have a default value
Assignee | ||
Updated•6 years ago
|
Blocks: paymentrequest
Whiteboard: [webpayments-reserve]
Assignee | ||
Comment 15•6 years ago
|
||
Current status: I have a local patch to change our implementation. That part is easy. The hard part is that then a bunch of specs end up with undefined behavior, and I didn't even finish auditing the various affected IDLs. I will post that patch here, but it really needs someone to step up, audit the specs corresponding to the IDLs I had to change, figure out which of them should in fact have "= null" as a default value and which should not, and which of the latter then need the spec to actually define what happens when the dictionary is missing (probably all of them). And for the ones that do not want a default value, we will need to change our C++ to accept an Optional<whatever> and do whatever the relevant spec says with it. If this is urgent, we could land the changes as-is with a followup bug to do all that work, I guess, but I want to make sure it happens.
Assignee | ||
Comment 16•6 years ago
|
||
Assignee | ||
Comment 17•6 years ago
|
||
> audit the specs corresponding to the IDLs
Not helped by a bunch of those IDLs missing spec links, btw.
Assignee | ||
Comment 18•6 years ago
|
||
Marcos, do you have the bandwidth to do the spec bits here?
Flags: needinfo?(mcaceres)
Assignee | ||
Comment 19•6 years ago
|
||
Also, if spec issues get filed here, please mention them either in this bug or directly in https://github.com/heycam/webidl/issues/76 as cases where people were depending on dictionaries getting an empty-dictionary default value.
Comment 20•6 years ago
|
||
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #18) > Marcos, do you have the bandwidth to do the spec bits here? Yes. I'll do my best to help with this. Also happy to help updating the various bit of code in Gecko too. > If this is urgent, we could land the changes as-is with a followup bug to do > all that work, I guess, but I want to make sure it happens. It's kinda urgent (as in, I think we want this for 64), because it makes us fairly incompatible with other UAs and quite possibly with web content making using of the payments API.
Flags: needinfo?(mcaceres)
Assignee | ||
Updated•6 years ago
|
Attachment #9010975 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Updated•6 years ago
|
Priority: P2 → P1
Updated•6 years ago
|
Attachment #9011880 -
Flags: review?(kyle) → review+
Comment 22•6 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b9fcdd5dd97 Stop automatically giving dictionary-typed members of dictionaries a default value of null. r=qdot
Comment 23•6 years ago
|
||
Backout by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ae4c3b3528 Backed out changeset 1b9fcdd5dd97 because more code got added that doesn't build with it.
Comment 24•6 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/13ff5a2606eb Stop automatically giving dictionary-typed members of dictionaries a default value of null. r=qdot
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/13ff5a2606eb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•6 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•