Remove permission check #326

marcoscaceres · 2021-09-09T05:15:34Z

Closes #324

Please see #324 for rationale and discussion.

The following tasks have been completed:

Modified Web platform tests

Implementation commitment:

Chromium (https://bugs.chromium.org/p/chromium/issues/detail?id=)
Gecko (https://bugzilla.mozilla.org/show_bug.cgi?id=)
WebKit - does not implement permission

Preview | Diff

rakuco

Personally I'm still undecided about this one and would like to hear from @reillyeon, especially as I don't know if adapting the Chromium implementation to this change would count as breaking backward compatibility, or if there are plans to add a UI for this permission in the pipeline.

rakuco · 2021-09-15T06:57:11Z

index.html

@@ -345,22 +287,6 @@ <h3>
          </li>
          <li>Run the following steps <a>in parallel</a>:
            <ol>
-              <li>Let |state:PermissionState| be the result of <a>requesting


Without these steps, I think you can drop the "run the following steps in parallel above", as all the remaining sub-steps are run from a queued global task.

Ah, good suggestion!

Acquiring the lock needs to happen in parallel.

index.html

rakuco · 2021-10-06T10:17:47Z

index.html

@@ -343,67 +285,49 @@ <h3>
          </li>
          <li>Let |promise:Promise| be [=a new promise=].
          </li>
-          <li>Run the following steps <a>in parallel</a>:
+          <li>
+            <a>Queue a global task</a> on the <a>screen wake lock task


Do we still need to queue a global task here? We're already in the main thread at this point.

We don't technically need to but this provides implementations with the freedom to not return a settled Promise synchronously from this method, as otherwise this method doesn't need to be Promise-returning at all.

as otherwise this method doesn't need to be Promise-returning at all.

Wouldn't this be a case of "we thought there should be async stuff happening, now there's not but we can't break backwards compatibility"? It's how we ended up with WakeLockSentinel.release() returning a resolved promise after #299, for example.

I'm not saying I'm in favor of doing that, at this point I'm more curious about whether this is standard practice, or even if I'm looking at it from the wrong perspective.

rakuco

This was discussed at TPAC 2022: https://www.w3.org/2022/09/15-dap-minutes.html#t21

The consensus is that this PR should be merged, but the pending review comments still need to be addressed.

marcoscaceres · 2022-09-27T07:02:54Z

@rakuco, could we merge this and then revise the global task stuff above?... I think the visibility checks being done in parallel are also wrong (should be done sync I think, because the document could disappear by the time the check is done), so we might need to give the whole spec a once over.

However, what's more important now is to remove the permission check.

index.html

marcoscaceres · 2022-09-27T07:24:46Z

Quick update... fixed the merge conflicts and fixed the broken cross reference to [=Document/visibility state=]

rakuco

There are some issues with the latest merge commit that resulted in some wrong steps in request() that need to be fixed.

@rakuco, could we merge this and then revise the global task stuff above?... I think the visibility checks being done in parallel are also wrong (should be done sync I think, because the document could disappear by the time the check is done), so we might need to give the whole spec a once over.

Let's not block on the queue global task stuff then. With that said, I don't think the visibility checks are done in parallel, the idea behind queuing a global task while in parallel was to prevent that. We spent quite a lot of time in #299 trying to get this right, so I was hoping that the current version already made sense.

I'm also curious about WebKit/WebKit#4733 -- the PR mentions there's a pending discussion about prompts, so I wonder (or would like to double check) if we may end up having to revert this PR not too far in the future.

Finally: can you also send a WPT pull request removing the permission-related bits from the tests?

index.html

rakuco · 2022-09-27T16:39:38Z

index.html

-              </li>
-              <li data-tests="wakelock-request-denied.https.html">If |state| is
-              {{PermissionState/"denied"}}, then:
+              <li>If |document|'s [=Document/visibility state=] is "hidden", then:


Something went wrong with the merge in these steps too: all the main substeps within the "Queue a global task" step have been duplicated inside the If document.[[ActiveLocks]]["screen"] is empty substep (see the Preview link).

rakuco · 2022-09-27T16:47:11Z

index.html

@@ -343,67 +285,49 @@ <h3>
          </li>
          <li>Let |promise:Promise| be [=a new promise=].
          </li>
-          <li>Run the following steps <a>in parallel</a>:
+          <li>
+            <a>Queue a global task</a> on the <a>screen wake lock task


as otherwise this method doesn't need to be Promise-returning at all.

Wouldn't this be a case of "we thought there should be async stuff happening, now there's not but we can't break backwards compatibility"? It's how we ended up with WakeLockSentinel.release() returning a resolved promise after #299, for example.

I'm not saying I'm in favor of doing that, at this point I'm more curious about whether this is standard practice, or even if I'm looking at it from the wrong perspective.

index.html

marcoscaceres · 2022-09-28T04:47:38Z

Sent PR to remove test web-platform-tests/wpt#36121

Will start working on the other tests as part of #351

marcoscaceres · 2022-09-28T05:01:18Z

@rakuco wrote:

Wouldn't this be a case of "we thought there should be async stuff happening, now there's not but we can't break backwards compatibility"? It's how we ended up with WakeLockSentinel.release() returning a resolved promise after #299, for example.

Not exactly. This is just a defensive design strategy in case we need to reject the promise in the future for some reason.

I'm not saying I'm in favor of doing that, at this point I'm more curious about whether this is standard practice, or even if I'm looking at it from the wrong perspective.

(As a followup...)

It is. I think we should update release() to better reflect this. It should resolve the promise immediately if the ActiveLocks is not empty, but then should actually try to release at the OS level and then settle the final promise. Otherwise this could be racy.

anssiko · 2022-09-28T06:25:40Z

I’m happy to see @marcoscaceres continue his substantial contributions (also in #351) in this WG. As the co-chair it is also my duty to remind you to either join this WG or use License Grants from Non-Participants mechanism.

@xfq can you enable https://github.com/w3c/ash-nazg for this repo?

reillyeon · 2022-09-28T22:04:28Z

I think we should update release() to better reflect this. It should resolve the promise immediately if the ActiveLocks is not empty, but then should actually try to release at the OS level and then settle the final promise. Otherwise this could be racy.

I don't think this is racy. Firstly, it never makes sense to tell the site that it failed to release a lock, if there were even possible. The site has successfully expressed its desire to release the lock and it is up to the user agent to convey that to the underlying platform. There may be many other components of the system still holding the lock (including other sites, or other script on the same site) but that state is invisible to the current site.

marcoscaceres · 2022-09-29T06:47:02Z

@anssiko, thanks for the reminder; I'll see what I can do about joining (though chances are low until objections are addressed). Just want to point out that this PR predates me joining Apple, so I'm just making an editorial contribution here.

For #351, I can close that and maybe get @rakuco or someone else to send a PR to address the corresponding issue (#350)?

(now regretting not making this also a join deliverable with WebAppsWG 😢)

xfq · 2022-09-29T07:54:05Z

@xfq can you enable https://github.com/w3c/ash-nazg for this repo?

Tracked in w3c/ash-nazg#252

rakuco · 2022-09-29T16:31:07Z

index.html

@@ -309,11 +251,19 @@ <h3>
          The <dfn>request()</dfn> method
        </h3>
        <p data-tests="wakelock-type.https.any.html">
-          The <code>request(|type:WakeLockType|)</code> method steps are:
+          The `request(|type:WakeLockType|)` method steps are:


ReSpec doesn't seem to like this change. request(|type:WakeLockType|) is being written literally like that in the page.

rakuco · 2022-09-29T16:32:28Z

index.html

        </p>
        <ol class="algorithm">
          <li>Let |document:Document| be [=this=]'s [=relevant global
-          object=]'s [=associated Document=].
+          object=]'s [=associated `Document`=].


The change in the order of steps here looks unrelated. They might even make sense, but I'd rather do this in another PR.

rakuco · 2022-09-29T16:34:02Z

index.html

          <li data-tests="wakelock-document-hidden-manual.https.html">If
          |document|'s [=Document/visibility state=] is "`hidden`", return [=a
          promise rejected with=] {{"NotAllowedError"}} {{DOMException}}.
          </li>
          <li>Let |promise:Promise| be [=a new promise=].
          </li>
