Define requestPermission() behavior. #180

romandev · 2017-06-21T12:40:13Z

In the current spec, there is no definition of the requestPermission() method.
This change is fixing #94 issue.

In the current spec, there is no definition of the requestPermission() method. This change is fixing w3c#94 issue.

romandev · 2017-06-21T12:41:05Z

Thank you.

rsolomakhin · 2017-06-21T14:06:59Z

index.html

      interface PaymentManager {
        [SameObject] readonly attribute PaymentInstruments instruments;
        [SameObject] readonly attribute PaymentWallets wallets;
-                              Promise&lt;boolean&gt;   requestPermission();
+        [Exposed=Window] static Promise&lt;PermisionState&gt; requestPermission();


What does [Exposed=Window] do?

I believe that the [SecureContext] extended attribute applies to the entire interface (exposing it only in secure contexts) and the [Exposed=Window] is a further restriction, specifically, on requestPermission to cause it to only be exposed when the global context is Window (as opposed to in a Worker, for example -- where it won't be accessible given this directive).

Also, there's a typo here: PermisionState => PermissionState

Done.

FYI, I found a history of Notification.requestPermission().
https://lists.w3.org/Archives/Public/public-whatwg-archive/2014Jul/0178.html

Thanks @romandev, @rsolomakhin, and @dlongley!

@romandev, you deleted:
" The user agent is NOT REQUIRED to prompt the user to grant permission to the origin for each new supported payment method."

I think we still need that text.

@ianbjacobs, You're correct!

Done. Thanks.

rsolomakhin · 2017-06-21T14:09:52Z

index.html

+                allowing adding new payment instruments for the <a data-cite=
+                "!HTML#current-settings-object">current settings object</a>'s
+                origin is acceptable. If it is, set <var>permission</var> to
+                "granted", and "denied" otherwise.


Your implementation can also resolve p with "default". When would that happen?

In the implementation, I defined PaymentHandlerStatus as 'default' instead of 'prompt'.
Because the Notification spec[1] is doing like that.
But I awared that we can use PermissionState in Permission API spec[2] instead of our own definition.

[1] https://notifications.spec.whatwg.org/#enumdef-notificationpermission
[2] https://www.w3.org/TR/permissions/#idl-def-PermissionState

rsolomakhin · 2017-06-21T14:10:29Z

index.html

+              with <a>payment handler</a>.
+            </li>
+            <li>If <var>permission</var> is not "granted", then return a
+            <a>Promise</a> rejected with a <a>SecurityError</a>.


NotAllowedError might be better, IMHO.

rsolomakhin · 2017-06-21T14:10:41Z

index.html

+              with <a>payment handler</a>.
+            </li>
+            <li>If <var>permission</var> is not "granted", then return a
+            <a>Promise</a> rejected with a <a>SecurityError</a>.


rsolomakhin

Thank you for the patch! Good work!

dlongley · 2017-06-21T14:30:40Z

index.html

+                <li>Let <var>permission</var> be the result of running
+                <a data-cite="!permissions#dfn-retrieve-the-permission-state">
+                  retrieve the permission state algorithm</a> of the permission
+                  associated with <a>payment handler</a>.


Should this say for the permission associated with <a>payment handler's</a> <a>origin</a>?

romandev

@rsolomakhin, @dlongley

Thank you for review 👍

I addressed your comments.

romandev · 2017-06-21T16:23:12Z

index.html

      interface PaymentManager {
        [SameObject] readonly attribute PaymentInstruments instruments;
        [SameObject] readonly attribute PaymentWallets wallets;
-                              Promise&lt;boolean&gt;   requestPermission();
+        [Exposed=Window] static Promise&lt;PermisionState&gt; requestPermission();


Done.

FYI, I found a history of Notification.requestPermission().
https://lists.w3.org/Archives/Public/public-whatwg-archive/2014Jul/0178.html

romandev · 2017-06-21T16:23:29Z

index.html

+              with <a>payment handler</a>.
+            </li>
+            <li>If <var>permission</var> is not "granted", then return a
+            <a>Promise</a> rejected with a <a>SecurityError</a>.


romandev · 2017-06-21T16:23:36Z

index.html

+              with <a>payment handler</a>.
+            </li>
+            <li>If <var>permission</var> is not "granted", then return a
+            <a>Promise</a> rejected with a <a>SecurityError</a>.


romandev · 2017-06-21T16:46:09Z

index.html

+                <li>Let <var>permission</var> be the result of running
+                <a data-cite="!permissions#dfn-retrieve-the-permission-state">
+                  retrieve the permission state algorithm</a> of the permission
+                  associated with <a>payment handler</a>.


romandev · 2017-06-21T16:47:12Z

index.html

+                allowing adding new payment instruments for the <a data-cite=
+                "!HTML#current-settings-object">current settings object</a>'s
+                origin is acceptable. If it is, set <var>permission</var> to
+                "granted", and "denied" otherwise.


In the implementation, I defined PaymentHandlerStatus as 'default' instead of 'prompt'.
Because the Notification spec[1] is doing like that.
But I awared that we can use PermissionState in Permission API spec[2] instead of our own definition.

[1] https://notifications.spec.whatwg.org/#enumdef-notificationpermission
[2] https://www.w3.org/TR/permissions/#idl-def-PermissionState

ianbjacobs · 2017-06-21T17:44:15Z

Thanks @romandev! @rsolomakhin if you are satisfied your comments have been addressed, please give the green light to merge.

adrianhopebailie · 2017-06-22T07:27:19Z

index.html

+                </li>
+              </ol>
+            </li>
+            <li>Return <var>p</var>


Shouldn't we return the promise before we resolve it?

@adrianhopebailie Thank you for review.

As far as I know, we are already using the pattern in many places in this spec or other specs. [1][2]
Resolving the promise will happen in parallel due to the following sentence:
"Run the following steps in parallel:"

So, I think it makes sense. WDYT?
(If I'm missing something, please correct me! :) )

[1] https://w3c.github.io/payment-handler/#has-method
[2] https://w3c.github.io/ServiceWorker/#cache-match

You are right, happy for you to merge as is.

I find the pattern used in the Payment Request spec easier to follow. If you agree then perhaps we should change across the document in a new PR?

Let p be a new Promise

Return p and perform the remaining steps in parallel.

Example in https://www.w3.org/TR/payment-request/#show-method at step 9.

I'll do it in follow-up PR!
Could you merge this PR? I don't have a permission to merge.
Thank you!

This PR is initiated from w3c#180. We should change the patterns of returning promise to more readable patterns as follows: BEFORE Let p be a new promise. Run the following steps in parallel: Do something Resolve p return p AFTER Let p be a new promise. Return p and perform the remaining steps in parallel: Do something Resolve p FYI, the patterns are already widely used in PaymentRequest API spec.

…186) This PR is initiated from #180. We should change the patterns of returning promise to more readable patterns as follows: BEFORE Let p be a new promise. Run the following steps in parallel: Do something Resolve p return p AFTER Let p be a new promise. Return p and perform the remaining steps in parallel: Do something Resolve p FYI, the patterns are already widely used in PaymentRequest API spec.

Define requestPermission() behavior.

3272449

In the current spec, there is no definition of the requestPermission() method. This change is fixing w3c#94 issue.

rsolomakhin reviewed Jun 21, 2017

View reviewed changes

rsolomakhin requested changes Jun 21, 2017

View reviewed changes

dlongley reviewed Jun 21, 2017

View reviewed changes

romandev commented Jun 21, 2017

View reviewed changes

Addressed all review comments.

8de8212

romandev force-pushed the request_permission branch from 0bc7f15 to 8de8212 Compare June 21, 2017 16:57

romandev added 2 commits June 22, 2017 02:04

Fixed link error of NotAllowedError.

a32f0e7

Addressed @ianbjcobs's comment.

40ce582

rsolomakhin approved these changes Jun 21, 2017

View reviewed changes

adrianhopebailie suggested changes Jun 22, 2017

View reviewed changes

adrianhopebailie approved these changes Jun 22, 2017

View reviewed changes

ianbjacobs merged commit 6856ef2 into w3c:gh-pages Jun 23, 2017

romandev deleted the request_permission branch June 23, 2017 22:19

romandev mentioned this pull request Jun 23, 2017

Change the patterns of returning promise to more readable patterns. #186

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define requestPermission() behavior. #180

Define requestPermission() behavior. #180

romandev commented Jun 21, 2017

romandev commented Jun 21, 2017

rsolomakhin Jun 21, 2017

dlongley Jun 21, 2017 •

edited

dlongley Jun 21, 2017

romandev Jun 21, 2017

ianbjacobs Jun 21, 2017

romandev Jun 21, 2017

rsolomakhin Jun 21, 2017

romandev Jun 21, 2017

rsolomakhin Jun 21, 2017

romandev Jun 21, 2017

rsolomakhin Jun 21, 2017

romandev Jun 21, 2017

rsolomakhin left a comment

dlongley Jun 21, 2017

romandev Jun 21, 2017

romandev left a comment

romandev Jun 21, 2017

romandev Jun 21, 2017

romandev Jun 21, 2017

romandev Jun 21, 2017

romandev Jun 21, 2017

ianbjacobs commented Jun 21, 2017

adrianhopebailie Jun 22, 2017

romandev Jun 22, 2017

adrianhopebailie Jun 22, 2017

romandev Jun 23, 2017

Define requestPermission() behavior. #180

Define requestPermission() behavior. #180

Conversation

romandev commented Jun 21, 2017

romandev commented Jun 21, 2017

Choose a reason for hiding this comment

dlongley Jun 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsolomakhin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romandev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianbjacobs commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlongley Jun 21, 2017 •

edited