"Release a wake lock": should we queue a task to fire the "release" event? #293
Comments
|
I don't understand this algorithm. Is it intentional that the |
It might make sense to do something different later, but for now yeah, we always remove the lock from
OK, so does it make more sense to stop deferring the event and the change to |
|
Yeah, though you want to audit all callers, e.g., |
Correct,
Can you elaborate on why state isn't available in the "in parallel" blocks? I thought the algorithm would have access to |
|
It doesn't basically call it though, right? It goes in parallel first and calls it from there. If that wasn't meant to be in parallel, why does it say that? That's what I mean by audit. If you're going to change things around, you want to make sure it all still makes sense. |
|
OK. My understanding of that particular method is that that part runs in parallel to avoid potentially blocking on calling the OS (and dispatching the event in case we do change the algorithm). If that part didn't run in parallel, I don't think there'd be much point in returning a promise in the first place. |
|
Okay, but we are on the same page that if it goes in parallel, you cannot call "release a wake lock" synchronously from those in parallel steps? And I now see that release a wake lock itself says at the top it goes in parallel too... And then again in step 5? I think someone needs to have another look at all this and decide where in parallel is important and where it's noise. And keep in mind that when you are in parallel you cannot access things such as documents or global objects and need to queue a task to get back to the main thread. |
|
That someone would probably be me anyway :-) Dealing with the event loop from other specifications was very helpful, but it also left me with some questions about object creation in parallel steps and how promises and queued tasks are supposed to work together. It also left me paranoid about having to think about realms, global objects, task sources and objects creation and manipulation everywhere :-) These are all the parts of the spec that have "in parallel" in their text, along with some questions and comments:
|
Realm-agnostic things can be created in parallel. So, interfaces are realm-specific, and cannot be created there. For dictionaries, it depends on what the dictionary contains. Dictionary keys are strings, which are realm-agnostic, but their values vary. If one of the values is an interface, then that can't be created in parallel. If it's all primitives or sequences or records or similar, then it can.
You can't resolve a promise from in parallel. You need to queue a task to do so. (And, you can use the same task to resolve the promise and fire the event.) |
|
I'm afraid you will have to think about all those things, but I'm happy to help.
What you need to do for cases like this is queue a task to fire the event and resolve the promise (a task can itself be a series of steps). I'm not sure if we decided on ideal order there. I guess I'd resolve first... |
Hmm, I wonder if the examples in https://heycam.github.io/webidl/#es-promise-examples need to be adjusted then? Anyway, thanks for the help so far, @annevk and @domenic. I'll see what I can come up with tomorrow. |
|
Yeah, they should be (only 55 does it correctly). I'll see if I can put a fix out for that today. |
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.
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.
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.
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.
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.
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.
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.
(also related: #139)
I've been reviewing a Chromium CL that deals with bfcache, and ended up with a few questions about our "release a wake lock" algorithm and its relationship with the Page Lifecycle API and bfcache.
Since #242, the "release a wake lock" algorithm queues a task to fire the "release" event (and, since #279, to set the
[[Released]]internal slot totrue). The question is whether this should be done in a task or as a synchronous step in the algorithm.In the Chromium CL in question, we've noticed that when navigating away from a page, we do reach https://w3c.github.io/screen-wake-lock/#handling-document-loss-of-visibility, the "release a wake lock" algorithm does run but the queued task is never run, as the page is frozen first (even without bfcache, in which case I guess it'd be unloaded first).
Queuing a task to fire an event seems idiomatic, but in this case I wonder if it's the best choice, especially since this happens when the document both loses visibility and loses full activity. Or, at the very least, I wonder if changing the
[[Released]]slot should be done synchronously, or if doing that separately from firing the event makes even less sense.The text was updated successfully, but these errors were encountered: