Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(319)

Issue 2819153003: Worklet: Separate Worklet into MainThreadWorklet and ThreadedWorklet (Closed)

Created:
1 year ago by nhiroki
Modified:
1 year ago
Reviewers:
haraken, falken
CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, shimazu+worker_chromium.org, kinuko+worker_chromium.org, Raymond Toy, darktears, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, hongchan, blink-worker-reviews_chromium.org, Eric Willigers
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Worklet: Separate Worklet into MainThreadWorklet and ThreadedWorklet This is a preparation patch for introducing module script loading for main thread worklets. This should not change behavior. In the current implementation, classic script loading code is tied with Worklet class that is the common class of main thread worklets and threaded worklets. This is an obstacle to introduce module script loading only in main thread worklets. This CL separates the Worklet class into MainThreadWorklet and ThreadedWorklet as a stopgap. MainThreadWorklet will implement module script loading in following patches and ThreadedWorklet will keep classic script loading until module script loading becomes available for ThreadedWorklets. BUG=627945 Review-Url: https://codereview.chromium.org/2819153003 Cr-Commit-Position: refs/heads/master@{#465151} Committed: https://chromium.googlesource.com/chromium/src/+/0a5f98358dc00a9959a0b98a88b7e941a6c1f739

Patch Set 1 #

Total comments: 10

Patch Set 2 : address review comments #

Total comments: 6

Patch Set 3 : update comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+166 lines, -116 lines) Patch
M third_party/WebKit/Source/core/workers/BUILD.gn View 2 chunks +4 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/workers/MainThreadWorklet.h View 1 1 chunk +53 lines, -0 lines 0 comments Download
A + third_party/WebKit/Source/core/workers/MainThreadWorklet.cpp View 1 4 chunks +16 lines, -19 lines 0 comments Download
A third_party/WebKit/Source/core/workers/ThreadedWorklet.h View 1 1 chunk +54 lines, -0 lines 0 comments Download
A + third_party/WebKit/Source/core/workers/ThreadedWorklet.cpp View 1 4 chunks +16 lines, -19 lines 0 comments Download
M third_party/WebKit/Source/core/workers/Worklet.h View 1 2 3 chunks +10 lines, -15 lines 0 comments Download
M third_party/WebKit/Source/core/workers/Worklet.cpp View 1 1 chunk +1 line, -51 lines 0 comments Download
M third_party/WebKit/Source/modules/compositorworker/AnimationWorklet.h View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/compositorworker/AnimationWorklet.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/csspaint/PaintWorklet.h View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/csspaint/PaintWorklet.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/webaudio/AudioWorklet.h View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/webaudio/AudioWorklet.cpp View 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 29 (17 generated)
nhiroki
Hi, can you review this? This is a simple separation and should not change the ...
1 year ago (2017-04-17 04:01:00 UTC) #5
falken
overall makes sense https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/core/workers/MainThreadWorklet.h File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right): https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/core/workers/MainThreadWorklet.h#newcode21 third_party/WebKit/Source/core/workers/MainThreadWorklet.h:21: // Worklet class once threaded worklets ...
1 year ago (2017-04-17 05:35:54 UTC) #6
haraken
MainThreadWorklet and WorkerWorklet have a bunch of duplicated code. Can we probably consider sharing more ...
1 year ago (2017-04-17 07:31:56 UTC) #9
nhiroki
Thank you for your comments. I haven't update the patcheset yet (will upload the new ...
1 year ago (2017-04-17 07:51:24 UTC) #10
haraken
LGTM
1 year ago (2017-04-17 07:55:25 UTC) #11
nhiroki
Updated! https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/core/workers/MainThreadWorklet.h File third_party/WebKit/Source/core/workers/MainThreadWorklet.h (right): https://codereview.chromium.org/2819153003/diff/1/third_party/WebKit/Source/core/workers/MainThreadWorklet.h#newcode21 third_party/WebKit/Source/core/workers/MainThreadWorklet.h:21: // Worklet class once threaded worklets are ready ...
1 year ago (2017-04-17 11:35:05 UTC) #13
falken
lgtm https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Source/core/workers/ThreadedWorklet.h File third_party/WebKit/Source/core/workers/ThreadedWorklet.h (right): https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Source/core/workers/ThreadedWorklet.h#newcode19 third_party/WebKit/Source/core/workers/ThreadedWorklet.h:19: // A ThreadedWorklet is a worklet that runs ...
1 year ago (2017-04-18 03:37:53 UTC) #18
nhiroki
Thank you! https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Source/core/workers/ThreadedWorklet.h File third_party/WebKit/Source/core/workers/ThreadedWorklet.h (right): https://codereview.chromium.org/2819153003/diff/40001/third_party/WebKit/Source/core/workers/ThreadedWorklet.h#newcode19 third_party/WebKit/Source/core/workers/ThreadedWorklet.h:19: // A ThreadedWorklet is a worklet that ...
1 year ago (2017-04-18 04:05:20 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2819153003/60001
1 year ago (2017-04-18 04:05:42 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/196117)
1 year ago (2017-04-18 05:31:25 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2819153003/60001
1 year ago (2017-04-18 05:35:28 UTC) #26
commit-bot: I haz the power
1 year ago (2017-04-18 06:16:31 UTC) #29
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/0a5f98358dc00a9959a0b98a88b7...

Powered by Google App Engine
This is Rietveld 408576698