cc: Split Proxy and TaskRunnerProvider for the LayerTreeHost

cc: Split Proxy and TaskRunnerProvider for the LayerTreeHost

The LayerTreeHost uses the abstract part of Proxy to access the impl
side of the compositor and the TaskRunnerProvider to access the main
task runner and assertion checks.
Decoupling these 2 will allow the LayerTreeHost used only for
deserialization and commits on the client to be built without a proxy.

This is a follow-up patch to: https://codereview.chromium.org/1418953002/

BUG=527200
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/b7db1fe24ebd92ed43f995ee615f1b4b02cc7c43
Cr-Commit-Position: refs/heads/master@{#359196}

[Khushal, 2015-10-23 22:33:14]

khushalsagar@chromium.org changed reviewers:
+ danakj@chromium.org, dtrainor@chromium.org, enne@chromium.org,
vmpstr@chromium.org

[Khushal, 2015-10-23 22:33:14]

The LayerTreeHost used for deserialization and creating the LayerTreeHostImpl
shouldn't need a proxy or LayerTreeHostClient.
The abstract part of proxy and the LTHC live only on the main thread and the
LayerTreeHost used on the client should never need to use them.

What do you guys think?

[Khushal, 2015-10-23 22:44:05]

Description was changed from

==========
cc: Split Proxy and TaskRunnerProvider for the LayerTreeHost

The LayerTreeHost uses the abstract part of Proxy to access the impl
side of the compositor and the TaskRunnerProvider to access the main
task runner and assertion checks.
Decoupling these 2 will allow the LayerTreeHost used only for
deserialization and commits on the client to be built without a proxy.

BUG=527200
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Split Proxy and TaskRunnerProvider for the LayerTreeHost

The LayerTreeHost uses the abstract part of Proxy to access the impl
side of the compositor and the TaskRunnerProvider to access the main
task runner and assertion checks.
Decoupling these 2 will allow the LayerTreeHost used only for
deserialization and commits on the client to be built without a proxy.

This is a follow-up patch to: https://codereview.chromium.org/1418953002/

BUG=527200
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Khushal, 2015-11-02 23:54:41]

On 2015/10/23 22:33:14, Khushal wrote:
> The LayerTreeHost used for deserialization and creating the LayerTreeHostImpl
> shouldn't need a proxy or LayerTreeHostClient.
> The abstract part of proxy and the LTHC live only on the main thread and the
> LayerTreeHost used on the client should never need to use them.
> 
> What do you guys think?

friendly ping. :)

[danakj, 2015-11-03 00:03:11]

On Mon, Nov 2, 2015 at 3:54 PM, <khushalsagar@chromium.org> wrote:

> On 2015/10/23 22:33:14, Khushal wrote:
>
>> The LayerTreeHost used for deserialization and creating the
>> LayerTreeHostImpl
>> shouldn't need a proxy or LayerTreeHostClient.
>> The abstract part of proxy and the LTHC live only on the main thread and
>> the
>> LayerTreeHost used on the client should never need to use them.
>>
>
> What do you guys think?
>>
>
This doesn't sound quite right to me. The client needs to make an output
surface for example, that's done through the client.

The LayerTreeHost on the client does need a proxy, it's the public face of
cc, it's how you make a proxy, and the blimp/ embedder should be talking to
cc through a LTH, and plugging its content into it via the LTH API I would
expect. That would tell the proxy it wants a commit, which goes to
scheduler, etc..


>
> friendly ping. :)
>
> https://chromiumcodereview.appspot.com/1419283002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Khushal, 2015-11-03 00:53:27]

On 2015/11/03 00:03:11, danakj wrote:
> On Mon, Nov 2, 2015 at 3:54 PM, <mailto:khushalsagar@chromium.org> wrote:
> 
> > On 2015/10/23 22:33:14, Khushal wrote:
> >
> >> The LayerTreeHost used for deserialization and creating the
> >> LayerTreeHostImpl
> >> shouldn't need a proxy or LayerTreeHostClient.
> >> The abstract part of proxy and the LTHC live only on the main thread and
> >> the
> >> LayerTreeHost used on the client should never need to use them.
> >>
> >
> > What do you guys think?
> >>
> >
> This doesn't sound quite right to me. The client needs to make an output
> surface for example, that's done through the client.
> 
> The LayerTreeHost on the client does need a proxy, it's the public face of
> cc, it's how you make a proxy, and the blimp/ embedder should be talking to
> cc through a LTH, and plugging its content into it via the LTH API I would
> expect. That would tell the proxy it wants a commit, which goes to
> scheduler, etc..
> 
> 
> >
> > friendly ping. :)
> >
> > https://chromiumcodereview.appspot.com/1419283002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

The Channel on the client receives the inter-proxy messages intended for
ProxyImpl. If the embedder talks to it through the LTH then we'll need to add
those methods to the LTH and the proxy interface for all that communication.

While some calls to the LTH end up going to ProxyImpl directly, some are
internal to the communication the proxies have with each other.
For instance, the StartCommitOnImpl method for ProxyImpl will have to follow the
chain LTH->ProxyMain->RemoteChannelImpl->ProxyImpl. Similarly for the
BeginMainFrameAborted call.

