GRPC Stream implementation of HeliumStream

GRPC Stream implementation of HeliumStream

* GrpcStream implements HeliumStream and is also used by GrpcConnection which routes BlimpMessages on top of HeliumWrapper protos.
* Symmetric interface for both GrpcClient and GrpcEngine Streams that are thin wrappers around the overall GrpcStream.
* GrpcStream interacts with gRPC library: Supports CompletionQueue with a separate thread that assigns work-items using GrpcTags.
* Finally, a new assignment options opaque structure that avoids tight-coupling with the port-numbers and also eases testing as the client/engine stream setup is symmetric (using the same assignment options class in both).

* Unit-tests are added for GrpcStream classes (new HeliumStream implementation) and GrpcConnection (existing BlimpConnection implementation).
* Browser tests are modified to run in both gRPC and TCP mode to exercise the code more. (Two new targets: blimp_browsertests_grpc and blimp_browsertests_tcp with a simple 'UsingGrpc()' flag in two separate Grpc/TCP-oriented files referenced by the tests)

[Kevin M, 2016-10-31 21:33:24]

kmarshall@chromium.org changed reviewers:
+ kmarshall@chromium.org

[Kevin M, 2016-10-31 21:33:26]

Sorry, I couldn't resist leaving a few comments. Just didn't know if I'd
remember to bring them up in the future :\

Not sure how I feel about how we bubble up details about HeliumWrapper,
especially WRT the fact that it renders GrpcStream incompatible with
HeliumStream.

I would almost rather just put a BlimpMessage payload into a HeliumMessage, so
that the transitional interface doesn't leak out into the rest of the API.

https://codereview.chromium.org/2462183002/diff/1/blimp/common/proto/BUILD.gn
File blimp/common/proto/BUILD.gn (right):

https://codereview.chromium.org/2462183002/diff/1/blimp/common/proto/BUILD.gn...
blimp/common/proto/BUILD.gn:25: proto_library("helium_service") {
What about putting this inside //blimp/helium/service ?

https://codereview.chromium.org/2462183002/diff/1/blimp/common/proto/helium_s...
File blimp/common/proto/helium_service.proto (right):

https://codereview.chromium.org/2462183002/diff/1/blimp/common/proto/helium_s...
blimp/common/proto/helium_service.proto:12: bytes serialized_helium_message = 1;
Since we'll be sending BlimpMessages and HeliumMessages over here, how about
just "serialized_message" ?

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_connection.h
File blimp/net/grpc_connection.h (right):

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_connection.h...
blimp/net/grpc_connection.h:34: // Forwarders that convert from |BlimpMessage|s
to |HeliumMessage|s (and vice
converting => wrapping?

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_engine_strea...
File blimp/net/grpc_engine_stream.cc (right):

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_engine_strea...
blimp/net/grpc_engine_stream.cc:14: const std::string& ip_address,
Pass around validated IPEndpoints internally.

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_engine_stream.h
File blimp/net/grpc_engine_stream.h (right):

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_engine_strea...
blimp/net/grpc_engine_stream.h:17: #include
<grpc++/security/server_credentials.h>
General point (yeah I know I'm not supposed to review...) - there are a whole
lot of includes here, can we use prototypes when possible and remove includes
that aren't needed anymore?

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_engine_strea...
blimp/net/grpc_engine_stream.h:43: class GrpcEngineStream : public GrpcStream {
As this runs on its own thread, should we use a ThreadChecker for this class?

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_stream.cc
File blimp/net/grpc_stream.cc (right):

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_stream.cc#ne...
blimp/net/grpc_stream.cc:176: default:
General point: don't use default, so that accidental enum omissions are turned
into compile time errors.

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_stream.h
File blimp/net/grpc_stream.h (right):

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_stream.h#new...
blimp/net/grpc_stream.h:24: class GrpcStream {
Why doesn't this implement HeliumStream?

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_stream.h#new...
blimp/net/grpc_stream.h:99: 
Use a ThreadChecker?

[perumaal, 2016-10-31 21:40:14]

On 2016/10/31 21:33:26, Kevin M wrote:
> Sorry, I couldn't resist leaving a few comments. Just didn't know if I'd
> remember to bring them up in the future :\
> 
> Not sure how I feel about how we bubble up details about HeliumWrapper,
> especially WRT the fact that it renders GrpcStream incompatible with
> HeliumStream.
> 

It does use HeliumMessage. Note that the actual checked-in code name is
HeliumWrapper.
Will provide details offline.

[perumaal, 2016-10-31 22:11:01]

PTAL, added helium::Stream support.

https://codereview.chromium.org/2462183002/diff/1/blimp/common/proto/BUILD.gn
File blimp/common/proto/BUILD.gn (right):

https://codereview.chromium.org/2462183002/diff/1/blimp/common/proto/BUILD.gn...
blimp/common/proto/BUILD.gn:25: proto_library("helium_service") {
On 2016/10/31 21:33:25, Kevin M wrote:
> What about putting this inside //blimp/helium/service ?

I think there is a bit of code consolidation here *after* the grpc stuff is
checked in. However, I do not think that this one proto file necessitates a new
folder though.

https://codereview.chromium.org/2462183002/diff/1/blimp/net/DEPS
File blimp/net/DEPS (right):

https://codereview.chromium.org/2462183002/diff/1/blimp/net/DEPS#newcode2
blimp/net/DEPS:2: "+content/public",
Remove!

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_engine_stream.h
File blimp/net/grpc_engine_stream.h (right):

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_engine_strea...
blimp/net/grpc_engine_stream.h:17: #include
<grpc++/security/server_credentials.h>
On 2016/10/31 21:33:25, Kevin M wrote:
> General point (yeah I know I'm not supposed to review...) - there are a whole
> lot of includes here, can we use prototypes when possible and remove includes
> that aren't needed anymore?

Oops this is intentional for now because I was trying a few things.
When I send the full code-review I will check the includes.

https://codereview.chromium.org/2462183002/diff/1/blimp/net/grpc_engine_strea...
blimp/net/grpc_engine_stream.h:43: class GrpcEngineStream : public GrpcStream {
On 2016/10/31 21:33:25, Kevin M wrote:
> As this runs on its own thread, should we use a ThreadChecker for this class?

Yep, added a TODO.

[perumaal1, 2016-11-09 02:08:52]

Description was changed from

==========
WIP: GRPC Stream implementation of HeliumStream
DO NOT REVIEW YET

This is an initial look

* GrpcStream implements HeliumStream
* GrpcStream is also used by GrpcConnection which routes BlimpMessages on top of
HeliumWrapper protos.
* Symmetric interface for both GrpcClient and GrpcEngine Streams that are thin
wrappers around the overall GrpcStream.
* GrpcStream interacts with gRPC library: Supports CompletionQueue with a
separate thread that assigns work-items using GrpcTags.
==========

to

==========
GRPC Stream implementation of HeliumStream

* GrpcStream implements HeliumStream and is also used by GrpcConnection which
routes BlimpMessages on top of HeliumWrapper protos.
* Symmetric interface for both GrpcClient and GrpcEngine Streams that are thin
wrappers around the overall GrpcStream.
* GrpcStream interacts with gRPC library: Supports CompletionQueue with a
separate thread that assigns work-items using GrpcTags.
* Finally, a new assignment options opaque structure that avoids tight-coupling
with the port-numbers and also eases testing as the client/engine stream setup
is symmetric (using the same assignment options class in both).

* Unit-tests are added for GrpcStream classes (new HeliumStream implementation)
and GrpcConnection (existing BlimpConnection implementation).
* Browser tests are modified to run in both gRPC and TCP mode to exercise the
code more.
==========

[perumaal1, 2016-11-09 02:08:52]

perumaal@google.com changed reviewers:
+ gcasto@chromium.org

[perumaal1, 2016-11-09 02:09:22]

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

[perumaal1, 2016-11-09 02:09:22]

PTAL, ready for actual review.

[perumaal1, 2016-11-09 02:09:29]

The CQ bit was checked by perumaal@google.com

[commit-bot: I haz the power, 2016-11-09 02:09:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 02:09:55]

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

[commit-bot: I haz the power, 2016-11-09 02:09:56]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[perumaal1, 2016-11-09 02:12:01]

The CQ bit was checked by perumaal@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 02:12:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 02:18:07]

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

[commit-bot: I haz the power, 2016-11-09 02:18:08]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[perumaal, 2016-11-09 21:28:53]

perumaal@chromium.org changed reviewers:
+ maniscalco@chromium.org
- gcasto@chromium.org, perumaal@google.com

[perumaal, 2016-11-09 21:28:53]

Goals for this CL:
* Solicit high-level comments first. Agree on design/architecture.
* Run unit-tests/trybots :D and make sure end-to-end works great.
* Next, split CL into two pieces - one for the GrpcStream (HeliumStream) and one
for hooking up BlimpConnection -> GrpcStream.

https://codereview.chromium.org/2462183002/diff/20001/blimp/net/DEPS
File blimp/net/DEPS (right):

https://codereview.chromium.org/2462183002/diff/20001/blimp/net/DEPS#newcode2
blimp/net/DEPS:2: "+content/public",
REMOVE!

[perumaal, 2016-11-09 21:36:20]

Description was changed from

==========
GRPC Stream implementation of HeliumStream

* GrpcStream implements HeliumStream and is also used by GrpcConnection which
routes BlimpMessages on top of HeliumWrapper protos.
* Symmetric interface for both GrpcClient and GrpcEngine Streams that are thin
wrappers around the overall GrpcStream.
* GrpcStream interacts with gRPC library: Supports CompletionQueue with a
separate thread that assigns work-items using GrpcTags.
* Finally, a new assignment options opaque structure that avoids tight-coupling
with the port-numbers and also eases testing as the client/engine stream setup
is symmetric (using the same assignment options class in both).

* Unit-tests are added for GrpcStream classes (new HeliumStream implementation)
and GrpcConnection (existing BlimpConnection implementation).
* Browser tests are modified to run in both gRPC and TCP mode to exercise the
code more.
==========

to

==========
GRPC Stream implementation of HeliumStream

* GrpcStream implements HeliumStream and is also used by GrpcConnection which
routes BlimpMessages on top of HeliumWrapper protos.
* Symmetric interface for both GrpcClient and GrpcEngine Streams that are thin
wrappers around the overall GrpcStream.
* GrpcStream interacts with gRPC library: Supports CompletionQueue with a
separate thread that assigns work-items using GrpcTags.
* Finally, a new assignment options opaque structure that avoids tight-coupling
with the port-numbers and also eases testing as the client/engine stream setup
is symmetric (using the same assignment options class in both).

* Unit-tests are added for GrpcStream classes (new HeliumStream implementation)
and GrpcConnection (existing BlimpConnection implementation).
* Browser tests are modified to run in both gRPC and TCP mode to exercise the
code more. (Two new targets: blimp_browsertests_grpc and blimp_browsertests_tcp
with a simple 'UsingGrpc()' flag in two separate Grpc/TCP-oriented files
referenced by the tests)
==========

[perumaal, 2016-11-09 22:21:53]

https://codereview.chromium.org/2462183002/diff/20001/blimp/net/DEPS
File blimp/net/DEPS (right):

https://codereview.chromium.org/2462183002/diff/20001/blimp/net/DEPS#newcode2
blimp/net/DEPS:2: "+content/public",
On 2016/11/09 21:28:53, perumaal wrote:
> REMOVE!

Not needed anymore.

[perumaal, 2016-11-09 22:22:20]

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

[commit-bot: I haz the power, 2016-11-09 22:23:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[perumaal, 2016-11-10 02:30:11]

The CQ bit was checked by perumaal@chromium.org

[perumaal, 2016-11-10 02:30:21]

The CQ bit was unchecked by perumaal@chromium.org

[commit-bot: I haz the power, 2016-11-10 02:30:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 02:30:30]

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

[commit-bot: I haz the power, 2016-11-10 02:30:31]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[perumaal, 2016-11-10 02:35:24]

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

[commit-bot: I haz the power, 2016-11-10 02:35:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 02:39:12]

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

[commit-bot: I haz the power, 2016-11-10 02:39:13]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Garrett Casto, 2016-11-11 00:15:51]

gcasto@chromium.org changed reviewers:
+ gcasto@chromium.org

[Garrett Casto, 2016-11-11 00:15:53]

I added some super minor comments when skimming through because I couldn't help
myself. In general the structure seem fine, so could you break this out into
smaller CLs. In particular, I think that I would like to see a CL that is just
GrpcStream, subclasses, and tests.

https://codereview.chromium.org/2462183002/diff/80001/blimp/common/proto/BUIL...
File blimp/common/proto/BUILD.gn (right):

https://codereview.chromium.org/2462183002/diff/80001/blimp/common/proto/BUIL...
blimp/common/proto/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights
reserved.
You don't change the copyright date when you update a file
(https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md).

https://codereview.chromium.org/2462183002/diff/80001/blimp/common/proto/heli...
File blimp/common/proto/helium_service.proto (right):

https://codereview.chromium.org/2462183002/diff/80001/blimp/common/proto/heli...
blimp/common/proto/helium_service.proto:11: // This message wraps around a
serialized set of bytes from a
Nit: You are line wrapping really early here.

https://codereview.chromium.org/2462183002/diff/80001/blimp/net/grpc_client_s...
File blimp/net/grpc_client_stream.cc (right):

https://codereview.chromium.org/2462183002/diff/80001/blimp/net/grpc_client_s...
blimp/net/grpc_client_stream.cc:19: // This stub implements the gRPC service. We
use this to strema data to
Nit: strema -> stream.

https://codereview.chromium.org/2462183002/diff/80001/blimp/net/grpc_client_s...
File blimp/net/grpc_client_stream.h (right):

https://codereview.chromium.org/2462183002/diff/80001/blimp/net/grpc_client_s...
blimp/net/grpc_client_stream.h:12: #include <grpc/grpc.h>
Nit: We may want to have a discussion on how we reference grpc code from
Chromium. In general Chrome prefers using absolute paths for includes, but there
is certainly a case to be made to use relative includes here.

https://codereview.chromium.org/2462183002/diff/80001/blimp/net/grpc_client_s...
blimp/net/grpc_client_stream.h:34: // A |HeliumStream| that implements the gRPC
bidirectional streaming API for the
Nit: Generally speaking pipes are for variable names. I wouldn't expect pipes
around a class name or function name.

[perumaal, 2016-11-12 00:43:13]

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

[commit-bot: I haz the power, 2016-11-12 00:43:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-12 00:48:14]

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

[commit-bot: I haz the power, 2016-11-12 00:48:15]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Kevin M, 2016-11-14 18:55:06]

+1 for breaking this up into smaller CLs. There are simply too many changes here
(both in terms of LoC and file count) for me to effectively comment on the
overall approach.

[Kevin M, 2017-01-31 19:41:07]

kmarshall@chromium.org changed reviewers:
- kmarshall@chromium.org