cc:: Add remote mode to the compositor

cc:: Add remote mode to the compositor.

- Added RemoteChannelMain and RemoteChannelImpl to serialize and transmit
  inter-proxy messages between the main and impl components of the
  compositor using protobufs.
- Added RemoteChannelHost to be used by the cc embedder as the API for
  interacting with the impl components of the remote compositor.
- Added tests to verify the initialization and shutdown sequence for the
  remote compositor.

BUG=550687
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[Khushal, 2015-12-10 10:13:31]

Description was changed from

==========
cc:: Add remote mode to the compositor

BUG=550687
==========

to

==========
cc:: Add remote mode to the compositor

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

[Khushal, 2015-12-10 10:30:55]

Description was changed from

==========
cc:: Add remote mode to the compositor

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

to

==========
cc:: Add remote mode to the compositor.

- Added RemoteChannelMain and RemoteChannelImpl to serialize and transmit
  inter-proxy messages between the main and impl components of the
  compositor using protobufs.
- Added RemoteChannelHost to be used by the cc embedder as the API for
  interacting with the impl components of the remote compositor.
- Added tests to verify the initialization and shutdown sequence for the
  remote compositor.

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

[Khushal, 2015-12-10 10:31:19]

Description was changed from

==========
cc:: Add remote mode to the compositor.

- Added RemoteChannelMain and RemoteChannelImpl to serialize and transmit
  inter-proxy messages between the main and impl components of the
  compositor using protobufs.
- Added RemoteChannelHost to be used by the cc embedder as the API for
  interacting with the impl components of the remote compositor.
- Added tests to verify the initialization and shutdown sequence for the
  remote compositor.

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

to

==========
cc:: Add remote mode to the compositor.

- Added RemoteChannelMain and RemoteChannelImpl to serialize and transmit
  inter-proxy messages between the main and impl components of the
  compositor using protobufs.
- Added RemoteChannelHost to be used by the cc embedder as the API for
  interacting with the impl components of the remote compositor.
- Added tests to verify the initialization and shutdown sequence for the
  remote compositor.

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

[Khushal, 2015-12-10 10:31:19]

khushalsagar@chromium.org changed reviewers:
+ dtrainor@chromium.org

[David Trainor- moved to gerrit, 2015-12-11 17:13:45]

https://codereview.chromium.org/1513643010/diff/1/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/1513643010/diff/1/cc/cc.gyp#newcode609
cc/cc.gyp:609: 'proto/compositor_message_to_impl.proto',
Replace tabs with spaces

https://codereview.chromium.org/1513643010/diff/1/cc/cc.gyp#newcode613
cc/cc.gyp:613: 'proto/layer_tree_settings.proto',
alphabetical order

https://codereview.chromium.org/1513643010/diff/1/cc/proto/layer_tree_setting...
File cc/proto/layer_tree_settings.proto (right):

https://codereview.chromium.org/1513643010/diff/1/cc/proto/layer_tree_setting...
cc/proto/layer_tree_settings.proto:12: // TODO(khushalsagar): Serialize
LayerTreeSettings
TODO(nyquist) since he's almost done with this.

