Moving gRPC support interfaces out of cronet and into a new component.

Moving gRPC support interfaces out of cronet and into a new component.

BUG=650462
Committed: https://crrev.com/ef2a6d130430d93b29b4258c814ea94a3cbbfb31
Cr-Commit-Position: refs/heads/master@{#428099}

[Garrett Casto, 2016-08-25 21:14:41]

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

[commit-bot: I haz the power, 2016-08-25 21:15:16]

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-08-25 21:26:25]

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

[commit-bot: I haz the power, 2016-08-25 21:26:26]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[mef, 2016-08-26 20:17:35]

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

[mef, 2016-08-26 20:17:36]

Looks like a good start!

https://codereview.chromium.org/2273403003/diff/160001/components/grpc_suppor...
File components/grpc_support/cronet_bidirectional_stream.h (right):

https://codereview.chromium.org/2273403003/diff/160001/components/grpc_suppor...
components/grpc_support/cronet_bidirectional_stream.h:55:
CronetBidirectionalStream(Environment* environment, Delegate* delegate);
I think we should change CronetBidirectionalStream to use
UrlRequestContextGetter instead of [Cronet]Environment.

[blundell, 2016-08-29 06:35:52]

On 2016/08/26 20:17:36, mef wrote:
> Looks like a good start!
> 
>
https://codereview.chromium.org/2273403003/diff/160001/components/grpc_suppor...
> File components/grpc_support/cronet_bidirectional_stream.h (right):
> 
>
https://codereview.chromium.org/2273403003/diff/160001/components/grpc_suppor...
> components/grpc_support/cronet_bidirectional_stream.h:55:
> CronetBidirectionalStream(Environment* environment, Delegate* delegate);
> I think we should change CronetBidirectionalStream to use
> UrlRequestContextGetter instead of [Cronet]Environment.

As an aside, please change to a more-targeted name for the new component before
landing, if I'm understanding correctly based on the filenames that the new
component is still Cronet-specific.

(Hey, Garrett -- long time no see!)

[mef, 2016-08-29 13:56:43]

On 2016/08/29 06:35:52, blundell wrote:
> On 2016/08/26 20:17:36, mef wrote:
> > Looks like a good start!
> > 
> >
>
https://codereview.chromium.org/2273403003/diff/160001/components/grpc_suppor...
> > File components/grpc_support/cronet_bidirectional_stream.h (right):
> > 
> >
>
https://codereview.chromium.org/2273403003/diff/160001/components/grpc_suppor...
> > components/grpc_support/cronet_bidirectional_stream.h:55:
> > CronetBidirectionalStream(Environment* environment, Delegate* delegate);
> > I think we should change CronetBidirectionalStream to use
> > UrlRequestContextGetter instead of [Cronet]Environment.
> 
> As an aside, please change to a more-targeted name for the new component
before
> landing, if I'm understanding correctly based on the filenames that the new
> component is still Cronet-specific.
> 
> (Hey, Garrett -- long time no see!)

Actually we would want this component to be shared between Chrome and Cronet, so
maybe rename s/cronet/crgrpc/g ?
For the record I expect that there will be some changes / bugfixes in cronet
implementation based on our testing, which will have to be moved / merged into
this component.

[Garrett Casto, 2016-09-26 23:08:03]

Description was changed from

==========
[WIP] Moving gRPC support interfaces out of cronet and into a new component.

BUG=
==========

to

==========
Moving gRPC support interfaces out of cronet and into a new component.

BUG=650462
==========

[Garrett Casto, 2016-09-26 23:08:03]

gcasto@chromium.org changed reviewers:
+ blundell@chromium.org, sdefresne@chromium.org

[Garrett Casto, 2016-09-26 23:28:21]

Apologies for dropping this, it got pushed down the stack for a few weeks. I've
removed Environment in favor of just using URLRequestContextGetter directly,
which seems reasonable. 

Open issues
1) Testing: The Cronet bot still doesn't seem to do anything (the analyze step
always fails, and compilation doesn't actually compile Cronet). Misha and
Sylvain, it sounds like the two of you were working on having real tests, is
that still in the pipe? Do I need to test this locally instead? If so, are there
directions anywhere?
2) Naming: I didn't change grpc_support for now, because I wanted to have a
larger discussion about names here. It seems to me that everything relating to
gRPC shouldn't really be called cronet (e.g. cronet_c_for_grpc.h). That is, gRPC
is allowing for another program to essentially define a way to create and
control bidirectional streams and Cronet is one implementation of that. gRPC
shouldn't know that it's Cronet in particular that is performing this service.
It feels like the naming should reflect this (e.g. cronet_engine -> engine,
cronet_bidirectional_stream -> bidirectional_stream,
grpc_support::CronetBidirectionalStream -> grpc_support::BidirectionalStream,
...). Misha, what are your thoughts? I think that it removes some confusion
about what "Cronet" means, since currently the term is used for both an
interface and an implemenation.

I'm also not sure how this change should be reflected in the grpc repository. It
looks like they manually sync cronet_c_for_grpc.h. Is this something that they
are responsible for, or do we need to check in a new version when a change is
made in the Chromium repository?

PTAL

mef - Everything
sdefresne: BUILD.gn changes
blundell - For new component. This probably won't be finalized for a bit, but
since you expressed interest in the naming you have now volunteered yourself as
tribute :).

