Enable InitiatorUrl for ResourceTiming on modular scripts [chromium/src : main]
Guohui Deng (Gerrit)
Guohui Deng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Guohui Deng
| Commit-Queue | +1 |
1 comment
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 3 comments![]( "Open in Gerrit")
This looks reasonable to me, but I'm not a loading/modules expert. I'm chatting with Nate in a few min, I'll see if he has enough context to review.
// <script type="module">import "leaf.js";</script>I gather from the test that `<script type="module" src="leaf.js"</script>` has the same behavior (the document will be the initiator), which is good. Will that -- and should that -- always hold? I _think_ it works during initial page load because `GetIsolateIfRunningScript()` will be false. Is that also the case if dynamically inserting <script> via JavaScript?
<script type="module" src="/resource-timing/resources/module-script-importer-module-static.js"></script>Maybe it would make sense to break this up into separate tests? There's a lot going on and I'm having a hard time following each thread. I think this is testing four separate things?
1. module-script-importer-module-static.js (its importer and what it imports)
2. module-script-imported.js?label=document-static-import (its and what it imports)
3. module-script-importer-classic.js (just what it imports)
4. module-script-worker.js (worker imports)
I know these are all related, but I think separating them out would probably help readability.
Related details
- Guohui Deng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
- Guohui Deng
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin added 1 comment![]( "Open in Gerrit")
This looks resonable to me % Scott's question about how we're getting the initiatory for `<script type="module" src="leaf.js"</script>`.
Related details
- Guohui Deng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
Update: I am working on the improvement on the comments and splitting the tests (maybe add a few more cases).
For `<script type="module" src="leaf.js"</script>.` it's actually "resource statically included by html", (even though it's marked by `type='module`). It's loaded when the parser processes the html. It then falls into the "document" branch at the end.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 2 comments![]( "Open in Gerrit")
// <script type="module">import "leaf.js";</script>I gather from the test that `<script type="module" src="leaf.js"</script>` has the same behavior (the document will be the initiator), which is good. Will that -- and should that -- always hold? I _think_ it works during initial page load because `GetIsolateIfRunningScript()` will be false. Is that also the case if dynamically inserting <script> via JavaScript?
The comments here were lacking and I updated them -- I hope they now cover sufficiently. I also added a test case. (dynamically "adding" script)
<script type="module" src="/resource-timing/resources/module-script-importer-module-static.js"></script>Maybe it would make sense to break this up into separate tests? There's a lot going on and I'm having a hard time following each thread. I think this is testing four separate things?
1. module-script-importer-module-static.js (its importer and what it imports)
2. module-script-imported.js?label=document-static-import (its and what it imports)
3. module-script-importer-classic.js (just what it imports)
4. module-script-worker.js (worker imports)
I know these are all related, but I think separating them out would probably help readability.
Yeap that's too much in a test. I now broke it into 4:
1) document
2) dynamic importer
3) static importer
4) worker
I think the labels were too long, and I managed to shorten them a bit.
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 2 comments![]( "Open in Gerrit")
Sorry for the lag. Appreciate the test and comment cleanup! One more question on behavior.
document.head.appendChild(script);What would happen if this happened inside of a `setTimeout()` inside of a `<script src=somethingelse.js>`? Would it fallback to the `document_->Url()` case, or would it use the task attribution info?
Related details
- Guohui Deng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng added 3 comments![]( "Open in Gerrit")
// <script type="module">import "leaf.js";</script>Guohui DengI gather from the test that `<script type="module" src="leaf.js"</script>` has the same behavior (the document will be the initiator), which is good. Will that -- and should that -- always hold? I _think_ it works during initial page load because `GetIsolateIfRunningScript()` will be false. Is that also the case if dynamically inserting <script> via JavaScript?
The comments here were lacking and I updated them -- I hope they now cover sufficiently. I also added a test case. (dynamically "adding" script)
Seems this one is resolved.
document.head.appendChild(script);What would happen if this happened inside of a `setTimeout()` inside of a `<script src=somethingelse.js>`? Would it fallback to the `document_->Url()` case, or would it use the task attribution info?
Since it's the Javascript that requests the resource, it uses task attribution to find out the "initiator", regardless of whether the initiator is document or not. The "ResourceTimingContext" points the initaitor to the html file.
The same is true with `(initiator_info.is_imported_module && !initiator_info.referrer.empty())` branch -- as long as it's statical imported modular script, the branch catches it.
The exception is the CSS `referer` -- it catches resource referred by "CSS files", not "CSS code" that could be in the html. All the statically included resources in html are caught by "document fall back" branch -- they are fetched when the parser of the html file is working.
BTW I added a test case that covers the situation you asked about.
<script type="module" src="/resource-timing/resources/module-script-importer-module-static.js"></script>Guohui DengMaybe it would make sense to break this up into separate tests? There's a lot going on and I'm having a hard time following each thread. I think this is testing four separate things?
1. module-script-importer-module-static.js (its importer and what it imports)
2. module-script-imported.js?label=document-static-import (its and what it imports)
3. module-script-importer-classic.js (just what it imports)
4. module-script-worker.js (worker imports)
I know these are all related, but I think separating them out would probably help readability.
Yeap that's too much in a test. I now broke it into 4:
1) document
2) dynamic importer
3) static importer
4) workerI think the labels were too long, and I managed to shorten them a bit.
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 2 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
2 comments
document.head.appendChild(script);Guohui DengWhat would happen if this happened inside of a `setTimeout()` inside of a `<script src=somethingelse.js>`? Would it fallback to the `document_->Url()` case, or would it use the task attribution info?
Since it's the Javascript that requests the resource, it uses task attribution to find out the "initiator", regardless of whether the initiator is document or not. The "ResourceTimingContext" points the initaitor to the html file.
The same is true with `(initiator_info.is_imported_module && !initiator_info.referrer.empty())` branch -- as long as it's statical imported modular script, the branch catches it.
The exception is the CSS `referer` -- it catches resource referred by "CSS files", not "CSS code" that could be in the html. All the statically included resources in html are caught by "document fall back" branch -- they are fetched when the parser of the html file is working.
BTW I added a test case that covers the situation you asked about.
- Guohui Deng
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Guohui Deng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/61897.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Guohui Deng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Guohui Deng voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Enable InitiatorUrl for ResourceTiming on modular scripts
Worker scripts are not covered and will be addressed in a follow-up CL.
- M third_party/blink/renderer/core/loader/frame_fetch_context.cc
- M third_party/blink/renderer/core/loader/worker_fetch_context.cc
- M third_party/blink/renderer/core/scheduler/task_attribution_util.h
- M third_party/blink/renderer/core/script/dynamic_module_resolver.cc
- M third_party/blink/web_tests/external/wpt/lint.ignore
- A third_party/blink/web_tests/external/wpt/resource-timing/resources/module-script-appender.js
- A third_party/blink/web_tests/external/wpt/resource-timing/resources/module-script-imported.js
- A third_party/blink/web_tests/external/wpt/resource-timing/resources/module-script-importer-classic.js
- A third_party/blink/web_tests/external/wpt/resource-timing/resources/module-script-importer-module-dynamic.js
- A third_party/blink/web_tests/external/wpt/resource-timing/resources/module-script-importer-module-static.js
- A third_party/blink/web_tests/external/wpt/resource-timing/resources/module-script-worker.js
- M third_party/blink/web_tests/external/wpt/resource-timing/resources/test-initiator.js
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/module-script-from-document.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/module-script-from-dynamic-importer.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/module-script-from-static-importer.html
- A third_party/blink/web_tests/external/wpt/resource-timing/tentative/initiator-url/module-script-from-worker.html
Code-Review: +1 by Nate Chapin, +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/61897
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |