Oilpan: fix WorkerThreadStartupData finalization handling.

Oilpan: fix WorkerThreadStartupData finalization handling.

The WorkerThreadStartupData structure is used at creation-time to
configure new worker threads and their global scopes. To simplify
its lifetime and have the owning worker thread do the finalization,
move this structure off the heap.

The motivation for it being on the heap wasn't entirely compelling
to start with (introduced by https://codereview.chromium.org/191003010.)

R=haraken
BUG=472044
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192926

[sof, 2015-03-31 21:40:50]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-03-31 21:40:51]

please take a look.

The finalization point of WorkerThreadStartupData was a bit too implicit for my
liking/reading, so I added comment(s) to clarify.

[haraken, 2015-03-31 23:29:32]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-03-31 23:29:34]

LGTM

https://codereview.chromium.org/1041053005/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerThreadStartupData.h (right):

https://codereview.chromium.org/1041053005/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadStartupData.h:81: 

Add a comment and mention that this needs to be a CrossThreadPersistent because
the WorkerThreadStartupData is created in the main thread and destructed in the
worker thread.

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

https://codereview.chromium.org/1041053005/diff/20001/Source/modules/composit...
Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp:19: // passed
along to the created 'context'.

I'm a bit wondering if we could just pass startupData to the WorkerGlobalScope's
constructor. Then we don't need to pass a lot of things to the constructor.

https://codereview.chromium.org/1041053005/diff/20001/Source/modules/composit...
Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp:20:
RefPtrWillBeRawPtr<CompositorWorkerGlobalScope> context = adoptRefWillBeNoop(new
CompositorWorkerGlobalScope(startupData->m_scriptURL, startupData->m_userAgent,
thread, timeOrigin, startupData->m_starterOrigin,
startupData->m_workerClients.release()));

I'm just curious but .release() is needed?

[sof, 2015-04-01 08:05:10]

https://codereview.chromium.org/1041053005/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerThreadStartupData.h (right):

https://codereview.chromium.org/1041053005/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadStartupData.h:81: 
On 2015/03/31 23:29:33, haraken wrote:
> 
> Add a comment and mention that this needs to be a CrossThreadPersistent
because
> the WorkerThreadStartupData is created in the main thread and destructed in
the
> worker thread.

Done, better be very clear about such matters.

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

https://codereview.chromium.org/1041053005/diff/20001/Source/modules/composit...
Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp:19: // passed
along to the created 'context'.
On 2015/03/31 23:29:33, haraken wrote:
> 
> I'm a bit wondering if we could just pass startupData to the
WorkerGlobalScope's
> constructor. Then we don't need to pass a lot of things to the constructor.
> 

It would probably come out tidier that way; let me consider that in a followup.

https://codereview.chromium.org/1041053005/diff/20001/Source/modules/composit...
Source/modules/compositorworker/CompositorWorkerGlobalScope.cpp:20:
RefPtrWillBeRawPtr<CompositorWorkerGlobalScope> context = adoptRefWillBeNoop(new
CompositorWorkerGlobalScope(startupData->m_scriptURL, startupData->m_userAgent,
thread, timeOrigin, startupData->m_starterOrigin,
startupData->m_workerClients.release()));
On 2015/03/31 23:29:33, haraken wrote:
> 
> I'm just curious but .release() is needed?

Not with Oilpan, but otherwise yes -- ownership is transferred to the global
scope.

[sof, 2015-04-01 08:44:52]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-04-01 08:44:52]

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/1041053005/#ps40001
(title: "add comment clarifying use of CrossThreadPersistent<>")

[commit-bot: I haz the power, 2015-04-01 08:45:39]

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

[commit-bot: I haz the power, 2015-04-01 11:45:05]

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