Add glue between the client and engine for Blimp

Add glue between the client and engine for Blimp

- Add BlimpMessageProcessors to handle RENDER_WIDGET, INPUT, and
  COMPOSITOR message types to/from the client and engine.
- Tie the message processors into BlimpCompositor on the client and
  BlimpEngineSession on the server.
- Update BlimpView and BlimpCompositor to fix how we handle output
  surfaces and how we shut down and build the LayerTreeHost based on
  incoming RENDER_WIDGET and COMPOSITOR messages.

BUG=550693
Committed: https://crrev.com/160f8eca573286f3bb369975ac2e9de63b0cc477
Cr-Commit-Position: refs/heads/master@{#362299}

[David Trainor- moved to gerrit, 2015-11-18 00:31:41]

Description was changed from

==========
Add glue between the client and engine for Blimp

- Add BlimpMessageProcessors to handle RENDER_WIDGET message types
  to/from the client and engine.
- Tie the message processors into BlimpCompositor on the client and
  BlimpEngineSession on the server.
- Restructure the protobuf format to move INPUT and COMPOSITOR messages
  under the RENDER_WIDGET message type (as RenderWidgets can change
  during navigation and stale messages need to be dropped).
- Update BlimpView and BlimpCompositor to fix how we handle output
  surfaces and how we shut down and build the LayerTreeHost based on
  incoming RENDER_WIDGET messages.

BUG=550693
==========

to

==========
Add glue between the client and engine for Blimp

- Add BlimpMessageProcessors to handle RENDER_WIDGET message types
  to/from the client and engine.
- Tie the message processors into BlimpCompositor on the client and
  BlimpEngineSession on the server.
- Restructure the protobuf format to move INPUT and COMPOSITOR messages
  under the RENDER_WIDGET message type (as RenderWidgets can change
  during navigation and stale messages need to be dropped).
- Update BlimpView and BlimpCompositor to fix how we handle output
  surfaces and how we shut down and build the LayerTreeHost based on
  incoming RENDER_WIDGET messages.

BUG=550693
==========

[David Trainor- moved to gerrit, 2015-11-18 00:34:40]

dtrainor@chromium.org changed reviewers:
+ haibinlu@chromium.org, khushalsagar@chromium.org, kmarshall@chromium.org,
nyquist@chromium.org

[David Trainor- moved to gerrit, 2015-11-18 00:34:40]

ptal!

haibinlu@: blimp/engine and blimp/client/render_widget_message_processor

kmarshall@: blimp/engine/browser/host_render_widget_message_processor,
blimp/client/render_widget_message_processor, and *.proto.

khushalsagar@ and nyquist@: blimp/client.

cc'ed wez@ for fyi!

Thanks!  Hopefully after this and the input most of the glue will be in place...
then we just need to add a bit more glue for the navigation and resize messages,
but those are relatively simple.

[David Trainor- moved to gerrit, 2015-11-18 00:35:30]

Oh I'll look into adding unit tests for the message processors.  Sorry about
that!

[haibinlu, 2015-11-19 01:08:16]

minor comments for blimp/engine/

https://codereview.chromium.org/1450423002/diff/60001/blimp/client/compositor...
File blimp/client/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/1450423002/diff/60001/blimp/client/compositor...
blimp/client/compositor/blimp_compositor.cc:39: class BlackHoleMessageProcessor
: public BlimpMessageProcessor {
put this in blimp/net/common.h?

https://codereview.chromium.org/1450423002/diff/60001/blimp/common/proto/resi...
File blimp/common/proto/resize.proto (right):

https://codereview.chromium.org/1450423002/diff/60001/blimp/common/proto/resi...
blimp/common/proto/resize.proto:13: message ResizeMessage {
or just SizeMessage?

https://codereview.chromium.org/1450423002/diff/60001/blimp/engine/browser/ho...
File blimp/engine/browser/host_render_widget_message_processor.cc (right):

https://codereview.chromium.org/1450423002/diff/60001/blimp/engine/browser/ho...
blimp/engine/browser/host_render_widget_message_processor.cc:31:
render_widget_ids_[tab_id] = GetRenderWidgetId(tab_id) + 1;
a separate method to handle updating render widget id? or rename it to
OnRenderWidgetInitialized so that it handles both update and message sending.

https://codereview.chromium.org/1450423002/diff/60001/blimp/engine/browser/ho...
blimp/engine/browser/host_render_widget_message_processor.cc:99:
memcpy(payload.data(),
Can we avoid copying the payload?

https://codereview.chromium.org/1450423002/diff/60001/blimp/engine/browser/ho...
File blimp/engine/browser/host_render_widget_message_processor.h (right):

https://codereview.chromium.org/1450423002/diff/60001/blimp/engine/browser/ho...
blimp/engine/browser/host_render_widget_message_processor.h:33: virtual void
OnTouchEvent(scoped_ptr<blink::WebInputEvent> event) = 0;
does this include keyboard inputs etc? If so, maybe call it OnWebInputEvent?

https://codereview.chromium.org/1450423002/diff/60001/blimp/engine/browser/ho...
blimp/engine/browser/host_render_widget_message_processor.h:68: typedef
base::hash_map<int, RenderWidgetMessageDelegate*> DelegateMap;
use base/containers/small_map.h instead since # of tabs will be small?

[David Trainor- moved to gerrit, 2015-11-19 03:29:36]

Okay made the changes based on the discussion yesterday.  Also added unit tests
and cleaned this up a bit.  ptal!

https://codereview.chromium.org/1450423002/diff/60001/blimp/client/compositor...
File blimp/client/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/1450423002/diff/60001/blimp/client/compositor...
blimp/client/compositor/blimp_compositor.cc:39: class BlackHoleMessageProcessor
: public BlimpMessageProcessor {
On 2015/11/19 01:08:15, haibinlu wrote:
> put this in blimp/net/common.h?

Pulled it out to blimp/net/black_hole_blimp_message_processor.h.  Is that okay?

https://codereview.chromium.org/1450423002/diff/60001/blimp/engine/browser/ho...
File blimp/engine/browser/host_render_widget_message_processor.cc (right):

https://codereview.chromium.org/1450423002/diff/60001/blimp/engine/browser/ho...
blimp/engine/browser/host_render_widget_message_processor.cc:31:
render_widget_ids_[tab_id] = GetRenderWidgetId(tab_id) + 1;
On 2015/11/19 01:08:15, haibinlu wrote:
> a separate method to handle updating render widget id? or rename it to
> OnRenderWidgetInitialized so that it handles both update and message sending.

Done.

https://codereview.chromium.org/1450423002/diff/60001/blimp/engine/browser/ho...
blimp/engine/browser/host_render_widget_message_processor.cc:99:
memcpy(payload.data(),
On 2015/11/19 01:08:15, haibinlu wrote:
> Can we avoid copying the payload?

I don't think we can without changing the interface to use std::string (which
feels weird too).  I don't like that protobufs send raw bytes as an std::string
instead of an std::vector :(.  I'll put a TODO and look into changing the
RenderWidgetHost interface in another patch.

https://codereview.chromium.org/1450423002/diff/60001/blimp/engine/browser/ho...
File blimp/engine/browser/host_render_widget_message_processor.h (right):

https://codereview.chromium.org/1450423002/diff/60001/blimp/engine/browser/ho...
blimp/engine/browser/host_render_widget_message_processor.h:33: virtual void
OnTouchEvent(scoped_ptr<blink::WebInputEvent> event) = 0;
On 2015/11/19 01:08:15, haibinlu wrote:
> does this include keyboard inputs etc? If so, maybe call it OnWebInputEvent?

Done.

https://codereview.chromium.org/1450423002/diff/60001/blimp/engine/browser/ho...
blimp/engine/browser/host_render_widget_message_processor.h:68: typedef
base::hash_map<int, RenderWidgetMessageDelegate*> DelegateMap;
On 2015/11/19 01:08:16, haibinlu wrote:
> use base/containers/small_map.h instead since # of tabs will be small?

Ah good idea.  Done.

[David Trainor- moved to gerrit, 2015-11-19 17:01:21]

https://chromiumcodereview.appspot.com/1450423002/diff/120001/blimp/net/black...
File blimp/net/black_hole_blimp_message_processor.cc (right):

https://chromiumcodereview.appspot.com/1450423002/diff/120001/blimp/net/black...
blimp/net/black_hole_blimp_message_processor.cc:11: const
net::CompletionCallback& callback) {}
Oh oops this needs to call the callback.  My bad.

[haibinlu, 2015-11-19 19:21:17]

lgtm for blimp/engine

https://codereview.chromium.org/1450423002/diff/80001/blimp/client/render_wid...
File blimp/client/render_widget_message_processor_unittest.cc (right):

https://codereview.chromium.org/1450423002/diff/80001/blimp/client/render_wid...
blimp/client/render_widget_message_processor_unittest.cc:26: class
MockBlimpMessageProcessor : public BlimpMessageProcessor {
blimp/net/test_common.h has MockBlimpMessageProcessor.

https://codereview.chromium.org/1450423002/diff/80001/blimp/engine/browser/ho...
File blimp/engine/browser/host_render_widget_message_processor_unittest.cc
(right):

https://codereview.chromium.org/1450423002/diff/80001/blimp/engine/browser/ho...
blimp/engine/browser/host_render_widget_message_processor_unittest.cc:27: class
MockBlimpMessageProcessor : public BlimpMessageProcessor {
ditto. use test_common.h

https://codereview.chromium.org/1450423002/diff/80001/blimp/engine/browser/ho...
blimp/engine/browser/host_render_widget_message_processor_unittest.cc:128:
EXPECT_CALL(delegate1_, MockableOnCompositorMessageReceived()).Times(1);
add a test case to check the correct payload is received.

[Khushal, 2015-11-19 19:28:24]

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
File blimp/client/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.cc:121: void
BlimpCompositor::ReleaseAcceleratedWidget() {
We should be able to simplify this. The visibility and output surface logic is
so tightly coupled in the implementation here.

How about we document on the API of this class that ReleaseAcceleratedWidget can
only be called if we are invisible, store the visibility status and just DCHECK
that here.

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.cc:149: output_surface_request_pending_
= true;
nit: Have a method HandlePendingOutputSurfaceRequest()? So we call that from
here and SetVisible()?

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
File blimp/client/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:49: void SetVisible(bool visible);
We would want to inform the engine of the visibility changes too.

Though I don't think we need to have the visibility status part of the
compositor protocol. The status will change on the client and any message we get
from the engine would probably be stale anyway.

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:59: // gfx::AcceleratedWidget on
another compositor.
Is this for when we have multiple tabs?

I think when we design for multiple tabs, we will avoid this situation when we
move this to a higher layer. The blimp_view from which we get the input events
and the widget will be shared across tabs. So we could move the widget to this
layer that the view talks to and have the compositor get the output surface from
this layer. The layer would ensure that only the active compositor has the
output surface. It will ensure that it asks the active compositor to release the
surface before giving it to another compositor.

[nyquist, 2015-11-20 08:14:59]

https://codereview.chromium.org/1450423002/diff/140001/blimp/client/android/b...
File blimp/client/android/blimp_view.cc (right):

https://codereview.chromium.org/1450423002/diff/140001/blimp/client/android/b...
blimp/client/android/blimp_view.cc:89: if (window_ !=
gfx::kNullAcceleratedWidget) {
Optional nit: == and early return?

https://codereview.chromium.org/1450423002/diff/140001/blimp/client/composito...
File blimp/client/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/1450423002/diff/140001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.cc:84: if (!visible) {
Nit: Why the flipped conditional? I often find it easier with just if
(condition) than the negative version first.

https://codereview.chromium.org/1450423002/diff/140001/blimp/client/composito...
File blimp/client/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/1450423002/diff/140001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:34: // BlimpCompositor provides the
basic framework and setup to host a
Can this class be interacted with from any thread, or just from the main thread?
There are several threads involved usually in compositor-related code, so
clarifying it in a comment might be helpful for future readers.

https://codereview.chromium.org/1450423002/diff/140001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:63: // output surface.  This should
be called before calling
Should or must? I believe you use 'must' above.

https://codereview.chromium.org/1450423002/diff/140001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:69: // |dp_to_px| is the scale factor
required to move from dp (device pixels) to
For new readers, would we want to link to
https://developer.android.com/guide/practices/screens_support.html or something
along those lines?

https://codereview.chromium.org/1450423002/diff/140001/blimp/common/proto/com...
File blimp/common/proto/common.proto (right):

https://codereview.chromium.org/1450423002/diff/140001/blimp/common/proto/com...
blimp/common/proto/common.proto:16: }
Nit: Why no newline?

https://codereview.chromium.org/1450423002/diff/140001/blimp/common/proto/ren...
File blimp/common/proto/render_widget.proto (right):

https://codereview.chromium.org/1450423002/diff/140001/blimp/common/proto/ren...
blimp/common/proto/render_widget.proto:10: import "compositor.proto";
What are these two imports for?

[David Trainor- moved to gerrit, 2015-11-20 17:58:54]

https://codereview.chromium.org/1450423002/diff/60001/blimp/common/proto/resi...
File blimp/common/proto/resize.proto (right):

https://codereview.chromium.org/1450423002/diff/60001/blimp/common/proto/resi...
blimp/common/proto/resize.proto:13: message ResizeMessage {
On 2015/11/19 01:08:15, haibinlu wrote:
> or just SizeMessage?

Done.

https://codereview.chromium.org/1450423002/diff/80001/blimp/client/render_wid...
File blimp/client/render_widget_message_processor_unittest.cc (right):

https://codereview.chromium.org/1450423002/diff/80001/blimp/client/render_wid...
blimp/client/render_widget_message_processor_unittest.cc:26: class
MockBlimpMessageProcessor : public BlimpMessageProcessor {
On 2015/11/19 19:21:17, haibinlu wrote:
> blimp/net/test_common.h has MockBlimpMessageProcessor.

Do we need a component that exposes test_common?  Right now it's just baked into
the blimp/net unittests compiled code.

Kevin, do you have any ideas on how you want me to export that file?

https://codereview.chromium.org/1450423002/diff/80001/blimp/engine/browser/ho...
File blimp/engine/browser/host_render_widget_message_processor_unittest.cc
(right):

https://codereview.chromium.org/1450423002/diff/80001/blimp/engine/browser/ho...
blimp/engine/browser/host_render_widget_message_processor_unittest.cc:128:
EXPECT_CALL(delegate1_, MockableOnCompositorMessageReceived()).Times(1);
On 2015/11/19 19:21:17, haibinlu wrote:
> add a test case to check the correct payload is received.

Good point.  I baked this into the expected results for all of the tests that
call the method.

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
File blimp/client/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.cc:121: void
BlimpCompositor::ReleaseAcceleratedWidget() {
On 2015/11/19 19:28:24, Khushal wrote:
> We should be able to simplify this. The visibility and output surface logic is
> so tightly coupled in the implementation here.
> 
> How about we document on the API of this class that ReleaseAcceleratedWidget
can
> only be called if we are invisible, store the visibility status and just
DCHECK
> that here.

I think this will get simplified when we split out the intermediate class that
manages which BlimpCompositor is currently visible (we'll push the complexity up
there instead of pulling it down from BlimpView).  I'd rather keep BlimpView as
thin as possible for now if I can.

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.cc:149: output_surface_request_pending_
= true;
On 2015/11/19 19:28:24, Khushal wrote:
> nit: Have a method HandlePendingOutputSurfaceRequest()? So we call that from
> here and SetVisible()?

Done.

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
File blimp/client/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:49: void SetVisible(bool visible);
On 2015/11/19 19:28:24, Khushal wrote:
> We would want to inform the engine of the visibility changes too.
> 
> Though I don't think we need to have the visibility status part of the
> compositor protocol. The status will change on the client and any message we
get
> from the engine would probably be stale anyway.

Yeah we'll inform the engine via another route.  I'd almost rather let the
engine inform the local compositor via the compositor protocol.  I'll leave this
until we integrate the actual remote channel as right now this is a LTH.

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:59: // gfx::AcceleratedWidget on
another compositor.
On 2015/11/19 19:28:24, Khushal wrote:
> Is this for when we have multiple tabs?
> 
> I think when we design for multiple tabs, we will avoid this situation when we
> move this to a higher layer. The blimp_view from which we get the input events
> and the widget will be shared across tabs. So we could move the widget to this
> layer that the view talks to and have the compositor get the output surface
from
> this layer. The layer would ensure that only the active compositor has the
> output surface. It will ensure that it asks the active compositor to release
the
> surface before giving it to another compositor.

Yeah that's what we talked about, but since there's no higher level object built
in to manage tabs yet, that logic should go here (not in BlimpView).  When we
spin up the Linux client I don't want to duplicate any actual logic in
platform-specific code.

When we split out that higher level class we'll move that code from here to
there (as opposed to from BlimpView to there).  Since that class doesn't exist
yet it should just live in here.

ie:

Now:
BlimpView (Android) -> BlimpCompositor
SomeView  (Linux)   -> BlimpCompositor

Future:
- Keep BlimpView/SomeView the same (platform abstractions can be thin).
- Split BlimpCompositor instead.

BlimpView -> BlimpTabManager -> BlimpCompositor
SomeView  -> BlimpTabManager -> BlimpCompositor

https://codereview.chromium.org/1450423002/diff/120001/blimp/net/black_hole_b...
File blimp/net/black_hole_blimp_message_processor.cc (right):

https://codereview.chromium.org/1450423002/diff/120001/blimp/net/black_hole_b...
blimp/net/black_hole_blimp_message_processor.cc:11: const
net::CompletionCallback& callback) {}
On 2015/11/19 17:01:21, David Trainor wrote:
> Oh oops this needs to call the callback.  My bad.

Done.

[haibinlu, 2015-11-20 18:20:15]

https://codereview.chromium.org/1450423002/diff/80001/blimp/client/render_wid...
File blimp/client/render_widget_message_processor_unittest.cc (right):

https://codereview.chromium.org/1450423002/diff/80001/blimp/client/render_wid...
blimp/client/render_widget_message_processor_unittest.cc:26: class
MockBlimpMessageProcessor : public BlimpMessageProcessor {
On 2015/11/20 17:58:53, David Trainor wrote:
> On 2015/11/19 19:21:17, haibinlu wrote:
> > blimp/net/test_common.h has MockBlimpMessageProcessor.
> 
> Do we need a component that exposes test_common?  Right now it's just baked
into
> the blimp/net unittests compiled code.
> 
> Kevin, do you have any ideas on how you want me to export that file?

Ah, I see. probably not worth doing that right now.

[Khushal, 2015-11-23 17:40:11]

lgtm. Thanks! :)

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
File blimp/client/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.cc:121: void
BlimpCompositor::ReleaseAcceleratedWidget() {
On 2015/11/20 17:58:53, David Trainor wrote:
> On 2015/11/19 19:28:24, Khushal wrote:
> > We should be able to simplify this. The visibility and output surface logic
is
> > so tightly coupled in the implementation here.
> > 
> > How about we document on the API of this class that ReleaseAcceleratedWidget
> can
> > only be called if we are invisible, store the visibility status and just
> DCHECK
> > that here.
> 
> I think this will get simplified when we split out the intermediate class that
> manages which BlimpCompositor is currently visible (we'll push the complexity
up
> there instead of pulling it down from BlimpView).  I'd rather keep BlimpView
as
> thin as possible for now if I can.

SGTM.

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
File blimp/client/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:49: void SetVisible(bool visible);
On 2015/11/20 17:58:53, David Trainor wrote:
> On 2015/11/19 19:28:24, Khushal wrote:
> > We would want to inform the engine of the visibility changes too.
> > 
> > Though I don't think we need to have the visibility status part of the
> > compositor protocol. The status will change on the client and any message we
> get
> > from the engine would probably be stale anyway.
> 
> Yeah we'll inform the engine via another route.  I'd almost rather let the
> engine inform the local compositor via the compositor protocol.  I'll leave
this
> until we integrate the actual remote channel as right now this is a LTH.

Right now the LTH expects you to make it invisible before you ask it to release
the output surface. If we also want to do too that then we'll have to inform the
impl side locally about the visibility change. We could try dropping that
condition for us and just ask it to release the surface.

We can decide on this.

https://codereview.chromium.org/1450423002/diff/120001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:59: // gfx::AcceleratedWidget on
another compositor.
On 2015/11/20 17:58:53, David Trainor wrote:
> On 2015/11/19 19:28:24, Khushal wrote:
> > Is this for when we have multiple tabs?
> > 
> > I think when we design for multiple tabs, we will avoid this situation when
we
> > move this to a higher layer. The blimp_view from which we get the input
events
> > and the widget will be shared across tabs. So we could move the widget to
this
> > layer that the view talks to and have the compositor get the output surface
> from
> > this layer. The layer would ensure that only the active compositor has the
> > output surface. It will ensure that it asks the active compositor to release
> the
> > surface before giving it to another compositor.
> 
> Yeah that's what we talked about, but since there's no higher level object
built
> in to manage tabs yet, that logic should go here (not in BlimpView).  When we
> spin up the Linux client I don't want to duplicate any actual logic in
> platform-specific code.
> 
> When we split out that higher level class we'll move that code from here to
> there (as opposed to from BlimpView to there).  Since that class doesn't exist
> yet it should just live in here.
> 
> ie:
> 
> Now:
> BlimpView (Android) -> BlimpCompositor
> SomeView  (Linux)   -> BlimpCompositor
> 
> Future:
> - Keep BlimpView/SomeView the same (platform abstractions can be thin).
> - Split BlimpCompositor instead.
> 
> BlimpView -> BlimpTabManager -> BlimpCompositor
> SomeView  -> BlimpTabManager -> BlimpCompositor

Makes sense.

[nyquist, 2015-11-24 08:55:44]

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/android/b...
File blimp/client/android/blimp_view.cc (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/android/b...
blimp/client/android/blimp_view.cc:89: if (window_ !=
gfx::kNullAcceleratedWidget) {
Optional nit: == and early return?

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/composito...
File blimp/client/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.cc:84: if (!visible) {
Nit: Why the flipped conditional? I often find it easier with just |if
(condition)| than the negative version first.

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.cc:184: // Destroy the old compositor
instance.
This comment is a little bit confusing to me. The comment refers to a
'compositor', but this class itself is the BlimpCompositor, and the member it
resets is a LayerTreeHost. Could you make the comment clearer?

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/composito...
File blimp/client/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:34: // BlimpCompositor provides the
basic framework and setup to host a
Nit: Can this class be interacted with from any thread, or just from the main
thread?
There are several threads involved usually in compositor-related code, so
clarifying it in a comment might be helpful for future readers.

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:46: // Setting this to false will
make the compositor drop all of it's resources
Nit: I think we want to refer to things the pronoun it owns here? So "its"?

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:52: // TODO(dtrainor): Should this be
pulled from the engine all the time?
Nit: "pulled" for me here has the special meaning of a specific action that
would be performed by the client on the engine "all the time". Do you mean that
you're considering whether the engine should send this value with every message
/ update, or maybe something else?

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:63: // output surface.  This should
be called before calling
Nit: Should or must? I believe you use 'must' above.

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:69: // |dp_to_px| is the scale factor
required to move from dp (device pixels) to
Optional nit: For new readers, would we want to link to
https://developer.android.com/guide/practices/screens_support.html or something
along those lines?

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/render_wi...
File blimp/client/render_widget_message_processor.h (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/render_wi...
blimp/client/render_widget_message_processor.h:5: #ifndef
BLIMP_CLIENT_RENDER_WIDGET_MESSAGE_PROCESSOR_H_
Would it make sense to put these in some subfolder? renderwidget?
It seems a bit odd that this class is so important it has to live alone in the
top-level client-directory. Is it just because we have no browser/renderer parts
on the client?

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/render_wi...
blimp/client/render_widget_message_processor.h:17: }
Nit: // namespace blink?

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/render_wi...
blimp/client/render_widget_message_processor.h:22: }
Nit: end of namespace comment here and below?

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/render_wi...
blimp/client/render_widget_message_processor.h:63: void SetDelegate(const int
tab_id, RenderWidgetMessageDelegate* delegate);
Nit: Could you add a comment saying there can only be one RWMD per |tab_id|?

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/render_wi...
blimp/client/render_widget_message_processor.h:71: RenderWidgetMessageDelegate*
FindDelegate(const int tab_id);
Optional nit: Do we want to add a comment saying that nullptr will be returned
if it is not found? Or is that implicit since it's a pointer you return?

https://codereview.chromium.org/1450423002/diff/160001/blimp/common/proto/com...
File blimp/common/proto/common.proto (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/common/proto/com...
blimp/common/proto/common.proto:16: }
Nit: Why no newline anymore? Also, does this have to be part of this CL at all?

https://codereview.chromium.org/1450423002/diff/160001/blimp/common/proto/ren...
File blimp/common/proto/render_widget.proto (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/common/proto/ren...
blimp/common/proto/render_widget.proto:10: import "compositor.proto";
Nit: What are these two imports for?

[nyquist, 2015-11-24 08:59:39]

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/composito...
File blimp/client/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.cc:153: void
BlimpCompositor::DidInitializeOutputSurface() {}
Should this method reset output_surface_request_pending_?

[Kevin M, 2015-11-24 18:39:59]

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/render_wi...
File blimp/client/render_widget_message_processor.cc (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/render_wi...
blimp/client/render_widget_message_processor.cc:32:
blimp_message->set_type(BlimpMessage::INPUT);
Can be omitted, the MultiplexedSender sets the type.

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/render_wi...
blimp/client/render_widget_message_processor.cc:39:
outgoing_message_processor_->ProcessMessage(blimp_message.Pass(),
The output message processors only handle one message type, see
https://code.google.com/p/chromium/codesearch#chromium/src/blimp/net/blimp_me...

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/render_wi...
blimp/client/render_widget_message_processor.cc:54:
message.SerializeToArray(payload.data(), message.ByteSize());
message.SerializeWithCachedSizes?

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/render_wi...
blimp/client/render_widget_message_processor.cc:56:
compositor_message->set_payload(payload.data(),
What about serializing to *compositor_message->mutable_payload()->mutable_data()
directly?

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/render_wi...
blimp/client/render_widget_message_processor.cc:126: return 0U;
Does this indicate an error? Should callers be testing for zero?

https://codereview.chromium.org/1450423002/diff/160001/blimp/common/proto/com...
File blimp/common/proto/compositor.proto (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/common/proto/com...
blimp/common/proto/compositor.proto:16: // An ID that corresponds to
RenderWidgetMessage.
Corresponds to WGM.render_widget_id

https://codereview.chromium.org/1450423002/diff/160001/blimp/common/proto/com...
blimp/common/proto/compositor.proto:19: // This represents a serialized
CompositorMessage in
What's the reasoning behind serializing the message here? There are some nice
benefits to keeping the structure in protobuf format, e.g. less data to store
and copy around, and PB parse errors are detected earlier on and in common
codepaths. Having blimp depend on cc seems fine to me (not the other way around,
of course).

https://codereview.chromium.org/1450423002/diff/160001/blimp/engine/browser/b...
File blimp/engine/browser/blimp_engine_session.cc (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/engine/browser/b...
blimp/engine/browser/blimp_engine_session.cc:65:
render_widget_processor_(g_black_hole_message_processor.Pointer()) {
What's the purpose of this? Why not attach to a MultiplexedSender immediately?
It should be available at any time; it's not tied to the availability of a
BlimpConnection.

https://codereview.chromium.org/1450423002/diff/160001/blimp/engine/browser/h...
File blimp/engine/browser/host_render_widget_message_processor.cc (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/engine/browser/h...
blimp/engine/browser/host_render_widget_message_processor.cc:49: const
std::vector<uint8_t>& message) {
Take a compositor proto scoped ptr?

https://codereview.chromium.org/1450423002/diff/160001/blimp/engine/browser/h...
blimp/engine/browser/host_render_widget_message_processor.cc:56:
compositor_message->set_payload(message.data(),
set_allocated_payload?

https://codereview.chromium.org/1450423002/diff/160001/blimp/engine/browser/h...
File blimp/engine/browser/host_render_widget_message_processor.h (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/engine/browser/h...
blimp/engine/browser/host_render_widget_message_processor.h:28: class
HostRenderWidgetMessageProcessor : public BlimpMessageProcessor {
Maybe I'm getting the terminology mixed up, but should Host be Engine?

https://codereview.chromium.org/1450423002/diff/160001/blimp/net/black_hole_b...
File blimp/net/black_hole_blimp_message_processor.h (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/net/black_hole_b...
blimp/net/black_hole_blimp_message_processor.h:15: class BLIMP_NET_EXPORT
BlackHoleBlimpMessageProcessor
NullBlimpMessageProcessor?

Add a class description

[nyquist, 2015-11-24 20:24:47]

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/composito...
File blimp/client/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/1450423002/diff/160001/blimp/client/composito...
blimp/client/compositor/blimp_compositor.h:35: // LayerTreeHost (compositor). 
The rendering surface this compositor draws to
Nit: It's a little bit confusing having "(compositor)" here, since the class
name is named BlimpCompositor. Could you add an explanation saying that the
class that owns the LayerTreeHost is usually called the compositor?

[David Trainor- moved to gerrit, 2015-11-25 17:56:40]

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

[David Trainor- moved to gerrit, 2015-11-25 17:56:42]

rbyers@: can you look at blimp/engine/DEPS?
mmenke@: Can you look at blimp/client/DEPS?

Thanks! :)

https://chromiumcodereview.appspot.com/1450423002/diff/140001/blimp/client/an...
File blimp/client/android/blimp_view.cc (right):

https://chromiumcodereview.appspot.com/1450423002/diff/140001/blimp/client/an...
blimp/client/android/blimp_view.cc:89: if (window_ !=
gfx::kNullAcceleratedWidget) {
On 2015/11/20 08:14:59, nyquist wrote:
> Optional nit: == and early return?

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/140001/blimp/client/co...
File blimp/client/compositor/blimp_compositor.cc (right):

https://chromiumcodereview.appspot.com/1450423002/diff/140001/blimp/client/co...
blimp/client/compositor/blimp_compositor.cc:84: if (!visible) {
On 2015/11/20 08:14:59, nyquist wrote:
> Nit: Why the flipped conditional? I often find it easier with just if
> (condition) than the negative version first.

Good point!  Done.

https://chromiumcodereview.appspot.com/1450423002/diff/140001/blimp/client/co...
File blimp/client/compositor/blimp_compositor.h (right):

https://chromiumcodereview.appspot.com/1450423002/diff/140001/blimp/client/co...
blimp/client/compositor/blimp_compositor.h:34: // BlimpCompositor provides the
basic framework and setup to host a
On 2015/11/20 08:14:59, nyquist wrote:
> Can this class be interacted with from any thread, or just from the main
thread?
> There are several threads involved usually in compositor-related code, so
> clarifying it in a comment might be helpful for future readers.

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/140001/blimp/client/co...
blimp/client/compositor/blimp_compositor.h:63: // output surface.  This should
be called before calling
On 2015/11/20 08:14:59, nyquist wrote:
> Should or must? I believe you use 'must' above.

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/140001/blimp/client/co...
blimp/client/compositor/blimp_compositor.h:69: // |dp_to_px| is the scale factor
required to move from dp (device pixels) to
On 2015/11/20 08:14:59, nyquist wrote:
> For new readers, would we want to link to
> https://developer.android.com/guide/practices/screens_support.html or
something
> along those lines?

Good idea.  Done.

https://chromiumcodereview.appspot.com/1450423002/diff/140001/blimp/common/pr...
File blimp/common/proto/common.proto (right):

https://chromiumcodereview.appspot.com/1450423002/diff/140001/blimp/common/pr...
blimp/common/proto/common.proto:16: }
On 2015/11/20 08:14:59, nyquist wrote:
> Nit: Why no newline?

Removed the whole file now :).

https://chromiumcodereview.appspot.com/1450423002/diff/140001/blimp/common/pr...
File blimp/common/proto/render_widget.proto (right):

https://chromiumcodereview.appspot.com/1450423002/diff/140001/blimp/common/pr...
blimp/common/proto/render_widget.proto:10: import "compositor.proto";
On 2015/11/20 08:14:59, nyquist wrote:
> What are these two imports for?

nothing anymore!  Removed.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/an...
File blimp/client/android/blimp_view.cc (right):

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/an...
blimp/client/android/blimp_view.cc:89: if (window_ !=
gfx::kNullAcceleratedWidget) {
On 2015/11/24 08:55:43, nyquist wrote:
> Optional nit: == and early return?

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/co...
File blimp/client/compositor/blimp_compositor.cc (right):

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/co...
blimp/client/compositor/blimp_compositor.cc:84: if (!visible) {
On 2015/11/24 08:55:43, nyquist wrote:
> Nit: Why the flipped conditional? I often find it easier with just |if
> (condition)| than the negative version first.

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/co...
blimp/client/compositor/blimp_compositor.cc:153: void
BlimpCompositor::DidInitializeOutputSurface() {}
On 2015/11/24 08:59:39, nyquist wrote:
> Should this method reset output_surface_request_pending_?

We set it when we explicitly call SetOutputSurface() on the host.  If they fail
to initialize that's a different problem than whether or not we've answered the
initial request I think.  I think that other navigation CL had a mistake.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/co...
blimp/client/compositor/blimp_compositor.cc:184: // Destroy the old compositor
instance.
On 2015/11/24 08:55:43, nyquist wrote:
> This comment is a little bit confusing to me. The comment refers to a
> 'compositor', but this class itself is the BlimpCompositor, and the member it
> resets is a LayerTreeHost. Could you make the comment clearer?

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/co...
File blimp/client/compositor/blimp_compositor.h (right):

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/co...
blimp/client/compositor/blimp_compositor.h:34: // BlimpCompositor provides the
basic framework and setup to host a
On 2015/11/24 08:55:43, nyquist wrote:
> Nit: Can this class be interacted with from any thread, or just from the main
> thread?
> There are several threads involved usually in compositor-related code, so
> clarifying it in a comment might be helpful for future readers.

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/co...
blimp/client/compositor/blimp_compositor.h:35: // LayerTreeHost (compositor). 
The rendering surface this compositor draws to
On 2015/11/24 20:24:47, nyquist wrote:
> Nit: It's a little bit confusing having "(compositor)" here, since the class
> name is named BlimpCompositor. Could you add an explanation saying that the
> class that owns the LayerTreeHost is usually called the compositor?

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/co...
blimp/client/compositor/blimp_compositor.h:46: // Setting this to false will
make the compositor drop all of it's resources
On 2015/11/24 08:55:43, nyquist wrote:
> Nit: I think we want to refer to things the pronoun it owns here? So "its"?

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/co...
blimp/client/compositor/blimp_compositor.h:52: // TODO(dtrainor): Should this be
pulled from the engine all the time?
On 2015/11/24 08:55:43, nyquist wrote:
> Nit: "pulled" for me here has the special meaning of a specific action that
> would be performed by the client on the engine "all the time". Do you mean
that
> you're considering whether the engine should send this value with every
message
> / update, or maybe something else?

Changed it to set.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/co...
blimp/client/compositor/blimp_compositor.h:63: // output surface.  This should
be called before calling
On 2015/11/24 08:55:43, nyquist wrote:
> Nit: Should or must? I believe you use 'must' above.

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/co...
blimp/client/compositor/blimp_compositor.h:69: // |dp_to_px| is the scale factor
required to move from dp (device pixels) to
On 2015/11/24 08:55:43, nyquist wrote:
> Optional nit: For new readers, would we want to link to
> https://developer.android.com/guide/practices/screens_support.html or
something
> along those lines?

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/re...
File blimp/client/render_widget_message_processor.cc (right):

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/re...
blimp/client/render_widget_message_processor.cc:32:
blimp_message->set_type(BlimpMessage::INPUT);
On 2015/11/24 18:39:59, Kevin M wrote:
> Can be omitted, the MultiplexedSender sets the type.

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/re...
blimp/client/render_widget_message_processor.cc:39:
outgoing_message_processor_->ProcessMessage(blimp_message.Pass(),
On 2015/11/24 18:39:59, Kevin M wrote:
> The output message processors only handle one message type, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/blimp/net/blimp_me...

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/re...
blimp/client/render_widget_message_processor.cc:54:
message.SerializeToArray(payload.data(), message.ByteSize());
On 2015/11/24 18:39:59, Kevin M wrote:
> message.SerializeWithCachedSizes?

Ended up doing SerializeToString().

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/re...
blimp/client/render_widget_message_processor.cc:56:
compositor_message->set_payload(payload.data(),
On 2015/11/24 18:39:59, Kevin M wrote:
> What about serializing to
*compositor_message->mutable_payload()->mutable_data()
> directly?

Good call.  I ended up doing message->SerializeToString() which does effectively
what you want.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/re...
blimp/client/render_widget_message_processor.cc:126: return 0U;
On 2015/11/24 18:39:59, Kevin M wrote:
> Does this indicate an error? Should callers be testing for zero?

Yeah good point.  I put a comment in the header and added dchecks.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/re...
File blimp/client/render_widget_message_processor.h (right):

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/re...
blimp/client/render_widget_message_processor.h:5: #ifndef
BLIMP_CLIENT_RENDER_WIDGET_MESSAGE_PROCESSOR_H_
On 2015/11/24 08:55:43, nyquist wrote:
> Would it make sense to put these in some subfolder? renderwidget?
> It seems a bit odd that this class is so important it has to live alone in the
> top-level client-directory. Is it just because we have no browser/renderer
parts
> on the client?

Put these in compositor, as that's where they're actually used.  I'll look into
splitting it out a bit more in the future.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/re...
blimp/client/render_widget_message_processor.h:17: }
On 2015/11/24 08:55:43, nyquist wrote:
> Nit: // namespace blink?

I was explicitly asked not to for short namespace {} that just forward declared
things in a cc change.  I'm not sure if that's a general guideline or just a cc/
one though.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/re...
blimp/client/render_widget_message_processor.h:22: }
On 2015/11/24 08:55:43, nyquist wrote:
> Nit: end of namespace comment here and below?

See above.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/re...
blimp/client/render_widget_message_processor.h:63: void SetDelegate(const int
tab_id, RenderWidgetMessageDelegate* delegate);
On 2015/11/24 08:55:43, nyquist wrote:
> Nit: Could you add a comment saying there can only be one RWMD per |tab_id|?

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/client/re...
blimp/client/render_widget_message_processor.h:71: RenderWidgetMessageDelegate*
FindDelegate(const int tab_id);
On 2015/11/24 08:55:43, nyquist wrote:
> Optional nit: Do we want to add a comment saying that nullptr will be returned
> if it is not found? Or is that implicit since it's a pointer you return?

I assumed it was implicit.  I can add a comment though.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/common/pr...
File blimp/common/proto/common.proto (right):

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/common/pr...
blimp/common/proto/common.proto:16: }
On 2015/11/24 08:55:43, nyquist wrote:
> Nit: Why no newline anymore? Also, does this have to be part of this CL at
all?

Erased the file.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/common/pr...
File blimp/common/proto/compositor.proto (right):

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/common/pr...
blimp/common/proto/compositor.proto:16: // An ID that corresponds to
RenderWidgetMessage.
On 2015/11/24 18:39:59, Kevin M wrote:
> Corresponds to WGM.render_widget_id

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/common/pr...
blimp/common/proto/compositor.proto:19: // This represents a serialized
CompositorMessage in
On 2015/11/24 18:39:59, Kevin M wrote:
> What's the reasoning behind serializing the message here? There are some nice
> benefits to keeping the structure in protobuf format, e.g. less data to store
> and copy around, and PB parse errors are detected earlier on and in common
> codepaths. Having blimp depend on cc seems fine to me (not the other way
around,
> of course).

That would be ideal.  A few issues currently (which we can hopefully fix
eventually):
1. We have to send the data between the render and browser process, so we need
some serialized format at that point (if we use mojo eventually, we could do
this all browser side).
2. Currently it seems like Chromium doesn't support including ".proto" files in
other .proto files from another directory.  The way they're built (relative
paths, include paths, etc.) doesn't work.  We can change it, but we might have
to change every .proto file in Chromium as a part of fixing the current build
rules.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/common/pr...
File blimp/common/proto/render_widget.proto (right):

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/common/pr...
blimp/common/proto/render_widget.proto:10: import "compositor.proto";
On 2015/11/24 08:55:44, nyquist wrote:
> Nit: What are these two imports for?

Done.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/engine/br...
File blimp/engine/browser/blimp_engine_session.cc (right):

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/engine/br...
blimp/engine/browser/blimp_engine_session.cc:65:
render_widget_processor_(g_black_hole_message_processor.Pointer()) {
On 2015/11/24 18:39:59, Kevin M wrote:
> What's the purpose of this? Why not attach to a MultiplexedSender immediately?
> It should be available at any time; it's not tied to the availability of a
> BlimpConnection.

Once we have a valid BlimpMessageMultiplexer I can swap this out.  I'll do that
on the client in a follow up CL.  Haibin, are you building one here?  If not,
let me know and I can add one.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/engine/br...
File blimp/engine/browser/host_render_widget_message_processor.cc (right):

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/engine/br...
blimp/engine/browser/host_render_widget_message_processor.cc:49: const
std::vector<uint8_t>& message) {
On 2015/11/24 18:39:59, Kevin M wrote:
> Take a compositor proto scoped ptr?

Can't.  At this point we don't have one.  It's been sent from the render process
to the browser process, so it had to be serialized before.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/engine/br...
blimp/engine/browser/host_render_widget_message_processor.cc:56:
compositor_message->set_payload(message.data(),
On 2015/11/24 18:39:59, Kevin M wrote:
> set_allocated_payload?

Does that work?  It's an std::vector not an std::string :(.  I can work on
moving the transport medium to std::string in another patch.  I'll add a TODO.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/engine/br...
File blimp/engine/browser/host_render_widget_message_processor.h (right):

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/engine/br...
blimp/engine/browser/host_render_widget_message_processor.h:28: class
HostRenderWidgetMessageProcessor : public BlimpMessageProcessor {
On 2015/11/24 18:39:59, Kevin M wrote:
> Maybe I'm getting the terminology mixed up, but should Host be Engine?

I was trying to tie to RenderWidget and RenderWidgetHost.  But I can use engine.

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/net/black...
File blimp/net/black_hole_blimp_message_processor.h (right):

https://chromiumcodereview.appspot.com/1450423002/diff/160001/blimp/net/black...
blimp/net/black_hole_blimp_message_processor.h:15: class BLIMP_NET_EXPORT
BlackHoleBlimpMessageProcessor
On 2015/11/24 18:39:59, Kevin M wrote:
> NullBlimpMessageProcessor?
> 
> Add a class description

I can probably delete this if we can build the right processors now...

but also BlackHole is more fun than Null :P.

[Rick Byers, 2015-11-25 18:30:52]

blink/engine/DEPS LGTM

As discussed elsewhere, WebInputEvent will change over time and so won't be a
good long-term stable type for you.  But your incremental plan of starting here
(before protocol versioning is a real issue) seems fine to me.

[Kevin M, 2015-11-30 19:37:54]

lgtm

[David Trainor- moved to gerrit, 2015-11-30 19:43:27]

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

[David Trainor- moved to gerrit, 2015-11-30 19:43:28]

mmenke@: Can you look at blimp/client/DEPS?  Thanks! :)  I think I forgot to
actually add you as a reviewer earlier.  My bad.

[mmenke, 2015-11-30 19:50:08]

https://chromiumcodereview.appspot.com/1450423002/diff/260001/blimp/client/DEPS
File blimp/client/DEPS (right):

https://chromiumcodereview.appspot.com/1450423002/diff/260001/blimp/client/DE...
blimp/client/DEPS:9: "+net",
Can we restrict this to net/base for now?  (Mainly, if you end up using more
complex net classes, it's best to have a net reviewer).

[David Trainor- moved to gerrit, 2015-11-30 19:54:27]

done thanks!

https://chromiumcodereview.appspot.com/1450423002/diff/260001/blimp/client/DEPS
File blimp/client/DEPS (right):

https://chromiumcodereview.appspot.com/1450423002/diff/260001/blimp/client/DE...
blimp/client/DEPS:9: "+net",
On 2015/11/30 19:50:08, mmenke wrote:
> Can we restrict this to net/base for now?  (Mainly, if you end up using more
> complex net classes, it's best to have a net reviewer).

Done.

[mmenke, 2015-11-30 19:58:08]

On 2015/11/30 19:54:27, David Trainor wrote:
> done thanks!
> 
>
https://chromiumcodereview.appspot.com/1450423002/diff/260001/blimp/client/DEPS
> File blimp/client/DEPS (right):
> 
>
https://chromiumcodereview.appspot.com/1450423002/diff/260001/blimp/client/DE...
> blimp/client/DEPS:9: "+net",
> On 2015/11/30 19:50:08, mmenke wrote:
> > Can we restrict this to net/base for now?  (Mainly, if you end up using more
> > complex net classes, it's best to have a net reviewer).
> 
> Done.

LGTM

[David Trainor- moved to gerrit, 2015-11-30 21:16:01]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2015-11-30 21:16:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
haibinlu@chromium.org, khushalsagar@chromium.org, rbyers@chromium.org,
kmarshall@chromium.org, mmenke@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1450423002/#ps300001 (title: "Rebased")

[commit-bot: I haz the power, 2015-11-30 21:18:03]

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

[commit-bot: I haz the power, 2015-11-30 21:55:24]

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

[commit-bot: I haz the power, 2015-11-30 21:55:25]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[David Trainor- moved to gerrit, 2015-11-30 22:57:27]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2015-11-30 22:57:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
haibinlu@chromium.org, rbyers@chromium.org, kmarshall@chromium.org,
mmenke@chromium.org, khushalsagar@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1450423002/#ps320001 (title: "Fix build
break")

[commit-bot: I haz the power, 2015-11-30 22:58:14]

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

[commit-bot: I haz the power, 2015-12-01 00:53:13]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-12-01 00:54:10]

Description was changed from

==========
Add glue between the client and engine for Blimp

- Add BlimpMessageProcessors to handle RENDER_WIDGET, INPUT, and
  COMPOSITOR message types to/from the client and engine.
- Tie the message processors into BlimpCompositor on the client and
  BlimpEngineSession on the server.
- Update BlimpView and BlimpCompositor to fix how we handle output
  surfaces and how we shut down and build the LayerTreeHost based on
  incoming RENDER_WIDGET and COMPOSITOR messages.

BUG=550693
==========

to

==========
Add glue between the client and engine for Blimp

- Add BlimpMessageProcessors to handle RENDER_WIDGET, INPUT, and
  COMPOSITOR message types to/from the client and engine.
- Tie the message processors into BlimpCompositor on the client and
  BlimpEngineSession on the server.
- Update BlimpView and BlimpCompositor to fix how we handle output
  surfaces and how we shut down and build the LayerTreeHost based on
  incoming RENDER_WIDGET and COMPOSITOR messages.

BUG=550693

Committed: https://crrev.com/160f8eca573286f3bb369975ac2e9de63b0cc477
Cr-Commit-Position: refs/heads/master@{#362299}
==========

[commit-bot: I haz the power, 2015-12-01 00:54:11]

Patchset 17 (id:??) landed as
https://crrev.com/160f8eca573286f3bb369975ac2e9de63b0cc477
Cr-Commit-Position: refs/heads/master@{#362299}