[sdefresne, 2016-09-27 10:21:05]

On 2016/09/26 23:28:21, Garrett Casto wrote:
> Apologies for dropping this, it got pushed down the stack for a few weeks.
I've
> removed Environment in favor of just using URLRequestContextGetter directly,
> which seems reasonable. 
> 
> Open issues
> 1) Testing: The Cronet bot still doesn't seem to do anything (the analyze step
> always fails, and compilation doesn't actually compile Cronet). Misha and
> Sylvain, it sounds like the two of you were working on having real tests, is
> that still in the pipe? Do I need to test this locally instead? If so, are
there
> directions anywhere?

I remember fixing this bot, so it probably regressed. Maybe it would be worth
fixing it again and adding it to the CQ/waterfall before making further changes?

> 2) Naming: I didn't change grpc_support for now, because I wanted to have a
> larger discussion about names here. It seems to me that everything relating to
> gRPC shouldn't really be called cronet (e.g. cronet_c_for_grpc.h). That is,
gRPC
> is allowing for another program to essentially define a way to create and
> control bidirectional streams and Cronet is one implementation of that. gRPC
> shouldn't know that it's Cronet in particular that is performing this service.
> It feels like the naming should reflect this (e.g. cronet_engine -> engine,
> cronet_bidirectional_stream -> bidirectional_stream,
> grpc_support::CronetBidirectionalStream -> grpc_support::BidirectionalStream,
> ...). Misha, what are your thoughts? I think that it removes some confusion
> about what "Cronet" means, since currently the term is used for both an
> interface and an implemenation.
> 
> I'm also not sure how this change should be reflected in the grpc repository.
It
> looks like they manually sync cronet_c_for_grpc.h. Is this something that they
> are responsible for, or do we need to check in a new version when a change is
> made in the Chromium repository?
> 
> PTAL
> 
> mef - Everything
> sdefresne: BUILD.gn changes
> blundell - For new component. This probably won't be finalized for a bit, but
> since you expressed interest in the naming you have now volunteered yourself
as
> tribute :).

Regarding the build changes, can the build/config/ios/rules.gni changes be moved
to a separate CL? They are fixes that we may want early without waiting for the
full CL to be ready.

[blundell, 2016-09-28 13:51:58]

It's hard for me to understand what the relationship of the new component to
gRPC is. It doesn't seem to use gRPC as far as I can tell. Am I understanding
correctly that the dependency is actually the other way, i.e., that cronet is
*consumed* by gRPC via cronet_c_for_grpc.h?

[mef, 2016-09-28 15:32:26]

On 2016/09/26 23:28:21, Garrett Casto wrote:
> Apologies for dropping this, it got pushed down the stack for a few weeks.
I've
> removed Environment in favor of just using URLRequestContextGetter directly,
> which seems reasonable. 
I apologize, but I also will not be able to review it until next week.
> 
> Open issues
> 1) Testing: The Cronet bot still doesn't seem to do anything (the analyze step
> always fails, and compilation doesn't actually compile Cronet). Misha and
> Sylvain, it sounds like the two of you were working on having real tests, is
> that still in the pipe? 
I've used
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-cr...
before and it appeared to work,
but it seems that now it fails with cryptic error:

MBErr: target "cronet_test" not found in //testing/buildbot/gn_isolate_map.pyl

> Do I need to test this locally instead? If so, are there directions anywhere?
It is possible to build and run cronet_test on Mac with XCode. I'll write up
directions on how to do it, but it is VERY iOS-centric.

> 2) Naming: I didn't change grpc_support for now, because I wanted to have a
> larger discussion about names here. It seems to me that everything relating to
> gRPC shouldn't really be called cronet (e.g. cronet_c_for_grpc.h). That is,
gRPC
> is allowing for another program to essentially define a way to create and
> control bidirectional streams and Cronet is one implementation of that. gRPC
> shouldn't know that it's Cronet in particular that is performing this service.
> It feels like the naming should reflect this (e.g. cronet_engine -> engine,
> cronet_bidirectional_stream -> bidirectional_stream,
> grpc_support::CronetBidirectionalStream -> grpc_support::BidirectionalStream,
> ...). Misha, what are your thoughts? I think that it removes some confusion
> about what "Cronet" means, since currently the term is used for both an
> interface and an implemenation.
Sounds good to me. 
> 
> I'm also not sure how this change should be reflected in the grpc repository.
It
> looks like they manually sync cronet_c_for_grpc.h. Is this something that they
> are responsible for, or do we need to check in a new version when a change is
> made in the Chromium repository?
AFAIK they use an old snapshot of Cronet in GRPC GITHUB repository, but depend
on updated Cronet in internal builds.
We do need to come up with the proper plan for open source dependencies. 
> 
> PTAL
> 
> mef - Everything
> sdefresne: BUILD.gn changes
> blundell - For new component. This probably won't be finalized for a bit, but
> since you expressed interest in the naming you have now volunteered yourself
as
> tribute :).

[mef, 2016-09-28 15:32:29]

On 2016/09/26 23:28:21, Garrett Casto wrote:
> Apologies for dropping this, it got pushed down the stack for a few weeks.
I've
> removed Environment in favor of just using URLRequestContextGetter directly,
> which seems reasonable. 
I apologize, but I also will not be able to review it until next week.
> 
> Open issues
> 1) Testing: The Cronet bot still doesn't seem to do anything (the analyze step
> always fails, and compilation doesn't actually compile Cronet). Misha and
> Sylvain, it sounds like the two of you were working on having real tests, is
> that still in the pipe? 
I've used
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-cr...
before and it appeared to work,
but it seems that now it fails with cryptic error:

