Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2389193003:
Refactor BackingThreadHolder to WorkletBackingThreadHolder (Closed)
|
Created:
4 years, 2 months ago by hongchan Modified:
4 years, 2 months ago CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, shimazu+worker_chromium.org, falken, kinuko+worker_chromium.org, darktears, blink-reviews, horo+watch_chromium.org, blink-worker-reviews_chromium.org, Eric Willigers Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor BackingThreadHolder to WorkletThreadHolder
This CL refactors BackingThreadHolder, which was a part of
AbstractAnimationWorkletThread, to core/workers/WorkletThreadHolder.
Because AudioWorkletThread and AnimationWorkletThread share this
backing thread holder, it is reasonable to factor this part out and
make it a part of core/worker.
BUG=652750
TEST=(The build passes AnimationWorkletThreadTest.)
Committed: https://crrev.com/32dd9efba59837681df3260708f25024ede1fe98
Cr-Commit-Position: refs/heads/master@{#424551}
Patch Set 1 #
Total comments: 7
Patch Set 2 : The second attempt #Patch Set 3 : Rebase, comments, formatting after l-g-t-m #Patch Set 4 : Fixing compilation error for try bots #
Messages
Total messages: 31 (21 generated)
hongchan@chromium.org changed reviewers: + nhiroki@chromium.org
PTAL. We need this change to move this forward: https://codereview.chromium.org/2372303002/
https://codereview.chromium.org/2389193003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/WorkletThreadHolder.cpp (right): https://codereview.chromium.org/2389193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkletThreadHolder.cpp:11: #include "public/platform/Platform.h" This inclusion may not be necessary. https://codereview.chromium.org/2389193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkletThreadHolder.cpp:23: void WorkletThreadHolder::ensureInstance(WebThread* backgingThread) { DCHECK(isMainThread()); https://codereview.chromium.org/2389193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkletThreadHolder.cpp:30: void WorkletThreadHolder::clear() { DCHECK(isMainThread()); https://codereview.chromium.org/2389193003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/WorkletThreadHolder.h (right): https://codereview.chromium.org/2389193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkletThreadHolder.h:9: #include "core/workers/WorkerThread.h" "WorkerThread.h" looks unused. https://codereview.chromium.org/2389193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkletThreadHolder.h:24: static void clear(); Can you add a comment about that this may block the caller thread? https://codereview.chromium.org/2389193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkletThreadHolder.h:30: WorkletThreadHolder(std::unique_ptr<WorkerBackingThread>); explicit https://codereview.chromium.org/2389193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkletThreadHolder.h:41: static WorkletThreadHolder* s_instance; It looks like that this instance will be shared among AnimationWorklet and AudioWorklet because this is a static class member. Probably each WorkletThread class should have its own singleton.
hongchan@chromium.org changed reviewers: + haraken@chromium.org
nhiroki@ Thanks for your review.
> It looks like that this instance will be shared among AnimationWorklet and
> AudioWorklet because this is a static class member. Probably each
WorkletThread
> class should have its own singleton.
haraken@ I tried to refactor the current structure into something like this:
WorkletThreadHolder { }
AnimationWorkletThreadHolder : WorkletThreadHolder { }
AudioWorkletThreadHolder : WorkletThreadHolder { }
However, the advantage of sharing common interface is quite small here because
the majority of interfaces are static and the children (and only children) will
be singleton. As nhiroki@ suggested, I can certainly make each
*WorkletThreadHolder class into own signleton, then what is the benefit of this
refactoring?
I think it'd be much simpler to keep these inside of each *WorkletThread class.
Ugly, but it's easy to maintain. If you have a better design in mind, I would
love to hear.
On 2016/10/06 23:04:42, hoch wrote:
> nhiroki@ Thanks for your review.
>
> > It looks like that this instance will be shared among AnimationWorklet and
> > AudioWorklet because this is a static class member. Probably each
> WorkletThread
> > class should have its own singleton.
>
> haraken@ I tried to refactor the current structure into something like this:
>
> WorkletThreadHolder { }
> AnimationWorkletThreadHolder : WorkletThreadHolder { }
> AudioWorkletThreadHolder : WorkletThreadHolder { }
>
> However, the advantage of sharing common interface is quite small here because
> the majority of interfaces are static and the children (and only children)
will
> be singleton. As nhiroki@ suggested, I can certainly make each
> *WorkletThreadHolder class into own signleton, then what is the benefit of
this
> refactoring?
>
> I think it'd be much simpler to keep these inside of each *WorkletThread
class.
> Ugly, but it's easy to maintain. If you have a better design in mind, I would
> love to hear.
Specifically, what will be the difference between AnimationWorkletThreadHolder
and AudioWorkletThreadHolder?
My idea is to put Worklet-independent functionalities to WorkletThreadHolder and
add Worklet-dependent functionalities to *WorkletThread classes.
PTAL. Since the majority of BackingThreadHolder being static, there isn't much to refactor. Let me know what you think.
LGTM. This looks better than sharing nothing, at least. Maybe can we use a C++ template so that we can write like WorkletBackingThreadHolder<AnimationWorklet>::instance()? That way I think you can remove the code duplication in the static methods. You don't need to address the issue in this CL though.
The CQ bit was checked by hongchan@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
The CQ bit was checked by hongchan@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by hongchan@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by hongchan@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/2389193003/#ps60001 (title: "Fixing compilation error for try bots")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Refactor BackingThreadHolder to WorkletThreadHolder This CL refactors BackingThreadHolder, which was a part of AbstractAnimationWorkletThread, to core/workers/WorkletThreadHolder. Because AudioWorkletThread and AnimationWorkletThread share this backing thread holder, it is reasonable to factor this part out and make it a part of core/worker. BUG=652750 TEST=(The build passes AnimationWorkletThreadTest.) ========== to ========== Refactor BackingThreadHolder to WorkletThreadHolder This CL refactors BackingThreadHolder, which was a part of AbstractAnimationWorkletThread, to core/workers/WorkletThreadHolder. Because AudioWorkletThread and AnimationWorkletThread share this backing thread holder, it is reasonable to factor this part out and make it a part of core/worker. BUG=652750 TEST=(The build passes AnimationWorkletThreadTest.) Committed: https://crrev.com/32dd9efba59837681df3260708f25024ede1fe98 Cr-Commit-Position: refs/heads/master@{#424551} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/32dd9efba59837681df3260708f25024ede1fe98 Cr-Commit-Position: refs/heads/master@{#424551} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
