Add a QUIC proxy server to the list of QUIC servers supported at start up

Add a QUIC proxy server to the list of QUIC servers supported at start up

Data reduction proxy server is added to the list of QUIC
servers supported at start up. This will enable 0-RTT
requests for the first request to the data reduction QUIC
proxy (provided Chromium has used QUIC in the past to
connect to the data reduction proxy).

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet

BUG=658432
Committed: https://crrev.com/3b96695ae3cd9d92050740d6741ffae04f5c4cb7
Cr-Commit-Position: refs/heads/master@{#427531}

[tbansal1, 2016-10-21 22:14:06]

Description was changed from

==========
Add a QUIC proxy server to the list of proxy servers supported at start up

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

to

==========
Add a QUIC proxy server to the list of QUIC servers supported at start up

Data reduction proxy server is added to the list of QUIC
servers supported at start up. This will enable 0-RTT
requests for the first request to the data reduction QUIC 
proxy (provided Chromium has used QUIC in the past to connect 
to the data reduction proxy).

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

[tbansal1, 2016-10-21 22:14:28]

tbansal@chromium.org changed reviewers:
+ ryansturm@chromium.org

[tbansal1, 2016-10-21 22:14:29]

ryansturm: ptal. Thanks.

[tbansal1, 2016-10-21 22:28:27]

Description was changed from

==========
Add a QUIC proxy server to the list of QUIC servers supported at start up

Data reduction proxy server is added to the list of QUIC
servers supported at start up. This will enable 0-RTT
requests for the first request to the data reduction QUIC 
proxy (provided Chromium has used QUIC in the past to connect 
to the data reduction proxy).

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

to

==========
Add a QUIC proxy server to the list of QUIC servers supported at start up

Data reduction proxy server is added to the list of QUIC
servers supported at start up. This will enable 0-RTT
requests for the first request to the data reduction QUIC 
proxy (provided Chromium has used QUIC in the past to 
connect to the data reduction proxy).

BUG=658432
==========

[RyanSturm, 2016-10-21 22:35:10]

I haven't looked at tests, but the prod code seems reasonable. Here's a couple
of comments since I won't get to look at this in detail until next week.

https://codereview.chromium.org/2442633002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2442633002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:183:
void DataReductionProxyDelegate::RecordQuicProxyAtStartupStatus(
Make this an anonymous namespace method

https://codereview.chromium.org/2442633002/diff/20001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory.cc (right):

https://codereview.chromium.org/2442633002/diff/20001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory.cc:1830:
proxy_delegate_->GetQuicSupportedProxyServerAtStartUp().is_quic()) {
Can you only call GetQuicSupportedServerAtStartUp one time and just store it in
a temp variable?

[tbansal1, 2016-10-21 23:16:09]

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

[commit-bot: I haz the power, 2016-10-21 23:16:51]

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-22 02:29:25]

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

[commit-bot: I haz the power, 2016-10-22 02:29:26]

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

[tbansal1, 2016-10-24 17:06:36]

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

[tbansal1, 2016-10-24 17:06:53]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-10-24 17:07:12]

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

[tbansal1, 2016-10-24 17:09:14]

tbansal@chromium.org changed reviewers:
+ rkaplow@chromium.org, zhongyi@chromium.org

[tbansal1, 2016-10-24 17:09:15]

ryansturm: ptal.
zhongyi: ptal at //net.
rkaplow: ptal at histograms.xml.
Thanks.

