Add static requestPermission method on Window #167
Conversation
index.html
Outdated
| <a>query a permission</a> with |permissionDesc|. | ||
| </li> | ||
| <li> | ||
| If |permission| is "<a>denied</a>", | ||
| return <code>false</code>. | ||
| Let |status:PermissionStatus| be the result awaiting |
There was a problem hiding this comment.
"awaiting |resultPromise| to settle" -> "waiting for |resultPromise| to settle"
index.html
Outdated
| </li> | ||
| <li> | ||
| Let |status:PermissionStatus| be the result awaiting | ||
| |resultPromise| to settle. |
index.html
Outdated
| <ol> | ||
| <li> | ||
| Let |resultPromise| be the result of running | ||
| <a>query a permission</a> with |permissionDesc|. |
There was a problem hiding this comment.
Instead of using the "query a permission" algorithm and restating a lot of the same logic from above can we reuse the "obtain permission" algorithm?
There was a problem hiding this comment.
Sure, I have done that now
index.html
Outdated
| <a>query a permission</a> with |permissionDesc|. | ||
| </li> | ||
| <li> | ||
| If |permission| is "<a>denied</a>", | ||
| return <code>false</code>. | ||
| Let |status:PermissionStatus| be the result waiting for |
index.html
Outdated
| and abort these steps. | ||
| </li> | ||
| <li> | ||
| Let |signal| be the |options|’ dictionary member |
|
I was in the middle of a review! :( Can I please please pleast ask that you don't merge stuff before you have multiple browser vendors review the spec AND you have tests for each PR. |
| </li> | ||
| <li> | ||
| If |permission| is "<a>granted</a>", | ||
| return <code>true</code>. | ||
| If |resultPromise| rejects, return "<a>denied</a>". |
There was a problem hiding this comment.
This is misleading, because the IDL conversion might have caused the rejection in the permission spec (i.e., it's not denied - it's some other exception). However, I think it might be impossible for it to reject, because the algorithm is always providing a well-formed PermissionDescriptor.
There was a problem hiding this comment.
You could argue that even if you supply something invalid, then that couldn't be granted. Of course logging that would probably be useful.
Should I add a comment about us always providing a well-formed PermissionDescriptor?
There was a problem hiding this comment.
Should I add a comment about us always providing a well-formed PermissionDescriptor?
If we are sure we always send in a valid object, then we can be confident it won't ever reject based on what's in the permissions spec. If that's the case, then we don't need to handle rejections.
There was a problem hiding this comment.
Still, does that deserve a NOTE?
| @@ -221,7 +221,8 @@ <h2> | |||
| </ol> | |||
| <p> | |||
| To <dfn>obtain permission</dfn> for <a>wake lock type</a> | |||
| |type|, run these steps <a>in parallel</a>: | |||
| |type|, run these steps <a>in parallel</a>. This async algorithm | |||
| returns either "<a>granted</a>" or "<a>denied</a>". | |||
| </p> | |||
| <ol class="algorithm"> | |||
| <li> | |||
There was a problem hiding this comment.
Below this line, there is a PermissionDescriptor being created inside a pre... that's not correct. You need:
Let |permissionDesc:PermissionDescriptor| be the following be a newly created PermissionDescriptor.
Set |permissionDesc|.name to "wake-lock",
Set |permissionDesc|.type to | type|.
There was a problem hiding this comment.
This followed the approach used in Web USB. I can change of course, but @reillyeon input might be useful
There was a problem hiding this comment.
Because we got here from requestPermission(), we are already in WebIDL land inside the algorithm (so technically in the C++ implementation)... so you can literally just create: PermissionDescriptor permissionDesc; and then assign it the name (as above).
There was a problem hiding this comment.
Maybe you could file an issue for Web USB?
There was a problem hiding this comment.
Done - used your suggestions
| attribute of |status|. | ||
| </li> | ||
| <li> | ||
| If |state| is "<a>prompt</a>", run the following steps: |
There was a problem hiding this comment.
It doesn't feel right for this spec to override the results of the permission grant of the permission spec. Why not let it prompt?
Something is either wrong in this spec or in the Permissions spec.
There was a problem hiding this comment.
Permission spec allows you to query, and to prompt separately. These are separate algorithms.
I based this on existing spec text, but Web USB does similar (though it uses "ask") as well as device motion uses "default" the same way "prompt" is used here (as that spec doesn't depend on permission api)
|
Oh sorry @marcoscaceres - I am trying to balance things moving forward and major time-zone differences. You can still review and I can create additional PRs to fix what you find. We have a separate person responsible for testing - but I will need to follow up with them |
|
It happens :)
Ok, but don't rush ahead if you only have one implementer and no tests. That's just going to put us back where we were before. I'll send a PR template that we've used successfully in Payments.
heh, hi from SF where it's 4:23am! 💃 |
Sure, the current status is that the spec had so many issues that it needed major changes, so there is really no implementation of the current spec as this point, though the plan is to have that as soon as possible, including tests |
Fixes #156
This also corrects an error in the obtain permission algorithm
Preview | Diff