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

Editorial: Introduce and use "queue a global task" #5653

Merged
merged 1 commit into from
Jun 25, 2020
Merged

Editorial: Introduce and use "queue a global task" #5653

merged 1 commit into from
Jun 25, 2020

Conversation

domenic
Copy link
Member

@domenic domenic commented Jun 18, 2020

Part of #4980.

I'm trying GitHub's new-ish ability to target PRs at other outstanding PRs, and then have them automatically rebased onto master when theo ther PR gets merged. #5651 is the dependency, in particular. Marking "do not merge yet" until that's sorted.

There are other places that could benefit from this, but I did the low-hanging fruit for now. Some notable patterns:

  • Some instances are missing a task source, so we need to figure out what task source it would be.
  • Some instances are for features that are going to be removed (e.g. appcache) or revised (e.g. sessionStorage)
  • The EventSource and WebSocket sections often don't refer to an actual EventSource or WebSocket object whose relevant global object I could pick, and instead say things like "the readyState attribute". The temptation to fix them was great, but I resisted for now.

@domenic domenic added clarification Standard could be clearer do not merge yet Pull request must not be merged per rationale in comment topic: event loop labels Jun 18, 2020
@domenic domenic mentioned this pull request Jun 24, 2020
Open
Base automatically changed from bc-current-window to master June 24, 2020 17:20
@domenic domenic requested a review from annevk June 24, 2020 18:02
@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Jun 25, 2020
@domenic domenic merged commit 0be2040 into master Jun 25, 2020
@domenic domenic deleted the queue-document-task branch June 25, 2020 15:12
rakuco added a commit to rakuco/wake-lock that referenced this pull request Feb 15, 2021
@rakuco
Use "queue a global task" instead of "queue a task".
e669842 
According to @annevk, this is what specs should use in the future (see
whatwg/html#4980 and whatwg/html#5653).

While here, refer to the "relevant settings object" in WakeLock.request()
rather than the "current settings object", as we are inside a method of an
IDL interface and can rely on it being defined. I do not think this is a
user-visible change, and it looks cleaner.
rakuco added a commit to rakuco/wake-lock that referenced this pull request Feb 25, 2021
@rakuco
Use "queue a global task" instead of "queue a task".
419bb6b 
According to @annevk, this is what specs should use in the future (see
whatwg/html#4980 and whatwg/html#5653).

While here, refer to the "relevant settings object" in WakeLock.request()
rather than the "current settings object", as we are inside a method of an
IDL interface and can rely on it being defined. I do not think this is a
user-visible change, and it looks cleaner.
rakuco added a commit to rakuco/wake-lock that referenced this pull request Feb 28, 2021
@rakuco
Use "queue a global task" instead of "queue a task".
182a79d 
According to @annevk, this is what specs should use in the future (see
whatwg/html#4980 and whatwg/html#5653).

While here, refer to the "relevant settings object" in WakeLock.request()
rather than the "current settings object", as we are inside a method of an
IDL interface and can rely on it being defined. I do not think this is a
user-visible change, and it looks cleaner.
rakuco added a commit to rakuco/wake-lock that referenced this pull request Mar 22, 2021
@rakuco
editorial: Tidy up uses of "in parallel".
7ac57c1 
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
queuing global tasks (emphasis on "global" given whatwg/html#4980 and
whatwg/html#5653) from there when we need to manipulate promises or create
objects.

"Obtain permission" algorithm:
* Stop running in parallel. Callers should be responsible for choosing
  whether it should be run in parallel or not.

`WakeLock.request()`:
* Separate returning a promise and running steps in parallel. This style is
  more usual.
* Refer to the "relevant settings object" rather than the "current settings
  object", as we are inside a method of an IDL interface and can rely on it
  being defined. I do not think this is a user-visible change, and it looks
  cleaner.
* Queue a global task to reject `|promise|` when the permission request run
  in parallel is denied.
* Manipulate the `[[ActiveLocks]]` internal slot, check page visibility,
  invoke "acquire a wake lock", create a new WakeLockSentinel and resolve
  the returned promise in a queued global task.

`WakeLockSentinel.release()`:
* Do not run the "release a wake lock" algorithm in parallel (see the
  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.
* One user-visible consequence is that the "release" event is fired
  synchronously and before the function returns.

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

Big thanks to Anne van Kesteren, Domenic Denicola and Marcos Cáceres for
their input.

Fixes w3c#293.
@rakuco rakuco mentioned this pull request Mar 22, 2021
Merged
6 tasks
rakuco added a commit to w3c/screen-wake-lock that referenced this pull request Mar 22, 2021
@rakuco
editorial: Tidy up uses of "in parallel". (#299)
1e380dc 
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
queuing global tasks (emphasis on "global" given whatwg/html#4980 and
whatwg/html#5653) from there when we need to manipulate promises or create
objects.

"Obtain permission" algorithm:
* Stop running in parallel. Callers should be responsible for choosing
  whether it should be run in parallel or not.

`WakeLock.request()`:
* Separate returning a promise and running steps in parallel. This style is
  more usual.
* Refer to the "relevant settings object" rather than the "current settings
  object", as we are inside a method of an IDL interface and can rely on it
  being defined. I do not think this is a user-visible change, and it looks
  cleaner.
* Queue a global task to reject `|promise|` when the permission request run
  in parallel is denied.
* Manipulate the `[[ActiveLocks]]` internal slot, check page visibility,
  invoke "acquire a wake lock", create a new WakeLockSentinel and resolve
  the returned promise in a queued global task.

`WakeLockSentinel.release()`:
* Do not run the "release a wake lock" algorithm in parallel (see the
  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.
* One user-visible consequence is that the "release" event is fired
  synchronously and before the function returns.

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

Big thanks to Anne van Kesteren, Domenic Denicola and Marcos Cáceres for
their input.

Fixes #293.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk approved these changes

Assignees
No one assigned
Labels
clarification Standard could be clearer topic: event loop
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@domenic @annevk