[Cronet] Initialize disable_quic_bidirectional_streams and QUIC protocol

[Cronet] Initialize disable_quic_bidirectional_streams and QUIC protocol

BUG=596680
Committed: https://crrev.com/bf0e46a7433eea99e1f0dc4f11cd1e950a786570
Cr-Commit-Position: refs/heads/master@{#383838}

[xunjieli, 2016-03-28 19:13:31]

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

[mef, 2016-03-28 19:22:42]

lgtm

https://codereview.chromium.org/1836983002/diff/1/net/url_request/url_request...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/1836983002/diff/1/net/url_request/url_request...
net/url_request/url_request_context_builder.cc:195:
quic_disable_bidirectional_streams(false) {}
I think we should also initialize proxy_delegate and enable_token_binding while
we are here.

[xunjieli, 2016-03-28 19:57:19]

Thanks, PTAL at Patch 2. I added two tests.

https://codereview.chromium.org/1836983002/diff/1/net/url_request/url_request...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/1836983002/diff/1/net/url_request/url_request...
net/url_request/url_request_context_builder.cc:195:
quic_disable_bidirectional_streams(false) {}
On 2016/03/28 19:22:42, mef wrote:
> I think we should also initialize proxy_delegate and enable_token_binding
while
> we are here.

Maybe I am missing something, but these two aren't part of
HttpNetworkSessionParams struct.

[mef, 2016-03-28 20:17:33]

https://codereview.chromium.org/1836983002/diff/1/net/url_request/url_request...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/1836983002/diff/1/net/url_request/url_request...
net/url_request/url_request_context_builder.cc:195:
quic_disable_bidirectional_streams(false) {}
On 2016/03/28 19:57:19, xunjieli wrote:
> On 2016/03/28 19:22:42, mef wrote:
> > I think we should also initialize proxy_delegate and enable_token_binding
> while
> > we are here.
> 
> Maybe I am missing something, but these two aren't part of
> HttpNetworkSessionParams struct.

Oops, I was looking at HttpNetworkSession::Params.

https://codereview.chromium.org/1836983002/diff/20001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
(right):

https://codereview.chromium.org/1836983002/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:32:
if (enabled == QuicBidirectionalStreams.DISABLED) {
nits: The expression 'enabled == xyz.DISABLED' seems weird.

Also, setUp() is mentally associated with standard (implicit) setUp / tearDown,
but in this test it must be called directly.

Maybe this method should remain a 'setUp' without 'enabled' flag and
testQuicBidirectionalStreamDisabled should create another engine with the
'quic_disable_bidirectional_streams' flag set?

[xunjieli, 2016-03-28 20:25:51]

https://codereview.chromium.org/1836983002/diff/20001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
(right):

https://codereview.chromium.org/1836983002/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:32:
if (enabled == QuicBidirectionalStreams.DISABLED) {
On 2016/03/28 20:17:33, mef wrote:
> nits: The expression 'enabled == xyz.DISABLED' seems weird.

I can s/enabled/type if that makes it less weird? or anything you suggest.

> 
> Also, setUp() is mentally associated with standard (implicit) setUp /
tearDown,
> but in this test it must be called directly.
> 
> Maybe this method should remain a 'setUp' without 'enabled' flag and
> testQuicBidirectionalStreamDisabled should create another engine with the
> 'quic_disable_bidirectional_streams' flag set?

I tried to not duplicate the logic. We have an existing usage of private setUp
in SdchTest.java. I am adding more tests where the initial config is slightly
different, which require a private setUp method with those params passed in.
Having multiple CronetEngines in one unit test often complicates the logic and
obfuscates which engine is under test.

[xunjieli, 2016-03-29 17:11:47]

Misha: a friendly ping?

[mef, 2016-03-29 20:33:35]

lgtm

https://codereview.chromium.org/1836983002/diff/20001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
(right):

https://codereview.chromium.org/1836983002/diff/20001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:32:
if (enabled == QuicBidirectionalStreams.DISABLED) {
On 2016/03/28 20:25:51, xunjieli wrote:
> On 2016/03/28 20:17:33, mef wrote:
> > nits: The expression 'enabled == xyz.DISABLED' seems weird.
> 
> I can s/enabled/type if that makes it less weird? or anything you suggest.
> 
> > 
> > Also, setUp() is mentally associated with standard (implicit) setUp /
> tearDown,
> > but in this test it must be called directly.
> > 
> > Maybe this method should remain a 'setUp' without 'enabled' flag and
> > testQuicBidirectionalStreamDisabled should create another engine with the
> > 'quic_disable_bidirectional_streams' flag set?
> 
> I tried to not duplicate the logic. We have an existing usage of private setUp
> in SdchTest.java. I am adding more tests where the initial config is slightly
> different, which require a private setUp method with those params passed in.
> Having multiple CronetEngines in one unit test often complicates the logic and
> obfuscates which engine is under test.

Acknowledged.

[xunjieli, 2016-03-29 20:34:48]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2016-03-29 20:35:26]

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

[commit-bot: I haz the power, 2016-03-29 21:50:37]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-29 21:51:38]

Description was changed from

==========
[Cronet] Initialize disable_quic_bidirectional_streams and QUIC protocol

BUG=596680
==========

to

==========
[Cronet] Initialize disable_quic_bidirectional_streams and QUIC protocol

BUG=596680

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

[commit-bot: I haz the power, 2016-03-29 21:51:39]

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