Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2214263007:
[worklets] Split apart CompositorWorkerThread for sharing with AnimationWorkletThread. (Closed)
|
Created:
4 years, 4 months ago by ikilpatrick Modified:
4 years, 4 months ago CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, haraken, darktears, blink-reviews, Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[worklets] Split apart CompositorWorkerThread for sharing with AnimationWorkletThread.
This splits apart CompositorWorkerThread into:
- CompositorWorkerThread
- AbstractAnimationWorkletThread
And add AnimationWorkletThread which inherits from AbstractAnimationWorkletThread.
AnimationWorkletThread is basically a place holder class at the moment
which needs createWorkerGlobalScope and consoleMessageStorage
implemented (which will be implemented once AnimationWorkletGS is
checked in).
BUG=567358
Committed: https://crrev.com/d82384e62af55a9aae1182b4ae4c19dac325910a
Cr-Commit-Position: refs/heads/master@{#411052}
Patch Set 1 #Patch Set 2 : fix spelling. #
Total comments: 10
Patch Set 3 : address comments. #
Total comments: 2
Patch Set 4 : address comments. #
Total comments: 2
Patch Set 5 : rename abstract class. #Messages
Total messages: 33 (19 generated)
The CQ bit was checked by ikilpatrick@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 ikilpatrick@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 ========== [worklets] Split apart CompositorWorkerThread far sharing with AnimationWorkletThread. This splits apart CompositorWorkerThread into: - CompositorWorkerThread - AbstractCompositorWorkerThread And add AnimationWorkletThread which inherits from AbstractCompositorWorkerThread. AnimationWorkletThread is basically a place holder class at the moment which needs createWorkerGlobalScope and consoleMessageStorage implemented (which will be implemented once AnimationWorkletGS is checked in). BUG=567358 ========== to ========== [worklets] Split apart CompositorWorkerThread for sharing with AnimationWorkletThread. This splits apart CompositorWorkerThread into: - CompositorWorkerThread - AbstractCompositorWorkerThread And add AnimationWorkletThread which inherits from AbstractCompositorWorkerThread. AnimationWorkletThread is basically a place holder class at the moment which needs createWorkerGlobalScope and consoleMessageStorage implemented (which will be implemented once AnimationWorkletGS is checked in). BUG=567358 ==========
ikilpatrick@chromium.org changed reviewers: + majidvp@chromium.org, nhiroki@chromium.org, yhirano@chromium.org
Suggestions welcome for better name than AbstractCompositorWorkerThread :) should only be a temporary class.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.cpp (right): https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.cpp:10: #include "platform/WaitableEvent.h" CrossThreadFunctional, TraceEvent and WaitanleEvent headers seem unused. Please remove. https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.cpp:98: static BackingThreadHolder* s_instance; Is |BackingThreadHolder| accessible here? You have removed it from CompositorWorkerThread but it is not redefined here. https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.h (right): https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.h:31: AbstractCompositorWorkerThread(PassRefPtr<WorkerLoaderProxy>, WorkerReportingProxy&); I believe you need to #include WorkerLoaderProxy.h https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletThread.cpp (right): https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletThread.cpp:16: return wrapUnique(new AnimationWorkletThread(workerLoaderProxy, workerReportingProxy)); include "wtf/PtrUtil.h"
https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.cpp (right): https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.cpp:10: #include "platform/WaitableEvent.h" CrossThreadFunctional, TraceEvent and WaitanleEvent headers seem unused. Please remove. https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.cpp:98: static BackingThreadHolder* s_instance; Is |BackingThreadHolder| accessible here? You have removed it from CompositorWorkerThread but it is not redefined here. https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.h (right): https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.h:31: AbstractCompositorWorkerThread(PassRefPtr<WorkerLoaderProxy>, WorkerReportingProxy&); I believe you need to #include WorkerLoaderProxy.h https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletThread.cpp (right): https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletThread.cpp:16: return wrapUnique(new AnimationWorkletThread(workerLoaderProxy, workerReportingProxy)); include "wtf/PtrUtil.h"
The CQ bit was checked by ikilpatrick@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/2214263007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.cpp (right): https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.cpp:10: #include "platform/WaitableEvent.h" On 2016/08/08 12:46:48, majidvp wrote: > CrossThreadFunctional, TraceEvent and WaitanleEvent headers > seem unused. Please remove. Removed TraceEvent, CrossThreadFunctional used for crossThreadBind below; WaitableEvent also used below. https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.cpp:98: static BackingThreadHolder* s_instance; On 2016/08/08 12:46:48, majidvp wrote: > Is |BackingThreadHolder| accessible here? You have removed it from > CompositorWorkerThread but it is not redefined here. Redefined just above? https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.h (right): https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.h:31: AbstractCompositorWorkerThread(PassRefPtr<WorkerLoaderProxy>, WorkerReportingProxy&); On 2016/08/08 12:46:48, majidvp wrote: > I believe you need to #include WorkerLoaderProxy.h Done. https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletThread.cpp (right): https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletThread.cpp:16: return wrapUnique(new AnimationWorkletThread(workerLoaderProxy, workerReportingProxy)); On 2016/08/08 12:46:48, majidvp wrote: > include "wtf/PtrUtil.h" Done.
lgtm with nit. https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.cpp (right): https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.cpp:10: #include "platform/WaitableEvent.h" On 2016/08/08 13:54:58, ikilpatrick wrote: > On 2016/08/08 12:46:48, majidvp wrote: > > CrossThreadFunctional, TraceEvent and WaitanleEvent headers > > seem unused. Please remove. > > Removed TraceEvent, CrossThreadFunctional used for crossThreadBind below; > WaitableEvent also used below. Acknowledged. https://codereview.chromium.org/2214263007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.cpp:98: static BackingThreadHolder* s_instance; On 2016/08/08 13:54:57, ikilpatrick wrote: > On 2016/08/08 12:46:48, majidvp wrote: > > Is |BackingThreadHolder| accessible here? You have removed it from > > CompositorWorkerThread but it is not redefined here. > > Redefined just above? Acknowledged. Sorry I missed it! https://codereview.chromium.org/2214263007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletThread.cpp (right): https://codereview.chromium.org/2214263007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletThread.cpp:15: TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("compositor-worker"), "AnimationWorkletThread::create"); nit: should the trace category be "animation-worklet"?
Cool, thanks Majid! https://codereview.chromium.org/2214263007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletThread.cpp (right): https://codereview.chromium.org/2214263007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletThread.cpp:15: TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("compositor-worker"), "AnimationWorkletThread::create"); On 2016/08/08 14:43:07, majidvp wrote: > nit: should the trace category be "animation-worklet"? Done.
lgtm https://codereview.chromium.org/2214263007/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.h (right): https://codereview.chromium.org/2214263007/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.h:19: class MODULES_EXPORT AbstractCompositorWorkerThread : public WorkerThread { AnimationWorklet is CompositorWorklet, right? If we'll deprecate Compositor* prefix, how about AbstractAnimationWorkerThread or AbstractAnimationWorkletThread?
Sorry for a naive question. CompositorWorklet, CompositorWorker and AnimationWorklet share the same backing thread, is that OK?
The CQ bit was checked by ikilpatrick@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...
On 2016/08/09 08:45:14, yhirano (slow) wrote: > Sorry for a naive question. CompositorWorklet, CompositorWorker and > AnimationWorklet share the same backing thread, is that OK? Yeah this is ok; by design they are both meant to run on the compositor thread at the moment. The current plan is to expose the current CompositorWorker API behind an origin trial for a couple of origins; but AnimationWorklet wont be ready by that stage. After the CW origin trail (& implementing AnimationWorklet) the CompositorWorker API will be deleted, and we'll only have AnimationWorklet. My guess is that we'll do another origin trial with the AnimationWorklet API after that.
https://codereview.chromium.org/2214263007/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.h (right): https://codereview.chromium.org/2214263007/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AbstractCompositorWorkerThread.h:19: class MODULES_EXPORT AbstractCompositorWorkerThread : public WorkerThread { On 2016/08/09 00:54:11, nhiroki (ooo until Aug 15) wrote: > AnimationWorklet is CompositorWorklet, right? If we'll deprecate Compositor* > prefix, how about AbstractAnimationWorkerThread or > AbstractAnimationWorkletThread? Done.
Description was changed from ========== [worklets] Split apart CompositorWorkerThread for sharing with AnimationWorkletThread. This splits apart CompositorWorkerThread into: - CompositorWorkerThread - AbstractCompositorWorkerThread And add AnimationWorkletThread which inherits from AbstractCompositorWorkerThread. AnimationWorkletThread is basically a place holder class at the moment which needs createWorkerGlobalScope and consoleMessageStorage implemented (which will be implemented once AnimationWorkletGS is checked in). BUG=567358 ========== to ========== [worklets] Split apart CompositorWorkerThread for sharing with AnimationWorkletThread. This splits apart CompositorWorkerThread into: - CompositorWorkerThread - AbstractAnimationWorkletThread And add AnimationWorkletThread which inherits from AbstractAnimationWorkletThread. AnimationWorkletThread is basically a place holder class at the moment which needs createWorkerGlobalScope and consoleMessageStorage implemented (which will be implemented once AnimationWorkletGS is checked in). BUG=567358 ==========
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
The CQ bit was checked by ikilpatrick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from majidvp@chromium.org, nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/2214263007/#ps80001 (title: "rename abstract class.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [worklets] Split apart CompositorWorkerThread for sharing with AnimationWorkletThread. This splits apart CompositorWorkerThread into: - CompositorWorkerThread - AbstractAnimationWorkletThread And add AnimationWorkletThread which inherits from AbstractAnimationWorkletThread. AnimationWorkletThread is basically a place holder class at the moment which needs createWorkerGlobalScope and consoleMessageStorage implemented (which will be implemented once AnimationWorkletGS is checked in). BUG=567358 ========== to ========== [worklets] Split apart CompositorWorkerThread for sharing with AnimationWorkletThread. This splits apart CompositorWorkerThread into: - CompositorWorkerThread - AbstractAnimationWorkletThread And add AnimationWorkletThread which inherits from AbstractAnimationWorkletThread. AnimationWorkletThread is basically a place holder class at the moment which needs createWorkerGlobalScope and consoleMessageStorage implemented (which will be implemented once AnimationWorkletGS is checked in). BUG=567358 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [worklets] Split apart CompositorWorkerThread for sharing with AnimationWorkletThread. This splits apart CompositorWorkerThread into: - CompositorWorkerThread - AbstractAnimationWorkletThread And add AnimationWorkletThread which inherits from AbstractAnimationWorkletThread. AnimationWorkletThread is basically a place holder class at the moment which needs createWorkerGlobalScope and consoleMessageStorage implemented (which will be implemented once AnimationWorkletGS is checked in). BUG=567358 ========== to ========== [worklets] Split apart CompositorWorkerThread for sharing with AnimationWorkletThread. This splits apart CompositorWorkerThread into: - CompositorWorkerThread - AbstractAnimationWorkletThread And add AnimationWorkletThread which inherits from AbstractAnimationWorkletThread. AnimationWorkletThread is basically a place holder class at the moment which needs createWorkerGlobalScope and consoleMessageStorage implemented (which will be implemented once AnimationWorkletGS is checked in). BUG=567358 Committed: https://crrev.com/d82384e62af55a9aae1182b4ae4c19dac325910a Cr-Commit-Position: refs/heads/master@{#411052} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d82384e62af55a9aae1182b4ae4c19dac325910a Cr-Commit-Position: refs/heads/master@{#411052} |
