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.

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

Open
dtapuska opened this issue Oct 7, 2019 · 12 comments · Fixed by #5072
Open

Attempt to remove implied-document definition #4980

dtapuska opened this issue Oct 7, 2019 · 12 comments · Fixed by #5072
Labels
clarification Standard could be clearer topic: event loop

Comments

@dtapuska
Copy link
Contributor

dtapuska commented Oct 7, 2019

@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.

@annevk annevk added clarification Standard could be clearer topic: event loop labels Oct 8, 2019
@annevk
Copy link
Member

annevk commented Oct 8, 2019

That sounds great to me.

@domenic
Copy link
Member

domenic commented Oct 15, 2019

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:

Queue a task on the X task source, with document doc, to run the following steps:

Queue a task on the X task source of event loop, with document doc, to run the following steps:

Queue a task on the X task source of event loop to run the following steps. If event loop is a window event loop, this must be done with document doc.

(Only valid in worker event loops.) Queue a task on the X task source to run the following steps:

dtapuska added a commit to dtapuska/html that referenced this issue Nov 8, 2019
The event loop and the document source are attributed to the element's
node document and relevant realm.

Fixes whatwg#4980
domenic pushed a commit that referenced this issue Dec 5, 2019
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.
@dtapuska
Copy link
Contributor Author

dtapuska commented Dec 5, 2019

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.

@domenic
Copy link
Member

domenic commented Dec 5, 2019

Ah, I guess it was in the pull request description. Thanks for the heads-up.

@domenic domenic reopened this Dec 5, 2019
dtapuska added a commit to dtapuska/html that referenced this issue Dec 5, 2019
dtapuska added a commit to dtapuska/html that referenced this issue Dec 10, 2019
dtapuska added a commit to dtapuska/html that referenced this issue Dec 18, 2019
domenic pushed a commit that referenced this issue Dec 19, 2019
domenic pushed a commit that referenced this issue Mar 27, 2020
This accounts for <embed>, <frame>, <img>, <object>, form controls, and
a couple other spot-fixes.

Part of #4980.
domenic pushed a commit that referenced this issue Mar 27, 2020
This accounts for <marquee> and one that was missed for <frame>.

Part of #4980.
dtapuska added a commit to dtapuska/html that referenced this issue Mar 27, 2020
This accounts for <canvas>, <details> and <dialog>.

Part of whatwg#4980.
dtapuska added a commit to dtapuska/html that referenced this issue Mar 27, 2020
This accounts for media element section.

Part of whatwg#4980.
dtapuska added a commit to dtapuska/html that referenced this issue Mar 27, 2020
This accounts for the links and input sections.

Part of whatwg#4980.
dtapuska added a commit to dtapuska/html that referenced this issue Mar 30, 2020
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.
domenic pushed a commit that referenced this issue Mar 30, 2020
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.
domenic pushed a commit that referenced this issue Mar 30, 2020
This finishes the links and input sections.

Part of #4980.
dtapuska added a commit to dtapuska/html that referenced this issue Mar 30, 2020
This accounts for media element section.

Part of whatwg#4980.
dtapuska added a commit to dtapuska/html that referenced this issue Apr 2, 2020
This accounts for media element section.

Part of whatwg#4980.
domenic pushed a commit that referenced this issue Apr 2, 2020
This accounts for media element section. This requires a new
intermediate algorithm, "queue a media element task", since every media
element has its own task source. As a bonus, this fact is now much
clearer.

Part of #4980.
dtapuska added a commit to dtapuska/html that referenced this issue Apr 2, 2020
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.
@domenic
Copy link
Member

