Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(138)

Issue 1449953002: compositor-worker: Refactor CompositorWorkerManager (Closed)

Created:
5 years, 1 month ago by Ian Vollick
Modified:
4 years, 10 months ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, blink-reviews, dglazkov+blink, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, blink-reviews-api_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

compositor-worker: Refactor CompositorWorkerManager Much of the functionality in CompositorWorkerManager was related to shared compositor thread state. This patch moves that to CompositorWorkerSharedState, an implementation detail of CompositorWorkerThread. The shared state also now uses the real compositor thread. The existing compositor worker layout tests needed to be moved to a virtual test suite since they now depend on having a compositor thread. BUG=430155 Committed: https://crrev.com/d710cbc079b51b819349203ee408f36dc056055e Cr-Commit-Position: refs/heads/master@{#362590} Committed: https://crrev.com/1050cc60596ea57ff3bef6185ee788ca4b9a4730 Cr-Commit-Position: refs/heads/master@{#362924}

Patch Set 1 #

Patch Set 2 : fix build. #

Patch Set 3 : No more CompositorWorkerManager #

Patch Set 4 : fix unit test #

Patch Set 5 : rebase #

Patch Set 6 : fix components_unittests #

Patch Set 7 : fix components_unittests #

Patch Set 8 : fix layout tests. #

Patch Set 9 : #

Patch Set 10 : #

Total comments: 8

Patch Set 11 : . #

Total comments: 2

Patch Set 12 : Address reviewer comments. #

Total comments: 8

Patch Set 13 : . #

Patch Set 14 : . #

Patch Set 15 : clarify compositor thread ownership. guarantee initialization correctness. #

Total comments: 4

Patch Set 16 : Address reviewer feedback. #

Patch Set 17 : Fix hang in webexposed layout test. #

Patch Set 18 : guarantee backing thread deletion happens after shutdown is done. #

Total comments: 1

Patch Set 19 : go back to the approach in ps 17. #

Total comments: 3

