Clean up the script element processing model #7876
Conversation
Although honestly "blocking" seems like a confusing name for this???
The new name "result" anticipates import maps and other script types that don't use "script" structs here.
…ad event forever on things that never mark as ready such as type="foobar")
"script block" has always been weird; what is a block?
This makes it clear it's for script *elements*, not #concept-scripts. Un-exporting seems like a good idea in general. https://dontcallmedom.github.io/webdex/p.html#prepare%20a%20script%40%40html%25%25dfn shows monkeypatches and informative references only.
There was a problem hiding this comment.
This largely looks correct to me, but I did spot some weirdness. If @hiroshige-g has the time to read through it as well I think that would be good.
Are you planning on landing this as independent commits? Should they be prefixed with Editorial:?
I was planning to land this as one large commit, with the description given in the PR. It's just separate commits for easier review. |
I feel other kinds of names would be better, because this flag is not a simple "non-blocking" flag, and having "blocking", "parser-blocking", and "render-blocking" might be confusing. |
|
Yeah I agree the blocking name could be better. If you or someone else can come up with a good alternative I'll happily use that instead. Force-async might indeed be good, I'll have to re-read through the usage sites to make sure. |
|
I don't understand how the non-blocking/blocking thing functions. Where it currently unsets non-blocking it notes:
However, deep down in "prepare a script" for the matching condition (where non-blocking is unset) it says
Which doesn't guarantee execution upon seeing the |
|
I interpret the comment is about "Set the element's parser document to the Document", not the non-blocking flag. The non-blocking flag seems to make dynamically inserted It might be easier to understand if we rewrite Step 28 of prepare-a-script like:
|
|
While trying to figure out what a good name would be, I found an interesting difference between browsers: <script type=x></script>
<script>
console.log(document.querySelector("script").async)
</script>(Chrome/Safari appear compliant with true.) "Async fallback" might be another good name for this state as we use this when the |
There was a problem hiding this comment.
@hiroshige-g's comments still need to be addressed, but I figured I'd approve this from an editorial perspective.
|
OK, I think this is ready. Another review from @hiroshige-g would be appreciated before merging. The major changes are blocking => force async (and flipping the boolean back), and incorporating Hiroshige's rewrite of step 28. Both seemed to improve clarity quite a lot. |
| <p>Add the element to the end of the <dfn>list of scripts that will execute when the document | ||
| has finished parsing</dfn> associated with the element's <span>parser document</span>.</p> | ||
| <ol> | ||
| <li id="script-processing-src"> |
There was a problem hiding this comment.
nit: this id assignments look weird, but I don't have better ideas to keep the historical IDs (yeah, looks very historical) to work.
| <p>Each <code>Document</code> has a <dfn>pending parsing-blocking script</dfn>, which is a | ||
| <code>script</code> element or null, initially null.</p> | ||
|
|
||
| <p>Each <code>Document</code> has a <dfn>set of scripts that will execute as soon as |
There was a problem hiding this comment.
It might be easier to understand and more self-documented for future spec readers if we rename these to e.g.
- set of async scripts
- list of in-order scripts (I'm not sure what is the name for this -- I searched "async = false" and didn't find articles explicit names for this case. "in-order" is a word I used in Chromium implementation, but I'm not sure this is natural for e.g. web developers)
- list of defer scripts
There was a problem hiding this comment.
I am not 100% sure on this. I kind of like how the current names describe behavior, instead of relying on knowing what async or defer means.
There was a problem hiding this comment.
I'm also not so confident. Then let's leave them as-is.
|
LGTM! |
In order to - Reflect HTML spec updates, mainly whatwg/html#7876, and - Make the intervention logic clearer. Namely, this CL largely refactors the final part of `PrepareScript()` (corresponding to Steps 31 and 32), by changing the code structure according to html#7876 and restructuring the logic into three steps: 1. Determine per-spec scheduling type in `GetScriptSchedulingTypePerSpec()`. 2. Modify the scheduling type due to interventions, and 3. Actually schedule scripts based on the scheduling type and interact with ScriptRunner and parsers. Other parts are more straightforward and somehow mechanical: - Renaming ScriptLoader::`non_blocking_` to `force_async_`. - Updating spec comments (using https://docs.google.com/document/d/1vmnpbDvMR-ph0v88lC810xsE6kjWVsQP351_l65pxLs/edit#heading=h.2obuaw96wltr). - Other very minor refactoring. This CL shouldn't change the behavior. Bug: 1344772, 1340837, 1339112 Change-Id: I788f682531fa0d417338562a4f0420f4c8b49e72 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3795585 Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Minoru Chikamune <chikamune@chromium.org> Reviewed-by: Shunya Shishido <sisidovski@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/main@{#1031114}
In order to - Reflect HTML spec updates, mainly whatwg/html#7876, and - Make the intervention logic clearer. Namely, this CL largely refactors the final part of `PrepareScript()` (corresponding to Steps 31 and 32), by changing the code structure according to html#7876 and restructuring the logic into three steps: 1. Determine per-spec scheduling type in `GetScriptSchedulingTypePerSpec()`. 2. Modify the scheduling type due to interventions, and 3. Actually schedule scripts based on the scheduling type and interact with ScriptRunner and parsers. Other parts are more straightforward and somehow mechanical: - Renaming ScriptLoader::`non_blocking_` to `force_async_`. - Updating spec comments (using https://docs.google.com/document/d/1vmnpbDvMR-ph0v88lC810xsE6kjWVsQP351_l65pxLs/edit#heading=h.2obuaw96wltr). - Other very minor refactoring. This CL shouldn't change the behavior. Bug: 1344772, 1340837, 1339112 Change-Id: I788f682531fa0d417338562a4f0420f4c8b49e72 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3795585 Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Minoru Chikamune <chikamune@chromium.org> Reviewed-by: Shunya Shishido <sisidovski@chromium.org> Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/main@{#1031114} NOKEYCHECK=True GitOrigin-RevId: d2415ae4482085c4909f5cce94de255491a7f5da
<script type="foobar">) don't delay the load event indefinitely.<script>element, not about the separate script concept.This is probably best reviewed commit-by-commit. Everything is intended to be editorial except for the fix for #5217 which happened by accident.
I mainly did this as preparation for future script element integrations like import maps.
/cc @hiroshige-g as FYI
/parsing.html ( diff )
/scripting.html ( diff )
/xhtml.html ( diff )