Load modules off-main-thread [chromium/src : master]

6 views

Skip to first unread message

Nate Chapin (Gerrit)

unread,

Apr 26, 2018, 3:28:06 PM4/26/18

to Tsuyoshi Horo, Hiroshige Hayashizaki, Hiroki Nakagawa, Matt Falkenhagen, Kinuko Yasuda, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Nate Chapin would like Tsuyoshi Horo, Hiroshige Hayashizaki, Hiroki Nakagawa, Matt Falkenhagen and Kinuko Yasuda to review this change.

Load modules off-main-thread

* Merge DocumentModuleScriptFetcher into ModuleScriptFetcher and use
  it for documents and workers.
* Make WorkletModuleResponsesMap a worklet-thread-only object, and
  have it subclass ModuleScriptFetcher. It maintains its caching/proxy
  functionality while using the same API as ModuleScriptFetcher.
* With the above changes, no cross-thread module loading logic is needed
  for workers or worklets. Delete WorkerOrWorkletModuleScriptFetcher,
  WorkerModuleFetchCoordinator, and WorkerOrWorkletModuleFetchCoordinator.
* Plumb content settings state to workers so AllowScriptFromSource()
  works.


Change-Id: I4c6e8740d69a96a37711ff256417a5357586ae90
---
M chrome/renderer/content_settings_observer.cc
M chrome/renderer/content_settings_observer.h
M chrome/renderer/worker_content_settings_client.cc
M chrome/renderer/worker_content_settings_client.h
M third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/worklet-import-blocked-expected.txt
M third_party/WebKit/LayoutTests/http/tests/worklet/animation-worklet-csp-eval-expected.txt
M third_party/WebKit/LayoutTests/http/tests/worklet/layout-worklet-csp-eval-expected.txt
M third_party/WebKit/LayoutTests/http/tests/worklet/paint-worklet-csp-eval-expected.txt
M third_party/WebKit/LayoutTests/http/tests/worklet/resources/addmodule-window-with-unsafe-eval.html
M third_party/WebKit/LayoutTests/http/tests/worklet/resources/addmodule-window.html
M third_party/blink/renderer/core/BUILD.gn
M third_party/blink/renderer/core/exported/local_frame_client_impl.cc
M third_party/blink/renderer/core/exported/local_frame_client_impl.h
M third_party/blink/renderer/core/exported/web_shared_worker_impl.cc
M third_party/blink/renderer/core/frame/local_frame_client.h
M third_party/blink/renderer/core/layout/custom/layout_worklet.cc
M third_party/blink/renderer/core/layout/custom/layout_worklet_global_scope_proxy.cc
M third_party/blink/renderer/core/layout/custom/layout_worklet_global_scope_proxy.h
M third_party/blink/renderer/core/loader/BUILD.gn
M third_party/blink/renderer/core/loader/frame_loader.cc
M third_party/blink/renderer/core/loader/frame_loader.h
D third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.cc
D third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.h
M third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.cc
M third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.h
M third_party/blink/renderer/core/loader/modulescript/module_script_loader.cc
M third_party/blink/renderer/core/loader/modulescript/module_script_loader_test.cc
D third_party/blink/renderer/core/loader/modulescript/worker_or_worklet_module_script_fetcher.cc
D third_party/blink/renderer/core/loader/modulescript/worker_or_worklet_module_script_fetcher.h
M third_party/blink/renderer/core/loader/worker_fetch_context.cc
M third_party/blink/renderer/core/script/document_modulator_impl.cc
M third_party/blink/renderer/core/script/worker_modulator_impl.cc
M third_party/blink/renderer/core/script/worklet_modulator_impl.cc
M third_party/blink/renderer/core/script/worklet_modulator_impl.h
M third_party/blink/renderer/core/workers/BUILD.gn
M third_party/blink/renderer/core/workers/dedicated_worker.cc
M third_party/blink/renderer/core/workers/dedicated_worker.h
M third_party/blink/renderer/core/workers/global_scope_creation_params.cc
M third_party/blink/renderer/core/workers/global_scope_creation_params.h
M third_party/blink/renderer/core/workers/main_thread_worklet_test.cc
M third_party/blink/renderer/core/workers/threaded_worklet_messaging_proxy.cc
M third_party/blink/renderer/core/workers/threaded_worklet_messaging_proxy.h
M third_party/blink/renderer/core/workers/threaded_worklet_test.cc
M third_party/blink/renderer/core/workers/worker_content_settings_client.cc
M third_party/blink/renderer/core/workers/worker_content_settings_client.h
M third_party/blink/renderer/core/workers/worker_fetch_test_helper.h
M third_party/blink/renderer/core/workers/worker_global_scope.cc
M third_party/blink/renderer/core/workers/worker_global_scope.h
D third_party/blink/renderer/core/workers/worker_module_fetch_coordinator.cc
D third_party/blink/renderer/core/workers/worker_module_fetch_coordinator.h
D third_party/blink/renderer/core/workers/worker_module_fetch_coordinator_test.cc
M third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc
D third_party/blink/renderer/core/workers/worker_or_worklet_module_fetch_coordinator.h
D third_party/blink/renderer/core/workers/worker_or_worklet_module_fetch_coordinator_proxy.cc
D third_party/blink/renderer/core/workers/worker_or_worklet_module_fetch_coordinator_proxy.h
M third_party/blink/renderer/core/workers/worklet.cc
M third_party/blink/renderer/core/workers/worklet.h
M third_party/blink/renderer/core/workers/worklet_global_scope.cc
M third_party/blink/renderer/core/workers/worklet_global_scope.h
M third_party/blink/renderer/core/workers/worklet_module_responses_map.cc
M third_party/blink/renderer/core/workers/worklet_module_responses_map.h
M third_party/blink/renderer/modules/animationworklet/animation_worklet.cc
M third_party/blink/renderer/modules/animationworklet/animation_worklet_global_scope_test.cc
M third_party/blink/renderer/modules/animationworklet/animation_worklet_thread_test.cc
M third_party/blink/renderer/modules/csspaint/paint_worklet.cc
M third_party/blink/renderer/modules/csspaint/paint_worklet_global_scope_proxy.cc
M third_party/blink/renderer/modules/csspaint/paint_worklet_global_scope_proxy.h
M third_party/blink/renderer/modules/exported/web_embedded_worker_impl.cc
M third_party/blink/renderer/modules/webaudio/audio_worklet.cc
M third_party/blink/renderer/modules/webaudio/audio_worklet_global_scope_test.cc
M third_party/blink/renderer/modules/webaudio/audio_worklet_thread_test.cc
M third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
72 files changed, 338 insertions(+), 1,116 deletions(-)

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

Apr 26, 2018, 3:28:07 PM4/26/18

to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Hiroki Nakagawa, Tsuyoshi Horo, Matt Falkenhagen, Kinuko Yasuda, Hiroshige Hayashizaki, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

This patch is big, but it's *mostly* mechanical, so hopefully it's ok as a monolithic single CL?

I tried to highly the non-trivial stuff in the commit message and review comments.

10 comments:

File third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/worklet-import-blocked-expected.txt:
- Patch Set #9, Line 3: CONSOLE ERROR: Refused to load the script 'http://127.0.0.1:8000/resources/worklet.js' because it violates the following Content Security Policy directive: "script-src 'unsafe-inline'".
  Should I be worried about this additional console error?