For the client it might be better for the embedder to build the
RemoteChannelImpl which will hold the ProxyImpl and build a dummy LayerTreeHost
which will be used only for deserialization and commits. And this channel talks
back to a RemoteChannelImplClient that the embedder implements. This
ChannelClient will only have the API for the output surface requests, all other
calls from ProxyImpl are sent back to the channel on the server. The only
methods we hit on the LTH on the client will be the method added to the LTH for
deserialization, CreateLayerTreeHostImpl and FinishCommmitOnImplThread. Since
these methods are logically impl thread operations, they should not hit the LTHC
or the Proxy.

The LTH talks back to its client only on the main thread and the proxy interface
is what it uses on the main thread to get to the impl thread. That should not be
needed from the LTH on the client.

[Khushal, 2015-11-05 00:39:35]

Updated the tests for this.
Could you please take a look again?

[Khushal, 2015-11-06 19:30:06]

On 2015/11/05 00:39:35, Khushal wrote:
> Updated the tests for this.
> Could you please take a look again?

friendly ping. :)

[vmpstr, 2015-11-10 21:59:34]

mostly nits.

https://codereview.chromium.org/1419283002/diff/40001/cc/test/fake_proxy.h
File cc/test/fake_proxy.h (right):

https://codereview.chromium.org/1419283002/diff/40001/cc/test/fake_proxy.h#ne...
cc/test/fake_proxy.h:14: class FakeProxy : public Proxy {
Is fake proxy still being used?

We need to either remove it if it's not, or keep at least the default ctor if it
is.

https://codereview.chromium.org/1419283002/diff/40001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1419283002/diff/40001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:705: TaskRunnerProvider::Create(main_task_runner,
impl_task_runner);
Can you rework this a bit to highlight the differences?

something like:

scoped_ptr<TaskRunnerProvider> task_runner_provider = ...
scoped_ptr<Proxy> proxy;
if (impl_task_runner.get()) {
  proxy = ...
} else {
  proxy = ...
}
layer_tree_host->InitializeForTesting(...);

https://codereview.chromium.org/1419283002/diff/40001/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/1419283002/diff/40001/cc/test/layer_tree_test...
cc/test/layer_tree_test.h:257: return
task_runner_provider()->ImplThreadTaskRunner()
save it into a temporary impl_thread_task_runner, since you're accessing it
twice otherwise.

https://codereview.chromium.org/1419283002/diff/40001/cc/trees/proxy.h
File cc/trees/proxy.h (right):

https://codereview.chromium.org/1419283002/diff/40001/cc/trees/proxy.h#newcode87
cc/trees/proxy.h:87: virtual ~Proxy() {}
dtors should be at the top.

[Khushal, 2015-11-10 23:34:04]

Thanks. Done!

https://codereview.chromium.org/1419283002/diff/40001/cc/test/fake_proxy.h
File cc/test/fake_proxy.h (right):

https://codereview.chromium.org/1419283002/diff/40001/cc/test/fake_proxy.h#ne...
cc/test/fake_proxy.h:14: class FakeProxy : public Proxy {
On 2015/11/10 21:59:34, vmpstr wrote:
> Is fake proxy still being used?
> 
> We need to either remove it if it's not, or keep at least the default ctor if
it
> is.

Yes. The LayerTreeTests still need it, added the default constructor.

https://codereview.chromium.org/1419283002/diff/40001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1419283002/diff/40001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:705: TaskRunnerProvider::Create(main_task_runner,
impl_task_runner);
On 2015/11/10 21:59:34, vmpstr wrote:
> Can you rework this a bit to highlight the differences?
> 
> something like:
> 
> scoped_ptr<TaskRunnerProvider> task_runner_provider = ...
> scoped_ptr<Proxy> proxy;
> if (impl_task_runner.get()) {
>   proxy = ...
> } else {
>   proxy = ...
> }
> layer_tree_host->InitializeForTesting(...);

Done.

https://codereview.chromium.org/1419283002/diff/40001/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/1419283002/diff/40001/cc/test/layer_tree_test...
cc/test/layer_tree_test.h:257: return
task_runner_provider()->ImplThreadTaskRunner()
On 2015/11/10 21:59:34, vmpstr wrote:
> save it into a temporary impl_thread_task_runner, since you're accessing it
> twice otherwise.

Done.

https://codereview.chromium.org/1419283002/diff/40001/cc/trees/proxy.h
File cc/trees/proxy.h (right):

https://codereview.chromium.org/1419283002/diff/40001/cc/trees/proxy.h#newcode87
cc/trees/proxy.h:87: virtual ~Proxy() {}
On 2015/11/10 21:59:34, vmpstr wrote:
> dtors should be at the top.

Done.

[vmpstr, 2015-11-11 22:45:29]

lgtm

[Khushal, 2015-11-11 22:47:59]

On 2015/11/11 22:45:29, vmpstr wrote:
> lgtm

Thanks Vlad!

[Khushal, 2015-11-11 22:48:06]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2015-11-11 22:51:10]

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

[commit-bot: I haz the power, 2015-11-12 00:51:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-12 20:02:11]

Patchset 5 (id:??) landed as
https://crrev.com/b7db1fe24ebd92ed43f995ee615f1b4b02cc7c43
Cr-Commit-Position: refs/heads/master@{#359196}