-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Attempt to remove implied-document definition #4980
Comments
That sounds great to me. |
Yes, this would be lovely. It's just a ton of work. I count 202 uses of "queue a task" in HTML alone, not to mention the rest of the ecosystem. That said, @dtapuska has done heroic things before... and incremental progress is totally possible, i.e. we keep using implied document as a fallback until all call sites are updated. I'm unsure whether we should introduce a new "queue a document task" + "queue a document microtask" or just reuse "queue a task" and add a semi-optional parameter. (I.e., it's mandatory in window event loops.) It's worth thinking about how this would impact cases that are polymorphic over event loops. There aren't many of these; maybe just BroadcastChannel's postMessage(). Anyway I think the right thing to do is figure out what the various call sites look like. Ideas:
|
The event loop and the document source are attributed to the element's node document and relevant realm. Fixes whatwg#4980
This is a specialization of "queue a task", where the event loop and the document are derived from the element. This helps with #4980. For now only a couple instances were converted, but the other 200+ will follow.
I think we should reopen this issue until they all land no? Not sure what the magic was that closed it in github but it didn't have closes in the commit msg I don't think. |
Ah, I guess it was in the pull request description. Thanks for the heads-up. |
This helps with whatwg#4980.
This helps with whatwg#4980.
This helps with whatwg#4980.
This accounts for <embed>, <frame>, <img>, <object>, form controls, and a couple other spot-fixes. Part of #4980.
This accounts for <marquee> and one that was missed for <frame>. Part of #4980.
This accounts for <canvas>, <details> and <dialog>. Part of whatwg#4980.
This accounts for media element section. Part of whatwg#4980.
This accounts for the links and input sections. Part of whatwg#4980.
This accounts for <canvas>, <details> and <dialog>. Use the DOM Manipulation task source for committing offscreen canvas to the main thread. Use the user interaction task source for dialog interactions. Part of whatwg#4980.
This accounts for <canvas>, <details> and <dialog>. Additionally, this specifies task sources in two places where they were previously unspecified: * For committing offscreen canvas to the main thread, uses the DOM manipulation task source. * For dialog interactions, uses the user interaction task source. Part of #4980.
This finishes the links and input sections. Part of #4980.
This accounts for media element section. Part of whatwg#4980.
This accounts for media element section. Part of whatwg#4980.
Adjust the last remaining element/node based references. The rest of the references in the spec are either document or global objects which will come in another CL. Part of whatwg#4980.
I started working on this a bit more as another spec (portals) would benefit from it. However I ended up unsure what direction to go. Recap:
Here are some ideas:
Currently I am leaning toward doing either (1) + (3), or just (3). Do folks have thoughts? |
Per specifications, if you create a custom element in global A and move it into a document in global B, its relevant global object is A, but its node document's relevant global object is B. Could we adjust queue a task itself to take a source, steps, and either a platform object or a event loop? And if you pass an event loop directly we assert it's not a window event loop? Not a big fan of allowing browsing contexts to be passed around as they don't carry authority. |
OK, so "queue an element task" should stick around.
So, use overloading instead of separately-named algorithms? That's certainly possible, but I'm not sure it's better.
I understand you dislike them, but I think asking people to do "queue a global task given bc's |
Yeah, perhaps "queue an element task" should stay separate. The other thing we could do is special case elements in "queue a task", but I suppose we could also assert you're not passing them. I don't dislike browsing contexts, but they're just not the right target as they represent a sequence of targets. And encouraging people to think of them as if they're synonymous with their active document will create all kinds of bugs, in particular when queuing tasks from "in parallel" or some such. |
I guess it's not so bad to require callers to do "bc's active document"; I forgot the part of my post where I mentioned that was an option. I guess I see two possibilities now:
I like the latter. |
But would we eventually merge them into queue a task? Could you sketch what the eventual API here would look like? And why is it better to take a global than a platform object (of which a global is one)? |
I don't think we'd eventually merge them, unless we really like overloading. (Which I don't, at least.)
It's as in #5653:
Taking a generic platform object makes it too easy to do the wrong thing for elements, where (apparently) we want to use their node document instead of their relevant global object's associated Document. The price is that we sometimes have to say "queue a global task given X's relevant global object" instead of "queue an object task given X", but I think that's good. It also makes you consider whether you want to instead use the current global object. |
Thanks, I guess that's reasonable. I was thinking we would keep element task and use an assert to keep elements out of the more generic variant, but this works. |
Closes #77 and helps with whatwg/html#4980.
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.
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.
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.
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.
@bzbarsky has raised concerns a few times about the lack of formal definition of a task's document. The definition comes from the implied-document which is relatively handwavy.
Can we brainstorm some ideas how we can better formalize this?
For example I'm wondering if we can introduce a "queue a document task" and then we can change the call sites to "queue a document task given element's document". And then that could take the event loop from the document so we wouldn't need to use the definition of implied event loop either. Now that doesn't cover all the cases but would be a fair number of them.
The text was updated successfully, but these errors were encountered: