Blimp: create Mojo component for renderer/browser communication.

Blimp: create Mojo component for renderer/browser communication.

* Define new Mojo interface: "BlobChannelMojo".
* Add browser impl class "BlobChannelMojoImpl", add service registration
  logic.
* Add BlobChannelMojo connection code to BlimpContentRendererClient.
* Bifurcate ImageSerializationProcessor into client- and engine-specific
  versions. The engine specific portion is a client of the
  BlobChannelMojo service.
* Promote the skia, libwebp DEPS to the top-level of Blimp
  (blimp/client and blimp/engine will have their own
  ImageSerializationProcessor implementations.)
* Add new DEP for mojo.

R=nyquist@chromium.org
BUG=600719
Committed: https://crrev.com/db1502a140cda830761a84ea089017cd1a375783
Cr-Commit-Position: refs/heads/master@{#385510}

[Kevin M, 2016-04-04 20:46:16]

nyquist for overall review;
yzshen for new mojo/ DEPS;
bsalomon for skia/ DEPS refactoring;
urvang for libwebp/ DEPS refactoring.

Thanks!

[Kevin M, 2016-04-04 20:48:04]

Description was changed from

==========
Blimp: create Mojo component for renderer/browser communication.

Blimp: create Mojo component for renderer/browser communication.
* Define new Mojo interface: "BlobChannelMojo".
* Add browser impl class "BlobChannelMojoImpl", add service registration
  logic.
* Add BlobChannelMojo connection code to BlimpContentRendererClient.
* Bifurcate ImageSerializationProcessor into client- and engine-specific
  versions. The engine specific portion is a client of the
  BlobChannelMojo service.
* Promote the skia, libwebp DEPS to the top-level of Blimp
  (blimp/client and blimp/engine will have their own
  ImageSerializationProcessor implementations.)
* Add new DEP for mojo.

R=nyquist@chromium.org
BUG=
==========

to

==========
Blimp: create Mojo component for renderer/browser communication.

Blimp: create Mojo component for renderer/browser communication.
* Define new Mojo interface: "BlobChannelMojo".
* Add browser impl class "BlobChannelMojoImpl", add service registration
  logic.
* Add BlobChannelMojo connection code to BlimpContentRendererClient.
* Bifurcate ImageSerializationProcessor into client- and engine-specific
  versions. The engine specific portion is a client of the
  BlobChannelMojo service.
* Promote the skia, libwebp DEPS to the top-level of Blimp
  (blimp/client and blimp/engine will have their own
  ImageSerializationProcessor implementations.)
* Add new DEP for mojo.

R=nyquist@chromium.org
BUG=
==========

[Kevin M, 2016-04-04 20:48:04]

kmarshall@chromium.org changed reviewers:
+ reed@chromium.org, urvang@chromium.org, yzshen@chromium.org

[Kevin M, 2016-04-04 20:48:45]

kmarshall@chromium.org changed reviewers:
+ bsalomon@chromium.org
- reed@chromium.org

[yzshen1, 2016-04-05 17:44:14]

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel.mojom (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel.mojom:7: interface BlobChannelMojo {
I believe the convention is to use module/namespace "foo.mojom" (in this case
blimp.engine.mojom) and don't have "Mojo"/"Mojom" in interface/struct/etc.
names.

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel_mojo_impl.cc (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_mojo_impl.cc:11:
mojo::InterfaceRequest<BlobChannelMojo> request)
nit: in case you didn't notice, you can use a shorter name
BlobChannelMojoRequest

[urvang, 2016-04-05 18:21:25]

lgtm

I'm guessing you wanted me to look at WebP related changes, which seem trivial.

[nyquist, 2016-04-05 22:13:20]

by the way, the first line of the CL description somehow seems duplicated

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/BUILD.gn#newco...
blimp/engine/BUILD.gn:76: ":blob_channel",
Did you mean to just depend on //blimp/engine/mojo:blob_channel here?

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/BUILD.gn#newco...
blimp/engine/BUILD.gn:159: ":blob_channel_mojo",
Did you mean to just depend on the stuff in //blimp/engine/mojo here?

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/app/blimp_cont...
File blimp/engine/app/blimp_content_renderer_client.cc (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/app/blimp_cont...
blimp/engine/app/blimp_content_renderer_client.cc:18: BlobChannelMojoPtr
GetConnectedBlobChannel() {
Is this call blocking across the renderer and browser process?

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/BUILD.gn
File blimp/engine/mojo/BUILD.gn (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/BUILD.gn#...
blimp/engine/mojo/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights
reserved.
Did you mean to keep this or the other parts in //blimp/engine/BUILD.gn?

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel_mojo_impl.cc (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_mojo_impl.cc:28: new
BlobChannelMojoImpl(std::move(request));
Does this ever get deleted? Does it happen automatically when the binding is
over?
If so; could you add a comment about this here, so it doesn't look like it's
just leaking?

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel_mojo_impl.h (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_mojo_impl.h:14: class BlobChannelMojoImpl :
public BlobChannelMojo {
Could you make it clear from the documentation where this runs? As in; is this
in the renderer or in the browser?

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/renderer/engin...
File blimp/engine/renderer/engine_image_serialization_processor.h (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/renderer/engin...
blimp/engine/renderer/engine_image_serialization_processor.h:26: class
BLIMP_COMMON_EXPORT EngineImageSerializationProcessor
Should you rename the client-version as well in this CL or is that an upcoming
CL?

[Kevin M, 2016-04-06 00:01:44]

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/BUILD.gn#newco...
blimp/engine/BUILD.gn:76: ":blob_channel",
On 2016/04/05 22:13:19, nyquist wrote:
> Did you mean to just depend on //blimp/engine/mojo:blob_channel here?

Sorry - b/e/mojo/BUILD.gn should have been removed. I decided to stick with the
convention of using a monolithic BUILD.gn file for engine/ .

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/BUILD.gn#newco...
blimp/engine/BUILD.gn:159: ":blob_channel_mojo",
On 2016/04/05 22:13:19, nyquist wrote:
> Did you mean to just depend on the stuff in //blimp/engine/mojo here?

Acknowledged.

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/app/blimp_cont...
File blimp/engine/app/blimp_content_renderer_client.cc (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/app/blimp_cont...
blimp/engine/app/blimp_content_renderer_client.cc:18: BlobChannelMojoPtr
GetConnectedBlobChannel() {
On 2016/04/05 22:13:19, nyquist wrote:
> Is this call blocking across the renderer and browser process?

The renderer: yes (at startup), the browser: no. The handshake is practically
instantaneous even when the Mojo connection is cold.

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/BUILD.gn
File blimp/engine/mojo/BUILD.gn (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/BUILD.gn#...
blimp/engine/mojo/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights
reserved.
On 2016/04/05 22:13:19, nyquist wrote:
> Did you mean to keep this or the other parts in //blimp/engine/BUILD.gn?

rm'd

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel.mojom (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel.mojom:7: interface BlobChannelMojo {
On 2016/04/05 17:44:14, yzshen1 wrote:
> I believe the convention is to use module/namespace "foo.mojom" (in this case
> blimp.engine.mojom) and don't have "Mojo"/"Mojom" in interface/struct/etc.
> names.

Done. Also removed "mojo" from the impl class name, so
BlobChannelMojoImpl=>BlobChannelService.

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel_mojo_impl.cc (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_mojo_impl.cc:11:
mojo::InterfaceRequest<BlobChannelMojo> request)
On 2016/04/05 17:44:14, yzshen1 wrote:
> nit: in case you didn't notice, you can use a shorter name
> BlobChannelMojoRequest

Done.

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_mojo_impl.cc:28: new
BlobChannelMojoImpl(std::move(request));
On 2016/04/05 22:13:19, nyquist wrote:
> Does this ever get deleted? Does it happen automatically when the binding is
> over?
> If so; could you add a comment about this here, so it doesn't look like it's
> just leaking?

Yes, its StrongBinding |binding_| controls the lifetime. Added comment.

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
File blimp/engine/mojo/blob_channel_mojo_impl.h (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/mojo/blob_chan...
blimp/engine/mojo/blob_channel_mojo_impl.h:14: class BlobChannelMojoImpl :
public BlobChannelMojo {
On 2016/04/05 22:13:19, nyquist wrote:
> Could you make it clear from the documentation where this runs? As in; is this
> in the renderer or in the browser?

Done.

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/renderer/engin...
File blimp/engine/renderer/engine_image_serialization_processor.h (right):

https://codereview.chromium.org/1859753002/diff/1/blimp/engine/renderer/engin...
blimp/engine/renderer/engine_image_serialization_processor.h:26: class
BLIMP_COMMON_EXPORT EngineImageSerializationProcessor
On 2016/04/05 22:13:19, nyquist wrote:
> Should you rename the client-version as well in this CL or is that an upcoming
> CL?

Upcoming CL. I'm focusing on engine-specific things to scope down the reviews.

[Kevin M, 2016-04-06 00:01:57]

Description was changed from

==========
Blimp: create Mojo component for renderer/browser communication.

Blimp: create Mojo component for renderer/browser communication.
* Define new Mojo interface: "BlobChannelMojo".
* Add browser impl class "BlobChannelMojoImpl", add service registration
  logic.
* Add BlobChannelMojo connection code to BlimpContentRendererClient.
* Bifurcate ImageSerializationProcessor into client- and engine-specific
  versions. The engine specific portion is a client of the
  BlobChannelMojo service.
* Promote the skia, libwebp DEPS to the top-level of Blimp
  (blimp/client and blimp/engine will have their own
  ImageSerializationProcessor implementations.)
* Add new DEP for mojo.

R=nyquist@chromium.org
BUG=
==========

to

==========
Blimp: create Mojo component for renderer/browser communication.

* Define new Mojo interface: "BlobChannelMojo".
* Add browser impl class "BlobChannelMojoImpl", add service registration
  logic.
* Add BlobChannelMojo connection code to BlimpContentRendererClient.
* Bifurcate ImageSerializationProcessor into client- and engine-specific
  versions. The engine specific portion is a client of the
  BlobChannelMojo service.
* Promote the skia, libwebp DEPS to the top-level of Blimp
  (blimp/client and blimp/engine will have their own
  ImageSerializationProcessor implementations.)
* Add new DEP for mojo.

R=nyquist@chromium.org
BUG=
==========

[bsalomon, 2016-04-06 00:08:52]

bsalomon@google.com changed reviewers:
+ bsalomon@google.com

[bsalomon, 2016-04-06 00:08:52]

skia DEPS lgtm

[bsalomon, 2016-04-06 00:08:53]

skia DEPS lgtm

[yzshen1, 2016-04-06 15:24:23]

On 2016/04/06 00:08:53, bsalomon wrote:
> skia DEPS lgtm

mojo DEPS lgtm

[nyquist, 2016-04-06 17:12:13]

This is neat! lgtm

[Kevin M, 2016-04-06 17:58:37]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-04-06 17:58:38]

The patchset sent to the CQ was uploaded after l-g-t-m from urvang@chromium.org
Link to the patchset: https://codereview.chromium.org/1859753002/#ps20001
(title: "yzshen and nyquist feedback")

[commit-bot: I haz the power, 2016-04-06 17:59:10]

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

[Kevin M, 2016-04-06 18:17:30]

Description was changed from

==========
Blimp: create Mojo component for renderer/browser communication.

* Define new Mojo interface: "BlobChannelMojo".
* Add browser impl class "BlobChannelMojoImpl", add service registration
  logic.
* Add BlobChannelMojo connection code to BlimpContentRendererClient.
* Bifurcate ImageSerializationProcessor into client- and engine-specific
  versions. The engine specific portion is a client of the
  BlobChannelMojo service.
* Promote the skia, libwebp DEPS to the top-level of Blimp
  (blimp/client and blimp/engine will have their own
  ImageSerializationProcessor implementations.)
* Add new DEP for mojo.

R=nyquist@chromium.org
BUG=
==========

to

==========
Blimp: create Mojo component for renderer/browser communication.

* Define new Mojo interface: "BlobChannelMojo".
* Add browser impl class "BlobChannelMojoImpl", add service registration
  logic.
* Add BlobChannelMojo connection code to BlimpContentRendererClient.
* Bifurcate ImageSerializationProcessor into client- and engine-specific
  versions. The engine specific portion is a client of the
  BlobChannelMojo service.
* Promote the skia, libwebp DEPS to the top-level of Blimp
  (blimp/client and blimp/engine will have their own
  ImageSerializationProcessor implementations.)
* Add new DEP for mojo.

R=nyquist@chromium.org
BUG=600719
==========

[commit-bot: I haz the power, 2016-04-06 18:47:10]

Description was changed from

==========
Blimp: create Mojo component for renderer/browser communication.

* Define new Mojo interface: "BlobChannelMojo".
* Add browser impl class "BlobChannelMojoImpl", add service registration
  logic.
* Add BlobChannelMojo connection code to BlimpContentRendererClient.
* Bifurcate ImageSerializationProcessor into client- and engine-specific
  versions. The engine specific portion is a client of the
  BlobChannelMojo service.
* Promote the skia, libwebp DEPS to the top-level of Blimp
  (blimp/client and blimp/engine will have their own
  ImageSerializationProcessor implementations.)
* Add new DEP for mojo.

R=nyquist@chromium.org
BUG=600719
==========

to

==========
Blimp: create Mojo component for renderer/browser communication.

* Define new Mojo interface: "BlobChannelMojo".
* Add browser impl class "BlobChannelMojoImpl", add service registration
  logic.
* Add BlobChannelMojo connection code to BlimpContentRendererClient.
* Bifurcate ImageSerializationProcessor into client- and engine-specific
  versions. The engine specific portion is a client of the
  BlobChannelMojo service.
* Promote the skia, libwebp DEPS to the top-level of Blimp
  (blimp/client and blimp/engine will have their own
  ImageSerializationProcessor implementations.)
* Add new DEP for mojo.

R=nyquist@chromium.org
BUG=600719
==========

[commit-bot: I haz the power, 2016-04-06 18:47:11]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-06 18:48:19]

Description was changed from

==========
Blimp: create Mojo component for renderer/browser communication.

* Define new Mojo interface: "BlobChannelMojo".
* Add browser impl class "BlobChannelMojoImpl", add service registration
  logic.
* Add BlobChannelMojo connection code to BlimpContentRendererClient.
* Bifurcate ImageSerializationProcessor into client- and engine-specific
  versions. The engine specific portion is a client of the
  BlobChannelMojo service.
* Promote the skia, libwebp DEPS to the top-level of Blimp
  (blimp/client and blimp/engine will have their own
  ImageSerializationProcessor implementations.)
* Add new DEP for mojo.

R=nyquist@chromium.org
BUG=600719
==========

to

==========
Blimp: create Mojo component for renderer/browser communication.

* Define new Mojo interface: "BlobChannelMojo".
* Add browser impl class "BlobChannelMojoImpl", add service registration
  logic.
* Add BlobChannelMojo connection code to BlimpContentRendererClient.
* Bifurcate ImageSerializationProcessor into client- and engine-specific
  versions. The engine specific portion is a client of the
  BlobChannelMojo service.
* Promote the skia, libwebp DEPS to the top-level of Blimp
  (blimp/client and blimp/engine will have their own
  ImageSerializationProcessor implementations.)
* Add new DEP for mojo.

R=nyquist@chromium.org
BUG=600719

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

[commit-bot: I haz the power, 2016-04-06 18:48:20]

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