File third_party/WebKit/LayoutTests/http/tests/worklet/resources/addmodule-window.html:
- Patch Set #9, Line 5: <meta http-equiv="Content-Security-Policy" content="worker-src 'self'; script-src * 'unsafe-inline'">
  Here and in addmodule-window-with-unsafe-eval.html...
  Just using script-src here causes the addModule() promise to be rejected. I'm not sure why this worked before. Similar functionality in wpt worklet tests (https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/wpt/worklets/resources/csp-tests.js?sq=package:chromium&l=39) results in a rejected promise.
  I'm not familiar enough with CSP handling to know whether I created a bug or uncovered an existing bug.
File third_party/blink/renderer/core/frame/local_frame_client.h:
- Patch Set #9, Line 415: CreateWorkerContentSettingsClient() {
  These are plumbed through LocalFrameClient rather than depending directly on WebFrameClient because doing so requires the caller to include web_local_frame_impl.h, which is DEPS-forbidden.
File third_party/blink/renderer/core/loader/frame_loader.cc:
- Patch Set #9, Line 1761: ExecutionContext* origin_context) {
  Nothing in this function requires a Document, and we need to be able to do it from a worker/worklet scope.
  Also, this seems like it should be on FrameLoader.
File third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.cc:
- Patch Set #9, Line 17: bool WasModuleLoadSuccessful(
  This function should be a pure copy from document_module_script_fetcher.cc
File third_party/blink/renderer/core/loader/worker_fetch_context.cc:
- Patch Set #9, Line 120: // Settings::GetScriptEnabled() here.
  I think this comment is correct? Please sanity check.
- Patch Set #9, Line 288: FrameLoader::UpgradeInsecureRequest(request, global_scope_);
  Is this a reasonable place to do this?
File third_party/blink/renderer/core/script/worklet_modulator_impl.cc:
- Patch Set #9, Line 26: if (!module_responses_map_) {
  Is this a reasonable place to own the WorkletModuleResponsesMap?
File third_party/blink/renderer/core/workers/worklet_module_responses_map.h:
- Patch Set #9, Line 50:
  unused
File third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc:
- Patch Set #9, Line 442: if (IsMainThread()) {
  Because off-main-thread-fetch has mostly used RawResources prior to this CL, this block was never reached anywhere besides the main thread.

Hiroki Nakagawa (Gerrit)

unread,

Apr 26, 2018, 9:21:17 PM4/26/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Kouhei Ueno, Tsuyoshi Horo, Matt Falkenhagen, Kinuko Yasuda, Hiroshige Hayashizaki, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

+kouhei@ (main implementer of ES Modules)

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Hiroki Nakagawa (Gerrit)

unread,

Apr 26, 2018, 10:27:22 PM4/26/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Kouhei Ueno, Tsuyoshi Horo, Matt Falkenhagen, Kinuko Yasuda, Hiroshige Hayashizaki, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

4 comments:

File third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/worklet-import-blocked-expected.txt:

- Patch Set #9, Line 3: CONSOLE ERROR: Refused to load the script 'http://127.0.0.1:8000/resources/worklet.js' because it violates the following Content Security Policy directive: "script-src 'unsafe-inline'".
  Should I be worried about this additional console error?

- Yes. This should be fixed by sharing WorkletModuleResponsesMap among the global scopes. See my comment in worklet_modulator_impl.h
File third_party/WebKit/LayoutTests/http/tests/worklet/resources/addmodule-window.html:

- Patch Set #9, Line 5: <meta http-equiv="Content-Security-Policy" content="worker-src 'self'; script-src * 'unsafe-inline'">

- Here and in addmodule-window-with-unsafe-eval.html... […]
  I guess this worked before because Worklets used the resource fetcher associated with Document and fetched as a typical script. According to [1,2], our decision is to use 'script-src' for worklets, so we need to fix this before landing this CL.
  WorkletGlobalScope applies CSP directives to itself at [3] and they're referenced during the fetch [4]. I suspect worklet script fetch is treated as WebURLRequest::RequestContext::kRequestContextWorker and filtered by 'worker-src' instead of 'script-src' at [4]. Probably we should add kRequestContext*Worklet defined in the spec[5] and make sure they are filtered by 'script-src'.
  [1] https://chromium-review.googlesource.com/c/chromium/src/+/763169
  [2] https://github.com/w3c/webappsec-csp/issues/203
  [3] https://chromium.googlesource.com/chromium/src/+/5ed4537c67e7a0e38981dbde6a3971debcedbba5/third_party/blink/renderer/core/workers/worklet_global_scope.cc#55
  [4] https://chromium.googlesource.com/chromium/src/+/5ed4537c67e7a0e38981dbde6a3971debcedbba5/third_party/blink/renderer/core/loader/base_fetch_context.cc#140
  [5] https://fetch.spec.whatwg.org/#concept-request-destination
File third_party/blink/renderer/core/script/worklet_modulator_impl.cc:

- Patch Set #9, Line 26: if (!module_responses_map_) {
  Is this a reasonable place to own the WorkletModuleResponsesMap?

- By the spec[1], WorkletModuleResponsesMap is required to be shared among WorkletGlobalScopes (WorkletModulatorImpls) associated with the same parent document in order to share the identical module scripts among them.
  Probably `Worklet` class on the main thread primarily owns WorkletModuleResponsesMap, and passes a reference to the map to WorkletGlobalScope. Also, we need to make the map thread-safe using a lock.
  [1] https://drafts.css-houdini.org/worklets/#module-responses-map
File third_party/blink/renderer/core/workers/worklet_module_responses_map.h:

- Patch Set #9, Line 50:
  unused

- Good catch!

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Kouhei Ueno (Gerrit)

unread,

Apr 27, 2018, 12:30:14 AM4/27/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Hiroki Nakagawa, Tsuyoshi Horo, Matt Falkenhagen, Kinuko Yasuda, Hiroshige Hayashizaki, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

The high-level design lgtm. Simple code made me happy!
I'll defer to nhiroki@ for details.

Patch set 9:Code-Review +1

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Tsuyoshi Horo (Gerrit)

unread,

Apr 27, 2018, 1:49:15 AM4/27/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Kouhei Ueno, Hiroki Nakagawa, Matt Falkenhagen, Kinuko Yasuda, Hiroshige Hayashizaki, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

Patch Set 9: Code-Review+1

The high-level design lgtm. Simple code made me happy!
I'll defer to nhiroki@ for details.

I have a design question.

I think there was a plan to have a memory cache on the main thread to keep the worklet script byte code.
If I remember correctly, worklet is more light weight worker.
Many worklets will be executed on the a page.
So the cost of starting a worklet is important.

If the modules of worklets are loaded off-main-thread, the cost of loading modules will be non-negligible when many worklets are executed.

1 comment:

File third_party/blink/renderer/core/loader/worker_fetch_context.cc:

- Patch Set #9, Line 120: // Settings::GetScriptEnabled() here.
  I think this comment is correct? Please sanity check.

- Yes. Sounds good to me.

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Hiroki Nakagawa (Gerrit)

unread,

Apr 27, 2018, 3:26:50 AM4/27/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Hiroshige Hayashizaki, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

Patch Set 9:
(1 comment)

Patch Set 9: Code-Review+1
The high-level design lgtm. Simple code made me happy!
I'll defer to nhiroki@ for details.
I have a design question.
I think there was a plan to have a memory cache on the main thread to keep the worklet script byte code.
If I remember correctly, worklet is more light weight worker.
Many worklets will be executed on the a page.
So the cost of starting a worklet is important.
If the modules of worklets are loaded off-main-thread, the cost of loading modules will be non-negligible when many worklets are executed.