https://codereview.chromium.org/1513643010/diff/1/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1513643010/diff/1/cc/test/layer_tree_test.cc#...
cc/test/layer_tree_test.cc:1064: ProxyImpl* proxy_impl;
Can you leave this line below?

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_hos...
File cc/trees/remote_channel_host.cc (right):

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_hos...
cc/trees/remote_channel_host.cc:42: RemoteChannelHost::~RemoteChannelHost() {
remote_proto_channel_->SetProtoReceiver(nullptr) here?  You might have to update
your test expectation and remove that DCHECK though.  We shouldn't make any
assumptions about the RemoteProtoChannel lifecycle though.

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_imp...
File cc/trees/remote_channel_impl.cc (right):

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_imp...
cc/trees/remote_channel_impl.cc:81: DCHECK(false);
Remove?

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_imp...
cc/trees/remote_channel_impl.cc:103: {
You don't need this extra scope here.  But if it's consistent style for other
CompletionEvents across CC to do this I'm fine with it.

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_impl.h
File cc/trees/remote_channel_impl.h (right):

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_imp...
cc/trees/remote_channel_impl.h:67: scoped_ptr<LayerTreeHost> layer_tree_host;
Comment about the specifics/expectations of this LayerTreeHost here (and why
ownership seems flipped).

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_imp...
cc/trees/remote_channel_impl.h:108: // Called on impl thread.
Do you need this if you have it above?  I'm fine with it either way, just
pointing it out.

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_mai...
File cc/trees/remote_channel_main.cc (right):

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_mai...
cc/trees/remote_channel_main.cc:37: RemoteChannelMain::~RemoteChannelMain() {
Same here.   Add remote_proto_channel_->SetProtoReceiver(nullptr)?

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_mai...
cc/trees/remote_channel_main.cc:68: void
RemoteChannelMain::FinishAllRenderingOnImpl(CompletionEvent* completion) {}
Would it be good to just signal all of these completion events for now?

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_proto_chann...
File cc/trees/remote_proto_channel.h (right):

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_proto_chann...
cc/trees/remote_proto_channel.h:24: // TODO(khushalsagar): This should probably
include a closure that returns
We could influence channel throttling with that which might be nice.

[Khushal, 2015-12-11 22:49:37]

https://codereview.chromium.org/1513643010/diff/1/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/1513643010/diff/1/cc/cc.gyp#newcode609
cc/cc.gyp:609: 'proto/compositor_message_to_impl.proto',
On 2015/12/11 17:13:44, David Trainor wrote:
> Replace tabs with spaces

Done.

https://codereview.chromium.org/1513643010/diff/1/cc/cc.gyp#newcode613
cc/cc.gyp:613: 'proto/layer_tree_settings.proto',
On 2015/12/11 17:13:44, David Trainor wrote:
> alphabetical order

Done.

https://codereview.chromium.org/1513643010/diff/1/cc/proto/layer_tree_setting...
File cc/proto/layer_tree_settings.proto (right):

https://codereview.chromium.org/1513643010/diff/1/cc/proto/layer_tree_setting...
cc/proto/layer_tree_settings.proto:12: // TODO(khushalsagar): Serialize
LayerTreeSettings
On 2015/12/11 17:13:44, David Trainor wrote:
> TODO(nyquist) since he's almost done with this.

His patch has landed. I'll rebase and include his changes.

https://codereview.chromium.org/1513643010/diff/1/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1513643010/diff/1/cc/test/layer_tree_test.cc#...
cc/test/layer_tree_test.cc:1064: ProxyImpl* proxy_impl;
On 2015/12/11 17:13:44, David Trainor wrote:
> Can you leave this line below?

Done.
I changed the structure so we can get ProxyImpl from the test class for the
threaded case as well.

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_hos...
File cc/trees/remote_channel_host.cc (right):

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_hos...
cc/trees/remote_channel_host.cc:42: RemoteChannelHost::~RemoteChannelHost() {
On 2015/12/11 17:13:44, David Trainor wrote:
> remote_proto_channel_->SetProtoReceiver(nullptr) here?  You might have to
update
> your test expectation and remove that DCHECK though.  We shouldn't make any
> assumptions about the RemoteProtoChannel lifecycle though.

Done.
I was unsure about this too. But I think its safe to assume that the
RemoteProtoChannel will outlive its receivers.

Added that in the comments for that class.

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_imp...
File cc/trees/remote_channel_impl.cc (right):

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_imp...
cc/trees/remote_channel_impl.cc:81: DCHECK(false);
On 2015/12/11 17:13:44, David Trainor wrote:
> Remove?

Woops. Debugging fun. :P

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_imp...
cc/trees/remote_channel_impl.cc:103: {
On 2015/12/11 17:13:44, David Trainor wrote:
> You don't need this extra scope here.  But if it's consistent style for other
> CompletionEvents across CC to do this I'm fine with it.

Done. I don't think there is any particular style to follow for this.

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_impl.h
File cc/trees/remote_channel_impl.h (right):

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_imp...
cc/trees/remote_channel_impl.h:67: scoped_ptr<LayerTreeHost> layer_tree_host;
On 2015/12/11 17:13:44, David Trainor wrote:
> Comment about the specifics/expectations of this LayerTreeHost here (and why
> ownership seems flipped).

Done.

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_imp...
cc/trees/remote_channel_impl.h:108: // Called on impl thread.
On 2015/12/11 17:13:44, David Trainor wrote:
> Do you need this if you have it above?  I'm fine with it either way, just
> pointing it out.

I wasn't sure if having the comment only above the ChannelImpl implementation
makes it clear. But I guess having the "OnImpl" suffix should make it obvious
too.
Removed.

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_mai...
File cc/trees/remote_channel_main.cc (right):

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_mai...
cc/trees/remote_channel_main.cc:37: RemoteChannelMain::~RemoteChannelMain() {
On 2015/12/11 17:13:44, David Trainor wrote:
> Same here.   Add remote_proto_channel_->SetProtoReceiver(nullptr)?

Done.

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_channel_mai...
cc/trees/remote_channel_main.cc:68: void
RemoteChannelMain::FinishAllRenderingOnImpl(CompletionEvent* completion) {}
On 2015/12/11 17:13:44, David Trainor wrote:
> Would it be good to just signal all of these completion events for now?

Done.

I think it would be better to have the channel create the events and ProxyImpl
signal them.
I'll do that in a separate patch.

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_proto_chann...
File cc/trees/remote_proto_channel.h (right):

https://codereview.chromium.org/1513643010/diff/1/cc/trees/remote_proto_chann...
cc/trees/remote_proto_channel.h:24: // TODO(khushalsagar): This should probably
include a closure that returns
On 2015/12/11 17:13:44, David Trainor wrote:
> We could influence channel throttling with that which might be nice.

Yup. We could avoid blocking the main thread on the client with this approach
for initialization and commits, because the network layer will buffer the protos
till they are acked.

[Khushal, 2015-12-14 18:17:02]

khushalsagar@chromium.org changed reviewers:
+ vmpstr@chromium.org

[David Trainor- moved to gerrit, 2015-12-15 00:52:14]

looks fine to me. :)

https://chromiumcodereview.appspot.com/1513643010/diff/80001/cc/trees/layer_t...
File cc/trees/layer_tree_host.cc (right):

https://chromiumcodereview.appspot.com/1513643010/diff/80001/cc/trees/layer_t...
cc/trees/layer_tree_host.cc:268: void LayerTreeHost::ToProtobuf() {
Add todo to implement?

https://chromiumcodereview.appspot.com/1513643010/diff/80001/cc/trees/remote_...
File cc/trees/remote_channel_impl.h (right):

https://chromiumcodereview.appspot.com/1513643010/diff/80001/cc/trees/remote_...
cc/trees/remote_channel_impl.h:45: //
RemoteChannelImpl->Initialize()---PostTask---> RemoteChannelImpl::
:: for all of these?

[Khushal, 2015-12-15 05:05:30]

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

https://codereview.chromium.org/1513643010/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:268: void LayerTreeHost::ToProtobuf() {
On 2015/12/15 00:52:14, David Trainor wrote:
> Add todo to implement?

Done.

https://codereview.chromium.org/1513643010/diff/80001/cc/trees/remote_channel...
File cc/trees/remote_channel_impl.h (right):

https://codereview.chromium.org/1513643010/diff/80001/cc/trees/remote_channel...
cc/trees/remote_channel_impl.h:45: //
RemoteChannelImpl->Initialize()---PostTask---> RemoteChannelImpl::
On 2015/12/15 00:52:14, David Trainor wrote:
> :: for all of these?

Done.

[Khushal, 2016-01-05 22:25:04]

vmpstr@, could you take a look at this as well? Thanks! :)

[vmpstr, 2016-01-06 23:12:18]

On 2016/01/05 22:25:04, Khushal wrote:
> vmpstr@, could you take a look at this as well? Thanks! :)

Hey, sorry for the delay. I'll take a look shortly.

[vmpstr, 2016-01-07 19:08:12]

Can you please link a doc describing the design again? I know I had it
somewhere...

In particular, I'm wondering what RemoteClient and RemoteClientHost are.

https://codereview.chromium.org/1513643010/diff/120001/cc/proto/compositor_me...
File cc/proto/compositor_message_to_impl.proto (right):

https://codereview.chromium.org/1513643010/diff/120001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:16: Unknown = 0;
I think it's worth documenting each type here with a comment and when it is sent
and what should happen, etc

https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:538: proxy = std::move(proxy_main);
why not just proxy = ProxyMainForTest::CreateThreaded(...)?

https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:548: proxy = std::move(proxy_main);
Same here

https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:773: // It is important to create the
RemoteChannelHost before the LayerTreeHost
What would be creating remote channel host in non-test? blimp/ code?

https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:939: if (mode_ != CompositorMode::SingleThreaded) {
I think this should be explicitly mode_ == Threaded || mode_ == Remote.

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

https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.h:207: RemoteProtoChannelBridge
remote_proto_channel_bridge_;
What's this?

https://codereview.chromium.org/1513643010/diff/120001/cc/trees/compositor_mo...
File cc/trees/compositor_mode.h (right):

https://codereview.chromium.org/1513643010/diff/120001/cc/trees/compositor_mo...
cc/trees/compositor_mode.h:28: Deserializable,
This seem a bit awkward to me. We have two LayerTreeHosts in a blimp case, one
that lives on the server and one that lives on the client. The comment in
"Remote" case suggests that there the "true" layer tree host is on the server,
making the deserializable one an implementation detail of sorts. 

Is it possible to somehow not add a CompositorMode for it? Since the
CompositorMode should be "Remote" in blimp.

https://codereview.chromium.org/1513643010/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1513643010/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:205: void LayerTreeHost::InitializeDeserializable(
I think the rest of the review kind of depends on me not liking the
deserializable concept... Can we still run it in a remote mode but have a
different channel? Like a threaded channel? Or something similar?

My understanding is that blimp/ code will control the lifetime of this layer
tree host, and call FromProtobuf on it. After that's called, we might as well
treat that layer tree host as a threaded one, no?

https://codereview.chromium.org/1513643010/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1513643010/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.h:440: bool IsRemote() const;
At some point, these function should just become

CompositorMode mode() { return compositor_mode_; }

[Khushal, 2016-01-07 19:36:12]

On 2016/01/07 19:08:12, vmpstr wrote:
> Can you please link a doc describing the design again? I know I had it
> somewhere...
> 

Sure. I had described the structure briefly here:
https://docs.google.com/document/d/1_5jMV8vUC5nGtyTiNxomaNZ4LSiH3omhTmdp4L1qo...
I'll address the rest of the comments.
 
> In particular, I'm wondering what RemoteClient and RemoteClientHost are.
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/proto/compositor_me...
> File cc/proto/compositor_message_to_impl.proto (right):
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/proto/compositor_me...
> cc/proto/compositor_message_to_impl.proto:16: Unknown = 0;
> I think it's worth documenting each type here with a comment and when it is
sent
> and what should happen, etc
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
> File cc/test/layer_tree_test.cc (right):
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
> cc/test/layer_tree_test.cc:538: proxy = std::move(proxy_main);
> why not just proxy = ProxyMainForTest::CreateThreaded(...)?
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
> cc/test/layer_tree_test.cc:548: proxy = std::move(proxy_main);
> Same here
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
> cc/test/layer_tree_test.cc:773: // It is important to create the
> RemoteChannelHost before the LayerTreeHost
> What would be creating remote channel host in non-test? blimp/ code?
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
> cc/test/layer_tree_test.cc:939: if (mode_ != CompositorMode::SingleThreaded) {
> I think this should be explicitly mode_ == Threaded || mode_ == Remote.
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_test.h
> File cc/test/layer_tree_test.h (right):
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
> cc/test/layer_tree_test.h:207: RemoteProtoChannelBridge
> remote_proto_channel_bridge_;
> What's this?
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/trees/compositor_mo...
> File cc/trees/compositor_mode.h (right):
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/trees/compositor_mo...
> cc/trees/compositor_mode.h:28: Deserializable,
> This seem a bit awkward to me. We have two LayerTreeHosts in a blimp case, one
> that lives on the server and one that lives on the client. The comment in
> "Remote" case suggests that there the "true" layer tree host is on the server,
> making the deserializable one an implementation detail of sorts. 
> 
> Is it possible to somehow not add a CompositorMode for it? Since the
> CompositorMode should be "Remote" in blimp.
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/trees/layer_tree_ho...
> File cc/trees/layer_tree_host.cc (right):
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host.cc:205: void LayerTreeHost::InitializeDeserializable(
> I think the rest of the review kind of depends on me not liking the
> deserializable concept... Can we still run it in a remote mode but have a
> different channel? Like a threaded channel? Or something similar?
> 
> My understanding is that blimp/ code will control the lifetime of this layer
> tree host, and call FromProtobuf on it. After that's called, we might as well
> treat that layer tree host as a threaded one, no?
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/trees/layer_tree_ho...
> File cc/trees/layer_tree_host.h (right):
> 
>
https://codereview.chromium.org/1513643010/diff/120001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host.h:440: bool IsRemote() const;
> At some point, these function should just become
> 
> CompositorMode mode() { return compositor_mode_; }

[Khushal, 2016-01-08 21:17:19]

Updated the patch like we discussed to use a LayerTreeHost on the client as well
and avoid adding a RemoteChannelHost. Could you take another look? Thanks!

https://codereview.chromium.org/1513643010/diff/120001/cc/proto/compositor_me...
File cc/proto/compositor_message_to_impl.proto (right):

https://codereview.chromium.org/1513643010/diff/120001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:16: Unknown = 0;
On 2016/01/07 19:08:11, vmpstr wrote:
> I think it's worth documenting each type here with a comment and when it is
sent
> and what should happen, etc

Done.

https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:538: proxy = std::move(proxy_main);
On 2016/01/07 19:08:12, vmpstr wrote:
> why not just proxy = ProxyMainForTest::CreateThreaded(...)?

I think I was using a temp variable there earlier and the switch statement
didn't like that.
Changed it.

https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:548: proxy = std::move(proxy_main);
On 2016/01/07 19:08:11, vmpstr wrote:
> Same here

Done.

https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:773: // It is important to create the
RemoteChannelHost before the LayerTreeHost
On 2016/01/07 19:08:12, vmpstr wrote:
> What would be creating remote channel host in non-test? blimp/ code?

As we discussed. I've removed the RemoteChannelHost so we now use the
LayerTreeHost on the client as well.
Its the remote_client_layer_tree_host_ in LayerTreeTest which will be in blimp/
code. Updated this in the comments.

https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:939: if (mode_ != CompositorMode::SingleThreaded) {
On 2016/01/07 19:08:12, vmpstr wrote:
> I think this should be explicitly mode_ == Threaded || mode_ == Remote.

Done.

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

https://codereview.chromium.org/1513643010/diff/120001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.h:207: RemoteProtoChannelBridge
remote_proto_channel_bridge_;
On 2016/01/07 19:08:12, vmpstr wrote:
> What's this?

Its just a helper class to send protos directly between the RemoteProtoChannels
for the compositor on the server and client. Mocks out the external network
layer for the tests.
Updated this in the comments.

https://codereview.chromium.org/1513643010/diff/120001/cc/trees/compositor_mo...
File cc/trees/compositor_mode.h (right):

https://codereview.chromium.org/1513643010/diff/120001/cc/trees/compositor_mo...
cc/trees/compositor_mode.h:28: Deserializable,
On 2016/01/07 19:08:12, vmpstr wrote:
> This seem a bit awkward to me. We have two LayerTreeHosts in a blimp case, one
> that lives on the server and one that lives on the client. The comment in
> "Remote" case suggests that there the "true" layer tree host is on the server,
> making the deserializable one an implementation detail of sorts. 
> 

Updated the patch to remove the RemoteChannelHost. So now we do have 2
LayerTreeHosts and blimp/ will use the Remote client LayerTreeHost.

We don't need a seperate Compositor Mode for this now.

> Is it possible to somehow not add a CompositorMode for it? Since the
> CompositorMode should be "Remote" in blimp.

https://codereview.chromium.org/1513643010/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1513643010/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:205: void LayerTreeHost::InitializeDeserializable(
On 2016/01/07 19:08:12, vmpstr wrote:
> I think the rest of the review kind of depends on me not liking the
> deserializable concept... Can we still run it in a remote mode but have a
> different channel? Like a threaded channel? Or something similar?
> 

Updated this to use the remote mode and the RemoteChannelImpl.

> My understanding is that blimp/ code will control the lifetime of this layer
> tree host, and call FromProtobuf on it. After that's called, we might as well
> treat that layer tree host as a threaded one, no?

Its basically treated as the the threaded one but we can't use the threaded case
directly. The LayerTreeHostImpl has 2 dependencies, the content which it gets
from the LayerTreeHost on the server, and the output surface and visibility
status, which it needs from the LayerTreeHost on the client.
In the threaded case, both these come from the same LayerTreeHost, via the
LTH->Proxy->ThreadedChannel path. For the remote case, that logic is now
restricted to RemoteChannelImpl. For output surface and visibility, it will send
those requests to the client LayerTreeHost. For commits, it will send them to
the server LayerTreeHost.

https://codereview.chromium.org/1513643010/diff/120001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1513643010/diff/120001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.h:440: bool IsRemote() const;
On 2016/01/07 19:08:12, vmpstr wrote:
> At some point, these function should just become
> 
> CompositorMode mode() { return compositor_mode_; }

Updated these functions to IsRemoteServer and IsRemoteClient. The only purpose
of adding them here is for DCHECKs for calls that should be made either on the
server or on the client. Do you think we should avoid doing that?

[vmpstr, 2016-01-11 21:50:16]

I haven't looked at the tests yet, I'm just trying to understand what you have
and get comfortable with it. It's looking pretty good in the sense that it
isolates the remote case from the non-remote case, which I like.

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
File cc/proto/compositor_message_to_impl.proto (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
2016

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:19: Unknown = 0;
I think we went with SHOUT_CASE for enums
(https://codereview.chromium.org/1492533003/)

Can you also add a comment for the unkown case (ie when you would expect this)

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:22: // CompositorMessageToImpl of
Type::InitializeImpl to the client. This
nit: s/Type::/type /

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:38: MainThreadHasStoppedFlingingOnImpl
= 3;
Is there a corresponding start fling message? Maybe can either add the start
fling here as well or delay adding the stop one until later?

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:50: // The embedder of the remote
client compositor should process the
nit: Can you move this comment before "message InitializeImpl"

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:53: // The settings sent from the
server may be modified by the embedder.
formatting nit: move this line to the previous one

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/compositor_mo...
File cc/trees/compositor_mode.h (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/compositor_mo...
cc/trees/compositor_mode.h:18: SingleThreaded,
while here can you change these to SHOUT_CASE and maybe also enum class?

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:100: // Using an external begin frame source is not
supported in remote mode.
... in remote server mode.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:120: // TODO(khushalsagar): Add support for
providing an external begin frame
File a bug for this and reference it in here, please.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:220: InitializeProxy(RemoteChannelImpl::Create(this,
remote_proto_channel,
I think it's worth putting a comment here to kind of explain the overall
structure of the client

LTH owns RCI which owns foo

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:294: // TODO(khushalsagar, nyquist): Serialize
LayerTreeHost state to proto.
crbug?

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:301: // TODO(khushalsagar, nyquist): Deserialize
LayerTreeHost state from proto.
crbug?

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.h:122: // Should be called only for the deserializable
mode.
This isn't a mode anymore, can you update the comment? I think the
ToProtobuf/FromProtobuf should share a comment

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/proxy_main.cc
File cc/trees/proxy_main.cc (left):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/proxy_main.cc...
cc/trees/proxy_main.cc:380: DCHECK(task_runner_provider_->HasImplThread());
Should this be 

DCHECK(!external_begin_frame_source || task_runner_provider_->HasImplThread()) 

instead?

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
File cc/trees/remote_channel_impl.cc (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.cc:46: scoped_ptr<BeginFrameSource>
external_begin_frame_source) {
Is this supported or always null for now?

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.cc:58: proto::CompositorMessageToImpl to_impl_proto
= proto->to_impl();
nit: can you move the switch to a new function called HandleProtoMessage so that
we can easily find it later

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.cc:72: NOTREACHED() << "Should be called on the
server LayerTreeHost";
nit: Can you change the error to be more about what is wrong instead of how to
make it right? Like

"This should not be called on a client RemoteChannelImpl" or something

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.cc:118: NOTREACHED() << "Should not be called on
the remote client LayerTreeHost";
LayerTreeHost or RemoteChannelImpl?

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
File cc/trees/remote_channel_impl.h (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.h:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
2016

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.h:68: // it on the impl thread are run before it is
destroyed on the main thread.
Can you also add a comment block about communication between main and impl
threads other than initialization/deletion?

For example, messages from the main thread will be passed to the impl thread via
a post task. Or will this receive the messages from the server (why is it a
RemoteProtoChannel::ProtoReceiver)

Are messages from the impl thread going to be passed to the main thread here? Or
sent to the server? Or something else?

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.h:69: 
nit: remove blank line

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
File cc/trees/remote_channel_main.cc (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_main.cc:121: SendMessageProto(proto);
Should this be going through some sort of UI thread?

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
File cc/trees/remote_channel_main.h (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_main.h:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
2016

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_proto_...
File cc/trees/remote_proto_channel.h (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_proto_...
cc/trees/remote_proto_channel.h:25: // the status of processing this proto.
crbug?

[Khushal, 2016-01-13 02:07:12]

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
File cc/proto/compositor_message_to_impl.proto (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/01/11 21:50:15, vmpstr wrote:
> 2016

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:19: Unknown = 0;
On 2016/01/11 21:50:15, vmpstr wrote:
> I think we went with SHOUT_CASE for enums
> (https://codereview.chromium.org/1492533003/)
> 
> Can you also add a comment for the unkown case (ie when you would expect this)

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:22: // CompositorMessageToImpl of
Type::InitializeImpl to the client. This
On 2016/01/11 21:50:14, vmpstr wrote:
> nit: s/Type::/type /

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:38: MainThreadHasStoppedFlingingOnImpl
= 3;
On 2016/01/11 21:50:15, vmpstr wrote:
> Is there a corresponding start fling message? Maybe can either add the start
> fling here as well or delay adding the stop one until later?

Right now the message types here are basically serializing the communication
between ProxyMain and ProxyImpl. The stop message comes from blink, I don't know
if they need to or have a method to inform the compositor about when the fling
starts.

Right now I think this call just gets piped to the InputHandlerClient and we
probably don't need to use it right now, but since the ChannelMain interface
which will define these types will develop alongside the changes for the
threaded case, we can add the start call when its required to be added to both?

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:50: // The embedder of the remote
client compositor should process the
On 2016/01/11 21:50:15, vmpstr wrote:
> nit: Can you move this comment before "message InitializeImpl"

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:53: // The settings sent from the
server may be modified by the embedder.
On 2016/01/11 21:50:15, vmpstr wrote:
> formatting nit: move this line to the previous one

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/compositor_mo...
File cc/trees/compositor_mode.h (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/compositor_mo...
cc/trees/compositor_mode.h:18: SingleThreaded,
On 2016/01/11 21:50:15, vmpstr wrote:
> while here can you change these to SHOUT_CASE and maybe also enum class?

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:100: // Using an external begin frame source is not
supported in remote mode.
On 2016/01/11 21:50:15, vmpstr wrote:
> ... in remote server mode.

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:120: // TODO(khushalsagar): Add support for
providing an external begin frame
On 2016/01/11 21:50:15, vmpstr wrote:
> File a bug for this and reference it in here, please.

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:220: InitializeProxy(RemoteChannelImpl::Create(this,
remote_proto_channel,
On 2016/01/11 21:50:15, vmpstr wrote:
> I think it's worth putting a comment here to kind of explain the overall
> structure of the client
> 
> LTH owns RCI which owns foo

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:294: // TODO(khushalsagar, nyquist): Serialize
LayerTreeHost state to proto.
On 2016/01/11 21:50:15, vmpstr wrote:
> crbug?

Removed.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:301: // TODO(khushalsagar, nyquist): Deserialize
LayerTreeHost state from proto.
On 2016/01/11 21:50:15, vmpstr wrote:
> crbug?

Removed.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.h:122: // Should be called only for the deserializable
mode.
On 2016/01/11 21:50:15, vmpstr wrote:
> This isn't a mode anymore, can you update the comment? I think the
> ToProtobuf/FromProtobuf should share a comment

I'll just remove these. Tommy is already working on a patch to add these
methods. His will probably land before mine. I'll add the DCHECKs in the
implementation when I rebase and include his changes.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/proxy_main.cc
File cc/trees/proxy_main.cc (left):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/proxy_main.cc...
cc/trees/proxy_main.cc:380: DCHECK(task_runner_provider_->HasImplThread());
On 2016/01/11 21:50:15, vmpstr wrote:
> Should this be 
> 
> DCHECK(!external_begin_frame_source || task_runner_provider_->HasImplThread())

> 
> instead?

If the client does not use the external begin frame source then we'll not have
this DCHECK.
We could either make the IsThreaded() or IsRemoteServer() methods public on the
LayerTreeHost?

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
File cc/trees/remote_channel_impl.cc (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.cc:46: scoped_ptr<BeginFrameSource>
external_begin_frame_source) {
On 2016/01/11 21:50:15, vmpstr wrote:
> Is this supported or always null for now?

always null for now. Added a DCHECK.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.cc:58: proto::CompositorMessageToImpl to_impl_proto
= proto->to_impl();
On 2016/01/11 21:50:15, vmpstr wrote:
> nit: can you move the switch to a new function called HandleProtoMessage so
that
> we can easily find it later

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.cc:72: NOTREACHED() << "Should be called on the
server LayerTreeHost";
On 2016/01/11 21:50:15, vmpstr wrote:
> nit: Can you change the error to be more about what is wrong instead of how to
> make it right? Like
> 
> "This should not be called on a client RemoteChannelImpl" or something

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.cc:118: NOTREACHED() << "Should not be called on
the remote client LayerTreeHost";
On 2016/01/11 21:50:15, vmpstr wrote:
> LayerTreeHost or RemoteChannelImpl?

Basically this call should not be made to the proxy on the remote client
LayerTreeHost.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
File cc/trees/remote_channel_impl.h (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.h:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
On 2016/01/11 21:50:15, vmpstr wrote:
> 2016

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.h:68: // it on the impl thread are run before it is
destroyed on the main thread.
On 2016/01/11 21:50:15, vmpstr wrote:
> Can you also add a comment block about communication between main and impl
> threads other than initialization/deletion?
> 
> For example, messages from the main thread will be passed to the impl thread
via
> a post task. Or will this receive the messages from the server (why is it a
> RemoteProtoChannel::ProtoReceiver)
> 
> Are messages from the impl thread going to be passed to the main thread here?
Or
> sent to the server? Or something else?

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.h:69: 
On 2016/01/11 21:50:15, vmpstr wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
File cc/trees/remote_channel_main.cc (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_main.cc:121: SendMessageProto(proto);
On 2016/01/11 21:50:15, vmpstr wrote:
> Should this be going through some sort of UI thread?

The server is single threaded, so we just give the proto to the
RemoteProtoChannel on the same thread. The messages are expected to be received
on the UI/main thread on the client in the RemoteChannelImpl.
The comment on RemoteChannelImpl points that out.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
File cc/trees/remote_channel_main.h (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_channe...
cc/trees/remote_channel_main.h:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
On 2016/01/11 21:50:15, vmpstr wrote:
> 2016

Done.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_proto_...
File cc/trees/remote_proto_channel.h (right):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/remote_proto_...
cc/trees/remote_proto_channel.h:25: // the status of processing this proto.
On 2016/01/11 21:50:15, vmpstr wrote:
> crbug?

Done.

[Khushal, 2016-01-14 21:34:55]

khushalsagar@chromium.org changed reviewers:
+ ericrk@chromium.org

[Khushal, 2016-01-14 21:35:44]

+ericrk

[ericrk, 2016-01-20 22:32:29]

A few more comments - overall looks good. There's a bit more "if (remote) ...
else ..." in test code.... probably an OK amount at the moment - if we end up
with a lot more of this we might consider making
LayerTreeTestRemote/LayerTreeTestLocal subclasses of LayerTreeTest, but at the
moment I'm OK with the level of abstraction.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/proxy_main.cc
File cc/trees/proxy_main.cc (left):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/proxy_main.cc...
cc/trees/proxy_main.cc:380: DCHECK(task_runner_provider_->HasImplThread());
On 2016/01/13 02:07:12, Khushal wrote:
> On 2016/01/11 21:50:15, vmpstr wrote:
> > Should this be 
> > 
> > DCHECK(!external_begin_frame_source ||
task_runner_provider_->HasImplThread())
> 
> > 
> > instead?
> 
> If the client does not use the external begin frame source then we'll not have
> this DCHECK.
> We could either make the IsThreaded() or IsRemoteServer() methods public on
the
> LayerTreeHost?

Seems like we could use IsRemoteServer a few places - I'm not too opposed to
making this public.

https://codereview.chromium.org/1513643010/diff/160001/cc/proto/compositor_me...
File cc/proto/compositor_message_to_impl.proto (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:48: // Only one of the following
fields will be set per CompositorMessageToImpl.
To make sure I'm not misunderstanding - there will be more optionals added here
in the future, of which only one will be set?

https://codereview.chromium.org/1513643010/diff/160001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:466: if (task_runner_provider->HasImplThread()) {
would it make sense to make IsRemoteClient/IsRemoteServer protected on
LayerTreeHost and just call that here - keeps the somewhat magic HasImplThread
check in one place.

https://codereview.chromium.org/1513643010/diff/160001/cc/test/remote_channel...
File cc/test/remote_channel_impl_for_test.cc (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/test/remote_channel...
cc/test/remote_channel_impl_for_test.cc:40: return std::move(proxy_impl);
nit: from what I remember, std::move breaks RVO, so when returning a move-only
type that matches the return type, it's better to just return directly without
std::move.

https://codereview.chromium.org/1513643010/diff/160001/cc/test/remote_proto_c...
File cc/test/remote_proto_channel_bridge.h (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/test/remote_proto_c...
cc/test/remote_proto_channel_bridge.h:40: explicit
FakeRemoteProtoChannelMain(RemoteProtoChannelBridge* bridge,
nit: remove explicit on a 2 arg constructor.

https://codereview.chromium.org/1513643010/diff/160001/cc/test/threaded_chann...
File cc/test/threaded_channel_for_test.cc (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/test/threaded_chann...
cc/test/threaded_channel_for_test.cc:36: return std::move(proxy_impl);
same std::move comment from above.

https://codereview.chromium.org/1513643010/diff/160001/cc/trees/remote_channe...
File cc/trees/remote_channel_impl.h (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.h:175: // called on main thread.
why not move these up to the other "called on main thread" block?

[Khushal, 2016-01-22 01:03:24]

I had actually considered using a derived classes for running these
LayerTreeTests, but going forward we want to run the existing LayerTreeTests in
the remote mode as well and right now since they all derive from LayerTreeTest,
it seemed like these changes to the LayerTreeTest made more sense.

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/proxy_main.cc
File cc/trees/proxy_main.cc (left):

https://codereview.chromium.org/1513643010/diff/140001/cc/trees/proxy_main.cc...
cc/trees/proxy_main.cc:380: DCHECK(task_runner_provider_->HasImplThread());
On 2016/01/20 22:32:28, ericrk wrote:
> On 2016/01/13 02:07:12, Khushal wrote:
> > On 2016/01/11 21:50:15, vmpstr wrote:
> > > Should this be 
> > > 
> > > DCHECK(!external_begin_frame_source ||
> task_runner_provider_->HasImplThread())
> > 
> > > 
> > > instead?
> > 
> > If the client does not use the external begin frame source then we'll not
have
> > this DCHECK.
> > We could either make the IsThreaded() or IsRemoteServer() methods public on
> the
> > LayerTreeHost?
> 
> Seems like we could use IsRemoteServer a few places - I'm not too opposed to
> making this public.

Done.

https://codereview.chromium.org/1513643010/diff/160001/cc/proto/compositor_me...
File cc/proto/compositor_message_to_impl.proto (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/proto/compositor_me...
cc/proto/compositor_message_to_impl.proto:48: // Only one of the following
fields will be set per CompositorMessageToImpl.
On 2016/01/20 22:32:29, ericrk wrote:
> To make sure I'm not misunderstanding - there will be more optionals added
here
> in the future, of which only one will be set?

Yes. For message types like, BEGIN_MAIN_FRAME_ABORTED, which have additional
data, we'll add more optionals here.

https://codereview.chromium.org/1513643010/diff/160001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:466: if (task_runner_provider->HasImplThread()) {
On 2016/01/20 22:32:29, ericrk wrote:
> would it make sense to make IsRemoteClient/IsRemoteServer protected on
> LayerTreeHost and just call that here - keeps the somewhat magic HasImplThread
> check in one place.

We pass both the task runner provider and the proxy to the layer_tree_host in
InitializeForTesting, so we can't call the IsFoo methods on them here since the
LayerTreeHost would need the taskrunnerprovider for those.
I think this might end up being the only other place where we have to do this
check, since the tests that need to care about the mode are LayerTreeTests. The
other subclasses of the LayerTreeHost used in tests should be agnostic to the
mode the compositor is running in, right?

https://codereview.chromium.org/1513643010/diff/160001/cc/test/remote_channel...
File cc/test/remote_channel_impl_for_test.cc (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/test/remote_channel...
cc/test/remote_channel_impl_for_test.cc:40: return std::move(proxy_impl);
On 2016/01/20 22:32:29, ericrk wrote:
> nit: from what I remember, std::move breaks RVO, so when returning a move-only
> type that matches the return type, it's better to just return directly without
> std::move.

We need std::move here since the return needs to upcast ProxyImplForTest to
ProxyImpl. I remember because I had to return directly and the compiler started
yelling at me.

https://codereview.chromium.org/1513643010/diff/160001/cc/test/remote_proto_c...
File cc/test/remote_proto_channel_bridge.h (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/test/remote_proto_c...
cc/test/remote_proto_channel_bridge.h:40: explicit
FakeRemoteProtoChannelMain(RemoteProtoChannelBridge* bridge,
On 2016/01/20 22:32:29, ericrk wrote:
> nit: remove explicit on a 2 arg constructor.

Done.

https://codereview.chromium.org/1513643010/diff/160001/cc/test/threaded_chann...
File cc/test/threaded_channel_for_test.cc (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/test/threaded_chann...
cc/test/threaded_channel_for_test.cc:36: return std::move(proxy_impl);
On 2016/01/20 22:32:29, ericrk wrote:
> same std::move comment from above.

Needed for the upcast.

https://codereview.chromium.org/1513643010/diff/160001/cc/trees/remote_channe...
File cc/trees/remote_channel_impl.h (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.h:175: // called on main thread.
On 2016/01/20 22:32:29, ericrk wrote:
> why not move these up to the other "called on main thread" block?

I was trying to write the interface implementations first, followed by the
methods for the class. Is there a preferred style here?

[ericrk, 2016-01-22 18:36:59]

Thanks for the details - makes sense to me now. This LGTM

Thanks!

https://codereview.chromium.org/1513643010/diff/160001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:466: if (task_runner_provider->HasImplThread()) {
On 2016/01/22 01:03:24, Khushal wrote:
> On 2016/01/20 22:32:29, ericrk wrote:
> > would it make sense to make IsRemoteClient/IsRemoteServer protected on
> > LayerTreeHost and just call that here - keeps the somewhat magic
HasImplThread
> > check in one place.
> 
> We pass both the task runner provider and the proxy to the layer_tree_host in
> InitializeForTesting, so we can't call the IsFoo methods on them here since
the
> LayerTreeHost would need the taskrunnerprovider for those.
> I think this might end up being the only other place where we have to do this
> check, since the tests that need to care about the mode are LayerTreeTests.
The
> other subclasses of the LayerTreeHost used in tests should be agnostic to the
> mode the compositor is running in, right?

ok, fair - yeah, as long as we don't have this check all over the place, that
seems fine - and yes, I agree that other parts of the tests would be agnostic to
the mode.

https://codereview.chromium.org/1513643010/diff/160001/cc/test/remote_channel...
File cc/test/remote_channel_impl_for_test.cc (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/test/remote_channel...
cc/test/remote_channel_impl_for_test.cc:40: return std::move(proxy_impl);
On 2016/01/22 01:03:24, Khushal wrote:
> On 2016/01/20 22:32:29, ericrk wrote:
> > nit: from what I remember, std::move breaks RVO, so when returning a
move-only
> > type that matches the return type, it's better to just return directly
without
> > std::move.
> 
> We need std::move here since the return needs to upcast ProxyImplForTest to
> ProxyImpl. I remember because I had to return directly and the compiler
started
> yelling at me.

oh, true, missed that.

https://codereview.chromium.org/1513643010/diff/160001/cc/trees/remote_channe...
File cc/trees/remote_channel_impl.h (right):

https://codereview.chromium.org/1513643010/diff/160001/cc/trees/remote_channe...
cc/trees/remote_channel_impl.h:175: // called on main thread.
On 2016/01/22 01:03:24, Khushal wrote:
> On 2016/01/20 22:32:29, ericrk wrote:
> > why not move these up to the other "called on main thread" block?
> 
> I was trying to write the interface implementations first, followed by the
> methods for the class. Is there a preferred style here?

hmm... yeah, that is the style, I guess we don't really have a standard for the
"called on X thread" stuff... so this seems fine.

[Khushal, 2016-01-25 16:41:51]

On 2016/01/22 18:36:59, ericrk wrote:
> Thanks for the details - makes sense to me now. This LGTM
> 
> Thanks!
> 
>
https://codereview.chromium.org/1513643010/diff/160001/cc/test/layer_tree_tes...
> File cc/test/layer_tree_test.cc (right):
> 
>
https://codereview.chromium.org/1513643010/diff/160001/cc/test/layer_tree_tes...
> cc/test/layer_tree_test.cc:466: if (task_runner_provider->HasImplThread()) {
> On 2016/01/22 01:03:24, Khushal wrote:
> > On 2016/01/20 22:32:29, ericrk wrote:
> > > would it make sense to make IsRemoteClient/IsRemoteServer protected on
> > > LayerTreeHost and just call that here - keeps the somewhat magic
> HasImplThread
> > > check in one place.
> > 
> > We pass both the task runner provider and the proxy to the layer_tree_host
in
> > InitializeForTesting, so we can't call the IsFoo methods on them here since
> the
> > LayerTreeHost would need the taskrunnerprovider for those.
> > I think this might end up being the only other place where we have to do
this
> > check, since the tests that need to care about the mode are LayerTreeTests.
> The
> > other subclasses of the LayerTreeHost used in tests should be agnostic to
the
> > mode the compositor is running in, right?
> 
> ok, fair - yeah, as long as we don't have this check all over the place, that
> seems fine - and yes, I agree that other parts of the tests would be agnostic
to
> the mode.
> 
>
https://codereview.chromium.org/1513643010/diff/160001/cc/test/remote_channel...
> File cc/test/remote_channel_impl_for_test.cc (right):
> 
>
https://codereview.chromium.org/1513643010/diff/160001/cc/test/remote_channel...
> cc/test/remote_channel_impl_for_test.cc:40: return std::move(proxy_impl);
> On 2016/01/22 01:03:24, Khushal wrote:
> > On 2016/01/20 22:32:29, ericrk wrote:
> > > nit: from what I remember, std::move breaks RVO, so when returning a
> move-only
> > > type that matches the return type, it's better to just return directly
> without
> > > std::move.
> > 
> > We need std::move here since the return needs to upcast ProxyImplForTest to
> > ProxyImpl. I remember because I had to return directly and the compiler
> started
> > yelling at me.
> 
> oh, true, missed that.
> 
>
https://codereview.chromium.org/1513643010/diff/160001/cc/trees/remote_channe...
> File cc/trees/remote_channel_impl.h (right):
> 
>
https://codereview.chromium.org/1513643010/diff/160001/cc/trees/remote_channe...
> cc/trees/remote_channel_impl.h:175: // called on main thread.
> On 2016/01/22 01:03:24, Khushal wrote:
> > On 2016/01/20 22:32:29, ericrk wrote:
> > > why not move these up to the other "called on main thread" block?
> > 
> > I was trying to write the interface implementations first, followed by the
> > methods for the class. Is there a preferred style here?
> 
> hmm... yeah, that is the style, I guess we don't really have a standard for
the
> "called on X thread" stuff... so this seems fine.

Thanks ericrk!
vmpstr@, could you take a look at this as well? Thanks!

[David Trainor- moved to gerrit, 2016-01-25 17:47:48]

ui/android/resources lgtm

[Khushal, 2016-01-27 00:25:22]

friendly ping! :)

[Khushal, 2016-01-28 06:00:09]

The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-28 06:00:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1513643010/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1513643010/220001

[commit-bot: I haz the power, 2016-01-28 06:33:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-28 06:33:29]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Khushal, 2016-01-28 19:34:31]

The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-28 19:36:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1513643010/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1513643010/240001

[commit-bot: I haz the power, 2016-01-28 20:51:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-28 20:51:51]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Khushal, 2016-01-28 21:14:02]

The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-28 21:14:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1513643010/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1513643010/260001

[Khushal, 2016-01-28 22:05:07]

The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-28 22:06:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1513643010/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1513643010/280001

[vmpstr, 2016-01-28 22:48:27]

lgtm with a couple of comments

https://codereview.chromium.org/1513643010/diff/280001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1513643010/diff/280001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:476: default:
nit: Don't add default if you're handling all cases. For unhandled cases, the
compiler will complain.

https://codereview.chromium.org/1513643010/diff/280001/cc/test/test_hooks.h
File cc/test/test_hooks.h (right):

https://codereview.chromium.org/1513643010/diff/280001/cc/test/test_hooks.h#n...
cc/test/test_hooks.h:142: virtual void CreateRemoteClientHost(
Can you leave a comment here with intended usage?

[commit-bot: I haz the power, 2016-01-28 23:31:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-28 23:31:18]

Dry run: This issue passed the CQ dry run.

[Khushal, 2016-01-29 00:07:43]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-01-29 00:07:44]

The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org,
dtrainor@chromium.org, vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1513643010/#ps300001
(title: "Addressed vmpstr's comments.")

[Khushal, 2016-01-29 00:08:00]

Done. Thanks Vlad!

https://codereview.chromium.org/1513643010/diff/280001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/1513643010/diff/280001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:476: default:
On 2016/01/28 22:48:27, vmpstr wrote:
> nit: Don't add default if you're handling all cases. For unhandled cases, the
> compiler will complain.

done.

https://codereview.chromium.org/1513643010/diff/280001/cc/test/test_hooks.h
File cc/test/test_hooks.h (right):

https://codereview.chromium.org/1513643010/diff/280001/cc/test/test_hooks.h#n...
cc/test/test_hooks.h:142: virtual void CreateRemoteClientHost(
On 2016/01/28 22:48:27, vmpstr wrote:
> Can you leave a comment here with intended usage?

Done.

[commit-bot: I haz the power, 2016-01-29 00:09:34]

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

[commit-bot: I haz the power, 2016-01-29 01:15:13]

Description was changed from

==========
cc:: Add remote mode to the compositor.

- Added RemoteChannelMain and RemoteChannelImpl to serialize and transmit
  inter-proxy messages between the main and impl components of the
  compositor using protobufs.
- Added RemoteChannelHost to be used by the cc embedder as the API for
  interacting with the impl components of the remote compositor.
- Added tests to verify the initialization and shutdown sequence for the
  remote compositor.

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

to

==========
cc:: Add remote mode to the compositor.

- Added RemoteChannelMain and RemoteChannelImpl to serialize and transmit
  inter-proxy messages between the main and impl components of the
  compositor using protobufs.
- Added RemoteChannelHost to be used by the cc embedder as the API for
  interacting with the impl components of the remote compositor.
- Added tests to verify the initialization and shutdown sequence for the
  remote compositor.

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

[commit-bot: I haz the power, 2016-01-29 01:15:13]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-01-29 01:16:20]

Description was changed from

==========
cc:: Add remote mode to the compositor.

- Added RemoteChannelMain and RemoteChannelImpl to serialize and transmit
  inter-proxy messages between the main and impl components of the
  compositor using protobufs.
- Added RemoteChannelHost to be used by the cc embedder as the API for
  interacting with the impl components of the remote compositor.
- Added tests to verify the initialization and shutdown sequence for the
  remote compositor.

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

to

==========
cc:: Add remote mode to the compositor.

- Added RemoteChannelMain and RemoteChannelImpl to serialize and transmit
  inter-proxy messages between the main and impl components of the
  compositor using protobufs.
- Added RemoteChannelHost to be used by the cc embedder as the API for
  interacting with the impl components of the remote compositor.
- Added tests to verify the initialization and shutdown sequence for the
  remote compositor.

BUG=550687
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e0a38d4e0b43b05e76c4544d9e715107c8e0a50f
Cr-Commit-Position: refs/heads/master@{#372236}
==========

[commit-bot: I haz the power, 2016-01-29 01:16:21]

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