Patch Set 20 : Add documentation and ASSERTS for some subtleties. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+362 lines, -802 lines) Patch
M components/scheduler/child/webthread_impl_for_worker_scheduler.h View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download
M components/scheduler/child/webthread_impl_for_worker_scheduler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +7 lines, -2 lines 0 comments Download
M content/child/blink_platform_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +20 lines, -1 line 0 comments Download
M content/child/blink_platform_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 5 chunks +23 lines, -6 lines 0 comments Download
M content/renderer/render_thread_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +4 lines, -1 line 0 comments Download
M content/renderer/render_thread_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 5 chunks +61 lines, -55 lines 0 comments Download
M third_party/WebKit/LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/VirtualTestSuites View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/compositorworker/compositor-attribute-change.html View 1 2 3 4 5 6 7 1 chunk +0 lines, -13 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/compositorworker/compositor-attribute-change-worker.html View 1 2 3 4 5 6 7 1 chunk +0 lines, -23 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/compositorworker/compositor-proxy-disconnect.html View 1 2 3 4 5 6 7 1 chunk +0 lines, -16 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/compositorworker/compositor-proxy-disconnect-worker.html View 1 2 3 4 5 6 7 1 chunk +0 lines, -19 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/compositorworker/compositor-proxy-disconnect-worker-terminate.html View 1 2 3 4 5 6 7 1 chunk +0 lines, -37 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/compositorworker/compositor-proxy-postmessage.html View 1 2 3 4 5 6 7 1 chunk +0 lines, -46 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/compositorworker/resources/proxy-disconnect.js View 1 2 3 4 5 6 7 1 chunk +0 lines, -10 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/compositorworker/resources/proxy-echo.js View 1 2 3 4 5 6 7 1 chunk +0 lines, -7 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/compositorworker/resources/proxy-idle.js View 1 2 3 4 5 6 7 1 chunk +0 lines, -5 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/compositorworker/resources/proxy-mutate.js View 1 2 3 4 5 6 7 1 chunk +0 lines, -14 lines 0 comments Download
A third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/README.txt View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A + third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-attribute-change.html View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
A + third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-attribute-change-worker.html View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
A + third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-disconnect.html View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
A + third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-disconnect-worker.html View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
A + third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-disconnect-worker-terminate.html View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -3 lines 0 comments Download
A + third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-postmessage.html View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
A + third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-disconnect.js View 1 2 3 4 5 6 7 0 chunks +-1 lines, --1 lines 0 comments Download
A + third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-echo.js View 1 2 3 4 5 6 7 0 chunks +-1 lines, --1 lines 0 comments Download
A + third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-idle.js View 1 2 3 4 5 6 7 0 chunks +-1 lines, --1 lines 0 comments Download
A + third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/resources/proxy-mutate.js View 1 2 3 4 5 6 7 0 chunks +-1 lines, --1 lines 0 comments Download
M third_party/WebKit/Source/modules/InitModules.cpp View 3 chunks +0 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/modules/compositorworker/CompositorWorker.h View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -1 line 0 comments Download
D third_party/WebKit/Source/modules/compositorworker/CompositorWorkerManager.h View 1 2 1 chunk +0 lines, -58 lines 0 comments Download
D third_party/WebKit/Source/modules/compositorworker/CompositorWorkerManager.cpp View 1 2 1 chunk +0 lines, -152 lines 0 comments Download
D third_party/WebKit/Source/modules/compositorworker/CompositorWorkerManagerTest.cpp View 1 2 3 4 1 chunk +0 lines, -279 lines 0 comments Download
M third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.h View 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +154 lines, -8 lines 0 comments Download
A + third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThreadTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 16 chunks +56 lines, -30 lines 0 comments Download
M third_party/WebKit/Source/modules/modules.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +1 line, -3 lines 0 comments Download
M third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/Platform.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 69 (24 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1449953002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1449953002/140001
5 years ago (2015-11-27 04:22:15 UTC) #8
Ian Vollick
rmcilroy@chromium.org: Please review changes in components/scheduler/* piman@chromium.org: Please review changes in content/* haraken@chromium.org: Please review ...
5 years ago (2015-11-27 15:11:16 UTC) #10
Ian Vollick
+rbyers for Platform.h
5 years ago (2015-11-27 15:21:12 UTC) #11
sadrul
Some nits: https://codereview.chromium.org/1449953002/diff/180001/content/child/blink_platform_impl.cc File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/1449953002/diff/180001/content/child/blink_platform_impl.cc#newcode545 content/child/blink_platform_impl.cc:545: base::Thread::Options options; DCHECK(!compositor_thread_)? https://codereview.chromium.org/1449953002/diff/180001/content/renderer/render_thread_impl.cc File content/renderer/render_thread_impl.cc (right): ...
5 years ago (2015-11-27 19:03:08 UTC) #14
Ian Vollick
https://codereview.chromium.org/1449953002/diff/180001/content/child/blink_platform_impl.cc File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/1449953002/diff/180001/content/child/blink_platform_impl.cc#newcode545 content/child/blink_platform_impl.cc:545: base::Thread::Options options; On 2015/11/27 19:03:07, sadrul wrote: > DCHECK(!compositor_thread_)? ...
5 years ago (2015-11-27 19:56:11 UTC) #15
Rick Byers
WebKit/public LGTM with nit https://codereview.chromium.org/1449953002/diff/190001/third_party/WebKit/public/platform/Platform.h File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/1449953002/diff/190001/third_party/WebKit/public/platform/Platform.h#newcode437 third_party/WebKit/public/platform/Platform.h:437: virtual WebThread* compositorThread() const { ...
5 years ago (2015-11-27 20:50:40 UTC) #16
Ian Vollick
https://codereview.chromium.org/1449953002/diff/190001/third_party/WebKit/public/platform/Platform.h File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/1449953002/diff/190001/third_party/WebKit/public/platform/Platform.h#newcode437 third_party/WebKit/public/platform/Platform.h:437: virtual WebThread* compositorThread() const { return 0; } On ...
5 years ago (2015-11-27 21:15:11 UTC) #17
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1449953002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1449953002/210001
5 years ago (2015-11-27 22:18:02 UTC) #19
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years ago (2015-11-27 23:31:17 UTC) #21
haraken
modules/ LGTM
5 years ago (2015-11-29 23:49:49 UTC) #22
Ian Vollick
On 2015/11/29 23:49:49, haraken wrote: > modules/ LGTM +skyostil for components/scheduler/
5 years ago (2015-11-30 19:04:34 UTC) #25
Sami
components/scheduler/ lgtm.
5 years ago (2015-11-30 19:21:38 UTC) #26
piman
https://codereview.chromium.org/1449953002/diff/210001/components/scheduler/child/webthread_impl_for_worker_scheduler.cc File components/scheduler/child/webthread_impl_for_worker_scheduler.cc (right): https://codereview.chromium.org/1449953002/diff/210001/components/scheduler/child/webthread_impl_for_worker_scheduler.cc#newcode28 components/scheduler/child/webthread_impl_for_worker_scheduler.cc:28: : thread_(new base::Thread(name ? name : "")) { nit: ...
5 years ago (2015-11-30 21:41:45 UTC) #27
Ian Vollick
https://codereview.chromium.org/1449953002/diff/210001/components/scheduler/child/webthread_impl_for_worker_scheduler.cc File components/scheduler/child/webthread_impl_for_worker_scheduler.cc (right): https://codereview.chromium.org/1449953002/diff/210001/components/scheduler/child/webthread_impl_for_worker_scheduler.cc#newcode28 components/scheduler/child/webthread_impl_for_worker_scheduler.cc:28: : thread_(new base::Thread(name ? name : "")) { On ...
5 years ago (2015-12-01 04:00:04 UTC) #28
piman
https://codereview.chromium.org/1449953002/diff/210001/content/child/blink_platform_impl.cc File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/1449953002/diff/210001/content/child/blink_platform_impl.cc#newcode551 content/child/blink_platform_impl.cc:551: new scheduler::WebThreadImplForWorkerScheduler(name, options); On 2015/12/01 04:00:04, vollick wrote: > ...
5 years ago (2015-12-01 04:24:11 UTC) #29
Ian Vollick
https://codereview.chromium.org/1449953002/diff/210001/content/child/blink_platform_impl.cc File content/child/blink_platform_impl.cc (right): https://codereview.chromium.org/1449953002/diff/210001/content/child/blink_platform_impl.cc#newcode551 content/child/blink_platform_impl.cc:551: new scheduler::WebThreadImplForWorkerScheduler(name, options); On 2015/12/01 04:24:10, piman (slow to ...
5 years ago (2015-12-01 05:01:08 UTC) #30
Ian Vollick
On 2015/12/01 05:01:08, vollick wrote: > https://codereview.chromium.org/1449953002/diff/210001/content/child/blink_platform_impl.cc > File content/child/blink_platform_impl.cc (right): > > https://codereview.chromium.org/1449953002/diff/210001/content/child/blink_platform_impl.cc#newcode551 > ...
5 years ago (2015-12-01 19:07:50 UTC) #31
piman
https://codereview.chromium.org/1449953002/diff/270001/content/child/blink_platform_impl.h File content/child/blink_platform_impl.h (right): https://codereview.chromium.org/1449953002/diff/270001/content/child/blink_platform_impl.h#newcode195 content/child/blink_platform_impl.h:195: blink::WebThread* createThreadWithOptions(const char* name, nit: can you have this ...
5 years ago (2015-12-01 23:44:41 UTC) #32
piman
On Tue, Dec 1, 2015 at 3:44 PM, <piman@chromium.org> wrote: > > Really sorry to ...
5 years ago (2015-12-02 00:12:25 UTC) #33
piman
On Tue, Dec 1, 2015 at 3:44 PM, <piman@chromium.org> wrote: > > Really sorry to ...
5 years ago (2015-12-02 00:12:25 UTC) #34
Ian Vollick
https://codereview.chromium.org/1449953002/diff/270001/content/child/blink_platform_impl.h File content/child/blink_platform_impl.h (right): https://codereview.chromium.org/1449953002/diff/270001/content/child/blink_platform_impl.h#newcode195 content/child/blink_platform_impl.h:195: blink::WebThread* createThreadWithOptions(const char* name, On 2015/12/01 23:44:41, piman (slow ...
5 years ago (2015-12-02 00:26:40 UTC) #35
Ian Vollick
On 2015/12/02 00:12:25, piman (slow to review) wrote: > On Tue, Dec 1, 2015 at ...
5 years ago (2015-12-02 00:30:08 UTC) #36
piman
LGTM, thanks!
5 years ago (2015-12-02 01:01:48 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1449953002/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1449953002/290001
5 years ago (2015-12-02 01:26:26 UTC) #40
commit-bot: I haz the power
Committed patchset #16 (id:290001)
5 years ago (2015-12-02 02:25:39 UTC) #42
commit-bot: I haz the power
Patchset 16 (id:??) landed as https://crrev.com/d710cbc079b51b819349203ee408f36dc056055e Cr-Commit-Position: refs/heads/master@{#362590}
5 years ago (2015-12-02 02:26:44 UTC) #44
tkent
A revert of this CL (patchset #16 id:290001) has been created in https://codereview.chromium.org/1494483002/ by tkent@chromium.org. ...
5 years ago (2015-12-02 06:08:42 UTC) #45
Ian Vollick
On 2015/12/02 06:08:42, tkent wrote: > A revert of this CL (patchset #16 id:290001) has ...
5 years ago (2015-12-02 21:40:05 UTC) #47
Rick Byers
On 2015/12/02 21:40:05, vollick wrote: > On 2015/12/02 06:08:42, tkent wrote: > > A revert ...
5 years ago (2015-12-02 21:49:06 UTC) #48
Ian Vollick
On 2015/12/02 21:40:05, vollick wrote: > On 2015/12/02 06:08:42, tkent wrote: > > A revert ...
5 years ago (2015-12-02 21:49:32 UTC) #49
haraken
https://codereview.chromium.org/1449953002/diff/300042/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1449953002/diff/300042/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp#newcode136 third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:136: HashMap<const CompositorWorkerThread*, OwnPtr<WebThreadSupportingGC>> m_markedForDeletion; Why do we need the ...
5 years ago (2015-12-03 04:04:22 UTC) #50
Ian Vollick
On 2015/12/03 04:04:22, haraken wrote: > https://codereview.chromium.org/1449953002/diff/300042/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp > File > third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp > (right): > > ...
5 years ago (2015-12-03 04:20:12 UTC) #51
haraken
On 2015/12/03 04:20:12, vollick wrote: > On 2015/12/03 04:04:22, haraken wrote: > > > https://codereview.chromium.org/1449953002/diff/300042/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp ...
5 years ago (2015-12-03 04:38:58 UTC) #52
Ian Vollick
On 2015/12/03 04:38:58, haraken wrote: > On 2015/12/03 04:20:12, vollick wrote: > > On 2015/12/03 ...
5 years ago (2015-12-03 05:04:19 UTC) #53
haraken
The latest patch set LGTM. https://codereview.chromium.org/1449953002/diff/340001/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1449953002/diff/340001/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp#newcode22 third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:22: { It would be ...
5 years ago (2015-12-03 05:19:31 UTC) #54
Ian Vollick
On 2015/12/03 05:19:31, haraken wrote: > The latest patch set LGTM. > > https://codereview.chromium.org/1449953002/diff/340001/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp > ...
5 years ago (2015-12-03 05:52:22 UTC) #55
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1449953002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1449953002/360001
5 years ago (2015-12-03 05:53:01 UTC) #58
commit-bot: I haz the power
Committed patchset #20 (id:360001)
5 years ago (2015-12-03 07:05:04 UTC) #60
commit-bot: I haz the power
Patchset 20 (id:??) landed as https://crrev.com/1050cc60596ea57ff3bef6185ee788ca4b9a4730 Cr-Commit-Position: refs/heads/master@{#362924}
5 years ago (2015-12-03 07:05:47 UTC) #62
yhirano
https://codereview.chromium.org/1449953002/diff/340001/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1449953002/diff/340001/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp#newcode22 third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:22: { On 2015/12/03 05:19:31, haraken wrote: > > It ...
4 years, 10 months ago (2016-02-02 06:38:20 UTC) #64
haraken
https://codereview.chromium.org/1449953002/diff/340001/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp File third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp (right): https://codereview.chromium.org/1449953002/diff/340001/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp#newcode22 third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:22: { On 2016/02/02 06:38:20, yhirano wrote: > On 2015/12/03 ...
4 years, 10 months ago (2016-02-02 07:07:56 UTC) #65
haraken
On 2016/02/02 07:07:56, haraken wrote: > https://codereview.chromium.org/1449953002/diff/340001/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp > File > third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp > (right): > > ...
4 years, 10 months ago (2016-02-02 07:08:17 UTC) #66
yhirano
On 2016/02/02 07:08:17, haraken wrote: > On 2016/02/02 07:07:56, haraken wrote: > > > https://codereview.chromium.org/1449953002/diff/340001/third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp ...
4 years, 10 months ago (2016-02-02 07:49:39 UTC) #67
haraken
On 2016/02/02 07:49:39, yhirano wrote: > On 2016/02/02 07:08:17, haraken wrote: > > On 2016/02/02 ...
4 years, 10 months ago (2016-02-02 07:50:40 UTC) #68
yhirano
4 years, 10 months ago (2016-02-02 11:00:19 UTC) #69
Message was sent while issue was closed.
On 2016/02/02 07:50:40, haraken wrote:
> On 2016/02/02 07:49:39, yhirano wrote:
> > On 2016/02/02 07:08:17, haraken wrote:
> > > On 2016/02/02 07:07:56, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1449953002/diff/340001/third_party/WebKit/Sou...
> > > > File
> > > >
> >
third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1449953002/diff/340001/third_party/WebKit/Sou...
> > > >
> > >
> >
>
third_party/WebKit/Source/modules/compositorworker/CompositorWorkerThread.cpp:22:
> > > > {
> > > > On 2016/02/02 06:38:20, yhirano wrote:
> > > > > On 2015/12/03 05:19:31, haraken wrote:
> > > > > > 
> > > > > > It would be helpful to add ASSERT(isMainThread()) for a documenting
> > > purpose.
> > > > > > Also it would be helpful to have a comment and mention that this
> blocks
> > > > until
> > > > > > the worker has processed all tasks.
> > > > > > 
> > > > > > 
> > > > > 
> > > > > Hi haraken@, I wanna understand the comment. Is this really a blocking
> > > > > operation, given that the WebThreadSupportingGC is created from
> > > > > |createForThread| and doesn't own the backing thread?
> > > > 
> > > > Good point. A blocking operation happens in the WebThreadSupportingGC's
> > > > destructor. Unless you don't destruct it, it won't block.
> > > 
> > > s/Unless you don't destruct it/Unless you destruct it/
> > 
> > > A blocking operation happens in the WebThreadSupportingGC's destructor.
> > WebThread's destructor, you mean?
> 
> ah, yes.

Thanks!

Powered by Google App Engine
This is Rietveld 408576698