-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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.
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
Editorial: Introduce and use "queue a global task" #5653
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
03e02a2
to
8f90b6c
Compare
annevk
approved these changes
Jun 25, 2020
rakuco
added a commit
to rakuco/wake-lock
that referenced
this pull request
Feb 15, 2021
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
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
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
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.
6 tasks
rakuco
added a commit
to w3c/screen-wake-lock
that referenced
this pull request
Mar 22, 2021
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
EventSource
orWebSocket
object whose relevant global object I could pick, and instead say things like "thereadyState
attribute". The temptation to fix them was great, but I resisted for now.