Check effective proxy config before adding data reduction proxies

Check effective proxy config before adding data reduction proxies

Before this change, the data reduction proxies were added to the
list of acceptable proxies for a URL, if the effective proxy
configuration resolved to direct for it. This change further checks
that the effective configuration is not the data reduction proxy's.

BUG=425826
Committed: https://crrev.com/f5c403d7f2a789f755428505231861988f9c40fd
Cr-Commit-Position: refs/heads/master@{#302562}

[bengr, 2014-10-22 00:27:30]

bengr@chromium.org changed reviewers:
+ marq@chromium.org, mef@chromium.org

[bengr, 2014-10-22 00:27:31]

marq: components/*
mef: net/*, chrome/browser/net/*

[marq (ping after 24h), 2014-10-28 08:15:27]

LGTM with a nit and a note.

https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:445: proxy_service.config(),
This change will also need to be made in CrNet's network delegate; becuase this
is an API change, the crnet build will break when this change is merged, so
there's no risk of this change not being picked up.

https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:448: result);
No need to move this onto another line, as method calls can have multiple params
per line. Or, if you really want one param per line, also split url and
load_flags  on the first line.

[mef, 2014-10-28 17:01:32]

https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.h (right):

https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.h:82: const net::ProxyRetryInfoMap&
proxy_retry_info,
why did you loose |_map|?

https://codereview.chromium.org/653313003/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc
(right):

https://codereview.chromium.org/653313003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc:139:
!data_reduction_proxy_config.Equals(proxy_service_proxy_config)) {
I'm not sure I understand why we need this check. Shouldn't data reduction proxy
be used if  |proxy_service_proxy_config| equals to
|data_reduction_proxy_config|?

I'm also a bit concerned that ProxyConfig::Equals has a non-trivial performance
(with multiple vector comparisons and whatnot).

[bengr, 2014-10-29 21:12:37]

https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:445: proxy_service.config(),
On 2014/10/28 08:15:27, marq_ wrote:
> This change will also need to be made in CrNet's network delegate; becuase
this
> is an API change, the crnet build will break when this change is merged, so
> there's no risk of this change not being picked up.

Acknowledged.

https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:448: result);
On 2014/10/28 08:15:27, marq_ wrote:
> No need to move this onto another line, as method calls can have multiple
params
> per line. Or, if you really want one param per line, also split url and
> load_flags  on the first line.

Done.

https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.h (right):

https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.h:82: const net::ProxyRetryInfoMap&
proxy_retry_info,
On 2014/10/28 17:01:32, mef wrote:
> why did you loose |_map|?

Done.

https://codereview.chromium.org/653313003/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc
(right):

https://codereview.chromium.org/653313003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc:139:
!data_reduction_proxy_config.Equals(proxy_service_proxy_config)) {
On 2014/10/28 17:01:32, mef wrote:
> I'm not sure I understand why we need this check. Shouldn't data reduction
proxy
> be used if  |proxy_service_proxy_config| equals to
> |data_reduction_proxy_config|?
> 
> I'm also a bit concerned that ProxyConfig::Equals has a non-trivial
performance
> (with multiple vector comparisons and whatnot).

The point of this code is to add the data reduction proxies to a config if that
config returns direct. If the config is already that of the data reduction
proxy, then the data reduction proxies should not be added to it.

As for performance, the case of Equals() == false requires at most 3 boolean
checks, a check that two empty lists are empty and a check that the first http
proxy is the data reduction proxy in one case and not the other. For Equals() ==
true, the story is the same, except that there would be two more string checks.
Is that too much? For comparison, the proxy resolver fires up v8 in many cases,
which takes milliseconds.

[bengr, 2014-10-30 20:42:11]

On 2014/10/29 21:12:37, bengr wrote:
>
https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
> File chrome/browser/net/chrome_network_delegate.cc (right):
> 
>
https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
> chrome/browser/net/chrome_network_delegate.cc:445: proxy_service.config(),
> On 2014/10/28 08:15:27, marq_ wrote:
> > This change will also need to be made in CrNet's network delegate; becuase
> this
> > is an API change, the crnet build will break when this change is merged, so
> > there's no risk of this change not being picked up.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
> chrome/browser/net/chrome_network_delegate.cc:448: result);
> On 2014/10/28 08:15:27, marq_ wrote:
> > No need to move this onto another line, as method calls can have multiple
> params
> > per line. Or, if you really want one param per line, also split url and
> > load_flags  on the first line.
> 
> Done.
> 
>
https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
> File chrome/browser/net/chrome_network_delegate.h (right):
> 
>
https://codereview.chromium.org/653313003/diff/1/chrome/browser/net/chrome_ne...
> chrome/browser/net/chrome_network_delegate.h:82: const net::ProxyRetryInfoMap&
> proxy_retry_info,
> On 2014/10/28 17:01:32, mef wrote:
> > why did you loose |_map|?
> 
> Done.
> 
>
https://codereview.chromium.org/653313003/diff/1/components/data_reduction_pr...
> File
> components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc
> (right):
> 
>
https://codereview.chromium.org/653313003/diff/1/components/data_reduction_pr...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.cc:139:
> !data_reduction_proxy_config.Equals(proxy_service_proxy_config)) {
> On 2014/10/28 17:01:32, mef wrote:
> > I'm not sure I understand why we need this check. Shouldn't data reduction
> proxy
> > be used if  |proxy_service_proxy_config| equals to
> > |data_reduction_proxy_config|?
> > 
> > I'm also a bit concerned that ProxyConfig::Equals has a non-trivial
> performance
> > (with multiple vector comparisons and whatnot).
> 
> The point of this code is to add the data reduction proxies to a config if
that
> config returns direct. If the config is already that of the data reduction
> proxy, then the data reduction proxies should not be added to it.
> 
> As for performance, the case of Equals() == false requires at most 3 boolean
> checks, a check that two empty lists are empty and a check that the first http
> proxy is the data reduction proxy in one case and not the other. For Equals()
==
> true, the story is the same, except that there would be two more string
checks.
> Is that too much? For comparison, the proxy resolver fires up v8 in many
cases,
> which takes milliseconds.

@mef: what do you think?

[mef, 2014-10-30 21:05:36]

lgtm with nits.

https://codereview.chromium.org/653313003/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.h (right):

https://codereview.chromium.org/653313003/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.h:75: // ProxyService.ResolveProxy()
returns. |proxy_info| contains information
There is no |proxy_info| parameter.

https://codereview.chromium.org/653313003/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h
(right):

https://codereview.chromium.org/653313003/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:47:
// Also, apply the data reduction proxy configuration if another configuration
The comment overall seems a bit stale. Could you it to describe
|proxy_service_proxy_config| and |proxy_retry_info|?

https://codereview.chromium.org/653313003/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol_unittest.cc
(right):

https://codereview.chromium.org/653313003/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol_unittest.cc:887:
net::ProxyConfig proxy_config = net::ProxyConfig::CreateDirect();
Given that it is direct and doesn't change, does it make sense to call it
|proxy_config_direct|?

[bengr, 2014-11-03 23:26:58]

https://codereview.chromium.org/653313003/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.h (right):

https://codereview.chromium.org/653313003/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.h:75: // ProxyService.ResolveProxy()
returns. |proxy_info| contains information
On 2014/10/30 21:05:36, mef wrote:
> There is no |proxy_info| parameter.

Done.

https://codereview.chromium.org/653313003/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h
(right):

https://codereview.chromium.org/653313003/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h:47:
// Also, apply the data reduction proxy configuration if another configuration
On 2014/10/30 21:05:36, mef wrote:
> The comment overall seems a bit stale. Could you it to describe
> |proxy_service_proxy_config| and |proxy_retry_info|? 

Done.

https://codereview.chromium.org/653313003/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol_unittest.cc
(right):

https://codereview.chromium.org/653313003/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol_unittest.cc:887:
net::ProxyConfig proxy_config = net::ProxyConfig::CreateDirect();
On 2014/10/30 21:05:36, mef wrote:
> Given that it is direct and doesn't change, does it make sense to call it
> |proxy_config_direct|?

Done.

[bengr, 2014-11-04 01:08:58]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-11-04 01:10:29]

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

[commit-bot: I haz the power, 2014-11-04 03:03:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-04 03:03:51]

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