domenic commented Jun 17, 2020

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:

  • Each task needs an event loop, and if that event loop is a Window event loop, a document.
  • HTML's callers generally have nearby access to one of:
    • A browsing context (e.g., during navigation and history traversal)
    • A global object (e.g., in setTimeout() or various worker situations)
    • A document (e.g., during history traversal we have entry's Document)
    • An element (e.g., all current uses of "queue an element task")
  • The event loop is pretty trivial to compute from any of those inputs, so I'll gloss over that in what follows.

Here are some ideas:

  1. Introduce "queue browsing context task" which takes a browsing context
    • It computes document as the BC's active document
  2. Introduce "queue a global task" which takes a global object.
    • It computes document from the global object, if the global object is a Window, or null otherwise
    • If the caller has a Document, it needs to "queue a global task given document's relevant global object"
    • If the caller has a browsing context, it needs to "queue a global task given bc's WindowProxy's [[Window]] value", or alternately "queue a global task given bc's active document's relevant global object"
    • If the caller has an element, they will keep using "queue an element task". That's too common for us to require an indirection like "queue a global task using element's relevant global object"
  3. Replace "queue an element task" with "queue an object task" which takes any platform object.
    • It computes the document from the object's relevant global object, if that relevant global object is a Window, or null otherwise. (Per spec an element's node document is always equal to its relevant global object, I am pretty sure?)
    • This is easy to use if the caller has a global object, a document, or an element
    • If the caller has a browsing context, they could do "queue an object task given bc's active document".
    • The name is kind of bad.

Currently I am leaning toward doing either (1) + (3), or just (3). Do folks have thoughts?

@annevk
Copy link
Member

annevk commented Jun 18, 2020

(Per spec an element's node document is always equal to its relevant global object, I am pretty sure?)

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.

@domenic
Copy link
Member

domenic commented Jun 18, 2020

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.

OK, so "queue an element task" should stick around.

Could we adjust queue a task itself to take a source, steps, and either a platform object or a event loop?

So, use overloading instead of separately-named algorithms? That's certainly possible, but I'm not sure it's better.

Not a big fan of allowing browsing contexts to be passed around as they don't carry authority.

I understand you dislike them, but I think asking people to do "queue a global task given bc's WindowProxy's [[Window]] value" is unnecessary ceremony and I don't think it's reasonable to require.

@annevk
Copy link
Member

annevk commented Jun 18, 2020

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.

@domenic
Copy link
Member

domenic commented Jun 18, 2020

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:

  • "queue an object task"
  • "queue a global task", but make it easy to go from browsing context -> global, e.g. define "browsing context's current Window". This would probably be a good change independently, actually...

I like the latter.

@annevk
Copy link
Member

annevk commented Jun 24, 2020

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)?

@domenic
Copy link
Member

domenic commented Jun 24, 2020

But would we eventually merge them into queue a task?

I don't think we'd eventually merge them, unless we really like overloading. (Which I don't, at least.)

Could you sketch what the eventual API here would look like?

It's as in #5653:

  • "Queue a task" (task source, event loop, document-or-null, steps) is the base-level primitive
  • "Queue a global task" (task source, global object, steps) is used for "global" things
  • "Queue an element task" (task source, element, steps) is used for "element" things

And why is it better to take a global than a platform object (of which a global is one)?

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.

@annevk
Copy link
Member

annevk commented Jun 24, 2020

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.

domenic added a commit that referenced this issue Jun 24, 2020
domenic added a commit that referenced this issue Jun 25, 2020
domenic added a commit to WICG/portals that referenced this issue Jun 26, 2020
mfreed7 pushed a commit to mfreed7/html that referenced this issue Sep 11, 2020
rakuco added a commit to rakuco/wake-lock that referenced this issue 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.
domenic added a commit that referenced this issue Feb 19, 2021
domenic added a commit that referenced this issue Feb 22, 2021
rakuco added a commit to rakuco/wake-lock that referenced this issue 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 issue 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 issue 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.
rakuco added a commit to w3c/screen-wake-lock that referenced this issue 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
Labels
clarification Standard could be clearer topic: event loop
Development

Successfully merging a pull request may close this issue.

3 participants