Tidy up CompositorWorkerManager unit tests.

Tidy up CompositorWorkerManager unit tests.

Allow WorkerGlobalScopes to be created without a non-null WorkerClients;
the creation context for the new worker may not offer any 'clients',
hence supplying none (rather than an empty one) is simpler. It also
avoids creating a cross-thread heap object with Oilpan enabled.

To solidify the CompositorWorkerManagerTest's WorkerObjectProxy handling,
provide a test implementation of WorkerObjectProxy which do not forward
any of the notifications out of the worker context, but ignores. All
that is needed for these tests.

R=haraken,kinuko
BUG=436932
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196949

[sof, 2015-06-07 20:32:21]

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

[sof, 2015-06-07 20:32:22]

please take a look.

Addresses ToT webkit_unit_tests failures (Oilpan.)

[haraken, 2015-06-07 23:37:10]

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

[haraken, 2015-06-07 23:37:11]

LGTM

https://codereview.chromium.org/1164753004/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerThreadTest.cpp (right):

https://codereview.chromium.org/1164753004/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerThreadTest.cpp:188: clients.release(),

Can we just use 'nullptr'?

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

https://codereview.chromium.org/1164753004/diff/20001/Source/modules/composit...
Source/modules/compositorworker/CompositorWorkerManagerTest.cpp:76:
TestCompositorWorkerObjectProxy(ExecutionContext* context)

Add explicit.

https://codereview.chromium.org/1164753004/diff/20001/Source/modules/composit...
Source/modules/compositorworker/CompositorWorkerManagerTest.cpp:112:
clients.release(),

Can we just write 'nullptr'?

https://codereview.chromium.org/1164753004/diff/20001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.cpp (right):

https://codereview.chromium.org/1164753004/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.cpp:46:
ASSERT(context->isWorkerGlobalScope());

You can remove this. This check is already done in toWorkerGlobalScope(context).

[sadrul, 2015-06-08 02:04:12]

Thank you very much taking care of the problem!

[kinuko, 2015-06-08 04:36:59]

Thanks, lgtm with a few nits.

The reason the test didn't crash with nullptr WorkerObjectProxy seems we don't
run/pump run loop for the current thread.  sadrul: would be probably better to
cover that in a follow-up patch.

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

https://codereview.chromium.org/1164753004/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerObjectProxy.h:52: // Used only by Dedicated Worker.
Looks like this comment is stale, could we update?  (Now could be used by
in-process workers like DedicatedWorker and CompositorWorker)

https://codereview.chromium.org/1164753004/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerObjectProxy.h:55: static PassOwnPtr<WorkerObjectProxy>
create(ExecutionContext*, WorkerMessagingProxy*);
Could we have ASSERT(!messagingProxy) in .cc so that instantiating this using
public method wouldn't end up with null messaging proxy?

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

https://codereview.chromium.org/1164753004/diff/20001/Source/modules/composit...
Source/modules/compositorworker/CompositorWorker.cpp:50: return new
CompositorWorkerMessagingProxy(this, nullptr);
nit: probably CompositorWorkerMessagingProxy shouldn't take clients parameter
then.

https://codereview.chromium.org/1164753004/diff/20001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.cpp (right):

https://codereview.chromium.org/1164753004/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.cpp:48:
ASSERT(clients);
Should these rather return nullptr here if we allow null-clients, or could we
add an assertion like ASSERT(m_workerClients || isCompositorWorkerGlobalScope())
(with a comment to note that CW doesn't expose APIs that require clients impl)
in WorkerGlobalScope's ctor so that intention (for allowing null clients)
becomes clearer?

[sof, 2015-06-08 08:18:16]

Thanks for the reviews, landing right away.

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

https://codereview.chromium.org/1164753004/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerObjectProxy.h:52: // Used only by Dedicated Worker.
On 2015/06/08 04:36:58, kinuko wrote:
> Looks like this comment is stale, could we update?  (Now could be used by
> in-process workers like DedicatedWorker and CompositorWorker)

Certainly, done.

https://codereview.chromium.org/1164753004/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerObjectProxy.h:55: static PassOwnPtr<WorkerObjectProxy>
create(ExecutionContext*, WorkerMessagingProxy*);
On 2015/06/08 04:36:59, kinuko wrote:
> Could we have ASSERT(!messagingProxy) in .cc so that instantiating this using
> public method wouldn't end up with null messaging proxy?

That's well worth it, now done.

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

https://codereview.chromium.org/1164753004/diff/20001/Source/modules/composit...
Source/modules/compositorworker/CompositorWorker.cpp:50: return new
CompositorWorkerMessagingProxy(this, nullptr);
On 2015/06/08 04:36:59, kinuko wrote:
> nit: probably CompositorWorkerMessagingProxy shouldn't take clients parameter
> then. 

Moved the nullptr use into the CompositorWorkerMessagingProxy ctor instead.

https://codereview.chromium.org/1164753004/diff/20001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.cpp (right):

https://codereview.chromium.org/1164753004/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.cpp:46:
ASSERT(context->isWorkerGlobalScope());
On 2015/06/07 23:37:11, haraken wrote:
> 
> You can remove this. This check is already done in
toWorkerGlobalScope(context).

Removed (here&elsewhere.)

https://codereview.chromium.org/1164753004/diff/20001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScopeClient.cpp:48:
ASSERT(clients);
On 2015/06/08 04:36:59, kinuko wrote:
> Should these rather return nullptr here if we allow null-clients, or could we
> add an assertion like ASSERT(m_workerClients ||
isCompositorWorkerGlobalScope())
> (with a comment to note that CW doesn't expose APIs that require clients impl)
> in WorkerGlobalScope's ctor so that intention (for allowing null clients)
> becomes clearer?

But ServiceWorkerGlobalScopeClient isn't exposed to CompositorWorker?

(Unit test uses of workers will also supply a null WorkerClients.)

[sof, 2015-06-08 08:18:36]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-06-08 08:18:37]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1164753004/#ps40001
(title: "review improvements")

[commit-bot: I haz the power, 2015-06-08 08:19:02]

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

[sof, 2015-06-08 08:39:14]

The CQ bit was unchecked by sigbjornf@opera.com

[sof, 2015-06-08 08:40:15]

Holding off on this one due to http://crrev.com/1168073002 revert.

[sof, 2015-06-11 13:23:26]

With https://codereview.chromium.org/1158443008 relanding @ r196944, time for
this one to follow.

[sof, 2015-06-11 13:23:31]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-06-11 13:23:32]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1164753004/#ps60001
(title: "rebased")

[commit-bot: I haz the power, 2015-06-11 13:23:50]

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

[commit-bot: I haz the power, 2015-06-11 14:17:11]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196949