Add an option to disable net::BidirectionalStreamQuicImpl

Add an option to disable net::BidirectionalStreamQuicImpl

This CL adds an option to disable
net::BidirectionalStreamQuicImpl in net::HttpNetworkSession.
When the flag is set to true, alternative service job will
not be negotiated.

BUG=584338
Committed: https://crrev.com/888c2992e286221aaf7bac8b08c8b8a0399d06bc
Cr-Commit-Position: refs/heads/master@{#382084}

[xunjieli, 2016-03-14 16:46:06]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-03-14 16:47:35]

Description was changed from

==========
temp


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

to

==========
Add an option to disable net::BidirectionalStreamQuicImp

This CL adds an option to disable net::BidirectionalStreamQuicImpl in
net::HttpNetworkSession.
When the flag is set to true, alternative service job will
not be negotiated.

BUG=584338
==========

[xunjieli, 2016-03-14 17:21:15]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2016-03-14 17:24:30]

xunjieli@chromium.org changed reviewers:
+ mef@chromium.org, rch@chromium.org

[xunjieli, 2016-03-14 17:24:31]

PTAL. I added unit tests in net/ layer. For cronet layer, since the integration
tests are not there yet, I verified manually that the flag is plumbed all the
way through.

[xunjieli, 2016-03-14 17:26:25]

Description was changed from

==========
Add an option to disable net::BidirectionalStreamQuicImp

This CL adds an option to disable net::BidirectionalStreamQuicImpl in
net::HttpNetworkSession.
When the flag is set to true, alternative service job will
not be negotiated.

BUG=584338
==========

to

==========
Add an option to disable net::BidirectionalStreamQuicImp

This CL adds an option to disable 
net::BidirectionalStreamQuicImpl in net::HttpNetworkSession.
When the flag is set to true, alternative service job will
not be negotiated.

BUG=584338
==========

[mef, 2016-03-15 19:51:10]

looks reasonable to me.

https://codereview.chromium.org/1796253002/diff/40001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1796253002/diff/40001/net/http/http_network_s...
net/http/http_network_session.h:188: // If false,
net::BidirectionalStreamQuicImpl will not be used when
I think it should say 'If true'...

https://codereview.chromium.org/1796253002/diff/40001/net/http/http_network_s...
net/http/http_network_session.h:190: bool
disable_bidirectional_stream_quic_impl;
should we call it 'quic_disable_bidirectional_stream'?

[xunjieli, 2016-03-15 20:15:20]

https://codereview.chromium.org/1796253002/diff/40001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1796253002/diff/40001/net/http/http_network_s...
net/http/http_network_session.h:188: // If false,
net::BidirectionalStreamQuicImpl will not be used when
On 2016/03/15 19:51:10, mef wrote:
> I think it should say 'If true'...

Done.

https://codereview.chromium.org/1796253002/diff/40001/net/http/http_network_s...
net/http/http_network_session.h:190: bool
disable_bidirectional_stream_quic_impl;
On 2016/03/15 19:51:10, mef wrote:
> should we call it 'quic_disable_bidirectional_stream'?

I am not sure. It isn't a feature of QUIC (but a flag on the bidi wrapper on top
of QUIC). I am not sure if it should be lumped together with other quic feature
flags above.

[Ryan Hamilton, 2016-03-15 22:41:45]

https://codereview.chromium.org/1796253002/diff/40001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1796253002/diff/40001/net/http/http_network_s...
net/http/http_network_session.h:188: // If false,
net::BidirectionalStreamQuicImpl will not be used when
On 2016/03/15 20:15:20, xunjieli wrote:
> On 2016/03/15 19:51:10, mef wrote:
> > I think it should say 'If true'...
> 
> Done.

I think a better comment might be:

// If true, then bidirectional stream over QUIC will be disabled.

https://codereview.chromium.org/1796253002/diff/40001/net/http/http_network_s...
net/http/http_network_session.h:190: bool
disable_bidirectional_stream_quic_impl;
On 2016/03/15 20:15:20, xunjieli wrote:
> On 2016/03/15 19:51:10, mef wrote:
> > should we call it 'quic_disable_bidirectional_stream'?
> 
> I am not sure. It isn't a feature of QUIC (but a flag on the bidi wrapper on
top
> of QUIC). I am not sure if it should be lumped together with other quic
feature
> flags above.

I like mef's name.

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

https://codereview.chromium.org/1796253002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:192: return AlternativeService();
This also disable non-QUIC based AlternativeServices which doesn't sound right.
I would move this logic down to, say, line 256 with the other check which skip
QUIC.

https://codereview.chromium.org/1796253002/diff/60001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1796253002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_unittest.cc:1697:
Initialize(/*disable_bidirectional_stream_quic_impl=*/true);
Instead of adding a new arg to Initialized, how about just doing:

params_.disable_bidirectional_stream_quic_impl = true;

here?

[xunjieli, 2016-03-16 15:47:04]

Thanks a lot for the review. PTAL.

