Makes WebThreadSupportingGC constructible for an existing thread

Makes WebThreadSupportingGC constructible for an existing thread

Alternative approach for http://crrev.com/1274023003

Add WebThreadSupportingGC::createForThread(), which takes an
existing thread and add GC support for the thread.

If a thread is not given WebThreadSupportingGC creates a new thread
and owns it, otherwise it just uses the given thread.

BUG=518708
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201549

[kinuko, 2015-08-31 12:01:34]

kinuko@chromium.org changed reviewers:
+ haraken@chromium.org, sadrul@chromium.org

[kinuko, 2015-08-31 12:10:13]

Patchset #1 (id:1) has been deleted

[kinuko, 2015-08-31 12:10:24]

Patchset #1 (id:1) has been deleted

[kinuko, 2015-08-31 12:12:48]

To me this doesn't feel terribly bad, or it actually looks simpler than
https://codereview.chromium.org/1274023003, but I'd like to have your opinion
here.  (The code around constructors could be probably cleaned up a bit further)

The reason I prefer this approach is that this allows us to keep the thread + GC
concept just encapsulated in the WebThreadSupportingGC class, so that anyone who
wants to use a thread (with GC) can just create a new WebThreadSupportingGC,
regardless of whether it wants to use an existing platform thread or not, which
feels easy to understand.  How does this look to you?

[haraken, 2015-08-31 13:10:36]

On 2015/08/31 12:12:48, kinuko wrote:
> To me this doesn't feel terribly bad, or it actually looks simpler than
> https://codereview.chromium.org/1274023003, but I'd like to have your opinion
> here.  (The code around constructors could be probably cleaned up a bit
further)
> 
> The reason I prefer this approach is that this allows us to keep the thread +
GC
> concept just encapsulated in the WebThreadSupportingGC class, so that anyone
who
> wants to use a thread (with GC) can just create a new WebThreadSupportingGC,
> regardless of whether it wants to use an existing platform thread or not,
which
> feels easy to understand.  How does this look to you?

I'm fine with this approach.

[sadrul, 2015-09-01 09:25:34]

On 2015/08/31 13:10:36, haraken wrote:
> On 2015/08/31 12:12:48, kinuko wrote:
> > To me this doesn't feel terribly bad, or it actually looks simpler than
> > https://codereview.chromium.org/1274023003, but I'd like to have your
opinion
> > here.  (The code around constructors could be probably cleaned up a bit
> further)
> > 
> > The reason I prefer this approach is that this allows us to keep the thread
+
> GC
> > concept just encapsulated in the WebThreadSupportingGC class, so that anyone
> who
> > wants to use a thread (with GC) can just create a new WebThreadSupportingGC,
> > regardless of whether it wants to use an existing platform thread or not,
> which
> > feels easy to understand.  How does this look to you?
> 
> I'm fine with this approach.

Same here.

Thank you for making this change!

[haraken, 2015-09-01 09:32:38]

LGTM

https://codereview.chromium.org/1319363002/diff/20001/Source/modules/composit...
File Source/modules/compositorworker/CompositorWorkerManager.cpp (right):

https://codereview.chromium.org/1319363002/diff/20001/Source/modules/composit...
Source/modules/compositorworker/CompositorWorkerManager.cpp:110: m_thread =
nullptr;

Nit: We may want to run m_thread=nullptr before posting a task to destruct
m_thread and m_platformThread in the reverse order in which they are created.

https://codereview.chromium.org/1319363002/diff/20001/Source/platform/WebThre...
File Source/platform/WebThreadSupportingGC.cpp (right):

https://codereview.chromium.org/1319363002/diff/20001/Source/platform/WebThre...
Source/platform/WebThreadSupportingGC.cpp:42: if (ThreadState::current()) {

if (ThreadState::current() && m_owningThread)

It is redundant to enter a safe point if it doesn't have an owning thread.

[kinuko, 2015-09-01 09:48:56]

Thanks both!

https://codereview.chromium.org/1319363002/diff/20001/Source/modules/composit...
File Source/modules/compositorworker/CompositorWorkerManager.cpp (right):

https://codereview.chromium.org/1319363002/diff/20001/Source/modules/composit...
Source/modules/compositorworker/CompositorWorkerManager.cpp:110: m_thread =
nullptr;
On 2015/09/01 09:32:37, haraken wrote:
> 
> Nit: We may want to run m_thread=nullptr before posting a task to destruct
> m_thread and m_platformThread in the reverse order in which they are created.
> 

Done.

https://codereview.chromium.org/1319363002/diff/20001/Source/platform/WebThre...
File Source/platform/WebThreadSupportingGC.cpp (right):

https://codereview.chromium.org/1319363002/diff/20001/Source/platform/WebThre...
Source/platform/WebThreadSupportingGC.cpp:42: if (ThreadState::current()) {
On 2015/09/01 09:32:37, haraken wrote:
> 
> if (ThreadState::current() && m_owningThread)
> 
> It is redundant to enter a safe point if it doesn't have an owning thread.

Done.

[kinuko, 2015-09-01 09:49:00]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2015-09-01 09:49:00]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1319363002/#ps40001
(title: "addressed comments")

[commit-bot: I haz the power, 2015-09-01 09:49:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1319363002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1319363002/40001

[commit-bot: I haz the power, 2015-09-01 10:58:09]

Committed patchset #2 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201549