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

Scripts moved between documents #19632

Conversation

hiroshige-g
Copy link
Contributor

@hiroshige-g hiroshige-g commented Oct 10, 2019

This follows the changes in whatwg/html#2673, but also tests the issue at whatwg/html#2137 in favor of the current spec.

Originally at #5911.

@foolip
Copy link
Member

foolip commented Oct 11, 2019

@jgraham I discovered via https://github.com/foolip/wpt-stats/commit/f594c4dd3a0962ff26fb7a6c72fdc71d8b265ee9/checks?check_suite_id=260736134 that Taskcluster hasn't started for this PR. Can you report this to the team?

@hiroshige-g I see this is just a draft and it might not matter, but if you want to retrigger you can either close and reopen the PR, or just push new commits.

@foolip
Copy link
Member

foolip commented Oct 18, 2019

See web-platform-tests/wpt-pr-bot#69 for why @wpt-pr-bot assigned people here. I've removed everyone but left myself in the hope that it doesn't add anyone back.

@foolip foolip closed this Oct 18, 2019
@foolip foolip reopened this Oct 18, 2019
@foolip
Copy link
Member

foolip commented Oct 19, 2019

Weird, still no Taskcluster run here despite closing and reopening. Ping @jgraham.

@domenic
Copy link
Member

domenic commented Nov 4, 2019

It seems very likely that WIP pull requests do not get CI run, so I'm going to try pushing the "ready to review" button (and maybe closing/reopening) to see what happens.

@domenic domenic marked this pull request as ready for review November 4, 2019 21:00
@domenic domenic closed this Nov 4, 2019
@domenic domenic reopened this Nov 4, 2019
@domenic domenic force-pushed the scripts-between-documents-hiroshige branch from faa1022 to 9ad2afe Compare November 4, 2019 21:32
@foolip foolip removed their assignment Nov 5, 2019
@foolip
Copy link
Member

foolip commented Nov 5, 2019

@domenic that Taskcluster probably wasn't because this was a draft, but because the branch was based on 0d5ca04 before the Taskcluster config was in the repo. Just rebasing the branch ought to have done the trick, in other words.

@hiroshige-g hiroshige-g force-pushed the scripts-between-documents-hiroshige branch from bf39aab to 48c1189 Compare April 21, 2020 04:23
@hiroshige-g
Copy link
Contributor Author

Fixed TODOs, aligning with the latest whatwg/html#2673.

domenic added a commit to whatwg/html that referenced this pull request Apr 21, 2020
This causes scripts that move between documents between the preparation
and execution phases to not execute, aligning with most browsers. Closes
#2469.

This does not address #2137, which is about scripts moving between
documents between the parsing and preparation, or parsing and execution
phases.

Tests: web-platform-tests/wpt#19632
@domenic domenic closed this Apr 21, 2020
@domenic domenic reopened this Apr 21, 2020
@stephenmcgruer
Copy link
Contributor

Failed wpt-firefox-nightly-stability due to too many tests (timed out), and weirdly so did wpt-chrome-dev-results (but before it had even downloaded Chrome...).

Admin-merging.

@stephenmcgruer stephenmcgruer merged commit 299dd66 into web-platform-tests:master Apr 22, 2020
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this pull request Jun 11, 2020
The #prepare-a-script algorithm [1] includes a check asserting that a
script element's parser document == its node document, for
parser-inserted scripts. Spec discussion has been happening around this
at [2], and WPTs have been added for this in [3] and [4] as part of an
overall effort to test and better-specify the behavior of script elements
that move between documents.

Before this CL, Chromium had no concept of a parser document, and
therefore would execute scripts that were moved to another document
before ScriptLoader::PrepareScript was invoked.

This CL introduces a |parser_document_| to ScriptLoader, which is
populated from CreateElementFlags::ParserDocument(), similar to the
|parser_inserted_| flag.

[1]: https://html.spec.whatwg.org/C/#prepare-a-script
[2]: whatwg/html#2137
[2]: web-platform-tests/wpt#19632
[3]: web-platform-tests/wpt#23162

Bug: 721914, 1086507
Change-Id: I7a0980afb47be93f8ed9948658b2cc8e4fa04669
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2214301
Commit-Queue: Dominic Farolino <[email protected]>
Reviewed-by: Kouhei Ueno <[email protected]>
Reviewed-by: Hiroshige Hayashizaki <[email protected]>
Cr-Commit-Position: refs/heads/master@{#777203}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
The #prepare-a-script algorithm [1] includes a check asserting that a
script element's parser document == its node document, for
parser-inserted scripts. Spec discussion has been happening around this
at [2], and WPTs have been added for this in [3] and [4] as part of an
overall effort to test and better-specify the behavior of script elements
that move between documents.

Before this CL, Chromium had no concept of a parser document, and
therefore would execute scripts that were moved to another document
before ScriptLoader::PrepareScript was invoked.

This CL introduces a |parser_document_| to ScriptLoader, which is
populated from CreateElementFlags::ParserDocument(), similar to the
|parser_inserted_| flag.

[1]: https://html.spec.whatwg.org/C/#prepare-a-script
[2]: whatwg/html#2137
[2]: web-platform-tests/wpt#19632
[3]: web-platform-tests/wpt#23162

Bug: 721914, 1086507
Change-Id: I7a0980afb47be93f8ed9948658b2cc8e4fa04669
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2214301
Commit-Queue: Dominic Farolino <[email protected]>
Reviewed-by: Kouhei Ueno <[email protected]>
Reviewed-by: Hiroshige Hayashizaki <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#777203}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: f104795245d1a32fde965862578baeb6e75961be
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants