Closed Bug 1368949 Opened 3 years ago Closed Last year

Dictionaries inside dictionary should be optional, not always have a default value

Categories

(Core :: DOM: Bindings (WebIDL), enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: alchen, Assigned: bzbarsky)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [webpayments-reserve])

Attachments

(2 files, 1 obsolete file)

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
(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",
>             },
>           },
>         ],
>       }
>     );
>
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<>.
(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",
>             },
>           },
>         ],
>       }
>     );
>
STR
1. set "dom.payments.request.enabled" as true in about:config
2. Run "Test for PaymentRequest Constructor.html" in dictionaryInDictionary.zip
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
Blocks: 1318987
No longer blocks: 1318987
(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)
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)
De-assign myself for not actively working on this bug.
Assignee: btian → nobody
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)
Priority: P3 → P2
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)
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.
It sounds like specs are trying to say IFF the dictionary member dictionary (sic) is present, THEN the members of that member are required.
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.
Blocks: 1409664
See Also: → 1471165
Summary: Dictionaries inside dictionary should be optional → Dictionaries inside dictionary should be optional, not always have a default value
Duplicate of this bug: 1492295
Whiteboard: [webpayments-reserve]
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.
> audit the specs corresponding to the IDLs

Not helped by a bunch of those IDLs missing spec links, btw.
Marcos, do you have the bandwidth to do the spec bits here?
Flags: needinfo?(mcaceres)
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.
(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)
Attachment #9010975 - Attachment is obsolete: true
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Priority: P2 → P1
Attachment #9011880 - Flags: review?(kyle) → review+
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
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.
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
https://hg.mozilla.org/mozilla-central/rev/13ff5a2606eb
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: qe-verify-
Depends on: 1497111
No longer depends on: 1497111
Blocks: 1226475
Duplicate of this bug: 1226475
You need to log in before you can comment on or make changes to this bug.