Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Take paymentRequestID in constructor #413

Closed
wants to merge 2 commits into from
Closed

Take paymentRequestID in constructor #413

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eccd041
Take paymentRequestID in constructor
rvm4 Feb 7, 2017
276b44c
Update index.html
rvm4 Feb 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 17 additions & 3 deletions index.html
View file
Open in desktop
Expand Up @@ -206,8 +206,12 @@ <h2>
<dfn>PaymentRequest</dfn> interface
</h2>
<pre class="idl">
[Constructor(sequence&lt;PaymentMethodData&gt; methodData, PaymentDetails details, optional PaymentOptions options),
SecureContext]
[Constructor(
optional DOMString paymentRequestID,
Copy link
Collaborator

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

Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Member

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.

sequence&lt;PaymentMethodData&gt; methodData,
PaymentDetails details,
optional PaymentOptions options
), SecureContext]
interface PaymentRequest : EventTarget {
Promise&lt;PaymentResponse&gt; show();
Promise&lt;void&gt; abort();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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>.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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>.
Expand Down Expand Up @@ -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>
Expand Down