[worklets] Fix module loading when creating a new worklet global scope. #264
Conversation
| 2. Let |script| be the result of <a>fetch a module script tree</a> given | ||
| |resolvedModuleURL|, "anonymous" for the <a>CORS setting attribute</a>, and | ||
| |settingsObject|. | ||
| 2. <a>Fetch a module worklet script tree</a> given |resolvedModuleURL|, |outsideSettings|, |
There was a problem hiding this comment.
Slight problem as we don't have access to |outsideSettings| here.
There was a problem hiding this comment.
I don't understand this whole section. The module responses map stores every module, including imported ones. And you want to fetch and execute them all? That means that if you do import './a.js' and a.js does import './b.js', then the module responses map will have both a.js and b.js in it. Then you will execute the tree starting at a.js (so both a.js and b.js), but also you will separately execute the tree starting at b.js (so b.js will be executed twice). This seems bad?
What is the intention of this section?
There was a problem hiding this comment.
Yeah this was wrong. This only should be loading the top level requests.
This section is used for if a user-agent wants to create a new WorkletGlobalScope in addition to the ones it already has. (E.g. it may have killed one from the pool and needs to restart or something).
When instantiating the new WorkletGlobalScope it has to load all the previous top level modules loaded into it.
Added a "top level module url list" to the worklet which the fetch hook now populates. This reads off that to load it's code.
| @@ -336,8 +342,8 @@ the user agent <em>must</em> run the following steps: | |||
| 1. Let |insideSettings| be the {{WorkletGlobalScope}}'s associated <a>environment | |||
| settings object</a>. | |||
|
|
|||
| 2. <a>Fetch a module worklet script tree</a> given |resolvedModuleURL|, | |||
| |outsideSettings|, and |settingsObject|. | |||
| 2. <a>Fetch a module worklet script tree</a> given the current {{Worklet}}, | |||
There was a problem hiding this comment.
Nit: "the current Worklet" is not defined here. I think you want to add a step at the top of this method definition that does "Let worklet be this Worklet object" or similar. (Other acceptable phrasing: "the Worklet object on which this method was called".) Then you can use worklet later.
|
LGTM with one minor thing and the open question about destination. Awesome! |
Closes w3c/css-houdini-drafts#1000. This provides a baseline by porting over all the existing text from https://drafts.css-houdini.org/worklets/, 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 #2611. * Updated worklets to only be exposed in secure contexts. Closes w3c/css-houdini-drafts#505. * Makes the lifetime of creating and terminating WorkletGlobalScopes more explicit. Closes w3c/css-houdini-drafts#224. Closes w3c/css-houdini-drafts#389. * Explicitly start and stop the event loop for a given WorkletGlobalScope upon creation/termination. Closes w3c/css-houdini-drafts#843. Closes 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 w3c/css-houdini-drafts#264.
|
With whatwg/html#6056, we can close this. That PR incorporates a revised version of this patch; notably, this patch was missing any step to actually add URLs to the top-level URL list, but I added that in to HTML. |
@domenic can you take a quick look at this?