Make Data Reduction Proxy a best effort proxy

Make Data Reduction Proxy a best effort proxy

The data reduction proxy is used only if the effective proxy
configuration resolves to DIRECT for the requested URL. This
works by allowing the ProxyService to choose an effective proxy
configuration and to decide on the prioritized list of proxies
to use for a URL given that configuration. Just before the list
is
used, the DataReductionProxyNetworkDelegate's OnResolveProxy()
method provides an opportunity to change that list, which it
does, only if that list begins with DIRECT.

BUG=429826, 444169
Committed: https://crrev.com/70101ea5186d12e67ed99e67fdafd33813f8da67
Cr-Commit-Position: refs/heads/master@{#310182}

[bengr, 2014-12-20 00:21:29]

bengr@chromium.org changed reviewers:
+ marq@chromium.org, mmenke@google.com, sgurun@chromium.org

[bengr, 2014-12-20 00:21:30]

sgurun: android_webview/*
marq: chrome/browser/net/spdyproxy/*, and components/*
mmenke: chrome/browser/profile*

[marq (ping after 24h), 2014-12-20 01:09:48]

A more thorough description for this CL is needed.

I'll do a detailed review, but shouldn't someone else from the DRP client team
also review?

[bengr, 2014-12-22 23:41:56]

bengr@chromium.org changed reviewers:
+ mmenke@chromium.org
- mmenke@google.com

[bengr, 2014-12-30 18:49:19]

Patchset #1 (id:1) has been deleted

[bengr, 2014-12-30 19:30:29]

Patchset #3 (id:60001) has been deleted

[bengr, 2014-12-30 19:30:40]

Patchset #2 (id:40001) has been deleted

[bengr, 2014-12-30 19:32:18]

bengr@chromium.org changed reviewers:
+ asvitkine@chromium.org, sclittle@chromium.org

[bengr, 2014-12-30 19:32:19]

sclittle: chrome/browser/net/spdyproxy/* and components/*
asvitkine: histograms.xml

[sclittle, 2014-12-30 22:10:41]

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h
(right):

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h:5:
#ifndef
COMPONENTS_DATA_REDUCTION_PROXY_CORE_BROWSER_DATA_REDUCTION_PROXY_CONFIGURATOR_
Why no "_H_"?

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h:57:
bool fallback_restricted,
indentation is off

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_unittest.cc
(right):

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_unittest.cc:122:
TEST_F(DataReductionProxyConfigTest, TestDisable) {
Are there tests somewhere for ensuring that if a system proxy is set, then the
DRP won't be used?

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc
(left):

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:228:
if (data_reduction_proxy_params_->WasDataReductionProxyUsed(
Putting NOT_BYPASSED above PROXY_OVERRIDDEN like you did here should fix
https://code.google.com/p/chromium/issues/detail?id=444169.

Maybe add 444169 to BUG= list in CL description?

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:255:
if (!request->proxy_server().IsEmpty()) {
Nit: Maybe add a DCHECK or something here to assert that proxy_server() isn't
the DRP?

I know any requests that had the data reduction proxy as proxy_server() would
have been captured above in NOT_BYPASSED, but a DCHECK here would help ensure
that someone moving code around in the future wouldn't accidentally cause
NOT_BYPASSED to be counted as PROXY_OVERRIDDEN. Especially since we don't
currently have test coverage for these BypassedBytes histograms here.

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:257:
DataReductionProxyUsageStats::PROXY_OVERRIDDEN,
Moving PROXY_OVERRIDDEN down below SSL here slightly changes the meaning of
these histograms from before. IIUC, previously if a managed proxy tunneled SSL
traffic, those would be counted as bypassed bytes under ManagedProxyConfig. Now,
those bytes are counted under SSL.

Is this intended?

https://codereview.chromium.org/792803007/diff/100001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/792803007/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:58711: <suffix name="ManagedProxyConfig"
label="Bypass due to a managed config"/>
You removed ManagedProxyConfig from usage_stats, so maybe indicate somehow that
ManagedProxyConfig is deprecated as of M41 in the label?

[bengr, 2014-12-30 23:22:30]

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h
(right):

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h:5:
#ifndef
COMPONENTS_DATA_REDUCTION_PROXY_CORE_BROWSER_DATA_REDUCTION_PROXY_CONFIGURATOR_
On 2014/12/30 22:10:41, sclittle wrote:
> Why no "_H_"?

Whoa. Done.

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h:57:
bool fallback_restricted,
On 2014/12/30 22:10:41, sclittle wrote:
> indentation is off

Done.

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_unittest.cc
(right):

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_unittest.cc:122:
TEST_F(DataReductionProxyConfigTest, TestDisable) {
On 2014/12/30 22:10:41, sclittle wrote:
> Are there tests somewhere for ensuring that if a system proxy is set, then the
> DRP won't be used?

Yes: DataReductionProxyProtocolTest.OnResolveProxyHandler

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc
(left):

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:228:
if (data_reduction_proxy_params_->WasDataReductionProxyUsed(
On 2014/12/30 22:10:41, sclittle wrote:
> Putting NOT_BYPASSED above PROXY_OVERRIDDEN like you did here should fix
> https://code.google.com/p/chromium/issues/detail?id=444169.
> 
> Maybe add 444169 to BUG= list in CL description?

Done.

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:255:
if (!request->proxy_server().IsEmpty()) {
On 2014/12/30 22:10:41, sclittle wrote:
> Nit: Maybe add a DCHECK or something here to assert that proxy_server() isn't
> the DRP?
> 
> I know any requests that had the data reduction proxy as proxy_server() would
> have been captured above in NOT_BYPASSED, but a DCHECK here would help ensure
> that someone moving code around in the future wouldn't accidentally cause
> NOT_BYPASSED to be counted as PROXY_OVERRIDDEN. Especially since we don't
> currently have test coverage for these BypassedBytes histograms here.

Done.

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:257:
DataReductionProxyUsageStats::PROXY_OVERRIDDEN,
On 2014/12/30 22:10:41, sclittle wrote:
> Moving PROXY_OVERRIDDEN down below SSL here slightly changes the meaning of
> these histograms from before. IIUC, previously if a managed proxy tunneled SSL
> traffic, those would be counted as bypassed bytes under ManagedProxyConfig.
Now,
> those bytes are counted under SSL.
> 
> Is this intended?

This was intentional. I want to know how much traffic we could have compressed
if there wasn't another proxy in the mix. We don't compress HTTPS, so that
should be removed from consideration. Does that make sense?

https://codereview.chromium.org/792803007/diff/100001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/792803007/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:58711: <suffix name="ManagedProxyConfig"
label="Bypass due to a managed config"/>
On 2014/12/30 22:10:41, sclittle wrote:
> You removed ManagedProxyConfig from usage_stats, so maybe indicate somehow
that
> ManagedProxyConfig is deprecated as of M41 in the label?

Done.

[sclittle, 2014-12-30 23:54:26]

chrome/browser/net/spdyproxy/* and components/* LGTM

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/792803007/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:257:
DataReductionProxyUsageStats::PROXY_OVERRIDDEN,
On 2014/12/30 23:22:30, bengr wrote:
> On 2014/12/30 22:10:41, sclittle wrote:
> > Moving PROXY_OVERRIDDEN down below SSL here slightly changes the meaning of
> > these histograms from before. IIUC, previously if a managed proxy tunneled
SSL
> > traffic, those would be counted as bypassed bytes under ManagedProxyConfig.
> Now,
> > those bytes are counted under SSL.
> > 
> > Is this intended?
> 
> This was intentional. I want to know how much traffic we could have compressed
> if there wasn't another proxy in the mix. We don't compress HTTPS, so that
> should be removed from consideration. Does that make sense?

OK

[marq (ping after 24h), 2014-12-31 00:27:42]

A quick inspection LGTM; rubberstamping as OWNER/committer.

[mmenke, 2015-01-05 15:42:42]

profiles/ LGTM.  Didn't look at anything else.

https://codereview.chromium.org/792803007/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/792803007/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:26: #include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h"
Can't this be forward declared, and then included in the CC file (Needed there
for destruction, I believe)

[Alexei Svitkine (slow), 2015-01-05 16:56:48]

lgtm

[bengr, 2015-01-05 23:28:38]

https://codereview.chromium.org/792803007/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/792803007/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:26: #include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h"
On 2015/01/05 15:42:42, mmenke wrote:
> Can't this be forward declared, and then included in the CC file (Needed there
> for destruction, I believe)

Done.

[sclittle, 2015-01-06 01:42:47]

https://codereview.chromium.org/792803007/diff/180001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc
(right):

https://codereview.chromium.org/792803007/diff/180001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc:153:
!url.SchemeIsWSOrWSS()) {
Please add a DCHECK asserting that the proxy server is not the DRP before this
block.

[sclittle, 2015-01-06 18:13:24]

https://codereview.chromium.org/792803007/diff/220001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc
(left):

https://codereview.chromium.org/792803007/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc:150:
net::ProxyInfo* result) {
Can you add a DCHECK at the beginning of this function asserting that
result->proxy_server() is not the DRP?

[bengr, 2015-01-06 19:34:28]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2015-01-06 19:35:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/792803007/220001

[commit-bot: I haz the power, 2015-01-06 19:43:16]

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

[commit-bot: I haz the power, 2015-01-06 19:43:17]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sgurun-gerrit only, 2015-01-06 19:43:58]

https://codereview.chromium.org/792803007/diff/220001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/792803007/diff/220001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:166: if
(data_reduction_proxy_settings_.get()) {
why do we need this if?

[bengr, 2015-01-06 21:32:49]

https://codereview.chromium.org/792803007/diff/220001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/792803007/diff/220001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:166: if
(data_reduction_proxy_settings_.get()) {
On 2015/01/06 19:43:57, sgurun wrote:
> why do we need this if?

You are right. I was carrying over some cruft. Done.

https://codereview.chromium.org/792803007/diff/220001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc
(left):

https://codereview.chromium.org/792803007/diff/220001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc:150:
net::ProxyInfo* result) {
On 2015/01/06 18:13:24, sclittle wrote:
> Can you add a DCHECK at the beginning of this function asserting that
> result->proxy_server() is not the DRP?

Done.

[sgurun-gerrit only, 2015-01-06 21:37:18]

On 2015/01/06 21:32:49, bengr wrote:
>
https://codereview.chromium.org/792803007/diff/220001/android_webview/browser...
> File android_webview/browser/aw_browser_context.cc (right):
> 
>
https://codereview.chromium.org/792803007/diff/220001/android_webview/browser...
> android_webview/browser/aw_browser_context.cc:166: if
> (data_reduction_proxy_settings_.get()) {
> On 2015/01/06 19:43:57, sgurun wrote:
> > why do we need this if?
> 
> You are right. I was carrying over some cruft. Done.
> 
>
https://codereview.chromium.org/792803007/diff/220001/components/data_reducti...
> File
> components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc
> (left):
> 
>
https://codereview.chromium.org/792803007/diff/220001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc:150:
> net::ProxyInfo* result) {
> On 2015/01/06 18:13:24, sclittle wrote:
> > Can you add a DCHECK at the beginning of this function asserting that
> > result->proxy_server() is not the DRP?
> 
> Done.
only looked at android_webview
aw lgtm

[bengr, 2015-01-06 23:46:24]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2015-01-06 23:47:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/792803007/260001

[commit-bot: I haz the power, 2015-01-07 00:21:21]

Committed patchset #11 (id:260001)

[commit-bot: I haz the power, 2015-01-07 00:23:01]

Patchset 11 (id:??) landed as
https://crrev.com/70101ea5186d12e67ed99e67fdafd33813f8da67
Cr-Commit-Position: refs/heads/master@{#310182}