MBErr: target "cronet_test" not found in //testing/buildbot/gn_isolate_map.pyl

> Do I need to test this locally instead? If so, are there directions anywhere?
It is possible to build and run cronet_test on Mac with XCode. I'll write up
directions on how to do it, but it is VERY iOS-centric.

> 2) Naming: I didn't change grpc_support for now, because I wanted to have a
> larger discussion about names here. It seems to me that everything relating to
> gRPC shouldn't really be called cronet (e.g. cronet_c_for_grpc.h). That is,
gRPC
> is allowing for another program to essentially define a way to create and
> control bidirectional streams and Cronet is one implementation of that. gRPC
> shouldn't know that it's Cronet in particular that is performing this service.
> It feels like the naming should reflect this (e.g. cronet_engine -> engine,
> cronet_bidirectional_stream -> bidirectional_stream,
> grpc_support::CronetBidirectionalStream -> grpc_support::BidirectionalStream,
> ...). Misha, what are your thoughts? I think that it removes some confusion
> about what "Cronet" means, since currently the term is used for both an
> interface and an implemenation.
Sounds good to me. 
> 
> I'm also not sure how this change should be reflected in the grpc repository.
It
> looks like they manually sync cronet_c_for_grpc.h. Is this something that they
> are responsible for, or do we need to check in a new version when a change is
> made in the Chromium repository?
AFAIK they use an old snapshot of Cronet in GRPC GITHUB repository, but depend
on updated Cronet in internal builds.
We do need to come up with the proper plan for open source dependencies. 
> 
> PTAL
> 
> mef - Everything
> sdefresne: BUILD.gn changes
> blundell - For new component. This probably won't be finalized for a bit, but
> since you expressed interest in the naming you have now volunteered yourself
as
> tribute :).

[mef, 2016-09-28 15:34:35]

On 2016/09/28 13:51:58, blundell wrote:
> It's hard for me to understand what the relationship of the new component to
> gRPC is. It doesn't seem to use gRPC as far as I can tell. Am I understanding
> correctly that the dependency is actually the other way, i.e., that cronet is
> *consumed* by gRPC via cronet_c_for_grpc.h?

Exactly, this is cronet providing bidirectional_stream api that is consumed by
cronet_transport in grpc.

[sdefresne, 2016-09-28 15:51:39]

On 2016/09/28 15:32:26, mef wrote:
> On 2016/09/26 23:28:21, Garrett Casto wrote:
> > Apologies for dropping this, it got pushed down the stack for a few weeks.
> I've
> > removed Environment in favor of just using URLRequestContextGetter directly,
> > which seems reasonable. 
> I apologize, but I also will not be able to review it until next week.
> > 
> > Open issues
> > 1) Testing: The Cronet bot still doesn't seem to do anything (the analyze
step
> > always fails, and compilation doesn't actually compile Cronet). Misha and
> > Sylvain, it sounds like the two of you were working on having real tests, is
> > that still in the pipe? 
> I've used
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-cr...
> before and it appeared to work,
> but it seems that now it fails with cryptic error:
> 
> MBErr: target "cronet_test" not found in //testing/buildbot/gn_isolate_map.pyl

It looks like there is a file named testing/buildbot/gn_isolate_map.pyl in
chromium repository. Looking at the file, it appears to give a mapping between
test short name to the corresponding gn target. So, it would probably just be a
matter of adding an entry mapping cronet_test to
//components/cronet/ios/cronet_test (though if there are multiple cronet_test
target for different platforms, it may have to be renamed, or a group created).

> > Do I need to test this locally instead? If so, are there directions
anywhere?
> It is possible to build and run cronet_test on Mac with XCode. I'll write up
> directions on how to do it, but it is VERY iOS-centric.
> 
> > 2) Naming: I didn't change grpc_support for now, because I wanted to have a
> > larger discussion about names here. It seems to me that everything relating
to
> > gRPC shouldn't really be called cronet (e.g. cronet_c_for_grpc.h). That is,
> gRPC
> > is allowing for another program to essentially define a way to create and
> > control bidirectional streams and Cronet is one implementation of that. gRPC
> > shouldn't know that it's Cronet in particular that is performing this
service.
> > It feels like the naming should reflect this (e.g. cronet_engine -> engine,
> > cronet_bidirectional_stream -> bidirectional_stream,
> > grpc_support::CronetBidirectionalStream ->
grpc_support::BidirectionalStream,
> > ...). Misha, what are your thoughts? I think that it removes some confusion
> > about what "Cronet" means, since currently the term is used for both an
> > interface and an implemenation.
> Sounds good to me. 
> > 
> > I'm also not sure how this change should be reflected in the grpc
repository.
> It
> > looks like they manually sync cronet_c_for_grpc.h. Is this something that
they
> > are responsible for, or do we need to check in a new version when a change
is
> > made in the Chromium repository?
> AFAIK they use an old snapshot of Cronet in GRPC GITHUB repository, but depend
> on updated Cronet in internal builds.
> We do need to come up with the proper plan for open source dependencies. 
> > 
> > PTAL
> > 
> > mef - Everything
> > sdefresne: BUILD.gn changes
> > blundell - For new component. This probably won't be finalized for a bit,
but
> > since you expressed interest in the naming you have now volunteered yourself
> as
> > tribute :).

