Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add static requestPermission method on Window #167

Merged
merged 2 commits into from
Apr 1, 2019
Merged

Add static requestPermission method on Window #167

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
28035a6
Add static requestPermission method on Window
kenchris Mar 28, 2019
3882444
Fix Reilly's comments
kenchris Apr 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
187 changes: 114 additions & 73 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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|.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This followed the approach used in Web USB. I can change of course, but @reillyeon input might be useful

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could file an issue for Web USB?

Copy link
Contributor Author

@kenchris kenchris Apr 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - used your suggestions

Expand All @@ -235,24 +236,27 @@ <h2>
</pre>
</li>
<li>
Let |permission| be the result of running
Let |resultPromise| be the result of running
<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 of waiting for
|resultPromise| to settle.
</li>
<li>
If |permission| is "<a>granted</a>",
return <code>true</code>.
If |resultPromise| rejects, return "<a>denied</a>".
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, does that deserve a NOTE?

</li>
<li>
If |permission| is "<a>prompt</a>", run
the following steps:
Let |state| be the value of the
<a data-cite="permissions#dom-permissionstatus-state">state</a>
attribute of |status|.
</li>
<li>
If |state| is "<a>prompt</a>", run the following steps:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

<ol>
<li>
If the <a>obtain permission</a> steps were not <a>triggered by
user activation</a>, return <code>false</code>.
If these <a>obtain permission</a> steps were not
<a>triggered by user activation</a>, return "<a>denied</a>".
</li>
<li>
Return the result of <a>requesting permission
Expand All @@ -261,7 +265,7 @@ <h2>
</ol>
</li>
<li>
Return <code>false</code>.
Otherwise, return |state|.
</li>
</ol>
<div class="note">
Expand Down Expand Up @@ -366,6 +370,7 @@ <h2>
<pre class="idl">
[Constructor(WakeLockType type), SecureContext, Exposed=(DedicatedWorker, Window)]
interface WakeLock : EventTarget {
[Exposed=Window] static Promise&lt;PermissionState&gt; requestPermission(WakeLockType type);
readonly attribute WakeLockType type;
readonly attribute boolean active;
attribute EventHandler onactivechange;
Expand Down Expand Up @@ -428,6 +433,34 @@ <h3>Constructor</h3>
</li>
</ol>
</section>
<section data-dfn-for="WakeLock">
<h3><dfn>requestPermission()</dfn> static method</h3>
<p>
The <a>requestPermission()</a> method, when invoked, MUST run the
following steps. This algorithm resolves with either
"<a>granted</a>" or "<a>denied</a>".
</p>
<ol class="algorithm">
<li>
Let |promise| be a new <a>Promise</a> object.
kenchris marked this conversation as resolved.
Show resolved Hide resolved
</li>
<li>
Let |type| be the first argument.
</li>
<li>
Return |promise| and run the following steps <a>in parallel</a>:
<ol>
<li>
Let |state| be the result of running and waiting for the
<a>obtain permission</a> steps with |type|.
</li>
<li>
Resolve |promise| with |state|.
</li>
</ol>
</li>
</ol>
</section>
<section data-dfn-for="WakeLock">
<h3><dfn>type</dfn> attribute</h3>
<p>
Expand Down Expand Up @@ -471,101 +504,109 @@ <h3><dfn>request()</dfn> method</h3>
following steps:
</p>
<ol class="algorithm">
<li>
Let |promise| be a new <a>Promise</a> object.
</li>
<li>
Let |lock| be the <a>context object</a>.
</li>
<li>
If <a>obtain permission</a> steps for |lock|'s
|type| return <code>false</code>,
then reject |promise| with a "<a>NotAllowedError</a>"
<a>DOMException</a>, and abort these steps.
</li>
<li>
Let |options| be the first argument.
</li>
<li>
Let |signal| be the |options|’ dictionary member
of the same name if present, or <code>null</code> otherwise.
Let |lock| be the <a>context object</a>.
</li>
<li>
Let |record| be the <a>platform wake lock</a>'s
<a>state record</a> associated with
|lock|'s |type|.
</li>
<li>
If |signal|’s <a>aborted flag</a> is set, then
reject |promise| with an "<a>AbortError</a>"
<a>DOMException</a>, and abort these steps.
Let |promise| be a new <a>Promise</a> object.
</li>
<li>
If |signal| is not <code>null</code>, then
<a>add the following abort steps</a> to |signal|:
Return |promise| and run the following steps <a>in parallel</a>:
<ol>
<li>
Decrease |record|.<a>[[\RequestCounter]]</a> by one.
Let |state| be the result of running and waiting for the
<a>obtain permission</a> steps with |type|.
</li>
<li>
If |record|.<a>[[\RequestCounter]]</a> is 0, then
run the following steps <a>in parallel</a>:
<ol>
<li>
Run <a>release a wake lock</a> on |lock|.
</li>
</ol>
If |state| is "<a>denied</a>", then reject |promise|
with a "<a>NotAllowedError</a>" <a>DOMException</a>,
and abort these steps.
</li>
<li>
Let |signal| be the |options| dictionary member
of the same name if present, or <code>null</code> otherwise.
</li>
<li>
Reject |promise| with an "<a>AbortError</a>"
If |signal|’s <a>aborted flag</a> is set, then
reject |promise| with an "<a>AbortError</a>"
<a>DOMException</a>, and abort these steps.
</li>
</ol>
</li>
<li>
If the <a>current global object</a> is not a <a>DedicatedWorkerGlobalScope</a>
object, run the following steps:
<ol>
<li>
Let |document| be the <a>responsible document</a> of the
<a>current settings object</a>.
If |signal| is not <code>null</code>, then
<a>add the following abort steps</a> to |signal|:
<ol>
<li>
Decrease |record|.<a>[[\RequestCounter]]</a> by one.
</li>
<li>
If |record|.<a>[[\RequestCounter]]</a> is 0, then
run the following steps <a>in parallel</a>:
<ol>
<li>
Run <a>release a wake lock</a> on |lock|.
</li>
</ol>
</li>
<li>
Reject |promise| with an "<a>AbortError</a>"
<a>DOMException</a>, and abort these steps.
</li>
</ol>
</li>
<li>
If |document| is not <a>fully active</a>,
reject |promise| with a "<a>NotAllowedError</a>"
<a>DOMException</a>, and abort these steps.
If the <a>current global object</a> is not a <a>DedicatedWorkerGlobalScope</a>
object, run the following steps:
<ol>
<li>
Let |document| be the <a>responsible document</a> of the
<a>current settings object</a>.
</li>
<li>
If |document| is not <a>fully active</a>,
reject |promise| with a "<a>NotAllowedError</a>"
<a>DOMException</a>, and abort these steps.
</li>
<li>
If |lock|'s |type| is
"<a data-lt="WakeLockType.screen">screen</a>" and the
<a>Document</a> of the <a>top-level browsing context</a>
is <a>hidden</a>, reject |promise|
with a "<a>NotAllowedError</a>"
<a>DOMException</a>, and abort these steps.
</li>
</ol>
</li>
<li>
If |lock|'s |type| is
"<a data-lt="WakeLockType.screen">screen</a>" and the
<a>Document</a> of the <a>top-level browsing context</a>
is <a>hidden</a>, reject |promise|
with a "<a>NotAllowedError</a>"
<a>DOMException</a>, and abort these steps.
Increase |record|.<a>[[\RequestCounter]]</a> by one.
</li>
</ol>
</li>
<li>
Increase |record|.<a>[[\RequestCounter]]</a> by one.
</li>
<li>
If |record|.<a>[[\RequestCounter]]</a> is 1,
run the following steps <a>in parallel</a>:
<ol>
<li>
Let |success| be the the result of running
<a>acquire a wake lock</a> on |lock|.
If |record|.<a>[[\RequestCounter]]</a> is 1,
run the following steps:
<ol>
<li>
Let |success| be the the result of running and
waiting for <a>acquire a wake lock</a> on |lock|.
</li>
<li>
If |success| is <code>false</code> then reject
|promise| with a "<a>NotAllowedError</a>"
<a>DOMException</a>, and abort these steps.
</li>
</ol>
</li>
<li>
If |success| is <code>false</code> then reject
|promise| with a "<a>NotAllowedError</a>"
<a>DOMException</a>, and abort these steps.
Resolve |promise| with <code>void</code>.
</li>
</ol>
</li>
<li>
Resolve |promise| with <code>void</code>.
</li>
</ol>
</section>
<div class="note">
Expand Down