Convert enable_bidirectional_stream into build flag local to net.

Convert enable_bidirectional_stream into build flag local to net.

BUG=571696
Committed: https://crrev.com/ff34b82bb81e0f1be15226bb2a3c4a2b484b421c
Cr-Commit-Position: refs/heads/master@{#368589}

[mef, 2015-12-28 21:22:15]

mef@chromium.org changed reviewers:
+ mmenke@chromium.org, thakis@chromium.org

[mef, 2015-12-28 21:22:15]

PTAL, this is a followup to address Nico's comment here:
https://codereview.chromium.org/1326503003/diff/1270001/build/common.gypi

[mef, 2015-12-29 18:58:28]

https://codereview.chromium.org/1550843002/diff/1/net/http/bidirectional_stre...
File net/http/bidirectional_stream.h (right):

https://codereview.chromium.org/1550843002/diff/1/net/http/bidirectional_stre...
net/http/bidirectional_stream.h:16: #include "net/net_features.h"
This is causing compilation error when this header is included from
cronet/cronet_bidirectional_stream.h.

[Nico, 2015-12-30 22:36:22]

https://codereview.chromium.org/1550843002/diff/1/net/http/bidirectional_stre...
File net/http/bidirectional_stream.h (right):

https://codereview.chromium.org/1550843002/diff/1/net/http/bidirectional_stre...
net/http/bidirectional_stream.h:16: #include "net/net_features.h"
On 2015/12/29 18:58:28, mef wrote:
> This is causing compilation error when this header is included from
> cronet/cronet_bidirectional_stream.h.

Which?

https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
File net/http/bidirectional_stream.h (left):

https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.h:18: #error Only include this if
ENABLE_BIDIRECTIONAL_STREAM is defined
This is still important, else we'll get linker errors on bots building with
-frtti if someone includes this file without checking for the define.

[mef, 2015-12-30 23:20:46]

https://codereview.chromium.org/1550843002/diff/1/net/http/bidirectional_stre...
File net/http/bidirectional_stream.h (right):

https://codereview.chromium.org/1550843002/diff/1/net/http/bidirectional_stre...
net/http/bidirectional_stream.h:16: #include "net/net_features.h"
On 2015/12/30 22:36:21, Nico wrote:
> On 2015/12/29 18:58:28, mef wrote:
> > This is causing compilation error when this header is included from
> > cronet/cronet_bidirectional_stream.h.
> 
> Which?

Pending CL that uses bidirectional_stream in Cronet:
https://codereview.chromium.org/1412243012/diff/540001/components/cronet/andr...

https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
File net/http/bidirectional_stream.h (left):

https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.h:18: #error Only include this if
ENABLE_BIDIRECTIONAL_STREAM is defined
On 2015/12/30 22:36:21, Nico wrote:
> This is still important, else we'll get linker errors on bots building with
> -frtti if someone includes this file without checking for the define.

Ack, I'm open for suggestions.

[Nico, 2015-12-30 23:25:33]

https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
File net/http/bidirectional_stream.h (left):

https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.h:18: #error Only include this if
ENABLE_BIDIRECTIONAL_STREAM is defined
On 2015/12/30 23:20:46, mef wrote:
> On 2015/12/30 22:36:21, Nico wrote:
> > This is still important, else we'll get linker errors on bots building with
> > -frtti if someone includes this file without checking for the define.
> 
> Ack, I'm open for suggestions.

"Don't remove this"? :-) What breaks if you keep it?

[mef, 2015-12-30 23:42:05]

https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
File net/http/bidirectional_stream.h (left):

https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.h:18: #error Only include this if
ENABLE_BIDIRECTIONAL_STREAM is defined
On 2015/12/30 23:25:32, Nico wrote:
> On 2015/12/30 23:20:46, mef wrote:
> > On 2015/12/30 22:36:21, Nico wrote:
> > > This is still important, else we'll get linker errors on bots building
with
> > > -frtti if someone includes this file without checking for the define.
> > 
> > Ack, I'm open for suggestions.
> 
> "Don't remove this"? :-) What breaks if you keep it?