[sdefresne, 2016-09-28 15:54:19]

I won't have time to review before next week, sorry.

[Garrett Casto, 2016-09-29 16:11:35]

On 2016/09/28 15:54:19, sdefresne wrote:
> I won't have time to review before next week, sorry.

I'm also in Promo committee this week, so I'll be a little slow making changes.
I should be able to make the discussed naming changes and separate out the ios
GN changes tomorrow.

[blundell, 2016-10-03 08:14:05]

The naming seems fine now that I understand the code's purpose. Please add a
README.md to grpc_support explaining its purpose, as it's totally cryptic
otherwise.

Why can't this code go into //net rather than a component? It's essentially
exporting parts of the net stack as a library to gRPC, right?

[Garrett Casto, 2016-10-04 04:08:33]

On 2016/10/03 08:14:05, blundell wrote:
> The naming seems fine now that I understand the code's purpose. Please add a
> README.md to grpc_support explaining its purpose, as it's totally cryptic
> otherwise.
> 
> Why can't this code go into //net rather than a component? It's essentially
> exporting parts of the net stack as a library to gRPC, right?

Update on this CL: Adding cronet_tests to testing/buildbot/gn_isolate_map.pyl
seems to have actually gotten the tests working again, though for now I am
ignoring a PRESUBMIT check that is complaining about there not being a matching
entry in one of the JSON files in that directory. I'll have to figure that out
with someone from the infrastructure team at some point. The test itself is
currently failing, but I should be able to fix it by tomorrow. Having to do a
compile and test cycle through bots is so slow :(.

I haven't yet split this CL, but I will probably end up fixing the buildbot and
changing rules.gni in separate CLs.

Regarding moving this code to //net instead of //components, I can see the
appeal, though it feels like this isn't really part of the public net API, more
like glue code that translates between gRPCs expectations and the actual net
API. Long term it should probably exist in a place where it's shared between
both repositories, possibly in a separate repo. Misha, do you have thoughts on
this?

[Garrett Casto, 2016-10-04 23:46:25]

gcasto@chromium.org changed reviewers:
- sdefresne@chromium.org

[Garrett Casto, 2016-10-04 23:46:27]

Alright, the test passes and I think that this is ready for a real review at
this point. I've split out the buildbot changes and the rules.gni changes into
their own CLs (2396673002 and 2394663003 respectively), but I'm currently
keeping the changes in this CL as well just so that I can still run the tests.
I'll make sure those land before landing this change.

PTAL

[blundell, 2016-10-05 11:09:03]

still having a little trouble wrapping my head around this :).

https://codereview.chromium.org/2273403003/diff/660001/components/grpc_suppor...
File components/grpc_support/README.md (right):

https://codereview.chromium.org/2273403003/diff/660001/components/grpc_suppor...
components/grpc_support/README.md:8: Internally to Chrome, there will be two
different backings of this API. Cronet
I'm confused: you say that this directory contains the implementation, and then
that there are two implementations, the first of which lives somewhere else. Is
the second the one in //components/grpc_support itself? That seems a little
confusing.

Higher-level: Why is there a need for two implementations? Naively, it seems
like Cronet-for-iOS-apps and Chromium could use the same impl, similar to how
Cronet consumes //net.

[Garrett Casto, 2016-10-05 17:45:19]

On 2016/10/05 11:09:03, blundell wrote:
> still having a little trouble wrapping my head around this :).
> 
>
https://codereview.chromium.org/2273403003/diff/660001/components/grpc_suppor...
> File components/grpc_support/README.md (right):
> 
>
https://codereview.chromium.org/2273403003/diff/660001/components/grpc_suppor...
> components/grpc_support/README.md:8: Internally to Chrome, there will be two
> different backings of this API. Cronet
> I'm confused: you say that this directory contains the implementation, and
then
> that there are two implementations, the first of which lives somewhere else.
Is
> the second the one in //components/grpc_support itself? That seems a little
> confusing.
> 

Yeah, that was a mess of a paragraph. I've taken another pass, which I think
clarifies what is going on (one API and implementation, but the API takes a
void* which is essentially a parameter for the implementation, which was use to
move around a URLRequestContextGetter).

> Higher-level: Why is there a need for two implementations? Naively, it seems
> like Cronet-for-iOS-apps and Chromium could use the same impl, similar to how
> Cronet consumes //net.

Two implementations was an incorrect statement, it's the same implementation
with a different context (net::URLRequestContextGetter). Hopefully the new
verbage is clearer.

[blundell, 2016-10-06 08:31:08]

lgtm, thanks for the explanations and clarifications!

https://codereview.chromium.org/2273403003/diff/700001/components/grpc_suppor...
File components/grpc_support/DEPS (right):

https://codereview.chromium.org/2273403003/diff/700001/components/grpc_suppor...
components/grpc_support/DEPS:2: "+net",
Would it be OK to add DEPS here since this is just on the implementation, or
should this not grow DEPS beyond //net? If the latter, it would be good to note
that explicitly in a comment here.

https://codereview.chromium.org/2273403003/diff/700001/components/grpc_suppor...
File components/grpc_support/README.md (right):

