Introduce <script type=webbundle> [chromium/src : main]
Hayato Ito (Gerrit)
Attention is currently required from: Kouhei Ueno, Kunihiko Sakamoto, Hiroshige Hayashizaki, Tsuyoshi Horo.
Hayato Ito would like Kouhei Ueno, Kunihiko Sakamoto, Hiroshige Hayashizaki and Tsuyoshi Horo to review this change.
Introduce <script type=webbundle>
Introduce <script>-based API for subresource loading with Web Bundles.
See the design doc [1] for the motivation of switching from
<link>-based API to <script>-based API. The explainer [2] was already
updated to use <script>-based API.
This feature is guarded by `SubresourceWebBundles` flag.
We eventually drop the <link rel=webbundle> support and remove the
<link>-based API code once we can confirm <script>-based API can be
used as a replacement of <link>-based API.
This CL should be considered as the first step to switch to
<script>-based API. There are still gaps between <link>-based API and
<script>-based API, which will be addressed later [3].
This CL intentionally adds a very minimum test for <script
type=webbundle>. Tests will be added in follow-up CLs. The plan is:
1. We will convert the existing tests incrementally, replacing
<link>-based API with new <script>-based API. For example, for WPT,
we will convert:
from: wpt/web-bundle/subresource-loading/link-*.html
to: wpt/web-bundle/subresource-loading/script-.html
2. If we find an issue of the implementation or find a missing feature
by rewriting a test, we'll fix or update our implementation.
3. Continue until we can convert all existing tests.
4. Once we finish converting all tests, we can switch to
<script>-based API and be ready to drop <link>-based API.
We'll also add tests which are specific to <script>-based API as
necessary. These efforts should be tracked by crbug.com/1245166.
[1]: https://docs.google.com/document/d/1q_SodTcLuwya4cXt1gIRaVrkiaBfwWyPvkY1fqRKkgM/edit?usp=sharing&resourcekey=0-dqrFOGVCYsg8WRZ4RFgwuw
[2]: https://github.com/WICG/webpackage/blob/main/explainers/subresource-loading.md
[3]: https://github.com/WICG/webpackage/issues/670
Bug: 1245166
Change-Id: I5109b6e692baf10fd1d8a31a31d93176d4dc4ad2
---
M third_party/blink/renderer/core/BUILD.gn
M third_party/blink/renderer/core/html/html_script_element.cc
M third_party/blink/renderer/core/html/html_script_element.h
M third_party/blink/renderer/core/html/link_web_bundle.cc
M third_party/blink/renderer/core/html/link_web_bundle.h
M third_party/blink/renderer/core/html/parser/html_preload_scanner.cc
M third_party/blink/renderer/core/loader/build.gni
A third_party/blink/renderer/core/loader/web_bundle/DIR_METADATA
A third_party/blink/renderer/core/loader/web_bundle/OWNERS
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.cc
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.h
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule.cc
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule.h
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule_test.cc
A third_party/blink/renderer/core/loader/web_bundle/web_bundle_loader.cc
A third_party/blink/renderer/core/loader/web_bundle/web_bundle_loader.h
M third_party/blink/renderer/core/script/script_loader.cc
M third_party/blink/renderer/core/script/script_loader.h
M third_party/blink/renderer/core/script/xml_parser_script_runner.cc
M third_party/blink/renderer/platform/loader/fetch/subresource_web_bundle.h
A third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-subresource-load.https.tentative.html
21 files changed, 733 insertions(+), 131 deletions(-)
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Attention is currently required from: Kouhei Ueno, Kunihiko Sakamoto, Hiroshige Hayashizaki, Tsuyoshi Horo.
2 comments:
Patchset:
PTAL. This is just the first step to switch to <script>-based API.
File third_party/blink/renderer/core/loader/web_bundle/web_bundle_loader.h:
Patch Set #10, Line 1:
// Copyright 2021 The Chromium Authors. All rights reserved.WebBundleLoader is almost mechanically extracted from link_web_bundle.cc.
There should be no diff practically.WebBundleLoader is now used from both LinkWebBundle and ScriptWebBundle.
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Attention is currently required from: Kouhei Ueno, Kunihiko Sakamoto, Hiroshige Hayashizaki, Tsuyoshi Horo.
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/30816.
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
Hiroshige Hayashizaki (Gerrit)
Attention is currently required from: Kouhei Ueno, Hayato Ito, Kunihiko Sakamoto, Tsuyoshi Horo.
2 comments:
Patchset:
Changes in html_script_element and script_loader are to be blocked by spec side discussion (https://docs.google.com/document/d/1GEJ3wTERGEeTG_4J0QtAwaNXhPTza0tedd00A7vPVsw/edit). I'm starting the discussion there.
Also, I heard that the motiviation of this transition to <script> is to improve security.
Is there any list of security checks to be applied, cases to be blocked etc?
Should we ensure that all security checks are applied also to <script webbundle> as the same level of normal classic <script>? (I assume yes)
This might be helpful to review this CL and develop the spec concretely.
File third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-subresource-load.https.tentative.html:
Patch Set #10, Line 1:
<!DOCTYPE html>As for WPTs, I think we should add:
- Tests to confirm that there are no security bugs affecting ordinal scripts. e.g. if we modify the type attribute to "text/javascript" and textContent of <script type="webbundle"> after it is processed, is the script is evaluated as classic script? (It shouldn't be, and it isn't in the current patch set)
- Tests to confirm that webbundle obeys the same security checks as ordinal scripts. Maybe CSP? (in the current patch set updated texts to <script type=webbundle> bypasses CSP)
anyway I first focuses on the spec.
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hiroshige Hayashizaki (Gerrit)
Attention is currently required from: Kouhei Ueno, Hayato Ito, Kunihiko Sakamoto, Tsuyoshi Horo.
1 comment:
Patchset:
Changes in html_script_element and script_loader are to be blocked by spec side discussion (https:// […]
Note that this doesn't block codereview of other files.
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Attention is currently required from: Kouhei Ueno, Kunihiko Sakamoto, Hiroshige Hayashizaki, Tsuyoshi Horo.
2 comments:
Patchset:
Thanks! I understand that html_script_element and script_loader are to be blocked by spec side discussion. Let's discuss there.
Also, I heard that the motiviation of this transition to <script> is to improve security. Is there any list of security checks to be applied, cases to be blocked etc?
I've not heard of that. What I can share is https://github.com/WICG/webpackage/issues/580. Maybe we want to involve ko...@google.com in the spec discussion.
Should we ensure that all security checks are applied also to <script webbundle> as the same level of normal classic <script>? (I assume yes)
I assume yes, however, I'd let security folks judge it. I'm afraid I don't have a right answer.
> Note that this doesn't block codereview of other files.
Thanks. I might split this CL into several CLs so that this CL can focus on script_loader and html_script_element, though I've not decided yet.
File third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-subresource-load.https.tentative.html:
Patch Set #10, Line 1:
<!DOCTYPE html>
As for WPTs, I think we should add: […]
Thanks. It seems we'd first focuses on the spec.
Unless that, it'd be difficult to write a WPT.
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Attention is currently required from: Hayato Ito, Hiroshige Hayashizaki.
Hayato Ito uploaded patch set #15 to this change.
Introduce <script type=webbundle>
Introduce <script>-based API for subresource loading with Web Bundles.
See the design doc [1] for the motivation of switching from
<link>-based API to <script>-based API. The explainer [2] was already
updated to use <script>-based API.
This feature is guarded by `SubresourceWebBundles` flag.
We eventually drop the <link rel=webbundle> support and remove the
<link>-based API code once we can confirm <script>-based API can be
used as a replacement of <link>-based API.
This CL should be considered as the first step to switch to
<script>-based API. There are still gaps between <link>-based API and
<script>-based API, which will be addressed later [3].
This CL intentionally adds a very minimum test for <script>-based API
because there are already WPT tests for <script type=webbundle>. They
all have been marked as [ SKIP ] in TestExpectations until now. Now
some of them are passing after this CL.
We'll make the remaining tests pass in follow-up CLs, and also add
tests which are specific to <script>-based API as necessary. These
efforts should be tracked by crbug.com/1245166.
[1]: https://docs.google.com/document/d/1q_SodTcLuwya4cXt1gIRaVrkiaBfwWyPvkY1fqRKkgM/edit?usp=sharing&resourcekey=0-dqrFOGVCYsg8WRZ4RFgwuw
[2]: https://github.com/WICG/webpackage/blob/main/explainers/subresource-loading.md
[3]: https://github.com/WICG/webpackage/issues/670
Bug: 1245166
Change-Id: I5109b6e692baf10fd1d8a31a31d93176d4dc4ad2
---
M third_party/blink/renderer/core/html/link_web_bundle.cc
M third_party/blink/renderer/core/script/script_type_names.json5
A third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-subresource-load.https.tentative.html
M third_party/blink/renderer/platform/loader/fetch/subresource_web_bundle_list.cc
A third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-replace.https.tentative.html
M third_party/blink/renderer/core/html/parser/html_preload_scanner.cc
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.h
M third_party/blink/web_tests/TestExpectations
M third_party/blink/renderer/core/script/xml_parser_script_runner.cc
M third_party/blink/renderer/core/BUILD.gn
M third_party/blink/renderer/core/html/html_script_element.h
M third_party/blink/renderer/core/html/link_web_bundle.h
M third_party/blink/renderer/platform/loader/fetch/subresource_web_bundle.h
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule.cc
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule_test.cc
A third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-supports-webbundle.https.tentative.html
M third_party/blink/renderer/core/loader/build.gni
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule.h
M third_party/blink/renderer/core/script/script_loader.h
M third_party/blink/renderer/core/html/html_script_element.cc
M third_party/blink/renderer/core/script/script_loader.cc
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.cc
M third_party/blink/renderer/platform/loader/fetch/subresource_web_bundle_list.h
23 files changed, 664 insertions(+), 12 deletions(-)
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Attention is currently required from: Hiroshige Hayashizaki.
1 comment:
Patchset:
hiroshige@, I've updated the CL so it uses a microtask-based approach, as we discussed.
Could you please review this and give us feedback?
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hiroshige Hayashizaki (Gerrit)
Attention is currently required from: Kouhei Ueno, Hayato Ito, Kunihiko Sakamoto, Tsuyoshi Horo.
9 comments:
Patchset:
The direction looks good. Thanks!
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.cc:
Patch Set #18, Line 40:
ScriptWebBundle* ScriptWebBundle::CreateInline(ScriptElementBase& owner,Could you pass `Document& element_document` instead of `owner`?
It clarifies that ScriptWebBundle works on the element document and don't need other aspects of a script element. (Also reduces dependencies to ScriptElementBase from outside core/script)Patch Set #18, Line 41:
const String& inline_text) {nit: a name like `CreateOrReuseInline()` might be better?
Patch Set #18, Line 42:
inline_textnit: the name `source_text` might be more consistent with existing code using inline text.
Patch Set #18, Line 149:
ResourceFetcher* resource_fetcher = owner.GetDocument().Fetcher();This can be stale, e.g. when this is called after the script element is moved between Documents.
Probably it is better to have (Weak)Member<ResourceFetcher> in ScriptWebBundle?
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule.h:
Patch Set #18, Line 19:
class CORE_EXPORT ScriptWebBundleRule {nit: final?
File third_party/blink/renderer/core/script/script_loader.h:
Patch Set #18, Line 123:
void ReleaseResource();It's better to name this more webbundle-specific (e.g. `ReleaseWebBundleResource`), as currently this is only for web bundle, and there are no plans to generalize this mechanism.
File third_party/blink/renderer/core/script/xml_parser_script_runner.cc:
Patch Set #18, Line 97:
"webbundle script in XML documents are currently not supported."));I think actually webbundle script is supported, because it is already processed inside PrepareScript().
File third_party/blink/renderer/platform/loader/fetch/subresource_web_bundle_list.cc:
Patch Set #18, Line 44:
if (it->WillBeReleased() && it->GetBundleUrl() == bundle_url)Cross origin attribute (in other words credentials mode of the bundle) should also be checked to avoid matching web bundles with the same URL but different credentials mode.
However, in the current CL this is only used for ScriptWebBundle and ScriptWebBundle always use `kCrossOriginAttributeNotSet` so this doesn't affect observable behavior.Is my understanding above correct?
Is ScriptWebBundle going to support cross origin attributes other than `kCrossOriginAttributeNotSet`?
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Attention is currently required from: Kouhei Ueno, Kunihiko Sakamoto, Hiroshige Hayashizaki, Tsuyoshi Horo.
9 comments:
Patchset:
Thanks for the review and glad to hear the direction looks good!
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.cc:
Patch Set #18, Line 40:
ScriptWebBundle* ScriptWebBundle::CreateInline(ScriptElementBase& owner,
Could you pass `Document& element_document` instead of `owner`? […]
Sure. I should have done it. Done.
Patch Set #18, Line 41:
const String& inline_text) {nit: a name like `CreateOrReuseInline()` might be better?
Sounds better. Done.
Patch Set #18, Line 42:
inline_textnit: the name `source_text` might be more consistent with existing code using inline text.
Done
Patch Set #18, Line 149:
ResourceFetcher* resource_fetcher = owner.GetDocument().Fetcher();
This can be stale, e.g. when this is called after the script element is moved between Documents. […]
Ah, good point!
We are now using Document, instead of ScriptElementBase. Thus, this is no longer an issue? A microtask is now holding a Document from where the element was removed.
I appreciate if you could do double-check if that's fine.
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule.h:
Patch Set #18, Line 19:
class CORE_EXPORT ScriptWebBundleRule {nit: final?
Done
File third_party/blink/renderer/core/script/script_loader.h:
Patch Set #18, Line 123:
void ReleaseResource();
It's better to name this more webbundle-specific (e.g. […]
Sounds good! Done. I've updated the comment too.
File third_party/blink/renderer/core/script/xml_parser_script_runner.cc:
Patch Set #18, Line 97:
"webbundle script in XML documents are currently not supported."));
I think actually webbundle script is supported, because it is already processed inside PrepareScript […]
I see. Let's revert this. Thanks!
File third_party/blink/renderer/platform/loader/fetch/subresource_web_bundle_list.cc:
Patch Set #18, Line 44:
if (it->WillBeReleased() && it->GetBundleUrl() == bundle_url)
Is ScriptWebBundle going to support cross origin attributes other than `kCrossOriginAttributeNotSet`?
Yes, that's one of the requirements, but this CL doesn't support yet.
Our current plan is to add crossorign support in follow-up CLs.I've added TODO comment here. Thanks!
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Kouhei Ueno (Gerrit)
Attention is currently required from: Hayato Ito, Kunihiko Sakamoto, Hiroshige Hayashizaki, Tsuyoshi Horo.
Patch set 20:Code-Review +1
5 comments:
Patchset:
rough scan only - but didn't see red signals.
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.h:
Patch Set #20, Line 36:
ScriptWebBundle(Document& element_document, ScriptWebBundleRule&& rule);ScriptWebBundleRule&& -> ScriptWebBundleRule
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule.h:
Patch Set #20, Line 26:
HashSet<KURL>&& scope_urls,Omit && per style guide: https://google.github.io/styleguide/cppguide.html#Rvalue_references
We can simply move them twice.
File third_party/blink/renderer/platform/loader/fetch/subresource_web_bundle_list.h:
Patch Set #20, Line 30:
const KURL bundle_url) const;const KURL&
File third_party/blink/renderer/platform/loader/fetch/subresource_web_bundle_list.cc:
Patch Set #20, Line 9:
platform/wtf/casting.hAccidental addition?
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hiroshige Hayashizaki (Gerrit)
Attention is currently required from: Hayato Ito, Kunihiko Sakamoto, Tsuyoshi Horo.
2 comments:
Patchset:
3 WPT tests should be added:
- <script webbundle> is replaced with another one using replaceWith(),
- <script webbundle> is replaced with another one using remove() and then appendChild(), and
- <script webbundle> is moved to another Document (which should behave in the same way as remove(), i.e. web bundle is immediately removed from the old document and not added to the new document)
File third_party/blink/renderer/core/html/link_web_bundle.cc:
Patch Set #20, Line 70:
return true;IsScriptWebBundle() is true here (and thus is always true).
Is it intentional?Anyway IIUC this code is never reached if the implementation is correct, right?
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hiroshige Hayashizaki (Gerrit)
Attention is currently required from: Hayato Ito, Kunihiko Sakamoto, Tsuyoshi Horo.
1 comment:
Patchset:
As for `core/loader/web_bundle` I just briefly looked at the code and thus another look by a webbundle reviewer is preferrable.
The rest of the code basically look good (some minor issues might be remaining...checking)
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hiroshige Hayashizaki (Gerrit)
Attention is currently required from: Hayato Ito, Kunihiko Sakamoto, Tsuyoshi Horo.
2 comments:
Patchset:
Thanks! I understand that html_script_element and script_loader are to be blocked by spec side discu […]
Ack
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.cc:
Patch Set #18, Line 149:
ResourceFetcher* resource_fetcher = owner.GetDocument().Fetcher();Ah, good point! […]
Hmm,
[A] ScriptElementBase::GetDocument() at the time of PrepareScript()
[B] ScriptElementBase::GetDocument() at the time of `Node.RemovedFrom()` when the element is moved to another Document (or is removed).
[C] ScriptElementBase::GetDocument() at the time of the microtask execution.The current patch set seems to work, and the staleness of the previous patch set was fixed, because [A] == [B] != [C].
However, I feel storing a reference to [A] explicitly is better to ensure we refer to the same Document/ResourceFetcher both at the time of registration and unregistration.
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Attention is currently required from: Kunihiko Sakamoto, Hiroshige Hayashizaki, Tsuyoshi Horo.
8 comments:
Patchset:
3 WPT tests should be added: […]
Sure. Since we are going to have a spec for these behaviors, it would make sense to have WPT tests. Done.
Patchset:
Thanks for the reviews!
File third_party/blink/renderer/core/html/link_web_bundle.cc:
Patch Set #20, Line 70:
return true;
IsScriptWebBundle() is true here (and thus is always true). […]
That was a mistake. :( Sorry for that. That should be false. Fixed.
Because this code is never reached, let's also add NOTREACHED() here.
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.h:
Patch Set #20, Line 36:
ScriptWebBundle(Document& element_document, ScriptWebBundleRule&& rule);ScriptWebBundleRule&& -> ScriptWebBundleRule
Done
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.cc:
Patch Set #18, Line 149:
ResourceFetcher* resource_fetcher = owner.GetDocument().Fetcher();
Hmm, […]
I agree. Now ScriptWebBundle has a WeakMember<Document>, which is a reference to <A>. This would make our implementation robust and simpler. Thanks!
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule.h:
Patch Set #20, Line 26:
HashSet<KURL>&& scope_urls,
Omit && per style guide: https://google.github.io/styleguide/cppguide.html#Rvalue_references […]
Done. It seems this is no exception case to use &&. Just copy would be enough.
File third_party/blink/renderer/platform/loader/fetch/subresource_web_bundle_list.h:
Patch Set #20, Line 30:
const KURL bundle_url) const;const KURL&
Done
File third_party/blink/renderer/platform/loader/fetch/subresource_web_bundle_list.cc:
Patch Set #20, Line 9:
platform/wtf/casting.hAccidental addition?
Removed. I forgot to remove this.
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Kouhei Ueno (Gerrit)
Attention is currently required from: Hayato Ito, Kunihiko Sakamoto, Hiroshige Hayashizaki, Tsuyoshi Horo.
Patch set 21:Code-Review +1
1 comment:
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule.h:
Patch Set #20, Line 26:
HashSet<KURL>&& scope_urls,
Done. It seems this is no exception case to use &&. Just copy would be enough.
To clarify, HashSet<KURL> (not HashSet<KURL>&) should do the move, so it should be faster.
It should move construct the argument value, and then the value should be moved again to the member variable.
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Attention is currently required from: Kunihiko Sakamoto, Hiroshige Hayashizaki, Tsuyoshi Horo.
2 comments:
Patchset:
Thanks for the review. It seems I misunderstood the review comment.
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule.h:
Patch Set #20, Line 26:
HashSet<KURL>&& scope_urls,
To clarify, HashSet<KURL> (not HashSet<KURL>&) should do the move, so it should be faster. […]
Thanks! I got the intention.
Now the CL uses HashSet<KURL> (and uses std::move to save to the member variable) in the callee, and use std::move(scope_urls) in the caller.
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Attention is currently required from: Kunihiko Sakamoto, Hiroshige Hayashizaki, Tsuyoshi Horo.
1 comment:
Patchset:
As for `core/loader/web_bundle` I just briefly looked at the code and thus another look by a webbund […]
kasakamoto@, could you please start to review this CL, especially on webbundle related parts?
I think it is fine for webbundle folks to start to review this CL now.
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Kunihiko Sakamoto (Gerrit)
Attention is currently required from: Hayato Ito, Hiroshige Hayashizaki, Tsuyoshi Horo.
7 comments:
File content/browser/web_package/script_web_bundle_browsertest.cc:
Patch Set #23, Line 41:
SetBrowserClientForTesting(&browser_client_);We should save the returned value (old client) and restore it in TearDownOnMainThread().
nit: Add a semicolon.
Patch Set #23, Line 176:
const text = await response.text()ditto.
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.cc:
Patch Set #23, Line 86:
if (url.Protocol() == "urn")Please add
|| url.Protocol() == "uuid-in-package"
Patch Set #23, Line 135:
kCrossOriginAttributeNotSetShould we have a TODO comment about the request / credentials mode of bundle requests?
nit: extra whitespace
File third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-reuse-web-bundle-resource.https.tentative.html:
Patch Set #23, Line 34:
async function countWebBundleResourceEntry() {This looks tricky. Since this uses PerformanceObserver, this returns a promise that resolves:
- 1, if the resource loaded next is "subresource.wbn",
- 0, otherwise.
It does not resolve until some resource load occurs, and does not care subsequent loads after the first observer function invocation.
If we want to count resource loads, we can use something like this:
performance.clearResourceTimings();
// ... some operations that may load subresource.wbn ...
count = performance.getEntriesByType('resource').filter(e => e.name.endsWith("subresource.wbn")).length;
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Attention is currently required from: Kunihiko Sakamoto, Hiroshige Hayashizaki, Tsuyoshi Horo.
8 comments:
Patchset:
Thanks for the review!
File content/browser/web_package/script_web_bundle_browsertest.cc:
Patch Set #23, Line 41:
SetBrowserClientForTesting(&browser_client_);We should save the returned value (old client) and restore it in TearDownOnMainThread().
Done
nit: Add a semicolon.
Done
Patch Set #23, Line 176:
const text = await response.text()ditto.
Done
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.cc:
Patch Set #23, Line 86:
if (url.Protocol() == "urn")
Please add […]
Done. Now external/wpt/web-bundle/subresource-loading/script-subframe-from-web-bundle.https.tentative.html is passing.
Patch Set #23, Line 135:
kCrossOriginAttributeNotSetShould we have a TODO comment about the request / credentials mode of bundle requests?
Done. I remember I added TODO comment in the past, however, it seems I've removed it...
nit: extra whitespace
Done
File third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-reuse-web-bundle-resource.https.tentative.html:
Patch Set #23, Line 34:
async function countWebBundleResourceEntry() {
This looks tricky. Since this uses PerformanceObserver, this returns a promise that resolves: […]
Thanks! I didn't know that. Done.
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Kunihiko Sakamoto (Gerrit)
Attention is currently required from: Hayato Ito, Hiroshige Hayashizaki.
Patch set 24:Code-Review +1
1 comment:
Patchset:
lgtm
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Attention is currently required from: Hiroshige Hayashizaki.
2 comments:
Patchset:
kasakamoto@, could you please start to review this CL, especially on webbundle related parts? […]
Mark this as Resolved.
File third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-subresource-load.https.tentative.html:
Patch Set #10, Line 1:
<!DOCTYPE html>
Thanks. It seems we'd first focuses on the spec. […]
We no longer modify textContext. Thus we don't need any action?
If we need a follow-up task, please let us know that.
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hiroshige Hayashizaki (Gerrit)
Attention is currently required from: Hayato Ito.
Patch set 24:Code-Review +1
10 comments:
Patchset:
LGTM % minor comments in the code.
The comments about tests can be done in follow-up CLs.
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.cc:
Patch Set #24, Line 59:
ScriptWebBundle* reused_script_web_bundle = To<ScriptWebBundle>(found);DCHECK_EQ(reused_script_web_bundle->element_document_, element_document);
File third_party/blink/renderer/core/script/script_loader.cc:
Patch Set #24, Line 764:
element_->GetDocument()`element_document`
File third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-reuse-web-bundle-resource.https.tentative.html:
Patch Set #24, Line 1:
<meta charset="utf-8" />Add `<!doctype html>` at the top to avoid quirks mode.
Patch Set #24, Line 84:
}, "A webbundle should be fetched again when new script element is appended.");Could you also test that if we `appendChild(script1)` instead of `script2` the resource (`root.js`) is not loaded from the webbundle?
Could you also test that even if we remove/add the same script element multiple times nothing wrong happens. (Multiple microtasks would be posted, but they are ignored except for the first one)
Patch Set #24, Line 99:
}, "'remove(), then append()' should reuse webbundle resources");Could you also test that if we remove()/append() in separate tasks e.g. by doing `await new Promise(resolve => t.step_timeout(resolve, 0))` after `script1.remove();` then the resource is not reused?
Patch Set #24, Line 123:
another_document.append(script1);Could you also test that
- The resources are not loaded from the webbundle in the new Document (Probably better to use a <iframe>.contentDocument)
- Even if we move the script element back to the original Document, even immediately, the resources are not loaded from the webbundle in the original Document.
File third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-subresource-load.https.tentative.html:
Patch Set #10, Line 1:
<!DOCTYPE html>
Tests to confirm that webbundle obeys the same security checks as ordinal scripts. Maybe CSP?
This is still valid, but can be done in a follow-up CL (because now the changes around script element is much more straightforward).
File third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-supports-webbundle.https.tentative.html:
Patch Set #24, Line 13:
assert_false(HTMLScriptElement.supports('webbundle '));Also 'WEBBUNDLE'
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Patch set 26:Commit-Queue +2
9 comments:
Patchset:
Thanks for the reviews! I appreciate that.
I'm going to land the CL.
File third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.cc:
Patch Set #24, Line 59:
ScriptWebBundle* reused_script_web_bundle = To<ScriptWebBundle>(found);DCHECK_EQ(reused_script_web_bundle->element_document_, element_document);
Done
File third_party/blink/renderer/core/script/script_loader.cc:
Patch Set #24, Line 764:
element_->GetDocument()`element_document`
Done
File third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-reuse-web-bundle-resource.https.tentative.html:
Patch Set #24, Line 1:
<meta charset="utf-8" />Add `<!doctype html>` at the top to avoid quirks mode.
Done
Patch Set #24, Line 84:
}, "A webbundle should be fetched again when new script element is appended.");
Could you also test that if we `appendChild(script1)` instead of `script2` the resource (`root. […]
Sure. Done.
Could you also test that even if we remove/add the same script element multiple times nothing wrong […]
Done
Patch Set #24, Line 99:
}, "'remove(), then append()' should reuse webbundle resources");
Could you also test that if we remove()/append() in separate tasks e.g. […]
Done
Patch Set #24, Line 123:
another_document.append(script1);
Could you also test that […]
Thanks. Let me test that in a follow-up CL. I've added TODO comment.
File third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-supports-webbundle.https.tentative.html:
Patch Set #24, Line 13:
assert_false(HTMLScriptElement.supports('webbundle '));Also 'WEBBUNDLE'
Done
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Tsuyoshi Horo (Gerrit)
Attention is currently required from: Hayato Ito.
Patch set 26:Code-Review +1
2 comments:
Patchset:
lgtm with a nit/
Sorry for late review.
File content/browser/web_package/OWNERS:
Patch Set #26, Line 3:
hay...@chromium.orgnit: Maybe you should change OWNERS in a different CL, just incase for reverting.
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Hayato Ito (Gerrit)
Patch set 27:Commit-Queue +2
1 comment:
File content/browser/web_package/OWNERS:
Patch Set #26, Line 3:
hay...@chromium.orgnit: Maybe you should change OWNERS in a different CL, just incase for reverting.
That makes sense. Let me change OWNERS in a different CL. Thanks!
To view, visit change 3128843. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
26 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: content/browser/web_package/OWNERS
Insertions: 0, Deletions: 1.
@@ -1,5 +1,4 @@
kin...@chromium.org
kou...@chromium.org
-hay...@chromium.org
ho...@chromium.org
ksak...@chromium.org
```
Introduce <script type=webbundle>
Introduce <script>-based API for subresource loading with Web Bundles.
See the design doc [1] for the motivation of switching from
<link>-based API to <script>-based API. The explainer [2] was already
updated to use <script>-based API.
This feature is guarded by `SubresourceWebBundles` flag.
We eventually drop the <link rel=webbundle> support and remove the
<link>-based API code once we can confirm <script>-based API can be
used as a replacement of <link>-based API.
This CL should be considered as the first step to switch to
<script>-based API. There are still gaps between <link>-based API and
<script>-based API, which will be addressed later [3].
This CL intentionally adds a very minimum test for <script>-based API
because there are already WPT tests for <script type=webbundle>. They
all have been marked as [ SKIP ] in TestExpectations until now. Now
some of them are passing after this CL.
We'll make the remaining tests pass in follow-up CLs, and also add
tests which are specific to <script>-based API as necessary. These
efforts should be tracked by crbug.com/1245166.
[1]: https://docs.google.com/document/d/1q_SodTcLuwya4cXt1gIRaVrkiaBfwWyPvkY1fqRKkgM/edit?usp=sharing&resourcekey=0-dqrFOGVCYsg8WRZ4RFgwuw
[2]: https://github.com/WICG/webpackage/blob/main/explainers/subresource-loading.md
[3]: https://github.com/WICG/webpackage/issues/670
Bug: 1245166
Change-Id: I5109b6e692baf10fd1d8a31a31d93176d4dc4ad2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3128843
Commit-Queue: Hayato Ito <hay...@chromium.org>
Reviewed-by: Tsuyoshi Horo <ho...@chromium.org>
Reviewed-by: Kunihiko Sakamoto <ksak...@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiro...@chromium.org>
Reviewed-by: Kouhei Ueno <kou...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#933346}
---
M third_party/blink/renderer/core/html/link_web_bundle.cc
M third_party/blink/renderer/core/script/script_type_names.json5
M third_party/blink/renderer/platform/loader/fetch/subresource_web_bundle_list.cc
M third_party/blink/renderer/core/html/parser/html_preload_scanner.cc
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.h
M third_party/blink/web_tests/TestExpectations
A content/browser/web_package/script_web_bundle_browsertest.cc
M third_party/blink/renderer/core/BUILD.gn
M third_party/blink/renderer/core/html/html_script_element.h
M third_party/blink/renderer/core/html/link_web_bundle.h
A third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-reuse-web-bundle-resource.https.tentative.html
M content/test/BUILD.gn
M third_party/blink/renderer/platform/loader/fetch/subresource_web_bundle.h
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule.cc
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule_test.cc
A third_party/blink/web_tests/external/wpt/web-bundle/subresource-loading/script-supports-webbundle.https.tentative.html
M third_party/blink/renderer/core/loader/build.gni
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle_rule.h
M third_party/blink/renderer/core/script/script_loader.h
M third_party/blink/renderer/core/html/html_script_element.cc
M third_party/blink/renderer/core/script/script_loader.cc
A third_party/blink/renderer/core/loader/web_bundle/script_web_bundle.cc
M third_party/blink/renderer/platform/loader/fetch/subresource_web_bundle_list.h
23 files changed, 1,011 insertions(+), 12 deletions(-)
Blink WPT Bot (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/30816