Compilation of components/cronet because it has to include this file for
definition of BidirectionalStream::Delegate, yet it does not know about
net::ENABLE_BIDIRECTIONAL_STREAM build flag.

[mmenke, 2016-01-04 18:43:13]

https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
File net/http/bidirectional_stream.h (left):

https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.h:18: #error Only include this if
ENABLE_BIDIRECTIONAL_STREAM is defined
On 2015/12/30 23:42:04, mef wrote:
> On 2015/12/30 23:25:32, Nico wrote:
> > On 2015/12/30 23:20:46, mef wrote:
> > > On 2015/12/30 22:36:21, Nico wrote:
> > > > This is still important, else we'll get linker errors on bots building
> with
> > > > -frtti if someone includes this file without checking for the define.
> > > 
> > > Ack, I'm open for suggestions.
> > 
> > "Don't remove this"? :-) What breaks if you keep it?
> 
> Compilation of components/cronet because it has to include this file for
> definition of BidirectionalStream::Delegate, yet it does not know about
> net::ENABLE_BIDIRECTIONAL_STREAM build flag.

The code to declare the build flag is a "public" dependency...  I assume that
means there's some way cronet can transitively access the build flag?  If not,
why is it a public and not a private dependency?

[mef, 2016-01-04 20:19:54]

https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
File net/http/bidirectional_stream.h (left):

https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
net/http/bidirectional_stream.h:18: #error Only include this if
ENABLE_BIDIRECTIONAL_STREAM is defined
On 2016/01/04 18:43:13, mmenke (OOO Dec 19 to Jan 3) wrote:
> On 2015/12/30 23:42:04, mef wrote:
> > On 2015/12/30 23:25:32, Nico wrote:
> > > On 2015/12/30 23:20:46, mef wrote:
> > > > On 2015/12/30 22:36:21, Nico wrote:
> > > > > This is still important, else we'll get linker errors on bots building
> > with
> > > > > -frtti if someone includes this file without checking for the define.
> > > > 
> > > > Ack, I'm open for suggestions.
> > > 
> > > "Don't remove this"? :-) What breaks if you keep it?
> > 
> > Compilation of components/cronet because it has to include this file for
> > definition of BidirectionalStream::Delegate, yet it does not know about
> > net::ENABLE_BIDIRECTIONAL_STREAM build flag.
> 
> The code to declare the build flag is a "public" dependency...  I assume that
> means there's some way cronet can transitively access the build flag?  If not,
> why is it a public and not a private dependency?

Hrm, IIUIC move of enable_bidirectional_stream flag from base/common.gypi into
net.gypi made it essentially a private net/ flag and is not accessible from
other targets. 

I've added a separate cronet::enable_bidirectional_stream flag which doesn't
depend on net::enable_bidirectional_stream flag.

Is there some way to define a private dependency of
cronet::enable_bidirectional_stream on net::enable_bidirectional_stream flag?

[mmenke, 2016-01-04 21:02:36]

On 2016/01/04 20:19:54, mef wrote:
>
https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
> File net/http/bidirectional_stream.h (left):
> 
>
https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
> net/http/bidirectional_stream.h:18: #error Only include this if
> ENABLE_BIDIRECTIONAL_STREAM is defined
> On 2016/01/04 18:43:13, mmenke (OOO Dec 19 to Jan 3) wrote:
> > On 2015/12/30 23:42:04, mef wrote:
> > > On 2015/12/30 23:25:32, Nico wrote:
> > > > On 2015/12/30 23:20:46, mef wrote:
> > > > > On 2015/12/30 22:36:21, Nico wrote:
> > > > > > This is still important, else we'll get linker errors on bots
building
> > > with
> > > > > > -frtti if someone includes this file without checking for the
define.
> > > > > 
> > > > > Ack, I'm open for suggestions.
> > > > 
> > > > "Don't remove this"? :-) What breaks if you keep it?
> > > 
> > > Compilation of components/cronet because it has to include this file for
> > > definition of BidirectionalStream::Delegate, yet it does not know about
> > > net::ENABLE_BIDIRECTIONAL_STREAM build flag.
> > 
> > The code to declare the build flag is a "public" dependency...  I assume
that
> > means there's some way cronet can transitively access the build flag?  If
not,
> > why is it a public and not a private dependency?
> 
> Hrm, IIUIC move of enable_bidirectional_stream flag from base/common.gypi into
> net.gypi made it essentially a private net/ flag and is not accessible from
> other targets. 
> 
> I've added a separate cronet::enable_bidirectional_stream flag which doesn't
> depend on net::enable_bidirectional_stream flag.
> 
> Is there some way to define a private dependency of
> cronet::enable_bidirectional_stream on net::enable_bidirectional_stream flag?