https://codereview.chromium.org/2273403003/diff/700001/components/grpc_suppor...
components/grpc_support/README.md:14: a net::URLRequestContextGetter.
Would it be worth it to make a simple test that exercises c_for_grpc.h in
Chromium-specific fashion to serve as a basic example of how to exercise the API
within Chromium? (similar in spirit to something like content_shell)

https://codereview.chromium.org/2273403003/diff/700001/components/grpc_suppor...
File components/grpc_support/include/c_for_grpc.h (right):

https://codereview.chromium.org/2273403003/diff/700001/components/grpc_suppor...
components/grpc_support/include/c_for_grpc.h:11: extern "C" {
I assume that this file (and anything else in this directory) shouldn't have any
DEPS on Chromium at all, as it's exported to gRPC, right? I would suggest adding
a DEPS file that removes all allowances and notes that fact just to be
super-explicit.

[blundell, 2016-10-06 08:31:58]

btw my lgtm is just for the addition of the new component obviously, i defer to
mef@ on all the actual code changes within //components

[Garrett Casto, 2016-10-06 21:58:27]

https://codereview.chromium.org/2273403003/diff/700001/components/grpc_suppor...
File components/grpc_support/DEPS (right):

https://codereview.chromium.org/2273403003/diff/700001/components/grpc_suppor...
components/grpc_support/DEPS:2: "+net",
On 2016/10/06 08:31:08, blundell wrote:
> Would it be OK to add DEPS here since this is just on the implementation, or
> should this not grow DEPS beyond //net? If the latter, it would be good to
note
> that explicitly in a comment here.

This is just the implementation, so dependencies can be added here.

https://codereview.chromium.org/2273403003/diff/700001/components/grpc_suppor...
File components/grpc_support/README.md (right):

https://codereview.chromium.org/2273403003/diff/700001/components/grpc_suppor...
components/grpc_support/README.md:14: a net::URLRequestContextGetter.
On 2016/10/06 08:31:08, blundell wrote:
> Would it be worth it to make a simple test that exercises c_for_grpc.h in
> Chromium-specific fashion to serve as a basic example of how to exercise the
API
> within Chromium? (similar in spirit to something like content_shell)

I can file a bug for this, but currently gRPC doesn't have C++ bindings that let
you use this interface (just Objective-C).

https://codereview.chromium.org/2273403003/diff/700001/components/grpc_suppor...
File components/grpc_support/include/c_for_grpc.h (right):

https://codereview.chromium.org/2273403003/diff/700001/components/grpc_suppor...
components/grpc_support/include/c_for_grpc.h:11: extern "C" {
On 2016/10/06 08:31:08, blundell wrote:
> I assume that this file (and anything else in this directory) shouldn't have
any
> DEPS on Chromium at all, as it's exported to gRPC, right? I would suggest
adding
> a DEPS file that removes all allowances and notes that fact just to be
> super-explicit.

Added a new DEPS file.

[blundell, 2016-10-07 06:43:44]

still lgtm, thanks

https://codereview.chromium.org/2273403003/diff/720001/components/grpc_suppor...
File components/grpc_support/include/DEPS (right):

https://codereview.chromium.org/2273403003/diff/720001/components/grpc_suppor...
components/grpc_support/include/DEPS:3: ]
I think you need to explicitly - all the DEPS it inherits from parents to
actually enforce this.

[Garrett Casto, 2016-10-07 21:53:16]

Thanks for the review.

mef@, can you please take a look?

https://codereview.chromium.org/2273403003/diff/720001/components/grpc_suppor...
File components/grpc_support/include/DEPS (right):

https://codereview.chromium.org/2273403003/diff/720001/components/grpc_suppor...
components/grpc_support/include/DEPS:3: ]
On 2016/10/07 06:43:44, blundell wrote:
> I think you need to explicitly - all the DEPS it inherits from parents to
> actually enforce this.

Ah, whoops. Added "-" as a rule, which I'm assuming disallows everything.

[blundell, 2016-10-10 08:28:27]

On 2016/10/07 21:53:16, Garrett Casto wrote:
> Thanks for the review.
> 
> mef@, can you please take a look?
> 
>
https://codereview.chromium.org/2273403003/diff/720001/components/grpc_suppor...
> File components/grpc_support/include/DEPS (right):
> 
>
https://codereview.chromium.org/2273403003/diff/720001/components/grpc_suppor...
> components/grpc_support/include/DEPS:3: ]
> On 2016/10/07 06:43:44, blundell wrote:
> > I think you need to explicitly - all the DEPS it inherits from parents to
> > actually enforce this.
> 
> Ah, whoops. Added "-" as a rule, which I'm assuming disallows everything.