https://codereview.chromium.org/1796253002/diff/40001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1796253002/diff/40001/net/http/http_network_s...
net/http/http_network_session.h:188: // If false,
net::BidirectionalStreamQuicImpl will not be used when
On 2016/03/15 22:41:44, Ryan Hamilton wrote:
> On 2016/03/15 20:15:20, xunjieli wrote:
> > On 2016/03/15 19:51:10, mef wrote:
> > > I think it should say 'If true'...
> > 
> > Done.
> 
> I think a better comment might be:
> 
> // If true, then bidirectional stream over QUIC will be disabled. 

Done.

https://codereview.chromium.org/1796253002/diff/40001/net/http/http_network_s...
net/http/http_network_session.h:190: bool
disable_bidirectional_stream_quic_impl;
On 2016/03/15 22:41:44, Ryan Hamilton wrote:
> On 2016/03/15 20:15:20, xunjieli wrote:
> > On 2016/03/15 19:51:10, mef wrote:
> > > should we call it 'quic_disable_bidirectional_stream'?
> > 
> > I am not sure. It isn't a feature of QUIC (but a flag on the bidi wrapper on
> top
> > of QUIC). I am not sure if it should be lumped together with other quic
> feature
> > flags above.
> 
> I like mef's name.

Done. Great!

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

https://codereview.chromium.org/1796253002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl.cc:192: return AlternativeService();
On 2016/03/15 22:41:45, Ryan Hamilton wrote:
> This also disable non-QUIC based AlternativeServices which doesn't sound
right.
> I would move this logic down to, say, line 256 with the other check which skip
> QUIC.

Done.

https://codereview.chromium.org/1796253002/diff/60001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1796253002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_unittest.cc:1697:
Initialize(/*disable_bidirectional_stream_quic_impl=*/true);
On 2016/03/15 22:41:45, Ryan Hamilton wrote:
> Instead of adding a new arg to Initialized, how about just doing:
> 
> params_.disable_bidirectional_stream_quic_impl = true;
> 
> here?

Partially done. |params_| is private. I try not to make it protected. Since this
test class is only used for BidirectionalStream, I have added another method to
set the param.

[Ryan Hamilton, 2016-03-16 20:33:55]

LGTM, modulo a rename I should have caught the first time. Sorry!

https://codereview.chromium.org/1796253002/diff/60001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1796253002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_unittest.cc:1697:
Initialize(/*disable_bidirectional_stream_quic_impl=*/true);
On 2016/03/16 15:47:04, xunjieli wrote:
> On 2016/03/15 22:41:45, Ryan Hamilton wrote:
> > Instead of adding a new arg to Initialized, how about just doing:
> > 
> > params_.disable_bidirectional_stream_quic_impl = true;
> > 
> > here?
> 
> Partially done. |params_| is private. I try not to make it protected. Since
this
> test class is only used for BidirectionalStream, I have added another method
to
> set the param.

That works too. FWIW in tests, making members protected is totally fine. But
your approach is good!

https://codereview.chromium.org/1796253002/diff/80001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1796253002/diff/80001/net/http/http_network_s...
net/http/http_network_session.h:188: // If true, bidirectional stream over QUIC
will be disabled.
Gah! I should have noticed this in the previous review. But in the comment and
the variable, this should be "streams" not "stream" to be slightly more
readable. Sorry for the run-around.

[xunjieli, 2016-03-16 20:45:02]

Thanks for the review.

https://codereview.chromium.org/1796253002/diff/60001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1796253002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_unittest.cc:1697:
Initialize(/*disable_bidirectional_stream_quic_impl=*/true);
On 2016/03/16 20:33:55, Ryan Hamilton wrote:
> On 2016/03/16 15:47:04, xunjieli wrote:
> > On 2016/03/15 22:41:45, Ryan Hamilton wrote:
> > > Instead of adding a new arg to Initialized, how about just doing:
> > > 
> > > params_.disable_bidirectional_stream_quic_impl = true;
> > > 
> > > here?
> > 
> > Partially done. |params_| is private. I try not to make it protected. Since
> this
> > test class is only used for BidirectionalStream, I have added another method
> to
> > set the param.
> 
> That works too. FWIW in tests, making members protected is totally fine. But
> your approach is good!

Good to know! Matt advised me to keep members private in tests, so I've been
trying to do that, unless it's getting a bit ugly.

https://codereview.chromium.org/1796253002/diff/80001/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1796253002/diff/80001/net/http/http_network_s...
net/http/http_network_session.h:188: // If true, bidirectional stream over QUIC
will be disabled.
On 2016/03/16 20:33:55, Ryan Hamilton wrote:
> Gah! I should have noticed this in the previous review. But in the comment and
> the variable, this should be "streams" not "stream" to be slightly more
> readable. Sorry for the run-around.

Done. no problem at all! We should get this right. Readability is very important
:)

[Ryan Hamilton, 2016-03-16 20:47:10]