I don't think we need the build flag, just the define, right?  And that's
created in a header created at compile time, no?

[mef, 2016-01-04 21:48:26]

On 2016/01/04 21:02:36, mmenke (OOO Dec 19 to Jan 3) wrote:
> On 2016/01/04 20:19:54, mef wrote:
> >
>
https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
> > File net/http/bidirectional_stream.h (left):
> > 
> >
>
https://codereview.chromium.org/1550843002/diff/20001/net/http/bidirectional_...
> > net/http/bidirectional_stream.h:18: #error Only include this if
> > ENABLE_BIDIRECTIONAL_STREAM is defined
> > On 2016/01/04 18:43:13, mmenke (OOO Dec 19 to Jan 3) wrote:
> > > On 2015/12/30 23:42:04, mef wrote:
> > > > On 2015/12/30 23:25:32, Nico wrote:
> > > > > On 2015/12/30 23:20:46, mef wrote:
> > > > > > On 2015/12/30 22:36:21, Nico wrote:
> > > > > > > This is still important, else we'll get linker errors on bots
> building
> > > > with
> > > > > > > -frtti if someone includes this file without checking for the
> define.
> > > > > > 
> > > > > > Ack, I'm open for suggestions.
> > > > > 
> > > > > "Don't remove this"? :-) What breaks if you keep it?
> > > > 
> > > > Compilation of components/cronet because it has to include this file for
> > > > definition of BidirectionalStream::Delegate, yet it does not know about
> > > > net::ENABLE_BIDIRECTIONAL_STREAM build flag.
> > > 
> > > The code to declare the build flag is a "public" dependency...  I assume
> that
> > > means there's some way cronet can transitively access the build flag?  If
> not,
> > > why is it a public and not a private dependency?
> > 
> > Hrm, IIUIC move of enable_bidirectional_stream flag from base/common.gypi
into
> > net.gypi made it essentially a private net/ flag and is not accessible from
> > other targets. 
> > 
> > I've added a separate cronet::enable_bidirectional_stream flag which doesn't
> > depend on net::enable_bidirectional_stream flag.
> > 
> > Is there some way to define a private dependency of
> > cronet::enable_bidirectional_stream on net::enable_bidirectional_stream
flag?
> 
> I don't think we need the build flag, just the define, right?  And that's
> created in a header created at compile time, no?

Yes and no. We do want a certain feature (bidirectional stream) be enabled or
disabled depending on build configuration (chrome or cronet).
This seems to fit nicely into the build flag system and provides some
compile-time checks (e.g. that flag name is not misspelled).

It could certainly be done the old way (via custom define) just as well, but it
would loose those checks.

[mmenke, 2016-01-04 21:50:36]

https://codereview.chromium.org/1550843002/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1550843002/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:17: #include "net/net_features.h"
Why is this needed here, but not in, say,
net/http/http_stream_factory_impl_request.cc?  I had thought it was being
generated to include the define by the new rules, but maybe I was
misunderstanding things?

[mef, 2016-01-04 21:55:33]

https://codereview.chromium.org/1550843002/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl.cc (right):