I just did a quick check out of curiosity and that doesn't actually have any
effect (you should be able to see that by adding an include of //net to
c_for_grpc.h and then running "./buildtools/checkdeps/checkdeps.py
components/grpc_support/include".)

This is a small issue, sorry to harp on it!

[mef, 2016-10-11 16:02:39]

On 2016/10/10 08:28:27, blundell wrote:
> On 2016/10/07 21:53:16, Garrett Casto wrote:
> > Thanks for the review.
> > 
> > mef@, can you please take a look?
> > 
> >
>
https://codereview.chromium.org/2273403003/diff/720001/components/grpc_suppor...
> > File components/grpc_support/include/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/2273403003/diff/720001/components/grpc_suppor...
> > components/grpc_support/include/DEPS:3: ]
> > On 2016/10/07 06:43:44, blundell wrote:
> > > I think you need to explicitly - all the DEPS it inherits from parents to
> > > actually enforce this.
> > 
> > Ah, whoops. Added "-" as a rule, which I'm assuming disallows everything.
> 
> I just did a quick check out of curiosity and that doesn't actually have any
> effect (you should be able to see that by adding an include of //net to
> c_for_grpc.h and then running "./buildtools/checkdeps/checkdeps.py
> components/grpc_support/include".)
> 
> This is a small issue, sorry to harp on it!

Sorry for the delay, will look shortly.

[mef, 2016-10-11 22:09:37]

It looks pretty good, but I also have naming concerns. 
There might be interest to use bidirectional stream outside of grpc. 

Would it make sense to call this components/bidirectional_stream?

https://codereview.chromium.org/2273403003/diff/740001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/2273403003/diff/740001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:222: // Create
request_context_getter_ here so that it will be availble as soon
Would it be easier to have custom net::URLRequestContextGetter which is aware of
CronetEnvironment like in
https://codereview.chromium.org/2146643002/diff/420001/components/cronet/ios/...

https://codereview.chromium.org/2273403003/diff/740001/components/cronet/ios/...
File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (right):

https://codereview.chromium.org/2273403003/diff/740001/components/cronet/ios/...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:5: #import
<Foundation/Foundation.h>
We should be able to move cronet_bidirectional_stream_test.mm into
bidirectional_stream_test.cc and make it run as chrome unit test. 

Ideally as part of this CL, or soon after.

https://codereview.chromium.org/2273403003/diff/740001/components/grpc_suppor...
File components/grpc_support/include/c_for_grpc.h (right):

https://codereview.chromium.org/2273403003/diff/740001/components/grpc_suppor...
components/grpc_support/include/c_for_grpc.h:5: #ifndef
COMPONENTS_GRPC_SUPPORT_INCLUDE_C_FOR_GRPC_H_
The name 'c_for_grpc.h' is now even more confusing than before.
Would 'bidirectional_stream_c.h' be a little better?

[Garrett Casto, 2016-10-12 00:11:31]

On 2016/10/10 08:28:27, blundell wrote:
> On 2016/10/07 21:53:16, Garrett Casto wrote:
> > Thanks for the review.
> > 
> > mef@, can you please take a look?
> > 
> >
>
https://codereview.chromium.org/2273403003/diff/720001/components/grpc_suppor...
> > File components/grpc_support/include/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/2273403003/diff/720001/components/grpc_suppor...
> > components/grpc_support/include/DEPS:3: ]
> > On 2016/10/07 06:43:44, blundell wrote:
> > > I think you need to explicitly - all the DEPS it inherits from parents to
> > > actually enforce this.
> > 
> > Ah, whoops. Added "-" as a rule, which I'm assuming disallows everything.
> 
> I just did a quick check out of curiosity and that doesn't actually have any
> effect (you should be able to see that by adding an include of //net to
> c_for_grpc.h and then running "./buildtools/checkdeps/checkdeps.py
> components/grpc_support/include".)
> 
> This is a small issue, sorry to harp on it!

Hmm, so as far as I can tell, there doesn't actually seem to be a way to
disallow all dependencies. For now I've just removed the dependency on base,
with a comment to fix this at some point, since it seems like it's going to
require changes to checkdeps. I could also try listing some larger number of
directories, but it wasn't clear if it was worth it.

[Garrett Casto, 2016-10-12 00:23:36]

Regarding naming, I think that there is a larger question here which is why
there is a distinction between grpc_support::BidirectionalStream and
net::BidirectionalStream. My assumption was that net::BidirectionalStream
provided a generic interface and grpc_support::BidirectionalStream massaged it
into a format that was useful for gRPC. I would assume that generally other
callers would just directly use net::BidirectionalStream, and if they would
actually want to use the grpc_support::BidirectionalStream instead, it feels
like the interface for net::BidirectionalStream should just be changed to be
more like grpc_support::BidirectionalStream (or
grpc_support::BidirectionalStream should be added as an adapter class somewhere
in //net).

It seems like we still want some component that is gRPC specific that includes
the gRPC C API and implementation, and grpc_support seems like a better name
that bidirectional_stream for that.

https://codereview.chromium.org/2273403003/diff/740001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.cc (right):

https://codereview.chromium.org/2273403003/diff/740001/components/cronet/ios/...
components/cronet/ios/cronet_environment.cc:222: // Create
request_context_getter_ here so that it will be availble as soon
On 2016/10/11 22:09:37, mef wrote:
> Would it be easier to have custom net::URLRequestContextGetter which is aware
of
> CronetEnvironment like in
>
https://codereview.chromium.org/2146643002/diff/420001/components/cronet/ios/...

Yes. If nothing else this doesn't require getting approval to add
DetachFromThread to URLRequestContext, which seems like a win.

https://codereview.chromium.org/2273403003/diff/740001/components/cronet/ios/...
File components/cronet/ios/test/cronet_bidirectional_stream_test.mm (right):

https://codereview.chromium.org/2273403003/diff/740001/components/cronet/ios/...
components/cronet/ios/test/cronet_bidirectional_stream_test.mm:5: #import
<Foundation/Foundation.h>
On 2016/10/11 22:09:37, mef wrote:
> We should be able to move cronet_bidirectional_stream_test.mm into
> bidirectional_stream_test.cc and make it run as chrome unit test. 
> 
> Ideally as part of this CL, or soon after.

Seems like you want two tests, one that verifies that CronetEnvironment properly
sets up it's URLRequestContextGetter, and another that verifies that a
URLRequestContextGetter can be used to drive the gRPC C API. I can imagine that
a substantial fraction of this test is moved to be common code, but I would
think that a test of some form would stay.

https://codereview.chromium.org/2273403003/diff/740001/components/grpc_suppor...
File components/grpc_support/include/c_for_grpc.h (right):

https://codereview.chromium.org/2273403003/diff/740001/components/grpc_suppor...
components/grpc_support/include/c_for_grpc.h:5: #ifndef
COMPONENTS_GRPC_SUPPORT_INCLUDE_C_FOR_GRPC_H_
On 2016/10/11 22:09:37, mef wrote:
> The name 'c_for_grpc.h' is now even more confusing than before.
> Would 'bidirectional_stream_c.h' be a little better?

That seems fine.

[blundell, 2016-10-12 05:25:44]

I also don't understand why we would have a generic bidirectional stream that
lived in //components instead of //net.

[Garrett Casto, 2016-10-20 03:49:33]

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

[commit-bot: I haz the power, 2016-10-20 03:50:17]

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-10-20 04:15:20]

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

[commit-bot: I haz the power, 2016-10-20 04:15:21]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Garrett Casto, 2016-10-20 04:37:20]

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