https://codereview.chromium.org/2442633002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2442633002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:183:
void DataReductionProxyDelegate::RecordQuicProxyAtStartupStatus(
On 2016/10/21 22:35:10, Ryan Sturm wrote:
> Make this an anonymous namespace method

Then I will need to make enum  QuicProxyAtStartup as public, which I would
prefer not to do.

[RyanSturm, 2016-10-24 18:01:07]

lgtm % nits

https://codereview.chromium.org/2442633002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2442633002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:183:
void DataReductionProxyDelegate::RecordQuicProxyAtStartupStatus(
On 2016/10/24 17:09:15, tbansal1 wrote:
> On 2016/10/21 22:35:10, Ryan Sturm wrote:
> > Make this an anonymous namespace method
> 
> Then I will need to make enum  QuicProxyAtStartup as public, which I would
> prefer not to do.

Acknowledged.

https://codereview.chromium.org/2442633002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2442633002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:158:
net::HostPortPair("proxy.googlezip.net", 443));
nit: is it worth declaring "proxy.googlezip.net" at the top since it is used in
two places in this file?

https://codereview.chromium.org/2442633002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2442633002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:77991: +      label="No suppotable QUIC
proxy was available in the list of data
s/suppotable/supportable/

[tbansal1, 2016-10-24 18:35:49]

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

[tbansal1, 2016-10-24 18:36:07]

zhongyi, rkapolow: ptal.
Thanks.

https://codereview.chromium.org/2442633002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2442633002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:158:
net::HostPortPair("proxy.googlezip.net", 443));
On 2016/10/24 18:01:07, Ryan Sturm wrote:
> nit: is it worth declaring "proxy.googlezip.net" at the top since it is used
in
> two places in this file?

Done.

https://codereview.chromium.org/2442633002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2442633002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:77991: +      label="No suppotable QUIC
proxy was available in the list of data
On 2016/10/24 18:01:07, Ryan Sturm wrote:
> s/suppotable/supportable/

Done.

[commit-bot: I haz the power, 2016-10-24 18:36:30]

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

[rkaplow, 2016-10-24 18:41:58]

lgtm

[Zhongyi Shi, 2016-10-24 21:02:39]

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

[Zhongyi Shi, 2016-10-24 21:02:40]

Generally looks good to me, but you'll need rch@ to give a final review!

https://codereview.chromium.org/2442633002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/2442633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory_test.cc:632: }
nit: is that possible to move this setting up to line 537 so that the code is
more clean and readable matching the comments on line 503?

[commit-bot: I haz the power, 2016-10-24 21:08:01]

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

[commit-bot: I haz the power, 2016-10-24 21:08:02]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[tbansal1, 2016-10-24 21:10:12]

rch: ptal at //net/. Thanks.

https://codereview.chromium.org/2442633002/diff/80001/net/quic/chromium/quic_...
File net/quic/chromium/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/2442633002/diff/80001/net/quic/chromium/quic_...
net/quic/chromium/quic_stream_factory_test.cc:632: }
On 2016/10/24 21:02:40, Zhongyi Shi wrote:
> nit: is that possible to move this setting up to line 537 so that the code is
> more clean and readable matching the comments on line 503?

Done.

[tbansal1, 2016-10-24 21:10:15]

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

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

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

[Zhongyi Shi, 2016-10-24 21:13:30]

lgtm

[commit-bot: I haz the power, 2016-10-24 21:22:48]

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

[commit-bot: I haz the power, 2016-10-24 21:22:49]

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...)

[tbansal1, 2016-10-25 06:16:01]

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

[commit-bot: I haz the power, 2016-10-25 06:16:10]

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

[tbansal1, 2016-10-25 06:16:43]

Patchset #6 (id:120001) has been deleted

[tbansal1, 2016-10-25 06:23:02]

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

[commit-bot: I haz the power, 2016-10-25 06:23:13]

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

[tbansal1, 2016-10-25 06:23:14]

Patchset #6 (id:140001) has been deleted

[tbansal1, 2016-10-25 06:24:15]

Description was changed from

==========
Add a QUIC proxy server to the list of QUIC servers supported at start up

Data reduction proxy server is added to the list of QUIC
servers supported at start up. This will enable 0-RTT
requests for the first request to the data reduction QUIC 
proxy (provided Chromium has used QUIC in the past to 
connect to the data reduction proxy).

BUG=658432
==========

