Change from clients.openWindow to event.openWindow. by tommythorsen · Pull Request #106 · w3c/web-based-payment-handler

tommythorsen · 2017-03-14T14:50:22Z

This change introduces a new method on the PaymentRequestEvent, called
openWindow(). It is similar to clients.openWindow, but since it has a
tighter relationship to the PaymentRequest, it should be more convenient
for implementors to provide an experience that fits the payment flow.

See #97.

adrianhopebailie

LGTM, thanks @tommythorsen

tommythorsen · 2017-03-15T08:33:30Z

I have updated the PR to sit on top of @ianbjacobs' restructuring. Does it still look ok?

marcoscaceres · 2017-03-15T08:34:50Z

index.html

      [Exposed=ServiceWorker]
      interface PaymentRequestEvent : ExtendableEvent {
        readonly attribute PaymentAppRequest appRequest;
+        Promise<WindowClient> openWindow(DOMString url);


URL are USVStrings

From https://heycam.github.io/webidl/#idl-USVString:

Specifications should only use USVString for APIs that perform text processing and need a string of Unicode scalar values to operate on. Most APIs that use strings should instead be using DOMString, which does not make any interpretations of the code units in the string. When in doubt, use DOMString.

I was in doubt, so I used DOMString. What calls for USVString in this situation?

marcoscaceres · 2017-03-15T08:35:20Z

index.html

          This attribute contains the <a>payment app request</a> associated
          with this <a>payment request</a>.
        </dd>
+        <dt><code>openWindow</code> method</dt>


Please define methods in their own section.

I've done this, but now this section is inconsistent with the rest of the document. I can go through and give the other sections the same treatment, but that will be a separate PR.

marcoscaceres · 2017-03-15T08:35:51Z

index.html

+        <dt><code>openWindow</code> method</dt>
+        <dd>
+          This method is used by the payment handler to show a window to
+          the user.


Should like to algorithm that describes steps

marcoscaceres · 2017-03-15T08:36:38Z

index.html

-  <p class="issue">See <a href="https://github.com/w3c/webpayments-payment-apps-api/issues/97">issue 97</a> for discussion about opening window in a way that is consistent with payment flow and not confusing to the user. For example, there is consensus that in a desktop environment, a payment handler should <em>not</em> open a window in a new browser tab, as this is too dissociated from the checkout context.</p>
-
-  <p>User agents SHOULD display the origin of a running payment handler to the user.</p>
+  <p>A payment handler that requires visual display and user interaction, may call <code>PaymentRequestEvent.openWindow()</code>.


nit: <code>PaymentRequestEvent.openWindow()</code>. openWindow()`.

marcoscaceres · 2017-03-15T08:37:17Z

index.html

+    Since user agents know that this method is connected to the payment request event, they SHOULD render the window in a way that is consistent with the flow and not confusing to the user.</p>
+
+  <p>The resulting window client is bound to the tab/window that initiated the <a>payment request</a>.
+    A single <a>payment handler</a> SHOULD NOT be allowed to open more than one client window using this method.


All this needs to be in algorithm.

I'm working on this. Aiming to update this PR with an algorithm for openWindow tomorrow.

marcoscaceres · 2017-03-15T08:37:48Z

index.html

+
+  <p>The resulting window client is bound to the tab/window that initiated the <a>payment request</a>.
+    A single <a>payment handler</a> SHOULD NOT be allowed to open more than one client window using this method.
+    The exact behavior in the case where a payment handler calls <code>openWindow()</code> multiple times, is left to the user agent implementers to decide on.


It should probably throw.

(invalid state error)

Ok. I'll change this.

marcoscaceres · 2017-03-15T08:38:44Z

index.html

+    The exact behavior in the case where a payment handler calls <code>openWindow()</code> multiple times, is left to the user agent implementers to decide on.
+    User agents MAY, in this case, let subsequent calls to <code>openWindow()</code> resolve to the same <code>WindowClient</code> instance.</p>
+
+  <p>User agents SHOULD display the origin of the payment handler to the user.</p>


Should be in the privacy and security considerations section

marcoscaceres · 2017-03-15T08:50:17Z

This is not aimed at @tommythorsen - but in general, can we please be extremely disciplined with adding things to this spec.

We've had to waste a lot of time on the Payment Request spec dealing with technical debt built up by adding bad markup, random phrasing, conformance statements that don't actually apply to any products, etc.

Can we also be disciplined with running HTML5 Tidy over the document before any commit goes in - and make sure ReSpec is being utilized to the fullest (i.e., don't use <code> tags). And make sure we don't pollute the commit history with random commit messages.

Also, no one should be committing directly to gh-pages. @sideshowbarker, can you please set up branch protection to stop people doing that - i.e., only allow pull requests?

ReSpec is already screaming that this document has over 66 issues. That's already really alarming - we need to do better and not rush things into the spec.

If Editors need a "how to best use ReSpec" tutorial during the F2F, I'm happy to run that.

sideshowbarker · 2017-03-15T09:37:25Z

no one should be committing directly to gh-pages. @sideshowbarker, can you please set up branch protection to stop people doing that - i.e., only allow pull requests?

I doesn’t seem appropriate for me or anybody else from the W3C staff to try to unilaterally impose branch protection on a repo. That should be something for the people actually doing the work to discuss and decide on. https://github.com/orgs/w3c/teams/payment-app-api-editors shows a number of people with admin access to the repo who can set pull-request-required branch protection for the default branch if that’s what they want.

marcoscaceres · 2017-03-15T09:38:34Z

@sideshowbarker, fair. Can someone with admin please put branch protection on?

tommythorsen · 2017-03-15T10:30:39Z

@marcoscaceres

Can someone with admin please put branch protection on?

Done!

adrianhopebailie · 2017-03-15T12:58:03Z

can we please be extremely disciplined with adding things to this spec.

We've had to waste a lot of time on the Payment Request spec dealing with technical debt built up by adding bad markup, random phrasing, conformance statements that don't actually apply to any products, etc.

Can we also be disciplined with running HTML5 Tidy over the document before any commit goes in - and make sure ReSpec is being utilized to the fullest (i.e., don't use <code> tags). And make sure we don't pollute the commit history with random commit messages.

@marcoscaceres - thanks for this comment. I think I speak for all of the editors (@ianbjacobs , @adamroach , @tommythorsen , @jnormore and myself) when I say that we're fairly new to this (and if not then fairly new to specs that include WebIDL and browser algorithms or new to ReSpec or GitHub or all of it).

So, I am 100% in agreement that we try and avoid accumulating debt that we need to deal with later but also appeal to you to help us out here.

Feel free to point out things we're doing wrong (as you have above) or share some pro tips because we're keen to do them right.

While I appreciate you are putting a lot of effort into the PR spec right now your help as an editor here would be appreciated if you can spare some time. I think that at a minimum @tommythorsen , you and I could have an occasional call to sync up since timezones make it hard for you guys to join the TF calls.

tommythorsen · 2017-03-16T09:37:29Z

I think I have addressed all of @marcoscaceres' review comments. I have introduced a few more respec warnings, but I intend to take a closer look at the whole list of respec issues in another PR, after this one is finished.

marcoscaceres · 2017-03-16T11:19:41Z

Thanks for the update. Will follow up soon.

…

On 16 Mar 2017, at 8:37 pm, Tommy Thorsen ***@***.***> wrote: I think I have addressed all of @marcoscaceres' review comments. I have introduced a few more respec warnings, but I intend to take a closer look at the whole list of respec issues in another PR, after this one is finished. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ianbjacobs · 2017-03-16T13:09:07Z

Thank you @tommythorsen! I have looked at the changes. I look forward to @marcoscaceres' review.

@jakearchibald, your input would also be appreciated.

Ian

marcoscaceres · 2017-03-17T01:37:40Z

A general note on style and invoking algorithms: see how it's done in the Payment Request spec. Ideally, this spec should end up being edited in the same way as that one.

For other examples of excellent specs whose style we should follow:

dlongley · 2017-03-17T04:17:05Z

The general approach of opening the window via a call on the event (to make it context-aware) LGTM.

marcoscaceres · 2017-03-17T04:52:09Z

@marcoscaceres - thanks for this comment. I think I speak for all of the editors (@ianbjacobs , @adamroach , @tommythorsen , @jnormore and myself) when I say that we're fairly new to this (and if not then fairly new to specs that include WebIDL and browser algorithms or new to ReSpec or GitHub or all of it).

Good guides if getting started:

So, I am 100% in agreement that we try and avoid accumulating debt that we need to deal with later but also appeal to you to help us out here.

I'm happy to help. Please tag me on reviews and I'll get to them as quickly as possible. I'll have more time once we put Payment Request into CR.

tommythorsen · 2017-03-17T09:52:12Z

@marcoscaceres: Thanks for the pointers!

Did you have any specific change requests? This PR is blocked from merging by your earlier change request, so if you're happy with the current state of the PR, perhaps you would like to change to approval?

marcoscaceres · 2017-03-17T14:28:31Z

Odd? I added 30+ change requests earlier today?

…

On 17 Mar 2017, at 8:52 pm, Tommy Thorsen ***@***.***> wrote: @marcoscaceres: Thanks for the pointers! Did you have any specific change requests? This PR is blocked from merging by your earlier change request, so if you're happy with the current state of the PR, perhaps you would like to change to approval? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

marcoscaceres · 2017-03-17T14:46:13Z

@tommythorsen see my change requests above. I can see them all there

tommythorsen · 2017-03-17T14:48:34Z

30 change requests from today? The only change requests I can see are the 7 outdated ones from two days ago.

tommythorsen · 2017-03-20T11:52:23Z

@marcoscaceres: I still see no new change requests from you. I have not received an email about them either. Can you double-check that they have been submitted?

Can anyone else see these new change requests? @ianbjacobs? @adrianhopebailie?

marcoscaceres · 2017-03-17T01:39:59Z

index.html

+          <table>
+            <tr>
+              <th>Internal Slot</th>
+              <th>Description (<em>non-normative</em>)</th>


If we are going to have defaults, we should have a default column in the table.

marcoscaceres · 2017-03-17T01:43:07Z

index.html

+        <section>
+          <h3>Handling a payment request event</h3>
+          <p>Upon receiving a <a>payment request</a> by way of
+          <code>PaymentRequest.show()</code> and subsequent user


Nit: <code><a data-cite="!payment-request#show-method">PaymentRequest.show()</a></code>

marcoscaceres · 2017-03-17T01:43:43Z

index.html

+          <p>Upon receiving a <a>payment request</a> by way of
+          <code>PaymentRequest.show()</code> and subsequent user
+          selection of a payment handler, the <a>user agent</a>
+          MUST run the following steps or their equivalent:</p>


nit: remove "their equivalent"

marcoscaceres · 2017-03-17T01:44:23Z

index.html

+          MUST run the following steps or their equivalent:</p>
+          <ol>
+            <li>Let <var>registration</var> be the <a>service
+            worker registration</a> corresponding to the payment


Expected link to "payment handler" definition

marcoscaceres · 2017-03-17T01:47:11Z

index.html

+      than one client window using this method.</p>
+      <section>
+        <h3><dfn>Open Window Algorithm</dfn></h3>
+        <p class="note">This algorithm is <i>very</i> similar to


nit: class="ednote" ... should probably file a new bug for this, then we can link it to the spec.

See also: https://github.com/w3c/respec/wiki/Referencing-GitHub-issues-in-your-spec

marcoscaceres · 2017-03-17T04:37:31Z

index.html

+      know that this method is connected to the payment request
+      event, they SHOULD render the window in a way that is
+      consistent with the flow and not confusing to the user.</p>
+      <p>The resulting window client is bound to the tab/window


Same with this... all this reads very "non-normative".

marcoscaceres · 2017-03-17T04:39:33Z

index.html

-      conveyed using the following dictionary:
+    <section data-dfn-for="PaymentAppRequest" data-link-for="PaymentAppRequest">
+      <h3>Payment App Request</h3>
+      <p>The <a>payment app request</a> is


this is redundant. Talk about PaymentAppRequest instead.

marcoscaceres · 2017-03-17T04:41:17Z

index.html

+          associated with this <a>payment request</a>.</p>
+        </section>
+        <section>
+          <h3><dfn data-lt="openWindow()">openWindow</dfn>


nit: <dfn>openWindow()</dfn>

marcoscaceres · 2017-03-17T04:42:08Z

index.html

+          <h3><dfn data-lt="openWindow()">openWindow</dfn>
+          method</h3>
+          <p>This method is used by the payment handler to show a
+          window to the user. It invokes the <a>Open Window


Maybe say: "When called, it runs the ..." in lower case.

marcoscaceres · 2017-03-17T04:42:30Z

index.html

+          Algorithm</a>.</p>
+        </section>
+        <section>
+          <h3><dfn>respondWith</dfn> method</h3>


nit: missing "()"

marcoscaceres · 2017-03-20T13:02:04Z

Argh, my bad. Seems I didn't hit the request changes button.

This change introduces a new method on the PaymentRequestEvent, called openWindow(). It is similar to clients.openWindow, but since it has a tighter relationship to the PaymentRequest, it should be more convenient for implementors to provide an experience that fits the payment flow.

marcoscaceres

So, approving this for merge so we can deal with the much larger issues around event dispatching. All that needs to be rewritten.

marcoscaceres · 2017-03-28T03:08:26Z

I feel like we should still pull everything out of this spec an slowly add it back in one part at a time. We've again gone rushed a whole bunch of stuff into a spec without it being carefully checked 😭.

adrianhopebailie approved these changes Mar 14, 2017

View reviewed changes

tommythorsen force-pushed the openwindow branch 2 times, most recently from ec2e1a5 to 9996e16 Compare March 15, 2017 08:27

tommythorsen mentioned this pull request Mar 15, 2017

Different approaches to opening windows #97

Closed

marcoscaceres requested changes Mar 15, 2017

View reviewed changes

tommythorsen force-pushed the openwindow branch from 7198666 to 80ada6f Compare March 15, 2017 12:56

tommythorsen force-pushed the openwindow branch from 80ada6f to 45aae5b Compare March 16, 2017 09:33

tommythorsen force-pushed the openwindow branch 2 times, most recently from c235833 to 4471bbb Compare March 16, 2017 10:08

marcoscaceres requested changes Mar 20, 2017

View reviewed changes

tommythorsen mentioned this pull request Mar 21, 2017

Open Window Algorithm #115

Closed

Tommy Thorsen and others added 3 commits March 21, 2017 11:48

Add an algorithm for opening a window

d2c4a1f

fixed up markup a bit

b95eb07

Adress Marcos' review comments

d33e8f5

tommythorsen force-pushed the openwindow branch from 2fbdf15 to d33e8f5 Compare March 21, 2017 10:51

One more tiny change

5933c1d

marcoscaceres approved these changes Mar 28, 2017

View reviewed changes

ianbjacobs merged commit f396309 into w3c:gh-pages Mar 29, 2017

Conversation

tommythorsen commented Mar 14, 2017

Uh oh!

adrianhopebailie left a comment

Choose a reason for hiding this comment

Uh oh!

tommythorsen commented Mar 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoscaceres commented Mar 15, 2017

Uh oh!

sideshowbarker commented Mar 15, 2017

Uh oh!

marcoscaceres commented Mar 15, 2017

Uh oh!

tommythorsen commented Mar 15, 2017

Uh oh!

adrianhopebailie commented Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tommythorsen commented Mar 16, 2017

Uh oh!

marcoscaceres commented Mar 16, 2017 via email

Uh oh!

ianbjacobs commented Mar 16, 2017

Uh oh!

marcoscaceres commented Mar 17, 2017

Uh oh!

dlongley commented Mar 17, 2017

Uh oh!

marcoscaceres commented Mar 17, 2017

Uh oh!

tommythorsen commented Mar 17, 2017

Uh oh!

marcoscaceres commented Mar 17, 2017 via email

Uh oh!

marcoscaceres commented Mar 17, 2017

Uh oh!

tommythorsen commented Mar 17, 2017

Uh oh!

tommythorsen commented Mar 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

adrianhopebailie commented Mar 15, 2017 •

edited

Loading