[commit-bot: I haz the power, 2016-10-20 04:37:48]

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-10-20 05:03:51]

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

[commit-bot: I haz the power, 2016-10-20 05:03:52]

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

[Garrett Casto, 2016-10-20 21:47:19]

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

[commit-bot: I haz the power, 2016-10-20 21:47:59]

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-10-20 22:02:48]

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

[commit-bot: I haz the power, 2016-10-20 22:02:50]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[Garrett Casto, 2016-10-21 00:25:35]

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

[commit-bot: I haz the power, 2016-10-21 00:26:12]

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-10-21 01:09:51]

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

[commit-bot: I haz the power, 2016-10-21 01:09:53]

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

[Garrett Casto, 2016-10-21 05:29:58]

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

[commit-bot: I haz the power, 2016-10-21 05:30:10]

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

[Garrett Casto, 2016-10-21 19:54:42]

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

[commit-bot: I haz the power, 2016-10-21 19:55:25]

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

[Garrett Casto, 2016-10-24 18:24:30]

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

[commit-bot: I haz the power, 2016-10-24 18:25:08]

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-10-24 18:34:17]

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

[commit-bot: I haz the power, 2016-10-24 18:34:19]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Garrett Casto, 2016-10-24 18:34:26]

Okay, I think that this is ready for another look. The major change that I made
was for testing. Specifically I moved cronet_bidirectional_stream_test.mm to
grpc_support and defined a function grpc_support::GetTestStreamEngine() that has
two implementations (one in Cronet and one in grpc_support). This allows us to
test both that the bidirectional_stream code in grpc_support works and that
Cronet generates a stream_engine that works correctly. One of the biggest
annoyances of this is that there is now other code that lives in
components/cronet that depends on components/grpc_support for the
QuicTestServer. I think that it's livable, but not ideal.

https://codereview.chromium.org/2273403003/diff/1210001/components/grpc_suppo...
File components/grpc_support/bidirectional_stream.h (right):

https://codereview.chromium.org/2273403003/diff/1210001/components/grpc_suppo...
components/grpc_support/bidirectional_stream.h:122: ERR,
Note: This was changed because the Window compiler had an issue with this
conflicting with the logging macro ERROR. I'm open to changing the name.

https://codereview.chromium.org/2273403003/diff/1210001/components/grpc_suppo...
components/grpc_support/bidirectional_stream.h:195: //                         |
<--- READING <--- |
Note: Android compiler didn't like the "/  <" character sequence. I don't
remember the exact error message, something to do with multi line comments,
which I thought was strange.

[Garrett Casto, 2016-10-24 18:48:55]

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

[commit-bot: I haz the power, 2016-10-24 18:49: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-10-24 18:57:33]

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

[commit-bot: I haz the power, 2016-10-24 18:57:35]

Dry run: Try jobs failed on following builders:
  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-10-24 19:13:08]

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

[commit-bot: I haz the power, 2016-10-24 19:14:00]

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-10-24 21:45:45]

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

[commit-bot: I haz the power, 2016-10-24 21:45:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mef, 2016-10-26 22:19:45]

lgtm mod nits. Thanks a lot for doing this! Having cross-platform support for
GRPC will be fantastic!

https://codereview.chromium.org/2273403003/diff/1210001/components/grpc_suppo...
File components/grpc_support/bidirectional_stream.h (right):

https://codereview.chromium.org/2273403003/diff/1210001/components/grpc_suppo...
components/grpc_support/bidirectional_stream.h:122: ERR,
On 2016/10/24 18:34:25, Garrett Casto wrote:
> Note: This was changed because the Window compiler had an issue with this
> conflicting with the logging macro ERROR. I'm open to changing the name.

Acknowledged.