to

==========
Add a QUIC proxy server to the list of QUIC servers supported at start up

Data reduction proxy server is added to the list of QUIC
servers supported at start up. This will enable 0-RTT
requests for the first request to the data reduction QUIC 
proxy (provided Chromium has used QUIC in the past to 
connect to the data reduction proxy).

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet

BUG=658432
==========

[tbansal1, 2016-10-25 06:24:45]

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

[commit-bot: I haz the power, 2016-10-25 06:24:56]

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-25 06:24:57]

There were warnings when CQ was processing your CL:
 * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag
request. That bot was already specified

[commit-bot: I haz the power, 2016-10-25 06:31:32]

There were warnings when CQ was processing your CL:
 * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag
request. That bot was already specified

[commit-bot: I haz the power, 2016-10-25 07:01:30]

There were warnings when CQ was processing your CL:
 * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag
request. That bot was already specified

[commit-bot: I haz the power, 2016-10-25 07:31:24]

There were warnings when CQ was processing your CL:
 * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag
request. That bot was already specified

[commit-bot: I haz the power, 2016-10-25 07:39:25]

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

[commit-bot: I haz the power, 2016-10-25 07:39:26]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-10-25 15:56:39]

rch: ptal. I would like to submit this soo so it gets into the dev release
(expected release: tomorrow).

[Ryan Hamilton, 2016-10-25 19:20:15]

LGTM, modulo a rename.

https://codereview.chromium.org/2442633002/diff/160001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/2442633002/diff/160001/net/base/proxy_delegat...
net/base/proxy_delegate.h:89: virtual ProxyServer
GetQuicSupportedProxyServerAtStartUp() const = 0;
Since this just returns a ProxyServer which may or may not be QUIC (although we
expect it to be QUIC when we use it) I'd rename this something like:

virtual ProxyServer GetDefaultAlternativeProxy() const = 0;

(I'd also drop "AtStartup()" 'cause there's nothing to prevent this method from
being called later).

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

https://codereview.chromium.org/2442633002/diff/160001/net/quic/chromium/quic...
net/quic/chromium/quic_stream_factory.cc:1828:
quic_supported_proxy_server.is_quic());
If you take my suggestion, I'd nuke this DCHECK.

[tbansal1, 2016-10-25 20:46:02]

Patchset #7 (id:180001) has been deleted

[tbansal1, 2016-10-25 20:46:09]

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

[commit-bot: I haz the power, 2016-10-25 20:46:49]

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-25 20:46:51]

There were warnings when CQ was processing your CL:
 * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag
request. That bot was already specified

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

There were warnings when CQ was processing your CL:
 * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag
request. That bot was already specified

[tbansal1, 2016-10-25 21:26:15]

Patchset #7 (id:200001) has been deleted

[tbansal1, 2016-10-25 21:27:07]

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

[tbansal1, 2016-10-25 21:27:17]

Patchset #7 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-10-25 21:27:47]

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-25 21:27:48]

There were warnings when CQ was processing your CL:
 * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag
request. That bot was already specified

[commit-bot: I haz the power, 2016-10-25 21:32:18]

There were warnings when CQ was processing your CL:
 * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag
request. That bot was already specified

[tbansal1, 2016-10-25 21:47:17]

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

[tbansal1, 2016-10-25 21:47:19]

Patchset #7 (id:240001) has been deleted

[tbansal1, 2016-10-25 21:47:26]

https://codereview.chromium.org/2442633002/diff/160001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/2442633002/diff/160001/net/base/proxy_delegat...
net/base/proxy_delegate.h:89: virtual ProxyServer
GetQuicSupportedProxyServerAtStartUp() const = 0;
On 2016/10/25 19:20:15, Ryan Hamilton wrote:
> Since this just returns a ProxyServer which may or may not be QUIC (although
we
> expect it to be QUIC when we use it) I'd rename this something like:
> 
> virtual ProxyServer GetDefaultAlternativeProxy() const = 0;
> 
> (I'd also drop "AtStartup()" 'cause there's nothing to prevent this method
from
> being called later).

