Added command line flag to override list of proxies from Data Saver API

Added command line flag to override list of proxies from Data Saver API

This allows users and integration tests to use the Data Saver API, but
override the list of Data Reduction Proxies to use for HTTP proxies with
a custom list.

BUG=526827
Committed: https://crrev.com/49d43be4e60c434ae42e87a69f5aabdf5ccbddd4
Cr-Commit-Position: refs/heads/master@{#351172}

[sclittle, 2015-09-15 19:52:09]

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

[sclittle, 2015-09-15 19:52:10]

PTAL

[bengr, 2015-09-17 17:27:34]

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

https://codereview.chromium.org/1345523005/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.cc:45:
override_proxies_for_http_.push_back(net::ProxyServer::FromURI(
What happens if the string is not a valid proxy URI?

https://codereview.chromium.org/1345523005/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.cc:82:
if (use_override_proxies_for_http_ && !proxies_for_http_.empty()) {
Do you need this separate use_override_proxies_for_http_ bool? Can you just
check if override_proxies_for_http_ is non-empty()?

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

https://codereview.chromium.org/1345523005/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values_unittest.cc:62:
switches::kDataReductionProxyConfigHttpProxiesOverride,
Somewhere you should document that this is a semicolon-separated list of proxies
with scheme, host, and port, and without a trailing slash. Also, internally,
HTTP proxies don't need the scheme. Is the scheme required here?

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

https://codereview.chromium.org/1345523005/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc:87:
"data-reduction-proxy-config-http-proxies-override";
Egad. How about data-reduction-http-proxies? Could we then use that instead of
spdy-proxy-auth-origin, etc, for the non-config case as well?

[sclittle, 2015-09-22 19:01:59]

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

https://codereview.chromium.org/1345523005/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.cc:45:
override_proxies_for_http_.push_back(net::ProxyServer::FromURI(
On 2015/09/17 17:27:33, bengr wrote:
> What happens if the string is not a valid proxy URI?

Then an empty invalid net::ProxyServer() will be returned and either ignored or
treated as DIRECT. It's a command line flag, so I'm not too worried about it.

https://codereview.chromium.org/1345523005/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.cc:82:
if (use_override_proxies_for_http_ && !proxies_for_http_.empty()) {
On 2015/09/17 17:27:34, bengr wrote:
> Do you need this separate use_override_proxies_for_http_ bool? Can you just
> check if override_proxies_for_http_ is non-empty()?

The main reason to have the bool is to allow for specifying an empty proxy list,
i.e. pretending that the Data Saver API returned an empty proxy list. It also
seems more intuitive that specifically adding the flag with an empty list will
cause Chrome to use an empty DRP proxy list. I don't mind changing this if you'd
like though.

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

https://codereview.chromium.org/1345523005/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values_unittest.cc:62:
switches::kDataReductionProxyConfigHttpProxiesOverride,
On 2015/09/17 17:27:34, bengr wrote:
> Somewhere you should document that this is a semicolon-separated list of
proxies
> with scheme, host, and port, and without a trailing slash. Also, internally,
> HTTP proxies don't need the scheme. Is the scheme required here?

Documented in switches.cc.

The scheme isn't required here (it will default to HTTP), and it's OK to have a
trailing slash.

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

https://codereview.chromium.org/1345523005/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_switches.cc:87:
"data-reduction-proxy-config-http-proxies-override";
On 2015/09/17 17:27:34, bengr wrote:
> Egad. How about data-reduction-http-proxies? Could we then use that instead of
> spdy-proxy-auth-origin, etc, for the non-config case as well?

Renamed the flag, and added TODO in mutable_config_values.cc to support some of
the other flags.

It doesn't actually perfectly replace the old flags' functionality - there's
more work we'll need to do to actually replace the old flags properly, e.g.
we'll still need a flag to replace just the primary proxy to talk to the dev
proxy for some integration tests, although that isn't actually hooked up yet.

[bengr, 2015-09-28 16:15:54]

LGTM, with question.

https://codereview.chromium.org/1345523005/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc
(right):

https://codereview.chromium.org/1345523005/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:226:
return true;
What happens if none of the proxies specified on the command line are valid?

[sclittle, 2015-09-28 19:54:59]

https://codereview.chromium.org/1345523005/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc
(right):

https://codereview.chromium.org/1345523005/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:226:
return true;
On 2015/09/28 16:15:54, bengr wrote:
> What happens if none of the proxies specified on the command line are valid?

Then a DCHECK in net::ProxyServer::FromURI will fail.

[sclittle, 2015-09-28 19:56:21]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2015-09-28 19:57:01]

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

[sclittle, 2015-09-28 20:12:21]

The CQ bit was unchecked by sclittle@chromium.org

[commit-bot: I haz the power, 2015-09-28 20:13:11]

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

[commit-bot: I haz the power, 2015-09-28 20:13:12]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sclittle, 2015-09-28 20:13:32]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2015-09-28 20:15:55]

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

[commit-bot: I haz the power, 2015-09-28 20:33:08]

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

[commit-bot: I haz the power, 2015-09-28 20:33:09]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sclittle, 2015-09-28 21:41:56]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2015-09-28 21:43:26]

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

[commit-bot: I haz the power, 2015-09-28 22:36:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-28 22:37:27]

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