Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2657823002:
Worklet: Straighten layering of worklet script loading (Closed)
|
Created:
3 years, 11 months ago by nhiroki Modified:
3 years, 11 months ago CC:
chromium-reviews, tfarina, shimazu+worker_chromium.org, kinuko+worker_chromium.org, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWorklet: Straighten layering of worklet script loading
This is a preparation to switch Worklet to module script loading.
Before this CL, WorkletScriptLoader is entangled with Worklet. Worklet fetches a
script and passes it to WorkletScriptLoader with a promise resolver. Then,
WorkletScriptLoader loads it and settles the resolver.
After this CL, WorkletScriptLoader dedicates to script fetching and loading.
WorkletScriptLoader provides Client interface so that a loader client can
receive notifications from the loader. Worklet no longer takes care of script
fetching. Worklet implements the Client interface to wait for completion of
resource loading, evaluates a script and settles a resolver.
BUG=627945
Review-Url: https://codereview.chromium.org/2657823002
Cr-Commit-Position: refs/heads/master@{#446608}
Committed: https://chromium.googlesource.com/chromium/src/+/f1e393e2cab401597d52fe9b7ed90946c5d426d3
Patch Set 1 #
Total comments: 4
Patch Set 2 : maybe fix compile failures on win bots #
Total comments: 10
Patch Set 3 : address review comments #Patch Set 4 : rebase #Patch Set 5 : remove unnecessary header inclusions #Patch Set 6 : fix #
Total comments: 5
Patch Set 7 : take ResourceFetcher* directly #
Total comments: 8
Patch Set 8 : clean up #Patch Set 9 : address yhirano's comments #Patch Set 10 : clean up #
Messages
Total messages: 59 (41 generated)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Description was changed from ========== Worklet: Straighten layering of worklet script loading This is a preparation to switch Worklet to module script loading. Before this CL, WorkletScriptLoader is entangled with Worklet. Worklet fetches a script and passes it to WorkletScriptLoader with a promise resolver. Then, WorkletScriptLoader loads it and settles the resolver. After this CL, WorkletScriptLoader dedicates to script fetching and loading. WorkletScriptLoader provides Client interface so that a loader client can receives notifications from the loader. Worklet no longer take care of script fetching. It implements the Client interface and settles resolvers when resource loading finishes. BUG=627945 ========== to ========== Worklet: Straighten layering of worklet script loading This is a preparation to switch Worklet to module script loading. Before this CL, WorkletScriptLoader is entangled with Worklet. Worklet fetches a script and passes it to WorkletScriptLoader with a promise resolver. Then, WorkletScriptLoader loads it and settles the resolver. After this CL, WorkletScriptLoader dedicates to script fetching and loading. WorkletScriptLoader provides Client interface so that a loader client can receive notifications from the loader. Worklet no longer take care of script fetching. It implements the Client interface and settles resolvers when resource loading finishes. BUG=627945 ==========
Description was changed from ========== Worklet: Straighten layering of worklet script loading This is a preparation to switch Worklet to module script loading. Before this CL, WorkletScriptLoader is entangled with Worklet. Worklet fetches a script and passes it to WorkletScriptLoader with a promise resolver. Then, WorkletScriptLoader loads it and settles the resolver. After this CL, WorkletScriptLoader dedicates to script fetching and loading. WorkletScriptLoader provides Client interface so that a loader client can receive notifications from the loader. Worklet no longer take care of script fetching. It implements the Client interface and settles resolvers when resource loading finishes. BUG=627945 ========== to ========== Worklet: Straighten layering of worklet script loading This is a preparation to switch Worklet to module script loading. Before this CL, WorkletScriptLoader is entangled with Worklet. Worklet fetches a script and passes it to WorkletScriptLoader with a promise resolver. Then, WorkletScriptLoader loads it and settles the resolver. After this CL, WorkletScriptLoader dedicates to script fetching and loading. WorkletScriptLoader provides Client interface so that a loader client can receive notifications from the loader. Worklet no longer takes care of script fetching. Worklet implements the Client interface and settles resolvers when resource loading finishes. BUG=627945 ==========
nhiroki@chromium.org changed reviewers: + kouhei@chromium.org, yhirano@chromium.org
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2657823002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/worklet/resources/import-tests.js (right): https://codereview.chromium.org/2657823002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/worklet/resources/import-tests.js:24: assert_unreached('unexpected rejection: ' + error); You don't need this catch section. https://codereview.chromium.org/2657823002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/worklet/resources/import-tests.js:31: }).catch(function(error) { ditto (though unrelated to this CL) https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/Worklet.cpp (right): https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/Worklet.cpp:37: m_loaderAndResolvers.set(scriptLoader, resolver); You can remove m_loaderAndResolvers.remove(scriptLoader) if you call m_loaderAndResolvers.set(scriptLoader, resolver) after scriptLoader->fetchScript successfully finishes. Alternatively, you can call notifyCancelled in ScriptLoader::fetchScript. I'm not sure which is better. https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/Worklet.cpp:52: ScriptPromiseResolver* resolver = m_loaderAndResolvers.get(scriptLoader); How about calling m_loaderAndREsolvers.remove(scriptLoader) here? https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp (right): https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:16: : m_fetcher(frame->loader().documentLoader()->fetcher()), frame->document()->fetcher() is better? https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:31: m_client->notifyCanceled(this); I think calling clearResource first is slightly better. Ditto below. https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkletScriptLoader.h (right): https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkletScriptLoader.h:44: // Fetches an URL and loads it as a classic script. // Returns false if there is an error. notifyCancelled and notifyFinished will not be called in that case.
Thanks! I think this is a great step forward. Let me clarify high-level design
first.
The current design resembles a classic ResourceClient pattern, but I'd prefer to
have this similar to ModuleScriptLoader.
Specifically:
- Can we not expose ScriptResource to WorkletScriptLoader client?
-- After Yukari, we want to have small number of blink::Resource users. -> Limit
blink::Resource users to core/loader/**/*
-- Can we explicitly only expose info Worklet need? Probably just the
ScriptSourceCode?
- Can we unify notify{Finished,Cancelled}?
-- Managing invariants seems much easier then.
-- Also, I'd prefer to rename this notifyWorkletScriptFinished.
notify{Finished,Cancelled} suggests this is ResourceClient.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Worklet: Straighten layering of worklet script loading This is a preparation to switch Worklet to module script loading. Before this CL, WorkletScriptLoader is entangled with Worklet. Worklet fetches a script and passes it to WorkletScriptLoader with a promise resolver. Then, WorkletScriptLoader loads it and settles the resolver. After this CL, WorkletScriptLoader dedicates to script fetching and loading. WorkletScriptLoader provides Client interface so that a loader client can receive notifications from the loader. Worklet no longer takes care of script fetching. Worklet implements the Client interface and settles resolvers when resource loading finishes. BUG=627945 ========== to ========== Worklet: Straighten layering of worklet script loading This is a preparation to switch Worklet to module script loading. Before this CL, WorkletScriptLoader is entangled with Worklet. Worklet fetches a script and passes it to WorkletScriptLoader with a promise resolver. Then, WorkletScriptLoader loads it and settles the resolver. After this CL, WorkletScriptLoader dedicates to script fetching and loading. WorkletScriptLoader provides Client interface so that a loader client can receive notifications from the loader. Worklet no longer takes care of script fetching. Worklet implements the Client interface to wait for completion of resource loading, evaluates the script and settles resolvers. BUG=627945 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== Worklet: Straighten layering of worklet script loading This is a preparation to switch Worklet to module script loading. Before this CL, WorkletScriptLoader is entangled with Worklet. Worklet fetches a script and passes it to WorkletScriptLoader with a promise resolver. Then, WorkletScriptLoader loads it and settles the resolver. After this CL, WorkletScriptLoader dedicates to script fetching and loading. WorkletScriptLoader provides Client interface so that a loader client can receive notifications from the loader. Worklet no longer takes care of script fetching. Worklet implements the Client interface to wait for completion of resource loading, evaluates the script and settles resolvers. BUG=627945 ========== to ========== Worklet: Straighten layering of worklet script loading This is a preparation to switch Worklet to module script loading. Before this CL, WorkletScriptLoader is entangled with Worklet. Worklet fetches a script and passes it to WorkletScriptLoader with a promise resolver. Then, WorkletScriptLoader loads it and settles the resolver. After this CL, WorkletScriptLoader dedicates to script fetching and loading. WorkletScriptLoader provides Client interface so that a loader client can receive notifications from the loader. Worklet no longer takes care of script fetching. Worklet implements the Client interface to wait for completion of resource loading, evaluates a script and settles a resolver. BUG=627945 ==========
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for your comments. Updated. https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/Worklet.cpp (right): https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/Worklet.cpp:37: m_loaderAndResolvers.set(scriptLoader, resolver); On 2017/01/25 10:49:20, yhirano wrote: > You can remove m_loaderAndResolvers.remove(scriptLoader) if you call > m_loaderAndResolvers.set(scriptLoader, resolver) after scriptLoader->fetchScript > successfully finishes. Alternatively, you can call notifyCancelled in > ScriptLoader::fetchScript. I'm not sure which is better. I chose the latter way because it can consolidate paths to settle the resolver into notifyFinished(). This looks simpler. https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/Worklet.cpp:52: ScriptPromiseResolver* resolver = m_loaderAndResolvers.get(scriptLoader); On 2017/01/25 10:49:20, yhirano wrote: > How about calling m_loaderAndREsolvers.remove(scriptLoader) here? Done. https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp (right): https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:16: : m_fetcher(frame->loader().documentLoader()->fetcher()), On 2017/01/25 10:49:20, yhirano wrote: > frame->document()->fetcher() is better? Done. https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:31: m_client->notifyCanceled(this); On 2017/01/25 10:49:20, yhirano wrote: > I think calling clearResource first is slightly better. Ditto below. Done. https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkletScriptLoader.h (right): https://codereview.chromium.org/2657823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkletScriptLoader.h:44: // Fetches an URL and loads it as a classic script. On 2017/01/25 10:49:20, yhirano wrote: > // Returns false if there is an error. notifyCancelled and notifyFinished will > not be called in that case. Made this void-function and always call notifyFinished.
Thank you!
On 2017/01/25 11:32:40, kouhei wrote:
> Thanks! I think this is a great step forward. Let me clarify high-level design
> first.
>
> The current design resembles a classic ResourceClient pattern, but I'd prefer
to
> have this similar to ModuleScriptLoader.
> Specifically:
> - Can we not expose ScriptResource to WorkletScriptLoader client?
> -- After Yukari, we want to have small number of blink::Resource users. ->
Limit
> blink::Resource users to core/loader/**/*
> -- Can we explicitly only expose info Worklet need? Probably just the
> ScriptSourceCode?
Done. Worklet does not access ScriptResource anymore.
> - Can we unify notify{Finished,Cancelled}?
> -- Managing invariants seems much easier then.
> -- Also, I'd prefer to rename this notifyWorkletScriptFinished.
> notify{Finished,Cancelled} suggests this is ResourceClient.
Done. They are merged into notifyWorkletScriptLoadingFinished.
(notifyWorkletScriptLoaded could be better??)
kouhei@chromium.org changed reviewers: + toyoshim@chromium.org
mostly lg https://codereview.chromium.org/2657823002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp (right): https://codereview.chromium.org/2657823002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:17: : m_fetcher(frame->document()->fetcher()), m_client(client) {} Take ResourceFetcher* directly. frame->document()->fetcher() should be provided by Worklet https://codereview.chromium.org/2657823002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkletScriptLoader.h (right): https://codereview.chromium.org/2657823002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkletScriptLoader.h:22: // TODO(nhiroki): Switch to module script loading (https://crbug.com/627945) toyoshim: Should we move this to core/loader/?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you. Updated. https://codereview.chromium.org/2657823002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp (right): https://codereview.chromium.org/2657823002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:17: : m_fetcher(frame->document()->fetcher()), m_client(client) {} On 2017/01/26 11:52:47, kouhei wrote: > Take ResourceFetcher* directly. frame->document()->fetcher() should be provided > by Worklet Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm myside. Up to toyoshim@, yhirano@ https://codereview.chromium.org/2657823002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp (right): https://codereview.chromium.org/2657823002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:36: m_client->notifyWorkletScriptLoadingFinished(this, ScriptSourceCode()); Do we need to clear m_fetcher, m_client here too? (seeing that this is triggerred from contextDestroyed)
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Please see my comments on PS1. https://codereview.chromium.org/2657823002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp (right): https://codereview.chromium.org/2657823002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:19: DCHECK(isMainThread()); DCHECK(!m_wasScriptLoadSuccessful); and probably you don't need L25. https://codereview.chromium.org/2657823002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:36: m_client->notifyWorkletScriptLoadingFinished(this, ScriptSourceCode()); What should happen when cancel() is called after the loading finishes? https://codereview.chromium.org/2657823002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:40: DCHECK(isMainThread()); DCHECK(!m_wasScriptLoadSuccessful);
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated. https://codereview.chromium.org/2657823002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp (right): https://codereview.chromium.org/2657823002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:36: m_client->notifyWorkletScriptLoadingFinished(this, ScriptSourceCode()); On 2017/01/26 17:08:35, kouhei wrote: > Do we need to clear m_fetcher, m_client here too? (seeing that this is > triggerred from contextDestroyed) Done. It's not necessary because the loader should be cleared immediately after cancel(), but it would be saner.
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you! Updated. https://codereview.chromium.org/2657823002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/worklet/resources/import-tests.js (right): https://codereview.chromium.org/2657823002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/worklet/resources/import-tests.js:24: assert_unreached('unexpected rejection: ' + error); On 2017/01/25 10:49:20, yhirano wrote: > You don't need this catch section. Done. https://codereview.chromium.org/2657823002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/worklet/resources/import-tests.js:31: }).catch(function(error) { On 2017/01/25 10:49:20, yhirano wrote: > ditto (though unrelated to this CL) Done. https://codereview.chromium.org/2657823002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp (right): https://codereview.chromium.org/2657823002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:19: DCHECK(isMainThread()); On 2017/01/27 03:46:10, yhirano wrote: > DCHECK(!m_wasScriptLoadSuccessful); > > and probably you don't need L25. Added checks for |resource()| and |m_wasScriptLoadComplete|. https://codereview.chromium.org/2657823002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:36: m_client->notifyWorkletScriptLoadingFinished(this, ScriptSourceCode()); On 2017/01/27 03:46:10, yhirano wrote: > What should happen when cancel() is called after the loading finishes? Done. Added the guard. https://codereview.chromium.org/2657823002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkletScriptLoader.cpp:40: DCHECK(isMainThread()); On 2017/01/27 03:46:10, yhirano wrote: > DCHECK(!m_wasScriptLoadSuccessful); Done.
On 2017/01/27 03:46:10, yhirano wrote: > Please see my comments on PS1. Sorry, I missed these comments...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
no additional comments from my side. lgtm.
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2657823002/#ps200001 (title: "clean up")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2657823002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkletScriptLoader.h (right): https://codereview.chromium.org/2657823002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkletScriptLoader.h:22: // TODO(nhiroki): Switch to module script loading (https://crbug.com/627945) would be nice, but probably in another CL? We also have two (+one more your WIP) different *ScriptLoader.cpp outside core/loader now.
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1485497917433310,
"parent_rev": "90256822ea423fa30c95ed8db9075d414c63fbf2", "commit_rev":
"f1e393e2cab401597d52fe9b7ed90946c5d426d3"}
Message was sent while issue was closed.
Description was changed from ========== Worklet: Straighten layering of worklet script loading This is a preparation to switch Worklet to module script loading. Before this CL, WorkletScriptLoader is entangled with Worklet. Worklet fetches a script and passes it to WorkletScriptLoader with a promise resolver. Then, WorkletScriptLoader loads it and settles the resolver. After this CL, WorkletScriptLoader dedicates to script fetching and loading. WorkletScriptLoader provides Client interface so that a loader client can receive notifications from the loader. Worklet no longer takes care of script fetching. Worklet implements the Client interface to wait for completion of resource loading, evaluates a script and settles a resolver. BUG=627945 ========== to ========== Worklet: Straighten layering of worklet script loading This is a preparation to switch Worklet to module script loading. Before this CL, WorkletScriptLoader is entangled with Worklet. Worklet fetches a script and passes it to WorkletScriptLoader with a promise resolver. Then, WorkletScriptLoader loads it and settles the resolver. After this CL, WorkletScriptLoader dedicates to script fetching and loading. WorkletScriptLoader provides Client interface so that a loader client can receive notifications from the loader. Worklet no longer takes care of script fetching. Worklet implements the Client interface to wait for completion of resource loading, evaluates a script and settles a resolver. BUG=627945 Review-Url: https://codereview.chromium.org/2657823002 Cr-Commit-Position: refs/heads/master@{#446608} Committed: https://chromium.googlesource.com/chromium/src/+/f1e393e2cab401597d52fe9b7ed9... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as https://chromium.googlesource.com/chromium/src/+/f1e393e2cab401597d52fe9b7ed9...
Message was sent while issue was closed.
https://codereview.chromium.org/2657823002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/workers/WorkletScriptLoader.h (right): https://codereview.chromium.org/2657823002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/workers/WorkletScriptLoader.h:22: // TODO(nhiroki): Switch to module script loading (https://crbug.com/627945) On 2017/01/27 06:21:18, toyoshim wrote: > would be nice, but probably in another CL? Thanks! I'll make a follow-up CL :) > We also have two (+one more your WIP) different *ScriptLoader.cpp outside > core/loader now. I lied to you over chat. ModuleScriptLoader is under core/loader :p https://codereview.chromium.org/2555653002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
