-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add support for merchant validation #751
Conversation
Blocking on @aestes review/feedback. This is primarily to align with Apple Pay so really need someone from Apple to approve it. |
Gecko tracking bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1474499 |
WebKit implementation: https://trac.webkit.org/changeset/226766 |
index.html
Outdated
Validate a merchant algorithm | ||
</h2> | ||
<p> | ||
<dfn>Merchant validation</dfn> is the process by which <a>payment |
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.
Two missing a's
<dfn>Merchant validation</dfn> is the process by which a <a>payment
handler</a> validates the identity of a merchant.
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.
Thanks!
d365cf1
to
b6cb08b
Compare
b6cb08b
to
5de9181
Compare
Added tests for constructor and for onmerchantvalidation attribute: |
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.
This looks good, although I was surprised to see that MerchantValidationEvent.validationURL can be initialized from the eventInitDict. Is that there for payment handlers' sake?
@aestes, wrote:
It's not handler related - but could be used there too, I guess: It used to be a requirement in the DOM Spec that there be a 1:1 relationship between initialization dictionary members and Event interface attributes. @domenic, I see this has now changed in the inner event creation steps. It seems that requirement was loosened up a bit:
Or does the 1:1 relationship rule still hold? @aestes, the |
@domenic requesting prioritization on this when you have time, as currently I'm working on an implementation in Gecko - so would really appreciate your review. |
The idea is still to have 1:1, but there are a few legacy exceptions we worked to accomodate with that clause. Will do a full review tomorrow! |
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.
Overall makes sense; some things to fix
index.html
Outdated
"MerchantValidationEvent.MerchantValidationEvent()"><code>MerchantValidationEvent</code> | ||
Constructor</dfn> | ||
</h3> | ||
<p data-tests= |
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.
This may be new technology since we last worked on this, but now we have https://dom.spec.whatwg.org/#concept-event-constructor-ext. You should define "event constructing steps" for MerchantValidationEvent which:
- set [[waitForUpdate]] etc.
- Validate/normalize/set the default for the validationURL attribute (no internal slot for event attributes).
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.
The new machinery took me a little bit to get going... but it's much nicer 👌 Will eventually need to update the rest of the spec to use it.
index.html
Outdated
Constructor</dfn> | ||
</h3> | ||
<p data-tests= | ||
"MerchantValidationEvent/constructor.https.html, MerchantValidationEvent/constructor.http.html"> |
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.
The internal slots list, both in the constructor and in the table, seem incomplete. The constructor initializes waitForUpdate and validationURL. (validationURL should not be an internal slot.) The table lists request and validationURL. The complete() method operates on request and waitForUpdate.
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.
Oops, copy/pasta. I've added a [[waitForUpdate]]
slot and linked it properly. validationURL
is now set on the attribute, not a slot.
index.html
Outdated
<ol class="algorithm"> | ||
<li>Let <var>base</var> be the <a data-cite= | ||
"dom#context-object">context object</a>’s <a data-cite= | ||
"!html/multipage/webappapis.html#relevant-settings-object">relevant |
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.
There is no context object in constructors. But once you use event constructing steps you can use event's relevant settings object.
index.html
Outdated
</td> | ||
<td> | ||
The <a>PaymentRequest</a> instance that instantiated this | ||
<a>PaymentResponse</a>. |
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.
"this" is not a PaymentResponse, so this doesn't seem right...
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.
These slots are not needed anymore. I can get request from target
, after isTrusted
is checked.
index.html
Outdated
</li> | ||
<li>Set <var>event</var>.<a>[[\waitForUpdate]]</a> to true. | ||
</li> | ||
<li>Run the <a>validate a merchant algorithm</a> with |
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.
Wrong algorithm
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.
Oh crap. I went all circular (how embarrassing!)... this was supposed to:
- disable the UI.
- wait for the promise to settle, "abort the update" is it rejects...
- if it fulfills, then
value
needs to be cryptographically verified. - if the merchant is no good, abort the update.
- if merchant is validated, enable the UI.
index.html
Outdated
<p> | ||
For <a>payment methods</a> that support <a>merchant validation</a>, | ||
the user agent runs the <dfn>validate a merchant algorithm</dfn>. The | ||
algorithm takes a USVString <var>merchantSpecificURL</var>, provided |
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.
Either link/code-ify USVString or use scalar value string.
index.html
Outdated
<li>Otherwise, set <var>request</var>.<a>[[\updating]]</a> to | ||
false. | ||
</li> | ||
<li>Enable user interface, allowing the request for payment to |
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.
Missing "the"
8cfc91b
to
e54588b
Compare
@domenic, hopefully better now :/ |
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.
Only a couple more minor things
index.html
Outdated
<h3> | ||
<dfn data-lt= | ||
"MerchantValidationEvent.MerchantValidationEvent()"><code>MerchantValidationEvent</code> | ||
Constructor</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.
Lowercase "c"
index.html
Outdated
<p data-tests= | ||
"MerchantValidationEvent/constructor.https.html, MerchantValidationEvent/constructor.http.html"> | ||
The <dfn>event constructing steps for a | ||
<code>MerchantValidationEvent</code></dfn>, which take a |
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.
Needs to link to "event constructing steps" in DOM, not define a new set of steps.
<var>base</var>. | ||
</li> | ||
<li>If <var>validationURL</var> is failure, throw a | ||
<a>TypeError</a>. |
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.
Needs to initialize event's validationURL
attribute to validationURL
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.
D'oh! Added just below.
index.html
Outdated
<li>Initialize <var>event</var>.<a data-lt= | ||
"mechvalidation.waitForUpdate">[[\waitForUpdate]]</a> to false. | ||
</li> | ||
<li>Return <var>event</var>. |
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.
No need to return anything from these steps.
Third time lucky 🤞 |
closes #646
The following tasks have been completed:
Implementation commitment:
Impact on Payment Handler spec?
Preview | Diff