Take paymentRequestID in constructor

[rvm4]

We appear to use "?" and "optional" interchangeably to signify nullability in this webIDL definition. Is that intentional or should I convert the whole thing to one way or the other?

[rsolomakhin]

The optional keyword does not mean nullability, it means omitability. Suppose your have this IDL:

void func(optional DOMString stuff);

Then your JavaScript code func(null) will result in stuff === "null". That's a non-null string of 4 characters whose value is"null", not an actual null string. This IDL allows calling func().

Suppose your have this IDL instead:

void func(DOMString? stuff);

Then your JavaScript code func(null) will result in stuff === null. That's the actual null string. This IDL does not allow calling func().

[rvm4]

Oh, gotcha, makes sense. You asked in the issue for the paymentRequestID to be optional though, which means putting it at the end of the constructor after the options. I don't think this is desirable because my intuition is that many clients will use the paymentRequestID field. Are we okay with it be nullable and at the front of the constructor in order to promote usage?

[rsolomakhin]

👎 to nullable at the front of the constructor, because we don't have the data to backup the intuition of many clients using the new field. Let's use the optional keyword. OK to keep at the front. In practice this means that there will be 4 constructors:

PaymentRequest(PaymentMethodData methodData, PaymentDetails details);
PaymentRequest(PaymentMethodData methodData, PaymentDetails details, PaymentOptions options);
PaymentRequest(DOMString id, PaymentMethodData methodData, PaymentDetails details);
PaymentRequest(DOMString id, PaymentMethodData methodData, PaymentDetails details, PaymentOptions options);

[rvm4]

Oh duh, I have been writing in a one-constructor language for too long :/

[rvm4]

Shall I go ahead and merge then? Seems like it.

[rsolomakhin]

As long as your newly introduced line-breaks are not reflected in the resulting HTML, LGTM 👍

[rvm4]

I have no idea how to test that other than to git pull and manually verify. I can remove the line breaks if we prefer a nasty line wrap.

[rsolomakhin]

Line breaks are technically OK if we move methodData one line above, together with its type :-)

[domenic]

This should definitely not be merged as-is. It needs to update the constructor algorithm to actually store the paymentRequestID somewhere.

[rvm4]

Good call @domenic, I believe I have address that as well as the spacing from @rsolomakhin.

[marcoscaceres]

@rvm4, I think the paymentRequestID needs to be part of the PaymentDetails object, no?

[domenic]

It's invalid Web IDL to have optional arguments that are not at the end of the parameter list. You'll need multiple [Constructor] annotations, to trigger the overload resolution algorithm. Then you'll need to write two separate constructor algorithms (probably factoring out some shared logic).

[marcoscaceres]

We should just add this to PaymentDetails or PaymentOptions.

[rvm4]

I'm not a fan of putting it in PaymentDetails or PaymentOptions because it relegates the field to be second class.

[marcoscaceres]

It is second class. It's an optional thing - that's not a value judgement. Making a mess by overloading the constructor doesn't seem very good to me - specially given how complicated it already is to construct these objects.

[domenic]

OK, so now it is stored with the object. But it is never used by the entire spec, right? You need to wire up the getter for the paymentRequestID property to return the value of the [[paymentRequestID]] internal slot.

[marcoscaceres]

Exactly. This ReSpec is also warning that paymentRequestId is not defined for this exact reason.

[rvm4]

Sigh...okay, I can do the multiple constructor thing.

[domenic]

I'd suggest waiting on getting working group consensus on where it should go. It sounds like several people are very much in favor of putting it in PaymetnDetails or PaymentOptions, so any work on making it a separate argument would be wasted.

[zkoch]

Yeah, it might be easier/preferable to just put this in PaymentDetails. Any objection to that? Seems like less work. :)

[rvm4]

Putting this in payment details will naturally demote the importance of this field. Maybe this should be discussed in the f2f then.

[marcoscaceres]

I'm still not sure what you mean by "demotion"? It's an optional parameter with equal importance to any other optional parameter, no?

[rvm4]

Okay, please followup in PR 426 where I moved all this to the details dictionary.