cc: BlockingTaskRunner without MessageLoopProxy

cc: Compositor without MessageLoopProxy

Support using single-threaded compositor with a single
DelegatedRendererLayer on a thread that does not have a MessageLoop.

First, Allow BlockingTaskRunner to be used on a thread that does not
have a MessageLoopProxy. For these threads, all PostTask calls must
be wrapped in CapturePostTasks. Use PlatformThreadId rather than
MessageLoopProxy::BelongsToCurrent thread to check for current.

Then fix DCHECK failures in Proxy to allow MessageLoopProxy to be
NULL.

Add tests to verify these code paths are indeed working.

BUG=344087
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272473

[boliu, 2014-05-17 00:16:16]

Dana/Antoine, I'd like to get your opinion before continuing further. This is to
support android webview running the single-thread parent compositor on a thread
without a MessageLoop.

Instead of tying the life time of BlockingTaskRunner to MessageLoopProxy, just
refcount BlockingTaskRunner "normally". So the global vector keeps one ref. And
if that's the last ref, destroy the runner. This means that it's possible for
BlockingTaskRunner::current calls on the same thread to return two different
instances of BlockingTaskRunner (never at the same point in time of course). And
I think this should be ok?

I think this should not leak more than before.

[piman, 2014-05-17 00:32:31]

https://codereview.chromium.org/292493006/diff/1/cc/trees/blocking_task_runne...
File cc/trees/blocking_task_runner.cc (right):

https://codereview.chromium.org/292493006/diff/1/cc/trees/blocking_task_runne...
cc/trees/blocking_task_runner.cc:71: return task_runner_->PostTask(from_here,
task);
How is this going to work if you don't have a MessageLoopProxy?

[boliu, 2014-05-17 00:34:26]

https://codereview.chromium.org/292493006/diff/1/cc/trees/blocking_task_runne...
File cc/trees/blocking_task_runner.cc (right):

https://codereview.chromium.org/292493006/diff/1/cc/trees/blocking_task_runne...
cc/trees/blocking_task_runner.cc:71: return task_runner_->PostTask(from_here,
task);
On 2014/05/17 00:32:31, piman wrote:
> How is this going to work if you don't have a MessageLoopProxy?

It can't. All PostTask must be wrapped by CapturePostTasks.

[piman, 2014-05-17 00:51:47]

https://codereview.chromium.org/292493006/diff/1/cc/trees/blocking_task_runne...
File cc/trees/blocking_task_runner.cc (right):

https://codereview.chromium.org/292493006/diff/1/cc/trees/blocking_task_runne...
cc/trees/blocking_task_runner.cc:71: return task_runner_->PostTask(from_here,
task);
On 2014/05/17 00:34:25, boliu wrote:
> On 2014/05/17 00:32:31, piman wrote:
> > How is this going to work if you don't have a MessageLoopProxy?
> 
> It can't. All PostTask must be wrapped by CapturePostTasks.

Is that true today for SingleThreadProxy? (It can't be true for ThreadProxy).

It's hard to audit... the places that call into BlockingTaskRunner::PostTask are
far removed from places that create a CapturePostTask... We'll need to write a
lot of tests.


We should explicitly DCHECK(capture_ || task_runner_) here.


An alternative may be to keep the tasks, and run them on the next
SetCapture(true).

[boliu, 2014-05-17 01:08:54]

So...sounds like approach for now is ok, and just need a whole lot of end-to-end
test to exercise this whole thing?

https://codereview.chromium.org/292493006/diff/1/cc/trees/blocking_task_runne...
File cc/trees/blocking_task_runner.cc (right):

https://codereview.chromium.org/292493006/diff/1/cc/trees/blocking_task_runne...
cc/trees/blocking_task_runner.cc:71: return task_runner_->PostTask(from_here,
task);
On 2014/05/17 00:51:47, piman wrote:
> On 2014/05/17 00:34:25, boliu wrote:
> > On 2014/05/17 00:32:31, piman wrote:
> > > How is this going to work if you don't have a MessageLoopProxy?
> > 
> > It can't. All PostTask must be wrapped by CapturePostTasks.
> 
> Is that true today for SingleThreadProxy? (It can't be true for ThreadProxy).
> 

Right, this definitely can't work for threaded compositor. Things like Scheduler
and TileManager/RasterWorkerPools definitely depend on a working MessageLoop.

For single thread, with the simple case of a single DelegatedRendererLayer, init
and composite works. Tear down I fixed just now:
https://codereview.chromium.org/284233005/ Definitely need an end-to-end test
for this case.

How webview uses the single-thread parent compositor is mostly here:
https://codereview.chromium.org/266353003/diff/140001/android_webview/browser...

I'm not sure if there are layers that release resources outside of commits.
Those of course won't work.

> It's hard to audit... the places that call into BlockingTaskRunner::PostTask
are
> far removed from places that create a CapturePostTask... We'll need to write a
> lot of tests.
> 
> 
> We should explicitly DCHECK(capture_ || task_runner_) here.
> 
> 
> An alternative may be to keep the tasks, and run them on the next
> SetCapture(true).

I think the danger is BlockingTaskRunner could hold on to these tasks when
destroyed, and destruction can happen on any thread, so can't run these tasks on
destruction either.

DCHECK should be fine I think.

[boliu, 2014-05-22 19:36:07]

Wrote a new test harness that creates a base::SimpleThread with no MessageLoop,
and a SmokeTest to go along with it.

I still want to write a test with DelegatedRendererLayer, hopefully later today.
I think there's enough new stuff to worth another look? But feel free to wait
until I finish.

[piman, 2014-05-22 20:53:30]

It'd be nice to have at least one test that actually exercises the
BlockingTaskRunner (with a TextureLayer or a DelegatedRendererLayer)

https://codereview.chromium.org/292493006/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_no_message_loop.cc (right):

https://codereview.chromium.org/292493006/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_no_message_loop.cc:37: public
base::SimpleThread,
multiple implementation inheritance is unfortunate :(

Use DelegateSimpleThread as a member instead?

[boliu, 2014-05-22 23:17:37]

Added test with delegated layer with resources.

Ready for full review.

https://codereview.chromium.org/292493006/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_no_message_loop.cc (right):

https://codereview.chromium.org/292493006/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_no_message_loop.cc:37: public
base::SimpleThread,
On 2014/05/22 20:53:30, piman wrote:
> multiple implementation inheritance is unfortunate :(
> 
> Use DelegateSimpleThread as a member instead?

Done

[piman, 2014-05-22 23:36:23]

lgtm

https://codereview.chromium.org/292493006/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_no_message_loop.cc (right):

https://codereview.chromium.org/292493006/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_no_message_loop.cc:216:
resource.mailbox_holder.mailbox.SetName(arbitrary_mailbox);
nit: you can also use gpu::Mailbox::Generate().

[boliu, 2014-05-22 23:48:31]

https://codereview.chromium.org/292493006/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_no_message_loop.cc (right):

https://codereview.chromium.org/292493006/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_no_message_loop.cc:216:
resource.mailbox_holder.mailbox.SetName(arbitrary_mailbox);
On 2014/05/22 23:36:23, piman wrote:
> nit: you can also use gpu::Mailbox::Generate().

Done.

[boliu, 2014-05-22 23:49:01]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-05-22 23:50:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/292493006/120001

[commit-bot: I haz the power, 2014-05-23 10:45:49]

Change committed as 272473