Base Data Reduction Proxy configuration on vectors of servers per origin scheme.

Base Data Reduction Proxy configuration on vectors of servers per origin scheme.

Previously, the configuration was done by having multiple methods which
returned a single server and relied on the caller to understand which
was desired (origin vs fallback vs alternative vs alternative fallback).

The configuration was also limited to 2 servers and relied on checking
the validity of each value of the std::pair to understand if a value was
actually set or not.

Using a std::vector permits more than 2 servers to be used, and
makes it clearer that any values inside are valid.

The IsDataReductionProxy method also moves into DataReductionProxyConfig
instead of being implemented in DataReductionProxyConfigValues.

BUG=470587
Committed: https://crrev.com/552614698e11a3ed8aa23e59bf446be5f61dbce1
Cr-Commit-Position: refs/heads/master@{#330593}

[jeremyim, 2015-05-15 04:34:27]

jeremyim@chromium.org changed reviewers:
+ bengr@chromium.org

[jeremyim, 2015-05-15 04:34:27]

PTAL.

Thanks!

[bengr, 2015-05-19 15:20:38]

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc:40:
const std::vector<net::ProxyServer>& data_reduction_proxies) {
Why is this not a net::ProxyList?

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:62:
if (proxy.is_valid() && proxy.host_port_pair().Equals(host_port_pair)) {
#include "net/base/host_port_pair.h"

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:67: 
nit: remove blank line

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:141:
// If true, |proxy_info.proxy_servers[0]| will contain the name of the
nit: I slightly prefer proxy_servers.front()

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_unittest.cc
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_unittest.cc:26:
std::vector<net::ProxyServer> BuildProxyList(const std::string& first,
#include "net/proxy/proxy_server.h"

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_unittest.cc:69:
BuildProxyList("", ""));
nit: slightly prefer std::string() over "".

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:117:
test_context_->config()->test_params()->proxies_for_http(false)[0],
.front()?

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_event_creator.cc
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_event_creator.cc:52:
}
Remove curly braces.

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_event_creator.cc:57:
}
Remove curly braces.

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:205:
std::vector<net::ProxyServer> alt_proxies_for_http_;
I'd love to see these go. Would it be possible to have an enum that held the
configuration mode, instead, e.g., NORMAL, ALT, etc?

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h:65:
friend class DataReductionProxyParamsTest;
Could you virtualize SetProxiesForHttp instead, and provide provide protected
setters?

[jeremyim, 2015-05-19 18:25:38]

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.cc:40:
const std::vector<net::ProxyServer>& data_reduction_proxies) {
On 2015/05/19 15:20:37, bengr wrote:
> Why is this not a net::ProxyList?

net::ProxyList doesn't provide any significant benefits (along with loss of
using range based for loops), and exposes functionality that could be confusing
(*Fallback* methods) for how the data is used by the DRP components.

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:62:
if (proxy.is_valid() && proxy.host_port_pair().Equals(host_port_pair)) {
On 2015/05/19 15:20:37, bengr wrote:
> #include "net/base/host_port_pair.h"

Done.

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:67: 
On 2015/05/19 15:20:37, bengr wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:141:
// If true, |proxy_info.proxy_servers[0]| will contain the name of the
On 2015/05/19 15:20:37, bengr wrote:
> nit: I slightly prefer proxy_servers.front()

Done.

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_unittest.cc
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_unittest.cc:26:
std::vector<net::ProxyServer> BuildProxyList(const std::string& first,
On 2015/05/19 15:20:37, bengr wrote:
> #include "net/proxy/proxy_server.h"

Done.

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_unittest.cc:69:
BuildProxyList("", ""));
On 2015/05/19 15:20:37, bengr wrote:
> nit: slightly prefer std::string() over "".

Done.

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:117:
test_context_->config()->test_params()->proxies_for_http(false)[0],
On 2015/05/19 15:20:37, bengr wrote:
> .front()?

Done.

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_event_creator.cc
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_event_creator.cc:52:
}
On 2015/05/19 15:20:37, bengr wrote:
> Remove curly braces.

Done.

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_event_creator.cc:57:
}
On 2015/05/19 15:20:37, bengr wrote:
> Remove curly braces.

Done.

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:205:
std::vector<net::ProxyServer> alt_proxies_for_http_;
On 2015/05/19 15:20:37, bengr wrote:
> I'd love to see these go. Would it be possible to have an enum that held the
> configuration mode, instead, e.g., NORMAL, ALT, etc?

If a UI element were present which could change the data_reduction_alt.enabled
pref, then it's possible for a user to use the ALT as well as NORMAL proxies. It
probably makes more sense to leave these here for now, and to remove the class
entirely once the config service client is in use.

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h
(right):

https://codereview.chromium.org/1124073008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h:65:
friend class DataReductionProxyParamsTest;
On 2015/05/19 15:20:37, bengr wrote:
> Could you virtualize SetProxiesForHttp instead, and provide provide protected
> setters?

Done.

[bengr, 2015-05-19 19:16:49]

lgtm

[jeremyim, 2015-05-19 20:03:19]

The CQ bit was checked by jeremyim@chromium.org

[commit-bot: I haz the power, 2015-05-19 20:05:41]

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

[commit-bot: I haz the power, 2015-05-19 20:25:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-19 20:27:12]

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