Done.

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

https://codereview.chromium.org/2442633002/diff/160001/net/quic/chromium/quic...
net/quic/chromium/quic_stream_factory.cc:1828:
quic_supported_proxy_server.is_quic());
On 2016/10/25 19:20:15, Ryan Hamilton wrote:
> If you take my suggestion, I'd nuke this DCHECK.

Done.

[commit-bot: I haz the power, 2016-10-25 21:47:35]

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-25 21:47:37]

There were warnings when CQ was processing your CL:
 * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag
request. That bot was already specified

[commit-bot: I haz the power, 2016-10-25 22:02:16]

There were warnings when CQ was processing your CL:
 * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag
request. That bot was already specified

[Ryan Hamilton, 2016-10-25 22:18:52]

lgtm

[commit-bot: I haz the power, 2016-10-25 22:32:14]

There were warnings when CQ was processing your CL:
 * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag
request. That bot was already specified

[commit-bot: I haz the power, 2016-10-25 23:01:58]

There were warnings when CQ was processing your CL:
 * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag
request. That bot was already specified

[commit-bot: I haz the power, 2016-10-25 23:14:35]

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

[commit-bot: I haz the power, 2016-10-25 23:14:36]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-10-25 23:18:59]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-10-25 23:18:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, rkaplow@chromium.org, zhongyi@chromium.org
Link to the patchset: https://codereview.chromium.org/2442633002/#ps260001
(title: "Addressed rch comments")

[commit-bot: I haz the power, 2016-10-25 23:19:37]

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-25 23:19:40]

There were warnings when CQ was processing your CL:
 * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag
request. That bot was already specified

[commit-bot: I haz the power, 2016-10-25 23:25:41]

Description was changed from

==========
Add a QUIC proxy server to the list of QUIC servers supported at start up

Data reduction proxy server is added to the list of QUIC
servers supported at start up. This will enable 0-RTT
requests for the first request to the data reduction QUIC 
proxy (provided Chromium has used QUIC in the past to 
connect to the data reduction proxy).

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet

BUG=658432
==========

to

==========
Add a QUIC proxy server to the list of QUIC servers supported at start up

Data reduction proxy server is added to the list of QUIC
servers supported at start up. This will enable 0-RTT
requests for the first request to the data reduction QUIC 
proxy (provided Chromium has used QUIC in the past to 
connect to the data reduction proxy).

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet

BUG=658432
==========

[commit-bot: I haz the power, 2016-10-25 23:25:43]

Committed patchset #7 (id:260001)

[commit-bot: I haz the power, 2016-10-25 23:29:12]

Description was changed from

==========
Add a QUIC proxy server to the list of QUIC servers supported at start up

Data reduction proxy server is added to the list of QUIC
servers supported at start up. This will enable 0-RTT
requests for the first request to the data reduction QUIC 
proxy (provided Chromium has used QUIC in the past to 
connect to the data reduction proxy).

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet

BUG=658432
==========

to

==========
Add a QUIC proxy server to the list of QUIC servers supported at start up

Data reduction proxy server is added to the list of QUIC
servers supported at start up. This will enable 0-RTT
requests for the first request to the data reduction QUIC
proxy (provided Chromium has used QUIC in the past to
connect to the data reduction proxy).

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet

BUG=658432

Committed: https://crrev.com/3b96695ae3cd9d92050740d6741ffae04f5c4cb7
Cr-Commit-Position: refs/heads/master@{#427531}
==========

[commit-bot: I haz the power, 2016-10-25 23:29:14]

Patchset 7 (id:??) landed as
https://crrev.com/3b96695ae3cd9d92050740d6741ffae04f5c4cb7
Cr-Commit-Position: refs/heads/master@{#427531}