WorkletModuleResponsesMap class caches the worklet scripts as strings (not byte code). Before this CL, the map lives on the main thread and each global scope accesses it via PostTask to the main thread. After this CL, the map should still be primarily owned by the main thread, but its reference is distributed to the global scopes and they access the same map with the mutex lock.

See also my review comment:
https://chromium-review.googlesource.com/c/chromium/src/+/1026945/9/third_party/blink/renderer/core/script/worklet_modulator_impl.cc#26

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

Apr 27, 2018, 12:20:50 PM4/27/18

to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Hiroshige Hayashizaki, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

1 comment:

File third_party/blink/renderer/core/script/worklet_modulator_impl.cc:

- Patch Set #9, Line 26: if (!module_responses_map_) {

- By the spec[1], WorkletModuleResponsesMap is required to be shared among WorkletGlobalScopes (Workle […]
  Yeah, I totally misread how WorkletModuleResponsesMap was supposed to work, sorry for wasting your time. I'll try again :)

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Hiroshige Hayashizaki (Gerrit)

unread,

May 9, 2018, 2:32:13 PM5/9/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading-re...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

(Only related to thread-safety)

2 comments:

File third_party/blink/renderer/core/workers/worklet_module_responses_map.cc:
- Patch Set #10, Line 42: params_.emplace(*params);
  I expect String/KURL contained in |params| are copied to |params_| and thus the same StringImpl is referenced both from |params_| (which can be destructed in ~Entry() on non-main threads) and |params| (which will be destructed on the main thread) outside the mutex, which can cause a race condition.
  Tentative fix would be:
  params_.emplace(CrossThreadCopier<ModuleScriptCreationParams>::Copy(*params));
  I'm also trying another fix, but it seems to hit another subtleties in base::Bind().
  I'll add a comment when I figured out a solution, but anyway this doesn't block your implementation (the fix should be anyway quite local and won't affect overall design).
- Patch Set #10, Line 125: url
  Ditto.
  (If WorkletModuleResponsesMap is destructed on the main thread, then this doesn't cause bugs/crashes for now though)

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 16, 2018, 5:56:28 PM5/16/18

to Tsuyoshi Horo, Hiroshige Hayashizaki, Hiroki Nakagawa, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Yoav Weiss, chromium...@chromium.org, Hongchan Choi, Renée Wright, Raymond Toy, Xida Chen, Commit Bot, Alexis Menard, Eric Willigers, Thiago Farina, Kentaro Hara, Shane Stephens

Nate Chapin uploaded patch set #15 to this change.

Load modules off-main-thread

* Make WorkletModuleResponsesMap thread-safe. It is now created and
  owned by the worklet object on the main thread, and shared to
  worklet global scopes.
* Before this change, WorkerOrWorkletModuleScriptFetcher proxied
  fetches from worklet to the main thread. After this change,
  WorkletModuleScriptFetcher will check WorkletModuleResponsesMap and
  return a cached result if present. Otherwise, it will fetch on the
  current thread and cache the result in WorkletModuleResponsesMap on
  completion.
* Merge DocumentModuleScriptFetcher into ModuleScriptFetcher.
  WorkletModuleScriptFetcher still subclasses ModuleScriptFetcher, and
  defers to the ModuleScriptFetcher's fetching logic when
  WorkletModuleResponsesMap doesn't have a cache entry.


* With the above changes, no cross-thread module loading logic is
  needed for workers or worklets. Delete WorkerOrWorkletModuleScriptFetcher

  and WorkerOrWorkletModuleFetchCoordinator.


Change-Id: I4c6e8740d69a96a37711ff256417a5357586ae90
---
M third_party/blink/renderer/core/exported/local_frame_client_impl.cc
M third_party/blink/renderer/core/exported/local_frame_client_impl.h
M third_party/blink/renderer/core/frame/local_frame_client.h
M third_party/blink/renderer/core/layout/custom/layout_worklet_global_scope_proxy.cc


M third_party/blink/renderer/core/loader/BUILD.gn
M third_party/blink/renderer/core/loader/frame_loader.cc
M third_party/blink/renderer/core/loader/frame_loader.h
D third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.cc
D third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.h
M third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.cc
M third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.h
M third_party/blink/renderer/core/loader/modulescript/module_script_loader.cc
M third_party/blink/renderer/core/loader/modulescript/module_script_loader_test.cc
D third_party/blink/renderer/core/loader/modulescript/worker_or_worklet_module_script_fetcher.cc
D third_party/blink/renderer/core/loader/modulescript/worker_or_worklet_module_script_fetcher.h

A third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.cc
A third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.h


M third_party/blink/renderer/core/loader/worker_fetch_context.cc
M third_party/blink/renderer/core/script/document_modulator_impl.cc
M third_party/blink/renderer/core/script/worker_modulator_impl.cc
M third_party/blink/renderer/core/script/worklet_modulator_impl.cc

M third_party/blink/renderer/core/workers/BUILD.gn
M third_party/blink/renderer/core/workers/dedicated_worker.cc
M third_party/blink/renderer/core/workers/main_thread_worklet_test.cc
M third_party/blink/renderer/core/workers/threaded_worklet_messaging_proxy.cc
M third_party/blink/renderer/core/workers/threaded_worklet_test.cc
M third_party/blink/renderer/core/workers/worker_fetch_test_helper.h
M third_party/blink/renderer/core/workers/worker_global_scope.h
M third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc


D third_party/blink/renderer/core/workers/worker_or_worklet_module_fetch_coordinator_proxy.cc
D third_party/blink/renderer/core/workers/worker_or_worklet_module_fetch_coordinator_proxy.h
M third_party/blink/renderer/core/workers/worklet.cc

M third_party/blink/renderer/core/workers/worklet_global_scope.cc
M third_party/blink/renderer/core/workers/worklet_global_scope.h
M third_party/blink/renderer/core/workers/worklet_module_responses_map.cc
M third_party/blink/renderer/core/workers/worklet_module_responses_map.h

M third_party/blink/renderer/core/workers/worklet_module_responses_map_test.cc
M third_party/blink/renderer/modules/animationworklet/animation_worklet_global_scope_test.cc
M third_party/blink/renderer/modules/animationworklet/animation_worklet_thread_test.cc
M third_party/blink/renderer/modules/csspaint/paint_worklet_global_scope_proxy.cc
M third_party/blink/renderer/modules/webaudio/audio_worklet_global_scope_test.cc
M third_party/blink/renderer/modules/webaudio/audio_worklet_thread_test.cc
42 files changed, 476 insertions(+), 749 deletions(-)

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 16, 2018, 6:02:10 PM5/16/18

to Tsuyoshi Horo, Hiroshige Hayashizaki, Hiroki Nakagawa, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Yoav Weiss, chromium...@chromium.org, Hongchan Choi, Renée Wright, Raymond Toy, Xida Chen, Commit Bot, Alexis Menard, Eric Willigers, Thiago Farina, Kentaro Hara, Shane Stephens

Nate Chapin uploaded patch set #16 to this change.

Load worklet modules off-main-thread



* Make WorkletModuleResponsesMap thread-safe. It is now created and
  owned by the worklet object on the main thread, and shared to
  worklet global scopes.
* Before this change, WorkerOrWorkletModuleScriptFetcher proxied
  fetches from worklet to the main thread. After this change,
  WorkletModuleScriptFetcher will check WorkletModuleResponsesMap and
  return a cached result if present. Otherwise, it will fetch on the
  current thread and cache the result in WorkletModuleResponsesMap on
  completion.
* Merge DocumentModuleScriptFetcher into ModuleScriptFetcher.
  WorkletModuleScriptFetcher still subclasses ModuleScriptFetcher, and
  defers to the ModuleScriptFetcher's fetching logic when
  WorkletModuleResponsesMap doesn't have a cache entry.

* With the above changes, no cross-thread module loading logic is

  needed for workers or worklets. Delete WorkerOrWorkletModuleScriptFetcher
  and WorkerOrWorkletModuleFetchCoordinator.


Change-Id: I4c6e8740d69a96a37711ff256417a5357586ae90
---

M third_party/blink/renderer/core/exported/local_frame_client_impl.cc
M third_party/blink/renderer/core/exported/local_frame_client_impl.h
M third_party/blink/renderer/core/frame/local_frame_client.h
M third_party/blink/renderer/core/layout/custom/layout_worklet_global_scope_proxy.cc


M third_party/blink/renderer/core/loader/BUILD.gn
M third_party/blink/renderer/core/loader/frame_loader.cc
M third_party/blink/renderer/core/loader/frame_loader.h
D third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.cc
D third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.h
M third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.cc
M third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.h
M third_party/blink/renderer/core/loader/modulescript/module_script_loader.cc
M third_party/blink/renderer/core/loader/modulescript/module_script_loader_test.cc
D third_party/blink/renderer/core/loader/modulescript/worker_or_worklet_module_script_fetcher.cc
D third_party/blink/renderer/core/loader/modulescript/worker_or_worklet_module_script_fetcher.h

A third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.cc
A third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.h


M third_party/blink/renderer/core/loader/worker_fetch_context.cc
M third_party/blink/renderer/core/script/document_modulator_impl.cc
M third_party/blink/renderer/core/script/worker_modulator_impl.cc
M third_party/blink/renderer/core/script/worklet_modulator_impl.cc

M third_party/blink/renderer/core/workers/BUILD.gn
M third_party/blink/renderer/core/workers/dedicated_worker.cc
M third_party/blink/renderer/core/workers/main_thread_worklet_test.cc
M third_party/blink/renderer/core/workers/threaded_worklet_messaging_proxy.cc


M third_party/blink/renderer/core/workers/threaded_worklet_test.cc
M third_party/blink/renderer/core/workers/worker_fetch_test_helper.h
M third_party/blink/renderer/core/workers/worker_global_scope.h
M third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc

D third_party/blink/renderer/core/workers/worker_or_worklet_module_fetch_coordinator_proxy.cc
D third_party/blink/renderer/core/workers/worker_or_worklet_module_fetch_coordinator_proxy.h
M third_party/blink/renderer/core/workers/worklet.cc

M third_party/blink/renderer/core/workers/worklet_global_scope.cc
M third_party/blink/renderer/core/workers/worklet_global_scope.h
M third_party/blink/renderer/core/workers/worklet_module_responses_map.cc
M third_party/blink/renderer/core/workers/worklet_module_responses_map.h

M third_party/blink/renderer/core/workers/worklet_module_responses_map_test.cc
M third_party/blink/renderer/modules/animationworklet/animation_worklet_global_scope_test.cc
M third_party/blink/renderer/modules/animationworklet/animation_worklet_thread_test.cc


M third_party/blink/renderer/modules/csspaint/paint_worklet_global_scope_proxy.cc
M third_party/blink/renderer/modules/webaudio/audio_worklet_global_scope_test.cc
M third_party/blink/renderer/modules/webaudio/audio_worklet_thread_test.cc

42 files changed, 476 insertions(+), 749 deletions(-)

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 16, 2018, 7:43:39 PM5/16/18

to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroshige Hayashizaki, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

Ok, I think this is ready for review, having been more-or-less rewritten from scratch. PTAL :)

2 comments:

File third_party/blink/renderer/core/loader/worker_fetch_context.cc:
- Patch Set #17, Line 413: if (!web_context)
  This should only happen in unit tests.
File third_party/blink/renderer/core/workers/worklet_global_scope.cc:
- Patch Set #17, Line 58: GetContentSecurityPolicy()->SetOverrideURLForSelf(url_);
  This resolves the worklet CSP layout test failures, but it requires updating some unit tests that expect CSP in WorkletGlobalScope to resolve self based on the unique opaque origin.
  I'm a little lost reading through the spec, but looks like a worklet's outsideSettings map to "fetch client settings object" and insideSettings to "module map settings object" in https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-single-module-script
  "fetch client settings object" is the one used to intialize the request (and therefore providing the origin for CSP 'self' resolution). insideSettings is where the unique opaque origin is applied, so I don't think it's relevant for CSP?

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Tsuyoshi Horo (Gerrit)

unread,

May 16, 2018, 11:37:56 PM5/16/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroshige Hayashizaki, Hiroki Nakagawa, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

I may not fully understand the module loading logic.
But I have some questions.

3 comments:

File third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.cc:

Patch Set #17, Line 22: if (module_responses_map_->GetEntry(

What happens if the worklet is destroyed while loading the module?
Example:
 The page launches two worlkets (A and B):
  1. Worklet A starts loading a module "x.js".
     module_responses_map_->GetEntry() returns false.
     So fetches the module.
  2. Worklet B starts loading a module "x.js".
     module_responses_map_->GetEntry() returns true.
     So the worklet waits for Client::OnFetched or OnFailed() callback.
  3. Worklet A is stopped.
  4. Will the Worklet B receive any callback?
Do we have a test for such case?

File third_party/blink/renderer/core/loader/worker_fetch_context.cc:

- Patch Set #17, Line 413: if (!web_context)
  This should only happen in unit tests.

- Please write this comment in the code.
File third_party/blink/renderer/core/workers/worklet_module_responses_map.cc:
- Patch Set #17, Line 94: OnFetched
  Is it safe to call OnFetched() synchronously while having the lock of |mutex_|?
  Same as OnFailed() in this method.

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Hiroki Nakagawa (Gerrit)

unread,

May 16, 2018, 11:51:15 PM5/16/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Tsuyoshi Horo, Hiroshige Hayashizaki, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

I just skimmed through the CL and the direction of the changes looks good. I'll take a closer look later, especially around CSP handling.

5 comments:

File third_party/blink/renderer/core/frame/local_frame_client.h:
- Patch Set #17, Line 408: dedicated worker
  and worklet?
- Patch Set #17, Line 409: // the returned object is transferred to the caller.
  The second sentence would be redundant because it's obvious from the returned value (std::unique_ptr)
File third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.h:
- Patch Set #17, Line 47: DocumentModuleScriptFetcher
  ModuleScriptFetcher?
File third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.cc:

- Patch Set #17, Line 22: if (module_responses_map_->GetEntry(

- What happens if the worklet is destroyed while loading the module? […]
  Good point. The Worklet spec allows to stop worklets at any time, so such a case can happen. However, in the current Chome implementation, worklets never stop until the parent document is closed, so this is safe at least for now.
  japhet@: Can you leave a TODO comment about the case?
File third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc:
- Patch Set #17, Line 100:
  I'm not sure if WorkerFetchContext is available for main thread worklets. horo@ may know it.

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Hiroshige Hayashizaki (Gerrit)

unread,

May 17, 2018, 5:50:20 PM5/17/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

12 comments:

File third_party/blink/renderer/core/workers/worker_fetch_test_helper.h:
- Patch Set #17, Line 30: params == base::nullopt
  nit: |!params|?
File third_party/blink/renderer/core/workers/worklet_global_scope.h:
- Patch Set #17, Line 115: CrossThreadPersistent
  What is the intention behind switching this to CrossThreadPersistent?
File third_party/blink/renderer/core/workers/worklet_module_responses_map.h:
- Patch Set #17, Line 31: // across worklet threads.
  Some additional comments (here or somewhere else) about thread affinity might be helpful.
  (Please correct if my understanding is wrong)
  - All data linked from WorkletModuleResponsesMap are destroyed on the main thread and should always remain thread-safe. Thread-safe copies should be taken on everytime the data is passed in and out.
- Patch Set #17, Line 40: bool GetEntry(const KURL&,
  - Called on worlket threads.
- Patch Set #17, Line 45: void SetEntryParams(const KURL&,
  - Called on worklet threads.
- Patch Set #17, Line 51: void Dispose() LOCKS_EXCLUDED(mutex_);
  - Called on the main thread.
- Patch Set #17, Line 56:
  nit: final?
- Patch Set #17, Line 63: const ModuleScriptCreationParams& GetParams() const { return *params_; }
  It's better to call CrossThreadCopier inside GetParams() instead of in the caller, to
  ensure/clarify that |params_| should remain thread-safe and have to be deep-copied before use.
- Patch Set #17, Line 77: bool is_available_ = true;
  - |is_available_| is written on the main thread and read on main/worklet threads,
  - |entries_| is read/written on main/worklet threads,
  - |is_available_| and |entries_| should be accessed only when |mutex_| is locked.
- Patch Set #17, Line 82: std::unique_ptr
  What is the intention for making this unique_ptr (and making Entry::clients_ contain CrossThreadPersistent) rather than making everything Member?
File third_party/blink/renderer/core/workers/worklet_module_responses_map.cc:
- Patch Set #17, Line 43: CrossThreadCopier<ModuleScriptCreationParams>::Copy(*params));
  Could you add the following to ensure |params_| is thread-safe?
  DCHECK(params_->IsSafeToSendToAnotherThread());
  here (just after emplace()), and
```
bool ModuleScriptCreationParams::IsSafeToSendToAnotherThread() {
  return response_url_.IsSafeToSendToAnotherThread() &&
    source_text_.IsSafeToSendToAnotherThread();
}
```
- Patch Set #17, Line 43: CrossThreadCopier<ModuleScriptCreationParams>::Copy(*params)
  How about defining
  ModuleScriptCreationParams ModuleScriptCreationParams::IsolatedCopy() const
  and making CrossThreadCopier<ModuleScriptCreationParams>::Copy() just call IsolatedCopy(),
  and using the IsolatedCopy() here?

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 18, 2018, 2:27:45 PM5/18/18

to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroshige Hayashizaki, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

19 comments:

File third_party/blink/renderer/core/frame/local_frame_client.h:

- Patch Set #17, Line 408: dedicated worker
  and worklet?

- Done
- Patch Set #17, Line 409: virtual std::unique_ptr<WebWorkerFetchContext> CreateWorkerFetchContext() {
  The second sentence would be redundant because it's obvious from the returned value (std::unique_ptr […]
  Done
File third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.h:
- Patch Set #17, Line 47: ModuleScriptFetcher"; }
  ModuleScriptFetcher?
  Done
File third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.cc:

- Patch Set #17, Line 22: if (module_responses_map_->GetEntry(

Good point. The Worklet spec allows to stop worklets at any time, so such a case can happen. […]

I think this should work as-is, though I'm not sure whether you mean 2 worklets, or 2 worklet global scopes. If you mean 2 worklets, they won't share a WorkletModuleResponsesMap, so that's easy.

If you meant worklet global scopes, there are two cases I can see:
1. The worklet is disposed, and all global scopes are invalidated. In this case, any
   WorkletModuleResponseMap::Entry that is fetching will be failed and clients notified.
2. We selectively terminate global scopes. In this case, I believe we would cancel any
   pending fetches associated with the global scope that is being terminated, so the other
   global scope will get notified, but it will get an error when in theory it should keep the
   fetch going.

So yeah, (2) is the case I'm worried about. Will add a TODO for that. Though it's not clear to me what we should do for it.

File third_party/blink/renderer/core/loader/worker_fetch_context.cc:
- Patch Set #17, Line 413: // web_context should only be nullptr in unit tests.

- Please write this comment in the code.

- Done
File third_party/blink/renderer/core/workers/worker_fetch_test_helper.h:
- Patch Set #17, Line 30: params) {
  nit: |!params|?
  Done, and inverted for easier reading.
File third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc:

- Patch Set #17, Line 100:
  I'm not sure if WorkerFetchContext is available for main thread worklets. horo@ may know it.

- This CL makes it available, that's why all worklet GlobalScopeCreationParams now have a non-null WorkerClients, and ProvideWorkerFetchContextToWorker() is called on each WorkerClients.
File third_party/blink/renderer/core/workers/worklet_global_scope.h:

- Patch Set #17, Line 115: CrossThreadPersistent
  What is the intention behind switching this to CrossThreadPersistent?

- WorkerOrWorkletModuleFetchCoordinatorProxy was owned entirely on the WorkletGlobalScope's thread, so it could be a Member. WorkerOrWorkletModuleFetchCoordinatorProxy then held a CrossThreadPersistent<WorkletModuleResponsesMap>. Now WorkletGlobalScope holds the CrossThreadPersistent<WorkletModuleResponsesMap> directly.
File third_party/blink/renderer/core/workers/worklet_module_responses_map.h:
- Patch Set #17, Line 31: // across worklet threads. All access to this class should be mutex-guarded,
  Some additional comments (here or somewhere else) about thread affinity might be helpful. […]
  I think that's right.
- Patch Set #17, Line 40: // Otherwise, it's called on the completion of the fetch. See also the
  - Called on worlket threads.
  Done
- Patch Set #17, Line 45: scoped_refptr<base::SingleThreadTaskRunner> client_task_runner)
  - Called on worklet threads.
  Done
- Patch Set #17, Line 51: LOCKS_EXCLUDED(mutex_);
  - Called on the main thread.
  Done
- Patch Set #17, Line 56: e
  nit: final?
  Done
- Patch Set #17, Line 63: enum class State { kFetching, kFetched, kFailed };
  It's better to call CrossThreadCopier inside GetParams() instead of in the caller, to […]
  Done
- Patch Set #17, Line 77: State state_ = State::kFetching;
  - |is_available_| is written on the main thread and read on main/worklet threads, […]
  Done

- Patch Set #17, Line 82:

- What is the intention for making this unique_ptr (and making Entry::clients_ contain CrossThreadPers […]
  Entry may be accessed from multiple threads, and the clients may reside on multiple threads. At the very least, I think clients_ must use CrossThreadPersistent. It seemed easier to control Entry lifetime and correctness, but making it unique_ptr, since this map is the only place that will have a reference, and ensure its creation/destruction is mutex-locked.
File third_party/blink/renderer/core/workers/worklet_module_responses_map.cc:
- Patch Set #17, Line 43: DCHECK(params_->IsSafeToSendToAnotherThread());
  Could you add the following to ensure |params_| is thread-safe? […]
  Done
- Patch Set #17, Line 43: CK(params_->IsSafeToSendToAnotherThread());
  How about defining […]
  Done
- Patch Set #17, Line 94: ete this
  Is it safe to call OnFetched() synchronously while having the lock of |mutex_|? […]
  Good point. Also, the quoted spec clearly says to complete the algorithm asynchronously, which I'm not doing :)
  Changed to post a (same-thread) task.

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Hiroshige Hayashizaki (Gerrit)

unread,

May 18, 2018, 4:58:47 PM5/18/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

1 comment:

File third_party/blink/renderer/core/workers/worklet_module_responses_map.h:
- Patch Set #17, Line 82:
  Entry may be accessed from multiple threads, and the clients may reside on multiple threads. […]
  Sounds reasonable > making it unique_ptr, since this map is the only place that will have a reference, and ensure its creation/destruction is mutex-locked.
  But there seems to be a reference cycle:
  ModuleScriptFetcher::Client
  -(subclass)->WorkletModuleScriptFetcher
  -(CrossThreadPersistent)->WorkletModuleResponsesMap
  -(unique_ptr)->WorkletModuleResponsesMap::Entry
  -(CrossThreadPersistent *1)->ModuleScriptFetcher::Client
  On Dispose(), the link (*1) is removed as |clients_| is cleared, and thus the objects will be GC'ed.
  Probably adding a comment about this reference cycle breaking is helpful.

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 18, 2018, 5:39:27 PM5/18/18

to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroshige Hayashizaki, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

1 comment:

File third_party/blink/renderer/core/workers/worklet_module_responses_map.h:

- Patch Set #17, Line 82:

- Sounds reasonable > making it unique_ptr, since this map is the only place that will have a referenc […]
  The cycle is broken when SetParams() is called, which can be either for Dispose() or for fetch complete. I think breaking the cycle at fetch complete is sufficient, and it's unclear to me whether it needs to be explicitly documented, given that fetches often work on this principle elsewhere in blink. Do you still prefer that the cycle be documented?

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Tsuyoshi Horo (Gerrit)

unread,

May 20, 2018, 9:16:40 PM5/20/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroshige Hayashizaki, Hiroki Nakagawa, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

Patch set 19:Code-Review +1

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Hiroki Nakagawa (Gerrit)

unread,

May 21, 2018, 2:04:19 AM5/21/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Tsuyoshi Horo, Hiroshige Hayashizaki, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

7 comments:

File third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.cc:

- Patch Set #17, Line 22: if (module_responses_map_->GetEntry(

- I think this should work as-is, though I'm not sure whether you mean 2 worklets, or 2 worklet global […]
  Thank you for leaving the comment. Yes, I mean the case where there're 2+ worklet global scopes on a single thread and (2) happens.
  For now, I'm also not really sure how we should handle the case. Let's revisit it when we really need it.
File third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc:

- Patch Set #17, Line 100:

- This CL makes it available, that's why all worklet GlobalScopeCreationParams now have a non-null Wor […]
  Ack
File third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc:
- Patch Set #19, Line 99: ResourceFetcher* WorkerOrWorkletGlobalScope::EnsureFetcher() {
  DCHECK(IsContextThread());
- Patch Set #19, Line 107: ResourceFetcher* WorkerOrWorkletGlobalScope::Fetcher() const {
  DCHECK(IsContextThread());
File third_party/blink/renderer/core/workers/worklet_global_scope.cc:

- Patch Set #17, Line 58: GetContentSecurityPolicy()->SetOverrideURLForSelf(url_);

- This resolves the worklet CSP layout test failures, but it requires updating some unit tests that ex […]
  Looks like this is caused by that the current Module/Fetch implementation don't distinguish "fetch client settings object" (outsideSettings) and "module map settings object" (insideSettings). I heard kouhei@ and hiroshige@ are planning to fix this by redesigning the Modulator interface.
  I think overriding "self" of "module map settings object" is not a right fix. However, this seems like the only workaround for now. I'm fine with landing this CL if we make sure this change never affects other CSP rules. I guess this is safe because there are no networking APIs exposed to worklets that see this URL, but I'm not 100% sure. There might be other factors.
  If we land this as is, can you leave a comment like this?
  // CSP checks should resolve 'self' based on the "fetch client settings object", not "module map settings object". However, the current Module/Fetch implementation don't distinguish them and CSP checks resolve it based on "module map settings object". As a workaround, we override "module map settings object"'s URL for 'self' here. This should be safe because there are no APIs exposed to worklets that see this URL.
  // TODO(japhet, nhiroki): Make CSP checks resolve 'self' based on 'fetch client settings object' and stop overriding the URL for 'self'.
File third_party/blink/renderer/core/workers/worklet_module_responses_map.h:
- Patch Set #19, Line 86: bool is_available_ = true;
  bool is_available_ GUARDED_BY(mutex_) = true;
- Patch Set #19, Line 92: HashMap<KURL, std::unique_ptr<Entry>> entries_;
  HashMap<KURL, std::unique_ptr<Entry>> entries_ GUARDED_BY(mutex_);

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Hiroshige Hayashizaki (Gerrit)

unread,

May 21, 2018, 4:19:15 PM5/21/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

1 comment:

File third_party/blink/renderer/core/workers/worklet_module_responses_map.h:

- Patch Set #17, Line 82:

- The cycle is broken when SetParams() is called, which can be either for Dispose() or for fetch complete.

- I think breaking the cycle at fetch complete is sufficient, and it's unclear to me whether it needs to be explicitly documented, given that fetches often work on this principle elsewhere in blink.

- Sounds good, and the explicit documentation about the cycle is probably not needed.

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 21, 2018, 4:51:59 PM5/21/18

to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroshige Hayashizaki, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

5 comments:

File third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc:

- Patch Set #19, Line 99: ResourceFetcher* WorkerOrWorkletGlobalScope::EnsureFetcher() {
  DCHECK(IsContextThread());

- Done

- Patch Set #19, Line 107: }

- DCHECK(IsContextThread());
  Done
File third_party/blink/renderer/core/workers/worklet_global_scope.cc:
- Patch Set #17, Line 58: // the opaque origin of this worklet global scope). The current implementation
  Looks like this is caused by that the current Module/Fetch implementation don't distinguish "fetch c […]
  I used this suggested comment as a starting point, hopefully you still agree with my additions and adjustments :)
File third_party/blink/renderer/core/workers/worklet_module_responses_map.h:
- Patch Set #19, Line 86: bool is_available_ GUARDED_BY(mutex_) = true;

- bool is_available_ GUARDED_BY(mutex_) = true;

- Done, and removed comment "Access only when |mutex_| is locked." since it's now redundant.
- Patch Set #19, Line 92: HashMap<KURL, std::unique_ptr<Entry>> entries_ GUARDED_BY(mutex_);

- HashMap<KURL, std::unique_ptr<Entry>> entries_ GUARDED_BY(mutex_);

- Done, and removed comment "Access only when |mutex_| is locked." since it's now redundant.

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 21, 2018, 6:46:56 PM5/21/18

to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroshige Hayashizaki, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

1 comment:

File third_party/blink/renderer/core/workers/worklet_global_scope.cc:
- Patch Set #21, Line 64: GetContentSecurityPolicy()->SetupSelf(*document_security_origin_);
  Switched this from overriding url to doing a "proper" initialization of CSP with the document's security origin.

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Hiroshige Hayashizaki (Gerrit)

unread,

May 21, 2018, 7:45:20 PM5/21/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

uioiuo

1 comment:

File third_party/blink/renderer/core/workers/worklet_global_scope.cc:
- Patch Set #17, Line 58: // the opaque origin of this worklet global scope). The current implementation
  I used this suggested comment as a starting point, hopefully you still agree with my additions and a […]
  (Some thoughts and recap of offline discussion)
  Filed crbug.com/845285 for kouhei and I's plan for Modulator cleanup, but the main part of our effort doesn't tackle the root cause of this CSP issue directly.
  IIUC, there are two types of fetches on workers/worklets, (1) using insideSettings and (2) using outsideSettings.
  In the worklet cases, we intentionally disallow the use of insideSettings for fetch (1), and all fetches on worklets are module script fetching, which should use outsideSettings (2). Therefore, in the current PatchSet we pretend as if WorkletGlobalScope::Fetcher() is the ResourceFetcher for outsideSettings by setting the WorkletGlobalScope's ContentSecurityPolicy using the Document's SecurityOrigin.
  In the worker cases, both fetches of type (1) and (2) exist, but they use the same ResourceFetcher (i.e. WorkerGlobalScope::Fetcher()), and this is not so problematic because in workers insideSettings and outsideSettings are relatively similar.
  I think this is acceptable for the short term.

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Hiroshige Hayashizaki (Gerrit)

unread,

May 21, 2018, 8:02:57 PM5/21/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

Patch Set 20:
(1 comment)
uioiuo

very nit: please ignore "uioiuo"; garbage generated while I was testing Chrome behavior for an unrelated issue.

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Hiroki Nakagawa (Gerrit)

unread,

May 21, 2018, 10:12:24 PM5/21/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroshige Hayashizaki, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

LGTM!

Patch set 21:Code-Review +1

1 comment:

File third_party/blink/renderer/core/workers/worklet_global_scope.cc:

- Patch Set #17, Line 58: // the opaque origin of this worklet global scope). The current implementation

- In the worklet cases, we intentionally disallow the use of insideSettings for fetch (1), and all fetches on worklets are module script fetching, which should use outsideSettings (2). Therefore, in the current PatchSet we pretend as if WorkletGlobalScope::Fetcher() is the ResourceFetcher for outsideSettings by setting the WorkletGlobalScope's ContentSecurityPolicy using the Document's SecurityOrigin.
  That's right. To be clear, (1) includes dynamic import.

- In the worker cases, both fetches of type (1) and (2) exist, but they use the same ResourceFetcher (i.e. WorkerGlobalScope::Fetcher()), and this is not so problematic because in workers insideSettings and outsideSettings are relatively similar.

- I'm now adding WPTs for referrer (https:/crbug.com/). Looks like referrer doesn't correctly work on module loading for top-level and static import (2)
  because they use insideSettings, not outsideSettings. We need a workaround to override the referrer only for (2)... :(

- I think this is acceptable for the short term.

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 22, 2018, 12:06:09 PM5/22/18

to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroki Nakagawa, Hiroshige Hayashizaki, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Kinuko Yasuda, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

Patch Set 21: Code-Review+1
(1 comment)
LGTM!

Thanks! Given the complexity of this change, I'm going to wait to land this until after M68 branch.

Kinuko Yasuda (Gerrit)

unread,

May 22, 2018, 10:40:58 PM5/22/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroki Nakagawa, Hiroshige Hayashizaki, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

lgtm

Patch set 21:Code-Review +1

2 comments:

File third_party/blink/renderer/core/exported/local_frame_client_impl.cc:
- Patch Set #21, Line 1129: return web_frame_->Client()->CreateWorkerFetchContext();
  Can't we just directly call this (and below) from renderer/core/layout/custom/layout_worklet_global_scope_proxy.cc etc as WebLocalFrameImpl is now in renderer/core?
File third_party/blink/renderer/core/workers/worklet_module_responses_map.h:
- Patch Set #21, Line 33: // internal state can be safely destructed from the main thread.
  Woohoo.

Nate Chapin (Gerrit)

unread,

May 23, 2018, 2:16:45 PM5/23/18

to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Kinuko Yasuda, Hiroki Nakagawa, Hiroshige Hayashizaki, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

1 comment:

File third_party/blink/renderer/core/exported/local_frame_client_impl.cc:

- Patch Set #21, Line 1129: return web_frame_->Client()->CreateWorkerFetchContext();

- Can't we just directly call this (and below) from renderer/core/layout/custom/layout_worklet_global_ […]
  New #includes of web_local_frame_impl.h are DEPS-forbidden: https://chromium.googlesource.com/chromium/src/+/master/third_party/blink/renderer/core/DEPS#43
  :/

Nate Chapin (Gerrit)

unread,

May 25, 2018, 6:21:13 PM5/25/18

to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Kinuko Yasuda, Hiroki Nakagawa, Hiroshige Hayashizaki, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

Patch set 21:Commit-Queue +2

Commit Bot (Gerrit)

unread,

May 25, 2018, 6:33:09 PM5/25/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Kinuko Yasuda, Hiroki Nakagawa, Hiroshige Hayashizaki, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

Try jobs failed on following builders:
  chromium_presubmit on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/chromium_presubmit/124530)

Nate Chapin (Gerrit)

unread,

May 25, 2018, 6:40:24 PM5/25/18

to Tsuyoshi Horo, Hiroshige Hayashizaki, Hiroki Nakagawa, Kouhei Ueno, Kinuko Yasuda, Matt Falkenhagen, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Yoav Weiss, chromium...@chromium.org, Hongchan Choi, Renée Wright, Raymond Toy, Xida Chen, Commit Bot, Alexis Menard, Eric Willigers, Thiago Farina, Kentaro Hara, Shane Stephens

Nate Chapin uploaded patch set #22 to this change.

Load worklet modules off-main-thread

* Make WorkletModuleResponsesMap thread-safe. It is now created and
  owned by the worklet object on the main thread, and shared to
  worklet global scopes.
* Before this change, WorkerOrWorkletModuleScriptFetcher proxied
  fetches from worklet to the main thread. After this change,
  WorkletModuleScriptFetcher will check WorkletModuleResponsesMap and
  return a cached result if present. Otherwise, it will fetch on the
  current thread and cache the result in WorkletModuleResponsesMap on
  completion.
* Merge DocumentModuleScriptFetcher into ModuleScriptFetcher.
  WorkletModuleScriptFetcher still subclasses ModuleScriptFetcher, and
  defers to the ModuleScriptFetcher's fetching logic when
  WorkletModuleResponsesMap doesn't have a cache entry.
* With the above changes, no cross-thread module loading logic is
  needed for workers or worklets. Delete WorkerOrWorkletModuleScriptFetcher
  and WorkerOrWorkletModuleFetchCoordinator.

TBR=ikilp...@chromium.org,rt...@chromium.org


Change-Id: I4c6e8740d69a96a37711ff256417a5357586ae90
---
M third_party/blink/renderer/core/exported/local_frame_client_impl.cc
M third_party/blink/renderer/core/exported/local_frame_client_impl.h
M third_party/blink/renderer/core/frame/local_frame_client.h
M third_party/blink/renderer/core/layout/custom/layout_worklet_global_scope_proxy.cc
M third_party/blink/renderer/core/loader/BUILD.gn
M third_party/blink/renderer/core/loader/frame_loader.cc
M third_party/blink/renderer/core/loader/frame_loader.h
D third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.cc
D third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.h

M third_party/blink/renderer/core/loader/modulescript/module_script_creation_params.h

43 files changed, 545 insertions(+), 746 deletions(-)

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 25, 2018, 6:41:07 PM5/25/18

to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Kinuko Yasuda, Hiroki Nakagawa, Hiroshige Hayashizaki, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

TBRing a couple of mechanical bits:
ikilpatrick for modules/csspaint/
rtoy for modules/webaudio/

Patch set 22:Commit-Queue +2

Commit Bot (Gerrit)

unread,

May 25, 2018, 6:41:24 PM5/25/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Kinuko Yasuda, Hiroki Nakagawa, Hiroshige Hayashizaki, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

CQ is trying the patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.
"Edit commit message" https://chromium-review.googlesource.com/c/1026945/22

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1026945/22

Bot data: {"action": "start", "triggered_at": "2018-05-25T22:41:05.0Z", "cq_cfg_revision": "6a25833cb93164fb378d8087f9dae5fc28fc9b1e", "revision": "fc2892b403d7456ad9d58bcc050ebc16d991f7f5"}

Ian Kilpatrick (Gerrit)

unread,

May 25, 2018, 6:59:07 PM5/25/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Kinuko Yasuda, Hiroki Nakagawa, Hiroshige Hayashizaki, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

Patch set 22:Code-Review +1

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Hiroshige Hayashizaki (Gerrit)

unread,

May 25, 2018, 7:01:27 PM5/25/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Ian Kilpatrick, Kinuko Yasuda, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

LGTM!

Hiroshige Hayashizaki (Gerrit)

unread,

May 25, 2018, 7:02:03 PM5/25/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Ian Kilpatrick, Kinuko Yasuda, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

LGTM!

Nate Chapin (Gerrit)

unread,

May 25, 2018, 7:05:34 PM5/25/18

to Tsuyoshi Horo, Hiroshige Hayashizaki, Ian Kilpatrick, Hiroki Nakagawa, Kouhei Ueno, Kinuko Yasuda, Matt Falkenhagen, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Yoav Weiss, chromium...@chromium.org, Hongchan Choi, Renée Wright, Raymond Toy, Xida Chen, Commit Bot, Alexis Menard, Eric Willigers, Thiago Farina, Kentaro Hara, Shane Stephens

Nate Chapin uploaded patch set #23 to this change.

Load worklet modules off-main-thread

* Make WorkletModuleResponsesMap thread-safe. It is now created and
  owned by the worklet object on the main thread, and shared to
  worklet global scopes.
* Before this change, WorkerOrWorkletModuleScriptFetcher proxied
  fetches from worklet to the main thread. After this change,
  WorkletModuleScriptFetcher will check WorkletModuleResponsesMap and
  return a cached result if present. Otherwise, it will fetch on the
  current thread and cache the result in WorkletModuleResponsesMap on
  completion.
* Merge DocumentModuleScriptFetcher into ModuleScriptFetcher.
  WorkletModuleScriptFetcher still subclasses ModuleScriptFetcher, and
  defers to the ModuleScriptFetcher's fetching logic when
  WorkletModuleResponsesMap doesn't have a cache entry.
* With the above changes, no cross-thread module loading logic is
  needed for workers or worklets. Delete WorkerOrWorkletModuleScriptFetcher
  and WorkerOrWorkletModuleFetchCoordinator.

TBR=ikilp...@chromium.org,rt...@chromium.org

Bug: 846938

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Nate Chapin (Gerrit)

unread,

May 25, 2018, 7:43:21 PM5/25/18

to blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroshige Hayashizaki, Ian Kilpatrick, Kinuko Yasuda, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Commit Bot, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

Patch set 23:Commit-Queue +2

Commit Bot (Gerrit)

unread,

May 25, 2018, 7:43:29 PM5/25/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroshige Hayashizaki, Ian Kilpatrick, Kinuko Yasuda, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

CQ is trying the patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.

"Edit commit message" https://chromium-review.googlesource.com/c/1026945/23

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1026945/23

Bot data: {"action": "start", "triggered_at": "2018-05-25T23:43:19.0Z", "cq_cfg_revision": "6a25833cb93164fb378d8087f9dae5fc28fc9b1e", "revision": "9ea183072f8349cf6246c68a88d0f3616613f0d4"}

Commit Bot (Gerrit)

unread,

May 25, 2018, 8:50:34 PM5/25/18

to Nate Chapin, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroshige Hayashizaki, Ian Kilpatrick, Kinuko Yasuda, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Alexis Menard, chromium...@chromium.org, Eric Willigers, Kentaro Hara, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

Commit Bot merged this change.

Approvals:
  Tsuyoshi Horo: Looks good to me
  Kinuko Yasuda: Looks good to me
  Hiroshige Hayashizaki: Looks good to me
  Kouhei Ueno: Looks good to me
  Hiroki Nakagawa: Looks good to me
  Ian Kilpatrick: Looks good to me
  Nate Chapin: Commit

Load worklet modules off-main-thread

* Make WorkletModuleResponsesMap thread-safe. It is now created and
  owned by the worklet object on the main thread, and shared to
  worklet global scopes.
* Before this change, WorkerOrWorkletModuleScriptFetcher proxied
  fetches from worklet to the main thread. After this change,
  WorkletModuleScriptFetcher will check WorkletModuleResponsesMap and
  return a cached result if present. Otherwise, it will fetch on the
  current thread and cache the result in WorkletModuleResponsesMap on
  completion.
* Merge DocumentModuleScriptFetcher into ModuleScriptFetcher.
  WorkletModuleScriptFetcher still subclasses ModuleScriptFetcher, and
  defers to the ModuleScriptFetcher's fetching logic when
  WorkletModuleResponsesMap doesn't have a cache entry.
* With the above changes, no cross-thread module loading logic is
  needed for workers or worklets. Delete WorkerOrWorkletModuleScriptFetcher
  and WorkerOrWorkletModuleFetchCoordinator.

TBR=ikilp...@chromium.org,rt...@chromium.org

Bug: 846938
Change-Id: I4c6e8740d69a96a37711ff256417a5357586ae90

Reviewed-on: https://chromium-review.googlesource.com/1026945
Commit-Queue: Nate Chapin <jap...@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilp...@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiro...@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhi...@chromium.org>
Reviewed-by: Kinuko Yasuda <kin...@chromium.org>
Reviewed-by: Tsuyoshi Horo <ho...@chromium.org>
Reviewed-by: Kouhei Ueno <kou...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562087}

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Kentaro Hara (Gerrit)

unread,

May 26, 2018, 12:51:22 AM5/26/18

to Nate Chapin, Commit Bot, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, eae+bli...@chromium.org, falken...@chromium.org, gavinp...@chromium.org, hiroshig...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, kochi+...@chromium.org, kouhei...@chromium.org, loading...@chromium.org, pdr+renderi...@chromium.org, shimazu...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, kinuko...@chromium.org, loading-re...@chromium.org, Hiroshige Hayashizaki, Ian Kilpatrick, Kinuko Yasuda, Hiroki Nakagawa, Tsuyoshi Horo, Kouhei Ueno, Matt Falkenhagen, Alexis Menard, chromium...@chromium.org, Eric Willigers, Hongchan Choi, Renée Wright, Raymond Toy, Shane Stephens, Thiago Farina, Xida Chen, Yoav Weiss

modules LGTM

Patch set 24:Code-Review +1

To view, visit change 1026945. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward

0 new messages

Search

Clear search

Close search

Google apps

Main menu