editorial: Remove the "obtain a permission" algorithm. #298
Conversation
| @@ -215,40 +215,6 @@ <h2> | |||
| </ol> | |||
| </li> | |||
There was a problem hiding this comment.
I am not sure we want to show any permission prompt without user activation @reillyeon ?
There was a problem hiding this comment.
But actually raphael, I am not sure it always have to be gates behind a user activation. It the user accepted it on a previous visit, it is auto granted and thus no user activation is needed. But if we want to show the prompt ("prompt" state) well then we probably need a user activation to do so
There was a problem hiding this comment.
In the WebNFC API we use custom logic to use the "query" permission algorithm when there isn't user activation and the "request" algorithm when there is. In contrast in the Idle Detection API we always use the "query" algorithm and provide a separate method which only requests permission and thus requires user activation.
Either approach is fine by me though the whole discussion is somewhat moot because Chromium doesn't implement the "screen-wake-lock" permission. It is always granted.
There was a problem hiding this comment.
In the WebNFC API we use custom logic to use the "query" permission algorithm when there isn't user activation and the "request" algorithm when there is.
From a quick look, the spec has a custom "obtain permission" algorithm that mimics "request permission" fairly closely and does not mention user activation anywhere, while the Chromium implementation always just calls its "request permission" equivalent and always blocks requests that were not user-activated. w3c/web-nfc#482 then references the explainer, which says users should query the current permission state and trigger "obtain permission" via a user-activated action. This mismatch sounds confusing to me.
In contrast in the Idle Detection API we always use the "query" algorithm and provide a separate method which only requests permission and thus requires user activation.
We used to do that, but decided to remove requestPermission() in #237.
Having said that, I guess the question is whether it is worth implementing a custom "request permission" algorithm that checks for user-activation somewhere in the steps (be it to always check for it upfront or only when the permission state is "prompt"), or to hope w3c/permissions#189 and/or w3c/permissions#194 are solved at some point so we can rely on the Permissions spec to take care of it.
There was a problem hiding this comment.
In the absence of support from the Permissions API specification I think we should define custom algorithms but try to make sure they are consistent between specifications that the desire the "request permission if there is user activation" behavior. This can form the draft of the language that will hopefully be upstreamed at some point.
At its core the WebNFC issue seems to highlight the concern that requiring user activation should not be specified normatively and browsers can make that determination on their own, citing differences between Chrome and Firefox. In that case it would seem that always using the default "request" algorithm is fine and the user activation requirement is an optional intervention that implementations may choose to implement for some or all permissions.
I personally feel that requiring user activation for permission prompts should be normatively specified so I support adding the necessary custom steps here.
There was a problem hiding this comment.
If asking for permission requires user activation then, whether there is a separate requestPermission() method or not, for developers the process of requesting a wake lock requires a user gesture if they can't assume they have permission. That seems to be working as intended. Applications which know they already have permission (perhaps because they asked for permission in the past) don't need a user gesture.
For example, a site might add a button to enable the wake lock (and thus have activation while making the initial request) but call request() without a user gesture in the visibilitychanged handler to reacquire the lock later.
I'd like to see a resolution to the issues raised on the WebNFC and Permissions APIs but I don't think that should block this change.
There was a problem hiding this comment.
(for the record, while we discuss this I've submitted #303 anyway to clean up the algorithm we currently have)
There was a problem hiding this comment.
Will you file issues in Web NFC and permission repos ?
There was a problem hiding this comment.
Who's "you" in this case and what issues are you referring to? :-) I've mentioned 2 issues in the Permissions spec and 1 in the Web-NFC spec, so it's not clear what other issues still need to be filed.
There was a problem hiding this comment.
yes, I see now. Sorry have only half been following this discussion from the side lines :)
Steps that run "in parallel" need to take several extra restrictions into account: we do not have access to documents or global objects, cannot create IDL interfaces or manipulate promises, for example, all of which we had been doing, along with needlessly running nested "in parallel" steps. This change attempts to rectify the situation by only running things in parallel when absolutely necessary (i.e. when reaching out to the OS), and queueing tasks from there when we need to manipulate promises or create objects (changes to the "obtain permission" algorithm, which also runs in parallel, are being tracked separately in w3c#298). `WakeLock.request()`: * Queue a task to reject `|promise|` when the permission request run in parallel is denied. * Manipulate the [[ActiveLocks]] internal slot, create a new WakeLockSentinel and resolve the returned promise in a queued task. `WakeLockSentinel.release()`: * Do not run the "release a wake lock" algorithm in parallel (see changes to the algorithm itself below). * Just return a resolved promise once the rest of the steps run. The returned promise does not have much use, but has been kept to avoid breaking API compatibility. "Acquire wake lock" algorithm: * Stop running everything in parallel. * Move all [[ActiveLocks]] manipulation to `WakeLock.request()` itself, and make the algorithm just ask the platform for a wake lock. "Release wake lock" algorithm: * Stop running everything in parallel. The only steps that still need to run in parallel are the ones asking the platform to release the wake lock. * Consequently, stop queueing a task to change [[Released]] and fire the "release" event and do it directly in the algorithm. Fixes w3c#293.
Conceptually, the steps remain the same: 1. Return "granted" or "denied" if the permission state is set to either value. 2. Otherwise, return "denied" if there is no user activation. 3. Call the "request permission to use" algorithm from the Permissions spec in case there is user activation. The difference is that we now do it in 4 steps rather than 10 by 1) using the shorthands provided by the Permissions spec that allow us to use a PermissionName rather than having to create a PermissionDescriptor in the algorithms and 2) checking the "permission state" directly rather than invoking Permissions.query() and manipulating a promise. Ideally, we would remove this algorithm altogether in favor of the "request permission to use" algorithm from the Permissions spec, but we need to keep it in order to check for user activation in case we are in a "prompt" state (see the discussion in w3c#298). By not manipulating promises anymore, this incidentally fixes w3c#187.
Conceptually, the steps remain the same: 1. Return "granted" or "denied" if the permission state is set to either value. 2. Otherwise, return "denied" if there is no user activation. 3. Call the "request permission to use" algorithm from the Permissions spec in case there is user activation. The difference is that we now do it in 4 steps rather than 10 by 1) using the shorthands provided by the Permissions spec that allow us to use a PermissionName rather than having to create a PermissionDescriptor in the algorithms and 2) checking the "permission state" directly rather than invoking Permissions.query() and manipulating a promise. Ideally, we would remove this algorithm altogether in favor of the "request permission to use" algorithm from the Permissions spec, but we need to keep it in order to check for user activation in case we are in a "prompt" state (see the discussion in #298). By not manipulating promises anymore, this incidentally fixes #187.
Steps that run "in parallel" need to take several extra restrictions into account: we do not have access to documents or global objects, cannot create IDL interfaces or manipulate promises, for example, all of which we had been doing, along with needlessly running nested "in parallel" steps. This change attempts to rectify the situation by only running things in parallel when absolutely necessary (i.e. when reaching out to the OS), and queueing tasks from there when we need to manipulate promises or create objects (changes to the "obtain permission" algorithm, which also runs in parallel, are being tracked separately in w3c#298). `WakeLock.request()`: * Queue a task to reject `|promise|` when the permission request run in parallel is denied. * Manipulate the [[ActiveLocks]] internal slot, create a new WakeLockSentinel and resolve the returned promise in a queued task. `WakeLockSentinel.release()`: * Do not run the "release a wake lock" algorithm in parallel (see changes to the algorithm itself below). * Just return a resolved promise once the rest of the steps run. The returned promise does not have much use, but has been kept to avoid breaking API compatibility. "Acquire wake lock" algorithm: * Stop running everything in parallel. * Move all [[ActiveLocks]] manipulation to `WakeLock.request()` itself, and make the algorithm just ask the platform for a wake lock. "Release wake lock" algorithm: * Stop running everything in parallel. The only steps that still need to run in parallel are the ones asking the platform to release the wake lock. * Consequently, stop queueing a task to change [[Released]] and fire the "release" event and do it directly in the algorithm. Fixes w3c#293.
I thought w3c#298 would have landed by now. This is not the case, so adjust the algorithm we currently have.
Steps that run "in parallel" need to take several extra restrictions into account: we do not have access to documents or global objects, cannot create IDL interfaces or manipulate promises, for example, all of which we had been doing, along with needlessly running nested "in parallel" steps. This change attempts to rectify the situation by only running things in parallel when absolutely necessary (i.e. when reaching out to the OS), and queueing tasks from there when we need to manipulate promises or create objects (changes to the "obtain permission" algorithm, which also runs in parallel, are being tracked separately in w3c#298). `WakeLock.request()`: * Queue a task to reject `|promise|` when the permission request run in parallel is denied. * Manipulate the [[ActiveLocks]] internal slot, create a new WakeLockSentinel and resolve the returned promise in a queued task. `WakeLockSentinel.release()`: * Do not run the "release a wake lock" algorithm in parallel (see changes to the algorithm itself below). * Just return a resolved promise once the rest of the steps run. The returned promise does not have much use, but has been kept to avoid breaking API compatibility. "Acquire wake lock" algorithm: * Stop running everything in parallel. * Move all [[ActiveLocks]] manipulation to `WakeLock.request()` itself, and make the algorithm just ask the platform for a wake lock. "Release wake lock" algorithm: * Stop running everything in parallel. The only steps that still need to run in parallel are the ones asking the platform to release the wake lock. * Consequently, stop queueing a task to change [[Released]] and fire the "release" event and do it directly in the algorithm. Fixes w3c#293.
I thought w3c#298 would have landed by now. This is not the case, so adjust the algorithm we currently have.
|
Seems like we should just assume Permissions spec will eventually do the right thing and add a note pointing the related issue over on the other repo. |
Steps that run "in parallel" need to take several extra restrictions into account: we do not have access to documents or global objects, cannot create IDL interfaces or manipulate promises, for example, all of which we had been doing, along with needlessly running nested "in parallel" steps. This change attempts to rectify the situation by only running things in parallel when absolutely necessary (i.e. when reaching out to the OS), and queueing tasks from there when we need to manipulate promises or create objects (changes to the "obtain permission" algorithm, which also runs in parallel, are being tracked separately in w3c#298). `WakeLock.request()`: * Queue a task to reject `|promise|` when the permission request run in parallel is denied. * Manipulate the [[ActiveLocks]] internal slot, create a new WakeLockSentinel and resolve the returned promise in a queued task. `WakeLockSentinel.release()`: * Do not run the "release a wake lock" algorithm in parallel (see changes to the algorithm itself below). * Just return a resolved promise once the rest of the steps run. The returned promise does not have much use, but has been kept to avoid breaking API compatibility. "Acquire wake lock" algorithm: * Stop running everything in parallel. * Move all [[ActiveLocks]] manipulation to `WakeLock.request()` itself, and make the algorithm just ask the platform for a wake lock. "Release wake lock" algorithm: * Stop running everything in parallel. The only steps that still need to run in parallel are the ones asking the platform to release the wake lock. * Consequently, stop queueing a task to change [[Released]] and fire the "release" event and do it directly in the algorithm. Fixes w3c#293.
I thought w3c#298 would have landed by now. This is not the case, so adjust the algorithm we currently have.
Steps that run "in parallel" need to take several extra restrictions into account: we do not have access to documents or global objects, cannot create IDL interfaces or manipulate promises, for example, all of which we had been doing, along with needlessly running nested "in parallel" steps. This change attempts to rectify the situation by only running things in parallel when absolutely necessary (i.e. when reaching out to the OS), and queueing tasks from there when we need to manipulate promises or create objects (changes to the "obtain permission" algorithm, which also runs in parallel, are being tracked separately in w3c#298). `WakeLock.request()`: * Queue a task to reject `|promise|` when the permission request run in parallel is denied. * Manipulate the [[ActiveLocks]] internal slot, create a new WakeLockSentinel and resolve the returned promise in a queued task. `WakeLockSentinel.release()`: * Do not run the "release a wake lock" algorithm in parallel (see changes to the algorithm itself below). * Just return a resolved promise once the rest of the steps run. The returned promise does not have much use, but has been kept to avoid breaking API compatibility. "Acquire wake lock" algorithm: * Stop running everything in parallel. * Move all [[ActiveLocks]] manipulation to `WakeLock.request()` itself, and make the algorithm just ask the platform for a wake lock. "Release wake lock" algorithm: * Stop running everything in parallel. The only steps that still need to run in parallel are the ones asking the platform to release the wake lock. * Consequently, stop queueing a task to change [[Released]] and fire the "release" event and do it directly in the algorithm. Fixes w3c#293.
I thought w3c#298 would have landed by now. This is not the case, so adjust the algorithm we currently have.
rakuco
force-pushed
the
remove-obtain-permission-algorithm
branch
from
0ac1555 to
dcf96b1
Compare
|
I've rebased the PR to resolve the conflicts, but it's still unclear how to proceed, as there are different schools of thought here: it'd be great to have this solved in the Permissions spec, but I don't see a lot of activity there and don't know if anyone's working on those items; OTOH, defining a custom permission model here that only checks for user activation under some circumstances sounds... unconventional? |
|
My position remains that we make it the permission spec's responsibility. |
|
@rakuco, any chance of a rebase? |
This may have made more sense back when we had a custom `WakeLockPermissionDescriptor`, but even then this felt like implementing a custom permission model for no reason. We are just interested in knowing whether we have permission to request a wake lock from the platform, and we can use the Permission spec's "request permission to use" algorithm for that just fine. One important change is related to checking transient activation: the "obtain a permission" algorithm essentially denied requests when "screen-wake-lock"'s permission state was set to "prompt" AND there was no transient user activation. In general, an API should either always or never be user-activation gated. Given Blink is the only current implementation of this spec and it does not currently gate this API on user activation, the transient user activation check was removed altogether for now. Together with w3c#297, this fixes w3c#187 and fixes w3c#198.
rakuco
force-pushed
the
remove-obtain-permission-algorithm
branch
from
dcf96b1 to
0221e02
Compare
Done. A while ago you mentioned adding a note to reference w3c/permissions#194; do you still think it makes sense? |
|
@marcoscaceres @xfq the checks are failing with do you know if this needs to be fixed in the spec or in the GitHub action? |
|
Temporary hiccup, @rakuco. If it happens again, just rerun the action (usually fixes it). |
I don't think it's necessary, tbh. We will take that up in w3c/permissions#194. |
With w3c/screen-wake-lock#298 the spec was changed and matches what we'd already been doing in the Blink implementation: instead of calling a custom "obtain permission" algorithm, we just delegate everything to the standard "request a permission" algorithm implemented elsewhere. Update the comment and, while at it, fold WakeLock::ObtainPermission() into WakeLock::DoRequest(). There should be no user-visible changes. Change-Id: Iabf981df5cdbe04d0b4e67f0980a933c5167a3d0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2988046 Reviewed-by: Reilly Grant <[email protected]> Commit-Queue: Reilly Grant <[email protected]> Commit-Queue: Raphael Kubo da Costa <[email protected]> Auto-Submit: Raphael Kubo da Costa <[email protected]> Cr-Commit-Position: refs/heads/master@{#897071}
With w3c/screen-wake-lock#298 the spec was changed and matches what we'd already been doing in the Blink implementation: instead of calling a custom "obtain permission" algorithm, we just delegate everything to the standard "request a permission" algorithm implemented elsewhere. Update the comment and, while at it, fold WakeLock::ObtainPermission() into WakeLock::DoRequest(). There should be no user-visible changes. Change-Id: Iabf981df5cdbe04d0b4e67f0980a933c5167a3d0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2988046 Reviewed-by: Reilly Grant <[email protected]> Commit-Queue: Reilly Grant <[email protected]> Commit-Queue: Raphael Kubo da Costa <[email protected]> Auto-Submit: Raphael Kubo da Costa <[email protected]> Cr-Commit-Position: refs/heads/master@{#897071} NOKEYCHECK=True GitOrigin-RevId: 0e90f4e8c702fd782337f26c93221e6eeca26eaf
This may have made more sense back when we had a custom
WakeLockPermissionDescriptor, but even then this felt like implementing acustom permission model for no reason.
We are just interested in knowing whether we have permission to request a
wake lock from the platform, and we can use the Permission spec's "request
permission to use" algorithm for that just fine.
One important change is related to checking transient activation: the
"obtain a permission" algorithm essentially denied requests when
"screen-wake-lock"'s permission state was set to "prompt" AND there was no
transient user activation. In general, an API should either always or never
be user-activation gated. Given Blink is the only current implementation of
this spec and it does not currently gate this API on user activation, the
transient user activation check was removed altogether for now.
Together with #297, this fixes #187 and fixes #198.
Preview | Diff