https://codereview.chromium.org/1550843002/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:17: #include "net/net_features.h"
On 2016/01/04 21:50:36, mmenke (OOO Dec 19 to Jan 3) wrote:
> Why is this needed here, but not in, say,
> net/http/http_stream_factory_impl_request.cc?  I had thought it was being
> generated to include the define by the new rules, but maybe I was
> misunderstanding things?

I think your understanding matches mine. :) 
Without this include BUILDFLAG(ENABLE_BIDIRECTIONAL_STREAM) is triggering a
compilation error.
Which IMHO is a good thing.

It is not needed in net/http/http_stream_factory_impl_request.cc because it is
included in net/http/http_stream_factory_impl_request.h.

[mef, 2016-01-06 22:51:43]

PTAL. It turns out that build errors that I've seen were not caused by
localizing the flag, but by rogue define that I haven't removed.

https://codereview.chromium.org/1550843002/diff/40001/net/net.gyp
File net/net.gyp (left):

https://codereview.chromium.org/1550843002/diff/40001/net/net.gyp#oldcode293
net/net.gyp:293: 'defines': [
It turns out that I've missed this define and it was breaking the build.

[mmenke, 2016-01-07 18:16:41]

https://codereview.chromium.org/1550843002/diff/40001/net/http/bidirectional_...
File net/http/bidirectional_stream.h (right):

https://codereview.chromium.org/1550843002/diff/40001/net/http/bidirectional_...
net/http/bidirectional_stream.h:18: #if !BUILDFLAG(ENABLE_BIDIRECTIONAL_STREAM)
Should all these files include "build/buildflag.h"?

https://codereview.chromium.org/1550843002/diff/40001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1550843002/diff/40001/net/net.gyp#newcode10
net/net.gyp:10: 'enable_bidirectional_stream%': 0,
How does this get set to true for cronet?

[mef, 2016-01-07 19:41:31]

https://codereview.chromium.org/1550843002/diff/40001/net/http/bidirectional_...
File net/http/bidirectional_stream.h (right):

https://codereview.chromium.org/1550843002/diff/40001/net/http/bidirectional_...
net/http/bidirectional_stream.h:18: #if !BUILDFLAG(ENABLE_BIDIRECTIONAL_STREAM)
On 2016/01/07 18:16:41, mmenke wrote:
> Should all these files include "build/buildflag.h"?

It is included into "net/net_features.h"

https://codereview.chromium.org/1550843002/diff/40001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/1550843002/diff/40001/net/net.gyp#newcode10
net/net.gyp:10: 'enable_bidirectional_stream%': 0,
On 2016/01/07 18:16:41, mmenke wrote:
> How does this get set to true for cronet?

as usual, enable_bidirectional_stream=1 for GYP generation.
For example
https://codereview.chromium.org/1412243012/diff/600001/components/cronet/tool...

[mmenke, 2016-01-07 20:05:42]

LGTM

[mef, 2016-01-07 21:15:57]

On 2016/01/07 20:05:42, mmenke wrote:
> LGTM

Matt, thanks!
Nico, PTAL.

[Nico, 2016-01-10 00:56:35]

lgtm

Looks, great, thanks :-)

In general, I think it's good if cronet stays "chrome's network library, as a
library" though. From what I understand, the idea is that this feature might
become a part of chrome's network library in chrome eventually, so this seems
fine. But if there are features that apps will need but chrome won't, they
should probably be implemented in another library that's built on top of cronet
that apps can use, instead of in cronet itself.

[mef, 2016-01-11 14:15:01]

The CQ bit was checked by mef@chromium.org

[mef, 2016-01-11 14:15:01]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1550843002/#ps80001
(title: "Sync")

[commit-bot: I haz the power, 2016-01-11 14:15:22]

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

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

Committed patchset #5 (id:80001)

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

Description was changed from

==========
Convert enable_bidirectional_stream into build flag local to net.

BUG=571696
==========

to

==========
Convert enable_bidirectional_stream into build flag local to net.

BUG=571696

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

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

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