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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 1 week ago by nhiroki
Modified:
6 months, 1 week 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 29 (17 generated)
nhiroki
Hi, can you review this? This is a simple separation and should not change the ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week ago (2017-04-17 07:51:24 UTC) #10
haraken
LGTM
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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 ...
6 months, 1 week 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
6 months, 1 week 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)
6 months, 1 week 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
6 months, 1 week ago (2017-04-18 05:35:28 UTC) #26
commit-bot: I haz the power
6 months, 1 week 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa