Alphan Chen [:alchen] Reporter
	Description • 7 years ago

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

	Alphan Chen [:alchen] 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",
>             },
>           },
>         ],
>       }
>     );
>

	Alphan Chen [:alchen] 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<>.

	Alphan Chen [:alchen] 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",
>             },
>           },
>         ],
>       }
>     );
>

	Alphan Chen [:alchen] 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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	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.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	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?

	Ben Tian (inactive)
	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.

	Ben Tian (inactive)
	Comment 8 • 7 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	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

	Boris Zbarsky [:bzbarsky] 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.

	Jean-Yves Avenard [:jya]
	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.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	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.

	Boris Zbarsky [:bzbarsky] 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.

	Boris Zbarsky [:bzbarsky] 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.

	Boris Zbarsky [:bzbarsky] 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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 18 • 6 years ago

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

	Boris Zbarsky [:bzbarsky] 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.

	Marcos Caceres [:marcosc]
	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.

	Pulsebot
	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

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

	Pulsebot
	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

	Arthur Iakab [arthur_iakab]
	Comment 25 • 6 years ago
bugherder

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