-          <li>Run the following steps <a>in parallel</a>:
+          <li>[=In parallel=]:


Was there a particular reason to change the original version? It's just saying the same thing.

Just reads better.

rakuco · 2022-09-29T16:39:11Z

index.html

            <ol>
-              <li>Let |state:PermissionState| be the result of <a>requesting
-              permission to use</a> "`screen-wake-lock`".
+              <li>Attempt to [=acquire a wake lock=] of type


The steps still look weird:

The check for whether document.[[ActiveLocks]]["screen"] is empty was dropped.

Only acquiring the wake lock happened in parallel, document.[[ActiveLocks]] was only read and manipulated from the the main thread to avoid races, but now all these steps are happening in parallel.

rakuco · 2022-09-29T17:04:35Z

index.html

@@ -721,32 +624,29 @@ <h3>
          <li>Remove |lock| from
          |document|.{{Document/[[ActiveLocks]]}}[|type|].
          </li>
+          <li>[=Queue a task=] on the [=screen wake lock task source=] to:


The changes to this section also look unrelated to permissions and should be sent in a separate PR.

marcoscaceres · 2022-10-04T05:49:39Z

@rakuco could you take this over? (please remove the changes you don't agree with.) As I'm not a member of this group, I can't make any further changes.

marcoscaceres requested review from reillyeon and rakuco September 9, 2021 05:15

rakuco reviewed Sep 15, 2021

View reviewed changes

marcoscaceres added 2 commits September 30, 2021 16:13

Remove permission check

3b695c2

Review feedback

f70fd9e

marcoscaceres force-pushed the permissions branch from 239a418 to f70fd9e Compare September 30, 2021 06:31

marcoscaceres requested a review from rakuco September 30, 2021 06:31

fixup markup

0972342

reillyeon requested changes Sep 30, 2021

View reviewed changes

index.html Show resolved Hide resolved

rakuco reviewed Oct 6, 2021

View reviewed changes

miketaylr mentioned this pull request Nov 13, 2021

Ensure "screen-wake-lock" permission is integrated in Screen Wake Lock API (or just remove it?) w3c/permissions#319

Closed

anssiko mentioned this pull request Jun 28, 2022

Devices and Sensors WG - TPAC 2022 agenda w3c/devicesensors-wg#56

Closed

willmorgan approved these changes Sep 15, 2022

View reviewed changes

rakuco suggested changes Sep 16, 2022

View reviewed changes

Merge branch 'gh-pages' into permissions

04d0d22

marcoscaceres commented Sep 27, 2022

View reviewed changes

index.html Outdated Show resolved Hide resolved

Update index.html

b455083

marcoscaceres commented Sep 27, 2022

View reviewed changes

index.html Outdated Show resolved Hide resolved

Update index.html

0b3b13c

marcoscaceres requested review from rakuco and reillyeon September 27, 2022 07:25

reillyeon approved these changes Sep 27, 2022

View reviewed changes

rakuco suggested changes Sep 27, 2022

View reviewed changes

marcoscaceres commented Sep 27, 2022

View reviewed changes

index.html Outdated Show resolved Hide resolved

Update index.html

3152f77

marcoscaceres mentioned this pull request Sep 28, 2022

Require transient activation to request lock #350

Open

Update index.html

a94e74d

marcoscaceres commented Sep 28, 2022

View reviewed changes

index.html Outdated Show resolved Hide resolved

Update index.html

f521b67

marcoscaceres mentioned this pull request Sep 28, 2022

Remove wakelockpermissiondescriptor.https.html web-platform-tests/wpt#36121

Open

fix markup

bcb1470

marcoscaceres mentioned this pull request Sep 29, 2022

Require transient activation #351

Open

4 tasks

rakuco reviewed Sep 29, 2022

View reviewed changes

marcoscaceres closed this Oct 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove permission check #326

Remove permission check #326

marcoscaceres commented Sep 9, 2021 •

edited by pr-preview bot

Loading

rakuco left a comment

rakuco Sep 15, 2021

marcoscaceres Sep 30, 2021

marcoscaceres Sep 28, 2022

rakuco Oct 6, 2021

reillyeon Sep 27, 2022

rakuco Sep 27, 2022

rakuco left a comment

marcoscaceres commented Sep 27, 2022 •

edited

Loading

marcoscaceres commented Sep 27, 2022

rakuco left a comment

rakuco Sep 27, 2022

rakuco Sep 27, 2022

marcoscaceres commented Sep 28, 2022

marcoscaceres commented Sep 28, 2022

anssiko commented Sep 28, 2022

reillyeon commented Sep 28, 2022

marcoscaceres commented Sep 29, 2022

xfq commented Sep 29, 2022

rakuco Sep 29, 2022

rakuco Sep 29, 2022

rakuco Sep 29, 2022

marcoscaceres Oct 4, 2022

rakuco Sep 29, 2022

rakuco Sep 29, 2022

marcoscaceres commented Oct 4, 2022

Remove permission check #326

Remove permission check #326

Conversation

marcoscaceres commented Sep 9, 2021 • edited by pr-preview bot Loading

rakuco 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

Choose a reason for hiding this comment

rakuco left a comment

Choose a reason for hiding this comment

marcoscaceres commented Sep 27, 2022 • edited Loading

marcoscaceres commented Sep 27, 2022

rakuco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres commented Sep 28, 2022

marcoscaceres commented Sep 28, 2022

anssiko commented Sep 28, 2022

reillyeon commented Sep 28, 2022

marcoscaceres commented Sep 29, 2022

xfq commented Sep 29, 2022

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

marcoscaceres commented Oct 4, 2022

marcoscaceres commented Sep 9, 2021 •

edited by pr-preview bot

Loading

marcoscaceres commented Sep 27, 2022 •

edited

Loading