New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Take paymentRequestID in constructor #413
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -206,8 +206,12 @@ <h2> | |
<dfn>PaymentRequest</dfn> interface | ||
</h2> | ||
<pre class="idl"> | ||
[Constructor(sequence<PaymentMethodData> methodData, PaymentDetails details, optional PaymentOptions options), | ||
SecureContext] | ||
[Constructor( | ||
optional DOMString paymentRequestID, | ||
sequence<PaymentMethodData> methodData, | ||
PaymentDetails details, | ||
optional PaymentOptions options | ||
), SecureContext] | ||
interface PaymentRequest : EventTarget { | ||
Promise<PaymentResponse> show(); | ||
Promise<void> abort(); | ||
|
@@ -277,7 +281,7 @@ <h2> | |
Constructor | ||
</h2> | ||
<p> | ||
The <a>PaymentRequest</a> is constructed using the supplied | ||
The <a>PaymentRequest</a> is constructed using the supplied <var>paymentRequestID</var>, | ||
<var>methodData</var> list including any <a>payment method</a> | ||
specific <a data-lt="PaymentMethodData.data">data</a>, the payment | ||
<var>details</var>, and the payment <var>options</var>. The | ||
|
@@ -556,6 +560,8 @@ <h2> | |
</li> | ||
<li>Let <var>request</var> be a new <a>PaymentRequest</a>. | ||
</li> | ||
<li>Set <var>request</var>.<a>[[\paymentRequestID]]</a> to <var>paymentRequestID</var>. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly. This ReSpec is also warning that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sigh...okay, I can do the multiple constructor thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it might be easier/preferable to just put this in PaymentDetails. Any objection to that? Seems like less work. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Putting this in payment details will naturally demote the importance of this field. Maybe this should be discussed in the f2f then. |
||
</li> | ||
<li>Set <var>request</var>.<a>[[\options]]</a> to <var>options</var>. | ||
</li> | ||
<li>Set <var>request</var>.<a>[[\state]]</a> to <i>created</i>. | ||
|
@@ -842,6 +848,14 @@ <h2> | |
Description (<em>non-normative</em>) | ||
</th> | ||
</tr> | ||
<tr> | ||
<td> | ||
<dfn>[[\paymentRequestID]]</dfn> | ||
</td> | ||
<td> | ||
The <code>paymentRequestID</code> supplied or generated during construction. | ||
</td> | ||
</tr> | ||
<tr> | ||
<td> | ||
<dfn>[[\serializedMethodData]]</dfn> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just add this to PaymentDetails or PaymentOptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of putting it in PaymentDetails or PaymentOptions because it relegates the field to be second class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.