QUIC - Disable Preconnect when QUIC can be spoken to a server with 0RTT

QUIC - Disable Preconnect when QUIC can be spoken to a server with 0RTT
handshake.

+ Intend to disable Preconnects via an experiment.
+ Preconnects are not disabled by default.
+ Moved QuicStreamFactoryPeer code into a separate file.
+ Added utility code for tests to populate a dummy server config into the
cache.
+ Added unittests to test disabling of Preconnects and
  ZeroRTT test.

R=rch@chromium.org

Committed: https://crrev.com/d2e74caa01ed529d7079dbc4c9fb488463fb5a81
Cr-Commit-Position: refs/heads/master@{#363864}

[ramant (doing other things), 2015-12-04 06:57:09]

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

[commit-bot: I haz the power, 2015-12-04 06:57:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1502453003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1502453003/1

[commit-bot: I haz the power, 2015-12-04 08:51:39]

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

[commit-bot: I haz the power, 2015-12-04 08:51:39]

Dry run: This issue passed the CQ dry run.

[ramant (doing other things), 2015-12-04 18:13:01]

Patchset #2 (id:20001) has been deleted

[ramant (doing other things), 2015-12-04 18:23:49]

Patchset #2 (id:40001) has been deleted

[ramant (doing other things), 2015-12-04 18:28:45]

Patchset #2 (id:60001) has been deleted

[ramant (doing other things), 2015-12-04 18:40:32]

Patchset #2 (id:80001) has been deleted

[ramant (doing other things), 2015-12-04 18:49:15]

Patchset #2 (id:100001) has been deleted

[ramant (doing other things), 2015-12-04 18:49:22]

Patchset #2 (id:120001) has been deleted

[ramant (doing other things), 2015-12-04 18:53:03]

Patchset #2 (id:140001) has been deleted

[ramant (doing other things), 2015-12-04 18:55:26]

Description was changed from

==========
QUIC - Disable Preconnect when QUIC can be spoken to a server with 0RTT
handshake. Preconnects are not disabled by default.

Intend to disable Preconnects via an experiment.

Moved QuicStreamFactoryPeer code into a separate file.

Added utility code for tests to populate a dummy server config into the
cache.

Added unittests to test disabling of Preconnects.
==========

to

==========
QUIC - Disable Preconnect when QUIC can be spoken to a server with 0RTT
handshake.

+ Intend to disable Preconnects via an experiment. 
+ Preconnects are not disabled by default.
+ Moved QuicStreamFactoryPeer code into a separate file.
+ Added utility code for tests to populate a dummy server config into the
cache.
+ Added unittests to test disabling of Preconnects and
  ZeroRTT test.

R=rch@chromium.org
==========

[ramant (doing other things), 2015-12-04 18:55:27]

rtenneti@chromium.org changed reviewers:
+ rch@chromium.org

[ramant (doing other things), 2015-12-04 18:56:51]

Hi Ryan and Ian,
  This is a first cut at disabling Preconnects. Most of the changes are for
unittests. PTAL.

thanks
raman

[ramant (doing other things), 2015-12-04 18:56:58]

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

[commit-bot: I haz the power, 2015-12-04 18:58:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1502453003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1502453003/160001

[commit-bot: I haz the power, 2015-12-04 20:09:52]

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

[commit-bot: I haz the power, 2015-12-04 20:09:53]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2015-12-05 04:16:18]

Thanks for doing this. I assume you intend to wire this up via finch?

Just a few nits, but LGTM! otherwise.

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

https://codereview.chromium.org/1502453003/diff/160001/net/http/http_network_...
net/http/http_network_session.h:179: bool disable_preconnects;
I would suggest something like:

bool quic_disable_preconnect_if_0rtt.

https://codereview.chromium.org/1502453003/diff/160001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1502453003/diff/160001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:753: TEST_P(HttpStreamFactoryTest,
PreconnectDisabled) {
It'd be good to have a test where preconnect works when QUIC can not do 0-rtt.

https://codereview.chromium.org/1502453003/diff/160001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:793: request.url =
GURL("https://www.google.com:443");
nit: do you need :443?

https://codereview.chromium.org/1502453003/diff/160001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1502453003/diff/160001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:691: host_port_pair, privacy_mode)));
Is this indented correctly?

https://codereview.chromium.org/1502453003/diff/160001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1502453003/diff/160001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:231: bool IsQuicDoingZeroRTT(const HostPortPair&
host_port_pair,
nit: I suggest a name like SupportsZeroRTT or ZeroRTTEnabledFor()

I think this should take QuicServerId, just to be consistent.

[ramant (doing other things), 2015-12-07 20:04:49]

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

[ramant (doing other things), 2015-12-07 20:05:39]

Thanks very much Ryan for the comments (sorry for the delay in fixing, wasn't
100% healthy). PTAL.

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

https://codereview.chromium.org/1502453003/diff/160001/net/http/http_network_...
net/http/http_network_session.h:179: bool disable_preconnects;
On 2015/12/05 04:16:17, Ryan Hamilton wrote:
> I would suggest something like:
> 
> bool quic_disable_preconnect_if_0rtt.

Done.

https://codereview.chromium.org/1502453003/diff/160001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1502453003/diff/160001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:753: TEST_P(HttpStreamFactoryTest,
PreconnectDisabled) {
On 2015/12/05 04:16:17, Ryan Hamilton wrote:
> It'd be good to have a test where preconnect works when QUIC can not do 0-rtt.

Done.

https://codereview.chromium.org/1502453003/diff/160001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:793: request.url =
GURL("https://www.google.com:443");
On 2015/12/05 04:16:17, Ryan Hamilton wrote:
> nit: do you need :443?

Done.

https://codereview.chromium.org/1502453003/diff/160001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1502453003/diff/160001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:691: host_port_pair, privacy_mode)));
On 2015/12/05 04:16:17, Ryan Hamilton wrote:
> Is this indented correctly?

Done.

https://codereview.chromium.org/1502453003/diff/160001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1502453003/diff/160001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:231: bool IsQuicDoingZeroRTT(const HostPortPair&
host_port_pair,
On 2015/12/05 04:16:17, Ryan Hamilton wrote:
> nit: I suggest a name like SupportsZeroRTT or ZeroRTTEnabledFor()
> 
> I think this should take QuicServerId, just to be consistent.

Done.

[commit-bot: I haz the power, 2015-12-07 20:10:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1502453003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1502453003/180001

[ramant (doing other things), 2015-12-07 20:13:11]

https://codereview.chromium.org/1502453003/diff/180001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1502453003/diff/180001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:754: TEST_P(HttpStreamFactoryTest,
QuicDisablePreConnectIfNoZeroRTT) {
nit: I am not very happy with this name. Should we call this
QuicDontDisablePreConnectIfNoZeroRTT? thanks

[commit-bot: I haz the power, 2015-12-07 22:51:00]

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

[commit-bot: I haz the power, 2015-12-07 22:51:00]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2015-12-08 20:49:45]

lgtm

https://codereview.chromium.org/1502453003/diff/180001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1502453003/diff/180001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:754: TEST_P(HttpStreamFactoryTest,
QuicDisablePreConnectIfNoZeroRTT) {
On 2015/12/07 20:13:11, ramant wrote:
> nit: I am not very happy with this name. Should we call this
> QuicDontDisablePreConnectIfNoZeroRTT? thanks

Sure. Or UsePreconnectIfNoZeroRTT?

[ramant (doing other things), 2015-12-08 22:24:39]

Thanks much Ryan
raman

https://codereview.chromium.org/1502453003/diff/180001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_unittest.cc (right):

https://codereview.chromium.org/1502453003/diff/180001/net/http/http_stream_f...
net/http/http_stream_factory_impl_unittest.cc:754: TEST_P(HttpStreamFactoryTest,
QuicDisablePreConnectIfNoZeroRTT) {
On 2015/12/08 20:49:45, Ryan Hamilton wrote:
> On 2015/12/07 20:13:11, ramant wrote:
> > nit: I am not very happy with this name. Should we call this
> > QuicDontDisablePreConnectIfNoZeroRTT? thanks
> 
> Sure. Or UsePreconnectIfNoZeroRTT?

Done.

[ramant (doing other things), 2015-12-08 22:24:44]

The CQ bit was checked by rtenneti@chromium.org

[ramant (doing other things), 2015-12-08 22:24:45]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1502453003/#ps200001
(title: "Fix test name")

[commit-bot: I haz the power, 2015-12-08 22:27:07]

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

[commit-bot: I haz the power, 2015-12-09 00:52:03]

Description was changed from

==========
QUIC - Disable Preconnect when QUIC can be spoken to a server with 0RTT
handshake.

+ Intend to disable Preconnects via an experiment. 
+ Preconnects are not disabled by default.
+ Moved QuicStreamFactoryPeer code into a separate file.
+ Added utility code for tests to populate a dummy server config into the
cache.
+ Added unittests to test disabling of Preconnects and
  ZeroRTT test.

R=rch@chromium.org
==========

to

==========
QUIC - Disable Preconnect when QUIC can be spoken to a server with 0RTT
handshake.

+ Intend to disable Preconnects via an experiment. 
+ Preconnects are not disabled by default.
+ Moved QuicStreamFactoryPeer code into a separate file.
+ Added utility code for tests to populate a dummy server config into the
cache.
+ Added unittests to test disabling of Preconnects and
  ZeroRTT test.

R=rch@chromium.org
==========

[commit-bot: I haz the power, 2015-12-09 00:52:04]

Committed patchset #4 (id:200001)

[commit-bot: I haz the power, 2015-12-09 00:53:39]

Description was changed from

==========
QUIC - Disable Preconnect when QUIC can be spoken to a server with 0RTT
handshake.

+ Intend to disable Preconnects via an experiment. 
+ Preconnects are not disabled by default.
+ Moved QuicStreamFactoryPeer code into a separate file.
+ Added utility code for tests to populate a dummy server config into the
cache.
+ Added unittests to test disabling of Preconnects and
  ZeroRTT test.

R=rch@chromium.org
==========

to

==========
QUIC - Disable Preconnect when QUIC can be spoken to a server with 0RTT
handshake.

+ Intend to disable Preconnects via an experiment.
+ Preconnects are not disabled by default.
+ Moved QuicStreamFactoryPeer code into a separate file.
+ Added utility code for tests to populate a dummy server config into the
cache.
+ Added unittests to test disabling of Preconnects and
  ZeroRTT test.

R=rch@chromium.org

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

[commit-bot: I haz the power, 2015-12-09 00:53:40]

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