- From: Marcos Cáceres <notifications@github.com>
- Date: Tue, 11 Apr 2017 20:02:21 -0700
- To: w3c/webpayments-payment-apps-api <webpayments-payment-apps-api@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/webpayments-payment-apps-api/pull/126/review/32270726@github.com>
marcoscaceres requested changes on this pull request.
Getting there... note that I didn't review the actual content. Just mostly the markup.
> @@ -61,6 +61,16 @@
],
status: "WD"
},
+ "BASIC-CARD": {
You should remove this. Just cite "[[!payment-method-basic-card]]"
> <p>This specification relies on several other underlying
specifications.</p>
<dl>
<dt>Payment Request API</dt>
- <dd>The terms <dfn>PaymentRequest</dfn>,
- <dfn>PaymentResponse</dfn>, and <dfn>user accepts the payment
- request algorithm</dfn> are defined by the Payment Request
- API specification [[!PAYMENT-REQUEST-API]].</dd>
+ <dd>The terms <dfn>payment method</dfn>, <dfn>PaymentRequest</dfn>,
all this need to be referenced as (see how I did it in the PR spec):
```HTML
<dfn data-cite="!PAYMENT-REQUEST-API#payment-method">payment method</dfn>
```
> <p>This specification relies on several other underlying
specifications.</p>
<dl>
<dt>Payment Request API</dt>
- <dd>The terms <dfn>PaymentRequest</dfn>,
- <dfn>PaymentResponse</dfn>, and <dfn>user accepts the payment
- request algorithm</dfn> are defined by the Payment Request
- API specification [[!PAYMENT-REQUEST-API]].</dd>
+ <dd>The terms <dfn>payment method</dfn>, <dfn>PaymentRequest</dfn>,
+ <dfn>PaymentResponse</dfn>, <dfn>supportedMethods</dfn>, <dfn>paymentDetailsModifier</dfn>, <dfn>paymentDetails</dfn>, <dfn>PaymentMethodData</dfn>, <dfn>ID</dfn> and <dfn>user accepts the payment request algorithm</dfn> are defined by the Payment Request
+ API specification [[!PAYMENT-REQUEST-API]]. This specification also relies on the Payment Request API usage of <dfn>Promise</dfn>, <dfn>internal slot</dfn>, <dfn>TypeError</dfn>, <dfn>JSON.stringify</dfn>, and <dfn>JSON-serializable</dfn>.</dd>
This is not correct:
> This specification also relies on the Payment Request API usage of <dfn>Promise</dfn>
Promise comes from the ES6 spec.
> <p>This specification relies on several other underlying
specifications.</p>
<dl>
<dt>Payment Request API</dt>
- <dd>The terms <dfn>PaymentRequest</dfn>,
- <dfn>PaymentResponse</dfn>, and <dfn>user accepts the payment
- request algorithm</dfn> are defined by the Payment Request
- API specification [[!PAYMENT-REQUEST-API]].</dd>
+ <dd>The terms <dfn>payment method</dfn>, <dfn>PaymentRequest</dfn>,
+ <dfn>PaymentResponse</dfn>, <dfn>supportedMethods</dfn>, <dfn>paymentDetailsModifier</dfn>, <dfn>paymentDetails</dfn>, <dfn>PaymentMethodData</dfn>, <dfn>ID</dfn> and <dfn>user accepts the payment request algorithm</dfn> are defined by the Payment Request
+ API specification [[!PAYMENT-REQUEST-API]]. This specification also relies on the Payment Request API usage of <dfn>Promise</dfn>, <dfn>internal slot</dfn>, <dfn>TypeError</dfn>, <dfn>JSON.stringify</dfn>, and <dfn>JSON-serializable</dfn>.</dd>
(same with other terms here... again, you can probably just copy/paste from the PR spec.)
> <p>This specification relies on several other underlying
specifications.</p>
<dl>
<dt>Payment Request API</dt>
- <dd>The terms <dfn>PaymentRequest</dfn>,
- <dfn>PaymentResponse</dfn>, and <dfn>user accepts the payment
- request algorithm</dfn> are defined by the Payment Request
- API specification [[!PAYMENT-REQUEST-API]].</dd>
+ <dd>The terms <dfn>payment method</dfn>, <dfn>PaymentRequest</dfn>,
+ <dfn>PaymentResponse</dfn>, <dfn>supportedMethods</dfn>, <dfn>paymentDetailsModifier</dfn>, <dfn>paymentDetails</dfn>, <dfn>PaymentMethodData</dfn>, <dfn>ID</dfn> and <dfn>user accepts the payment request algorithm</dfn> are defined by the Payment Request
+ API specification [[!PAYMENT-REQUEST-API]]. This specification also relies on the Payment Request API usage of <dfn>Promise</dfn>, <dfn>internal slot</dfn>, <dfn>TypeError</dfn>, <dfn>JSON.stringify</dfn>, and <dfn>JSON-serializable</dfn>.</dd>
+ <dt>Payment Method Identifiers</dt>
+ <dd>The terms <dfn data-lt="payment method identifier|payment method identifiers">payment method identifier</dfn> is defined by the Payment Method
Nit: you don't need "payment method identifier"... the original term is derived from the `dfn`... so just:
```
<dfn data-lt="payment method identifiers">payment method identifier</dfn>
```
> <p>This specification relies on several other underlying
specifications.</p>
<dl>
<dt>Payment Request API</dt>
- <dd>The terms <dfn>PaymentRequest</dfn>,
- <dfn>PaymentResponse</dfn>, and <dfn>user accepts the payment
- request algorithm</dfn> are defined by the Payment Request
- API specification [[!PAYMENT-REQUEST-API]].</dd>
+ <dd>The terms <dfn>payment method</dfn>, <dfn>PaymentRequest</dfn>,
+ <dfn>PaymentResponse</dfn>, <dfn>supportedMethods</dfn>, <dfn>paymentDetailsModifier</dfn>, <dfn>paymentDetails</dfn>, <dfn>PaymentMethodData</dfn>, <dfn>ID</dfn> and <dfn>user accepts the payment request algorithm</dfn> are defined by the Payment Request
+ API specification [[!PAYMENT-REQUEST-API]]. This specification also relies on the Payment Request API usage of <dfn>Promise</dfn>, <dfn>internal slot</dfn>, <dfn>TypeError</dfn>, <dfn>JSON.stringify</dfn>, and <dfn>JSON-serializable</dfn>.</dd>
+ <dt>Payment Method Identifiers</dt>
+ <dd>The terms <dfn data-lt="payment method identifier|payment method identifiers">payment method identifier</dfn> is defined by the Payment Method
+ Identifier specification [[!METHOD-IDENTIFIERS]].</dd>
+ <dt>Basic Card Payment</dt>
+ <dd>The terms <dfn>basic-card</dfn>, <dfn>supportedNetworks</dfn>, and <dfn>supportedTypes</dfn> are defined in [[BASIC-CARD]].</dd>
Nit: shouldn't [[BASIC-CARD]] be normatively referenced? As in [[!BASIC-CARD]]
> <dt>HTML5</dt>
- <dd>The terms <dfn>global object</dfn>,<dfn>origin</dfn>,
- <dfn>queue a task</dfn>, <dfn>browsing context</dfn>,
+ <dd>The terms <dfn>global object</dfn>, <dfn>origin</dfn>,
Nit: origin is defined in the origin spec.
> @@ -191,28 +201,29 @@
<th>Message (optional)</th>
</tr>
<tr>
- <td><code><dfn>AbortError</dfn></code></td>
+ <td><dfn>AbortError</dfn></td>
These are all defined in WebIDL - not in DOM.
> @@ -191,28 +201,29 @@
<th>Message (optional)</th>
</tr>
<tr>
- <td><code><dfn>AbortError</dfn></code></td>
+ <td><dfn>AbortError</dfn></td>
(you can copy/paste this also from PR spec... but please don't define terms that are not used!)
> @@ -224,7 +235,7 @@
<th>Message (optional)</th>
</tr>
<tr>
- <td><code><dfn>NotAllowedError</dfn></code></td>
+ <td><dfn>NotAllowedError</dfn></td>
Please remove the table. See how we do it in PR spec.
> <dt>Service Workers</dt>
- <dd>The terms <dfn data-lt=
- "service worker|service workers">service worker</dfn>
- <dfn>service worker registration</dfn>, <dfn>active
- worker</dfn>, <dfn>installing worker</dfn>, <dfn>waiting
- worker</dfn>, <dfn>handle functional event</dfn>, <dfn>extend
- lifetime promises</dfn>, and <dfn data-lt=
- "scope url|scope urls">scope URL</dfn> are defined in
- [[SERVICE-WORKERS]].</dd>
+ <dd>The terms <dfn>service worker</dfn>, <dfn>ServiceWorkerRegistration</dfn>, <dfn>ServiceWorkerGlobalScope</dfn>, <dfn>handle functional event</dfn>, <dfn>extend lifetime promises</dfn>, and <dfn>scope URL</dfn> are defined in
As above... For cited interfaces, please use:
```HTML
<code><dfn>ServiceWorkerGlobalScope</dfn></code>
```
ReSpec will automatically "code" all cross referenced for you.
> @@ -278,7 +274,7 @@
of Instruments supported by the payment handler.</li>
</ul>
</li>
- <li>When the merchant calls Payment Request API (e.g., when
+ <li>When the merchant (or other <dfn>payee</dfn>) calls Payment Request API (e.g., when
Nit: please `<cite>` Payment Request API - or better yet, link to it.
> @@ -322,166 +319,164 @@
authentication, user interaction, and so on.</p>
</section>
<section id="registration">
- <h2>Payment Handler Registration</h2>
+ <h2><dfn>Payment Handler</dfn> Registration</h2>
Nit, this seems like an odd place to dfn "Payment Handler" - specially if this is to do with registration.
> <section>
- <h3>Extensions to the <a>ServiceWorkerRegistration</a>
- interface</h3>
- <p>The Service Worker specification defines a
- <code>ServiceWorkerRegistration</code> interface
- [[!SERVICE-WORKERS]], which this specification extends.</p>
- <pre id="service-worker-registration-idl" class="idl">
+ <h2>Extension to the ServiceWorkerRegistration interface</h2>
Nit: `<code>ServiceWorkerRegistration</code>`
>
- <dt><code>delete</code> method</dt>
- <dd>
- When called, this method executes the following steps:
+
+ <pre class="example highlight" title="Removing all instruments">
Nit: you don't need "highlight" in the class list - you might just want to say "js".
>
- <dt><code>delete</code> method</dt>
- <dd>
- When called, this method executes the following steps:
+
+ <pre class="example highlight" title="Removing all instruments">
+ return paymentManager.instruments.keys().then((keys) => {
This example doesn't make any sense... it's both a syntax error and doesn't do anything with `Promise.all`.
If we want to `clear()`, then we should add a clear method to the interface.
> Promise<PaymentInstrument> get(DOMString instrumentKey);
Promise<sequence<DOMString>> keys();
- Promise<boolean> has(DOMString instrumentKey);
- Promise<void> set(DOMString instrumentKey,
- PaymentInstrument details);
+ Promise<boolean> has(DOMString instrumentKey);
+ Promise<void> set(DOMString instrumentKey, PaymentInstrument details);
See below. Let's just add:
```
Promise<void> clear();
```
> };
</pre>
- <p>Where it appears, The <code>walletKey</code> parameter is
+ <p>Where it appears, The <dfn>walletKey</dfn> parameter is
s/parameter/argument?
> };
</pre>
- <p>Where it appears, The <code>walletKey</code> parameter is
+ <p>Where it appears, The <dfn>walletKey</dfn> parameter is
Actually, check what IDL uses... I can't remember if it is "parameter" or "argument" or something else.
>
- <dt><code>delete</code> method</dt>
- <dd>
- When called, this method executes the following steps:
+ <section>
+ <h2><dfn>delete</dfn> method</h2>
nit: please change to `<dfn>delete()</dfn>` ... and please do the same for other methods.
> <ol>
- <li> If the collection contains a <a>WalletDetails</a>
- with a matching <code>walletKey</code>, remove it from
+ <li> If the collection contains a <a>PaymentWallet</a>
+ with a matching <a>walletKey</a>, remove it from
nit: arguments should be marked up using `<var>`
> <ol>
<li>Resolve <var>p</var> with a sequence that contains all
- the <code>walletKey</code>s for the <a>WalletDetails</a>s
+ the <a>walletKey</a>s for the <a>PaymentWallet</a>s
This should say something about the ordering of the collection.
> @@ -653,7 +647,7 @@
</dl>
</section>
<section id="register-example">
This section should probably be marked as non-normative.
> of the <a>payee</a> web page. It MUST be formatted
according to the "<a href=
- "https://tools.ietf.org/html/rfc6454#section-6.1">Unicode
- Serialization of an Origin</a>" algorithm defined in
+ "https://tools.ietf.org/html/rfc6454#section-6.1">Unicode
please don't cite specs using URLs... just [[!rfc6454#section-6.1]].
> of the <a>payee</a> web page. It MUST be formatted
according to the "<a href=
- "https://tools.ietf.org/html/rfc6454#section-6.1">Unicode
- Serialization of an Origin</a>" algorithm defined in
+ "https://tools.ietf.org/html/rfc6454#section-6.1">Unicode
fixed comment above.
> - is conveyed using the following dictionary:
- <pre class="idl">
- dictionary PaymentAppResponse {
- DOMString methodName;
- object details;
- };
+
+ <section id="post-example">
+ <h2>Example of handling the <a>PaymentRequestEvent</a></h2>
+ <p>This example shows how to write a service worker that
+ listens to the <a>PaymentRequestEvent</a>. When a
+ <a>PaymentRequestEvent</a> is received, the service worker opens a window in order
+ to interact with the user.</p>
+ <pre class="example highlight" title=
+ "Handling the PaymentRequestEvent">
+ self.addEventListener('paymentrequest', function(e) {
Please remove this example - it's no good. Sorry :(
> - is conveyed using the following dictionary:
- <pre class="idl">
- dictionary PaymentAppResponse {
- DOMString methodName;
- object details;
- };
+
+ <section id="post-example">
+ <h2>Example of handling the <a>PaymentRequestEvent</a></h2>
+ <p>This example shows how to write a service worker that
+ listens to the <a>PaymentRequestEvent</a>. When a
+ <a>PaymentRequestEvent</a> is received, the service worker opens a window in order
+ to interact with the user.</p>
+ <pre class="example highlight" title=
+ "Handling the PaymentRequestEvent">
+ self.addEventListener('paymentrequest', function(e) {
Please move it to another bug, and I'll try to rewrite it. But it contains a bunch of bugs and just outright "oh god no!" 😱.
> - };
+
+ <section id="post-example">
+ <h2>Example of handling the <a>PaymentRequestEvent</a></h2>
+ <p>This example shows how to write a service worker that
+ listens to the <a>PaymentRequestEvent</a>. When a
+ <a>PaymentRequestEvent</a> is received, the service worker opens a window in order
+ to interact with the user.</p>
+ <pre class="example highlight" title=
+ "Handling the PaymentRequestEvent">
+ self.addEventListener('paymentrequest', function(e) {
+ e.respondWith(new Promise(function(resolve, reject) {
+ self.addEventListener('message', listener = function(e) {
+ self.removeEventListener('message', listener);
+ if (e.data.hasOwnProperty('name')) {
+ reject(e.data);
😱
> - is conveyed using the following dictionary:
- <pre class="idl">
- dictionary PaymentAppResponse {
- DOMString methodName;
- object details;
- };
+
+ <section id="post-example">
+ <h2>Example of handling the <a>PaymentRequestEvent</a></h2>
+ <p>This example shows how to write a service worker that
+ listens to the <a>PaymentRequestEvent</a>. When a
+ <a>PaymentRequestEvent</a> is received, the service worker opens a window in order
+ to interact with the user.</p>
+ <pre class="example highlight" title=
+ "Handling the PaymentRequestEvent">
+ self.addEventListener('paymentrequest', function(e) {
I'll convert it to use async syntax... will be much more legible. Remember the rule: if you see `.then()` anywhere, that's a code smell and likely an anti-pattern.
> - object details;
- };
+
+ <section id="post-example">
+ <h2>Example of handling the <a>PaymentRequestEvent</a></h2>
+ <p>This example shows how to write a service worker that
+ listens to the <a>PaymentRequestEvent</a>. When a
+ <a>PaymentRequestEvent</a> is received, the service worker opens a window in order
+ to interact with the user.</p>
+ <pre class="example highlight" title=
+ "Handling the PaymentRequestEvent">
+ self.addEventListener('paymentrequest', function(e) {
+ e.respondWith(new Promise(function(resolve, reject) {
+ self.addEventListener('message', listener = function(e) {
+ self.removeEventListener('message', listener);
+ if (e.data.hasOwnProperty('name')) {
... also, if async code nests more than 3 levels, that's also indicative that something is wrong.
> </pre>
- <dl>
- <dt><code>methodName</code> attribute</dt>
- <dd>
+ <p>Using the simple scheme described above, a trivial HTML
+ page that is loaded into the <a>payment handler window</a> to
+ implement the <em>basic card</em> scheme might look like the
+ following:</p>
+ <pre class="example highlight" title=
Nit: s/highlight/html
This should be a:
```HTML
<form>
<label>Cardholder Name: <input ... ></label>
</form>
```
> </pre>
- <dl>
- <dt><code>methodName</code> attribute</dt>
- <dd>
+ <p>Using the simple scheme described above, a trivial HTML
+ page that is loaded into the <a>payment handler window</a> to
+ implement the <em>basic card</em> scheme might look like the
+ following:</p>
+ <pre class="example highlight" title=
By "this" I mean the table below in the example.
> </pre>
- <dl>
- <dt><code>methodName</code> attribute</dt>
- <dd>
+ <p>Using the simple scheme described above, a trivial HTML
+ page that is loaded into the <a>payment handler window</a> to
+ implement the <em>basic card</em> scheme might look like the
+ following:</p>
+ <pre class="example highlight" title=
Feel free to remove and assign to me to add.
also, please remove the html and body tags.
> + /* Note: message sent from payment app is available in e.data */
+ form.onsubmit = function() {
+ /* See https://w3c.github.io/webpayments-methods-card/#basiccardresponse */
+ var basicCardResponse = {};
+ [ "cardholderName", "cardNumber","expiryMonth","expiryYear","cardSecurityCode"]
+ .forEach(function(field) {
+ basicCardResponse[field] = form.elements[field].value;
+ });
+
+ /* See https://w3c.github.io/webpayments-payment-apps-api/#sec-app-response */
+ var paymentAppResponse = {
+ methodName: "basic-card",
+ details: details
+ };
+
+ e.source.postMessage(paymentAppResponse);
Fire and forget? Shouldn't you wait for an ack from the other side?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/webpayments-payment-apps-api/pull/126#pullrequestreview-32270726
Received on Wednesday, 12 April 2017 03:02:58 UTC