https://codereview.chromium.org/2273403003/diff/1210001/components/grpc_suppo...
components/grpc_support/bidirectional_stream.h:195: //                         |
<--- READING <--- |
On 2016/10/24 18:34:26, Garrett Casto wrote:
> Note: Android compiler didn't like the "/  <" character sequence. I don't
> remember the exact error message, something to do with multi line comments,
> which I thought was strange.

Acknowledged.

https://codereview.chromium.org/2273403003/diff/1280001/components/cronet/ios...
File components/cronet/ios/test/BUILD.gn (right):

https://codereview.chromium.org/2273403003/diff/1280001/components/cronet/ios...
components/cronet/ios/test/BUILD.gn:24:
"//components/grpc_support:bidirectional_stream_unittest",
kudos for this!

https://codereview.chromium.org/2273403003/diff/1280001/components/cronet/ios...
File components/cronet/ios/test/get_stream_engine.mm (right):

https://codereview.chromium.org/2273403003/diff/1280001/components/cronet/ios...
components/cronet/ios/test/get_stream_engine.mm:18: }  // namespcae grpc_support
nit: namespace (sp).

https://codereview.chromium.org/2273403003/diff/1280001/components/grpc_suppo...
File components/grpc_support/BUILD.gn (right):

https://codereview.chromium.org/2273403003/diff/1280001/components/grpc_suppo...
components/grpc_support/BUILD.gn:38: "test/get_stream_engine.cc",
I might be wrong, but I think test/* source_sets should go into test/BUILD.gn.

[Garrett Casto, 2016-10-27 17:25:09]

The CQ bit was checked by gcasto@chromium.org

[Garrett Casto, 2016-10-27 17:25:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org, mef@chromium.org
Link to the patchset: https://codereview.chromium.org/2273403003/#ps1300001
(title: "Address comments")

[commit-bot: I haz the power, 2016-10-27 17:25:33]

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-10-27 18:57:53]

Description was changed from

==========
Moving gRPC support interfaces out of cronet and into a new component.

BUG=650462
==========

to

==========
Moving gRPC support interfaces out of cronet and into a new component.

BUG=650462
==========

[commit-bot: I haz the power, 2016-10-27 18:57:56]

Committed patchset #67 (id:1300001)

[commit-bot: I haz the power, 2016-10-27 19:02:06]

Description was changed from

==========
Moving gRPC support interfaces out of cronet and into a new component.

BUG=650462
==========

to

==========
Moving gRPC support interfaces out of cronet and into a new component.

BUG=650462

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

[commit-bot: I haz the power, 2016-10-27 19:02:09]

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

[mohsen, 2016-10-27 20:10:07]

A revert of this CL (patchset #67 id:1300001) has been created in
https://codereview.chromium.org/2454183002/ by mohsen@chromium.org.

The reason for reverting is: Causes failures on "Mac10.9 Tests" builder:

https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.9%20Tests/...

An example failure:

[ RUN      ]
BidirectionalStreamDelayRequestHeadersUntilFlush/BidirectionalStreamTest.StartExampleBidiStream/1
[12832:13571:1027/123821:4064068458447:ERROR:quic_simple_server.cc(97)] Listen()
failed: net::ERR_ADDRESS_IN_USE
[12832:13571:1027/123821:4064068534048:FATAL:quic_test_server.cc(87)] Check
failed: rv >= 0 (-147 vs. 0)Quic server fails to start
0   components_unittests                0x00000001095d79c3
_ZN4base5debug10StackTraceC1Ev + 19
1   components_unittests                0x00000001095ee9d7
_ZN7logging10LogMessageD2Ev + 71
2   components_unittests                0x0000000109461920
_ZN12grpc_support29StartQuicServerOnServerThreadERKN4base8FilePathEPNS0_13WaitableEventE
+ 608
3   components_unittests                0x00000001095d7f49
_ZN4base5debug13TaskAnnotator7RunTaskEPKcPNS_11PendingTaskE + 217
4   components_unittests                0x00000001095f8236
_ZN4base11MessageLoop7RunTaskEPNS_11PendingTaskE + 390
5   components_unittests                0x00000001095f850c
_ZN4base11MessageLoop21DeferOrRunPendingTaskENS_11PendingTaskE + 44
6   components_unittests                0x00000001095f8853
_ZN4base11MessageLoop6DoWorkEv + 371
7   components_unittests                0x00000001095fa223
_ZN4base19MessagePumpLibevent3RunEPNS_11MessagePump8DelegateE + 83
8   components_unittests                0x00000001095f7f9e
_ZN4base11MessageLoop10RunHandlerEv + 94
9   components_unittests                0x00000001096103f3
_ZN4base7RunLoop3RunEv + 115
10  components_unittests                0x0000000109630f92
_ZN4base6Thread10ThreadMainEv + 386
11  components_unittests                0x000000010962c6d7
_ZN4base12_GLOBAL__N_110ThreadFuncEPv + 87
12  libsystem_pthread.dylib             0x00007fff8e718899 _pthread_body + 138
13  libsystem_pthread.dylib             0x00007fff8e71872a _pthread_struct_init
+ 0
14  libsystem_pthread.dylib             0x00007fff8e71cfc9 thread_start + 13
.

[findit-for-me, 2016-10-27 20:16:49]

FYI: Findit identified this CL at revision 428099 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...