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

Status: RESOLVED FIXED

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

Description

[Alphan Chen [:alchen]]

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

Comment 1

[Alphan Chen [:alchen]]

(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",
>             },
>           },
>         ],
>       }
>     );
>

Comment 2

[Alphan Chen [:alchen]]

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<>.

Comment 3

[Alphan Chen [:alchen]]

(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",
>             },
>           },
>         ],
>       }
>     );
>

Comment 4

[Alphan Chen [:alchen]]

Attachment: dictionaryInDictionary.zip

STR
1. set "dom.payments.request.enabled" as true in about:config
2. Run "Test for PaymentRequest Constructor.html" in dictionaryInDictionary.zip

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Set P3 for now. Once we finish the major feature implementation of payment requests, we will be back to triage defects detected during testing.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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?

Comment 7

[Ben Tian (inactive)]

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.

Comment 8

[Ben Tian (inactive)]

De-assign myself for not actively working on this bug.

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Jean-Yves Avenard [:jya]]

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

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

It sounds like specs are trying to say IFF the dictionary member dictionary (sic) is present, THEN the members of that member are required.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Stop automatically giving dictionary-typed members of dictionaries a default value of null

Comment 17

[Boris Zbarsky [:bzbarsky]]

> audit the specs corresponding to the IDLs

Not helped by a bunch of those IDLs missing spec links, btw.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Marcos, do you have the bandwidth to do the spec bits here?

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

[Marcos Caceres [:marcosc]]

(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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Stop automatically giving dictionary-typed members of dictionaries a default value of null

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Backout by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
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

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/13ff5a2606eb