https://codereview.chromium.org/1796253002/diff/60001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1796253002/diff/60001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_unittest.cc:1697:
Initialize(/*disable_bidirectional_stream_quic_impl=*/true);
On 2016/03/16 20:45:01, xunjieli wrote:
> On 2016/03/16 20:33:55, Ryan Hamilton wrote:
> > On 2016/03/16 15:47:04, xunjieli wrote:
> > > On 2016/03/15 22:41:45, Ryan Hamilton wrote:
> > > > Instead of adding a new arg to Initialized, how about just doing:
> > > > 
> > > > params_.disable_bidirectional_stream_quic_impl = true;
> > > > 
> > > > here?
> > > 
> > > Partially done. |params_| is private. I try not to make it protected.
Since
> > this
> > > test class is only used for BidirectionalStream, I have added another
method
> > to
> > > set the param.
> > 
> > That works too. FWIW in tests, making members protected is totally fine. But
> > your approach is good!
> 
> Good to know! Matt advised me to keep members private in tests, so I've been
> trying to do that, unless it's getting a bit ugly.

I am happy to defer to Matt in all things! :>

https://codereview.chromium.org/1796253002/diff/100001/net/http/http_network_...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1796253002/diff/100001/net/http/http_network_...
net/http/http_network_session.h:189: bool quic_disable_bidirectional_stream;
Need to rename the variable, right?

[xunjieli, 2016-03-16 20:57:27]

Thanks!

https://codereview.chromium.org/1796253002/diff/100001/net/http/http_network_...
File net/http/http_network_session.h (right):

https://codereview.chromium.org/1796253002/diff/100001/net/http/http_network_...
net/http/http_network_session.h:189: bool quic_disable_bidirectional_stream;
On 2016/03/16 20:47:10, Ryan Hamilton wrote:
> Need to rename the variable, right?

Done. Sorry, missed a part of your comment.

[Ryan Hamilton, 2016-03-16 21:20:38]

lgtm!

[mef, 2016-03-18 15:24:28]

lgtm

[mef, 2016-03-18 15:25:26]

On 2016/03/18 15:24:28, mef wrote:
> lgtm

Oh, mod typo in description: net::BidirectionalStreamQuicImp

[mef, 2016-03-18 15:25:28]

On 2016/03/18 15:24:28, mef wrote:
> lgtm

Oh, mod typo in description: net::BidirectionalStreamQuicImp

[xunjieli, 2016-03-18 15:26:43]

Description was changed from

==========
Add an option to disable net::BidirectionalStreamQuicImp

This CL adds an option to disable 
net::BidirectionalStreamQuicImpl in net::HttpNetworkSession.
When the flag is set to true, alternative service job will
not be negotiated.

BUG=584338
==========

to

==========
Add an option to disable net::BidirectionalStreamQuicImpl

This CL adds an option to disable 
net::BidirectionalStreamQuicImpl in net::HttpNetworkSession.
When the flag is set to true, alternative service job will
not be negotiated.

BUG=584338
==========

[xunjieli, 2016-03-18 15:26:54]

On 2016/03/18 15:25:28, mef wrote:
> On 2016/03/18 15:24:28, mef wrote:
> > lgtm
> 
> Oh, mod typo in description: net::BidirectionalStreamQuicImp

Done. thanks!

[xunjieli, 2016-03-18 18:37:00]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-03-18 18:37:01]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org,
rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1796253002/#ps140001
(title: "Rebased")

[commit-bot: I haz the power, 2016-03-18 18:37:41]

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

[commit-bot: I haz the power, 2016-03-18 21:05:17]

Description was changed from

==========
Add an option to disable net::BidirectionalStreamQuicImpl

This CL adds an option to disable 
net::BidirectionalStreamQuicImpl in net::HttpNetworkSession.
When the flag is set to true, alternative service job will
not be negotiated.

BUG=584338
==========

to

==========
Add an option to disable net::BidirectionalStreamQuicImpl

This CL adds an option to disable 
net::BidirectionalStreamQuicImpl in net::HttpNetworkSession.
When the flag is set to true, alternative service job will
not be negotiated.

BUG=584338
==========

[commit-bot: I haz the power, 2016-03-18 21:05:18]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-03-18 21:06:31]

Description was changed from

==========
Add an option to disable net::BidirectionalStreamQuicImpl

This CL adds an option to disable 
net::BidirectionalStreamQuicImpl in net::HttpNetworkSession.
When the flag is set to true, alternative service job will
not be negotiated.

BUG=584338
==========

to

==========
Add an option to disable net::BidirectionalStreamQuicImpl

This CL adds an option to disable
net::BidirectionalStreamQuicImpl in net::HttpNetworkSession.
When the flag is set to true, alternative service job will
not be negotiated.

BUG=584338

Committed: https://crrev.com/888c2992e286221aaf7bac8b08c8b8a0399d06bc
Cr-Commit-Position: refs/heads/master@{#382084}
==========

[commit-bot: I haz the power, 2016-03-18 21:06:32]

Patchset 6 (id:??) landed as
https://crrev.com/888c2992e286221aaf7bac8b08c8b8a0399d06bc
Cr-Commit-Position: refs/heads/master@{#382084}