-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Move worklets into the HTML Standard #6056
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, exciting.
Should we annotate these IDL blocks with SecureContext?
<var>reservedEnvironment</var>'s <span | ||
data-x="concept-environment-target-browsing-context">target browsing context</span>, and <span | ||
data-x="concept-environment-active-service-worker">active service worker</span> to | ||
<var>reservedEnvironment</var>'s <span |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I find this as clear as the previous wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I preferred the way the worklets spec does it, which involves less repetition, so I normalized both to this style.
<ul> | ||
<li><p>Are thread-agnostic. That is, they are not designed to run on a dedicated separate thread, | ||
like each worker is. Implementations can run worklets wherever they choose (including on the main | ||
thread).</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this holds for audio worklets with shared memory, but I guess it does in theory, but not really in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they're thread-agnostic, just not process-agnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just that you wouldn't have a good time if you ran the audio worklet on the main thread.
Note to self: I'm also hoping to land #4352 soon, so this will need some minor updates there, by incorporating @Ms2ger's work in w3c/css-houdini-drafts#984 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a few potential issues and a couple nits, but largely looks good to me.
@sideshowbarker are there any special considerations with adding an additional file to the multipage version?
<ul> | ||
<li><p>Are thread-agnostic. That is, they are not designed to run on a dedicated separate thread, | ||
like each worker is. Implementations can run worklets wherever they choose (including on the main | ||
thread).</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just that you wouldn't have a good time if you ran the audio worklet on the main thread.
|
||
<dt>The <span data-x="concept-settings-object-cross-origin-isolated-capability">cross-origin | ||
isolated capability</span></dt> | ||
<dd><p>Return <span class="XXX">TODO</span>.</p></dd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This we can inherit, no? I guess there's again the question as to whether we require COEP to be specified because of imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I prefer leaving this as a TODO for now, ideally getting some tests.
<li> | ||
<p><span>Fetch a worklet script graph</span> given <var>moduleURL</var>, | ||
<var>insideSettings</var>, <var>worklet</var>'s <span>worklet destination type</span>, <span | ||
class="XXX">what credentials mode?</span>, <var>insideSettings</var>, and <var>worklet</var>'s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's either same-origin or include, right? Any reason not to go with same-origin? Or do we want to write tests before resolving this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it's whatever was originally used with addModule(). Fixing this seems to require some plumbing to store the credential alongside the URL. (Which leads to complicated questions about what if you addModule() the same module twice with two different credentials...) Maybe related: w3c/css-houdini-drafts#264
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments from an Audio WG point of view, let me know if I can be clearer, audio programming has very strict constraints that are a bit unusual.
<code>WorkletGlobalScope</code> is expected to be shared between multiple separate scripts. (This | ||
is similar to how a single <code>Window</code> is shared between multiple separate scripts.)</p> | ||
|
||
<h4 id="worklets-idempotent">Code idempotence</h4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section doesn't reflect the reality. The AudioWorklet
requires the opposite: a single scope, statefullness, memory sharing via accessing a global, etc.
This is the case for rather fundamental reasons: audio programming is very often stateful (to output an audio sample at time t, we need to know the value of the sample at t-1, and state that was computed prior to compute t), and of course performance: sharing large audio buffers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. This is ported straight from the worklets spec, but I guess we shouldn't add things that are known-wrong in the porting process.
I guess the simplest way to fix this would be to say "some worklets" instead of "worklets"... I'll try something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this section already says "Some specifications which use worklets", not all. So I think this section already reflects reality. Do you disagree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably worth calling out that worklets serve both use cases. Both stateless and stateful, and short and long-running operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added such a paragraph; PTAL.
data-x="">process</code>" property and convert it into a <code | ||
data-x="idl-Function">Function</code> at registration time, as part of the <code | ||
data-x="dom-FakeWorkletGlobalScope-registerFake">registerFake()</code> method steps.</p> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Audio WG is exploring this. Classes are nice, but are problematic for WASM integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, the idea here is to use classes, but instead of using the style in this toy example which Get()s and converts the method every time, use the architecture that audio worklet and paint worklet use, where you Get() and convert the method at registration time.
I just realized that w3c/css-houdini-drafts#264 is a pretty important bug fix (it fixes the problem described in w3c/css-houdini-drafts#264 (comment)) so I think I should pull that in before we land this. |
None that come to mind. We’re not maintaining a static list of output filenames anywhere — not in any code nor in any support files — so there’s nothing to update as far as that goes. Everything’s just keyed off the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this. Lots of details to figure out still, but that's no different from the status quo and this does improve on that, e.g., by having worklets limited to secure contexts.
cc @whatwg/documentation |
Closes w3c/css-houdini-drafts#1000.
This provides a baseline by porting over all the existing text from
https://drafts.css-houdini.org/worklets/#lifetime-of-the-worklet,
modernizing and restructuring it along the way. It does not yet fix many
of the open logged issues (although it does fix some; see below).
Notable changes from that document:
Rearranged sections to better match workers, and my sense of flow.
Moved worklet script fetching to be siblings with all the other script
fetching algorithms.
Improved clarity and guidance on what specifications that define
worklets should do, including fleshing out the fake worklet example.
Changed "create a WorkletGlobalScope", which took one set of
arguments, to "create a worklet global scope", which just takes a
Worklet instance. This appears to match better how the algorithm is
used, e.g. in
https://drafts.css-houdini.org/css-paint-api/#draw-a-paint-image step
10.
Updated "report an error" to bail out for non-EventTarget globals,
like WorkletGlobalScope. Closes Update [report an exception] to work with globals which aren't EventTargets. #2611.
Updated worklets to only be exposed in secure contexts. Closes
[worklets] restrict worklets to SecureContexts w3c/css-houdini-drafts#505.
Makes the lifetime of creating and terminating WorkletGlobalScopes
more explicit. Closes
Why is there a responsible browsing context, but no document? w3c/css-houdini-drafts#224. Closes
[worklets] Introduce an "owner document" for each WorkletGlobalScope. w3c/css-houdini-drafts#389.
Explicitly start and stop the event loop for a given
WorkletGlobalScope upon creation/termination. Closes
Worklets does not integrate with the event loop w3c/css-houdini-drafts#843.
Closes Worklet event loop is not run? w3c/css-houdini-drafts#318 for real.
Fixes creation of new worklet global scopes to only run the top-level
module scripts added via addModule(), which will automatically run
their dependencies. Previously it would run all module scripts loaded
into the worklet, so dependencies would be run in the order they were
fetched, not as part of the top-down module evaluation process. Closes
[worklets] Fix module loading when creating a new worklet global scope. w3c/css-houdini-drafts#264.
This PR has three TODOs in the source:
The credentials mode is missing from one call site of "fetch a worklet script graph". This is a preexisting problem, and probably we can survive with this and merge it. (I made it an XXX.) I think doing this properly might involve converting the module responses map from url-keyed to (url, credentials mode) keyed? I'll want to research what implementations do...
The "cross-origin isolated capability" is a TODO. Also a preexisting problem; can also probably be merged and fixed later.
The sentence "TODO Whenever a Document object is discarded, each WorkletGlobalScope whose owner document is that Document object, should clear its owner document." is not carried over from the Worklets spec. This is a bit trickier:Attempted to fix this with the latest commits; PTALThe only use of "owner document" in the HTML spec is to figure out if a WorkletGlobalScope is a secure context. There's probably an easier way to do this, e.g. copying over secure-ness at creation time, which would allow us to eliminate the "owner document" field entirely?
But, I think that "owner document" was added to try to address Why is there a responsible browsing context, but no document? w3c/css-houdini-drafts#224. Hmm, I guess I should maybe add the contents of https://github.com/w3c/css-houdini-drafts/pull/389/files into this PR? @annevk, what do you think?
After we get this merged, we can tackle some of the easier open issues, ideally with tests. For example, the minor TODOs above, or w3c/css-houdini-drafts#506, or w3c/css-houdini-drafts#985 .
Probably also w3c/css-houdini-drafts#505 since at least Chrome appears to be restricting worklets to secure contexts.Fixed this in this PRBefore merging this, I need to canvas the worklet-using specs and make sure they'll keep working, and update their links as appropriate (e.g. changing
create a WorkletGlobalScope
tocreate a worklet global scope
). That may reveal more<dfn>
s to export./acknowledgements.html ( diff )
/iana.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/infrastructure.html ( diff )
/named-characters.html ( diff )
/obsolete.html ( diff )
/parsing.html ( diff )
/references.html ( diff )
/rendering.html ( diff )
/syntax.html ( diff )
/webappapis.html ( diff )
/webstorage.html ( diff )
/window-object.html ( diff )
/workers.html ( diff )
/xhtml.html ( diff )
/worklets.html ( diff )