Add field trial handling to enable the Data Reduction Proxy config service.

Add field trial handling to enable the Data Reduction Proxy config service.

This field trial will allow the config service client to be enabled as well
as providing a means to set the config service URL. Values on the command
line will override values coming from the field trials.

BUG=466753
Committed: https://crrev.com/26fd4200a79cccf6dd15b6ee6cb2c31c0eef6834
Cr-Commit-Position: refs/heads/master@{#333635}

[jeremyim, 2015-06-05 22:57:46]

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

[jeremyim, 2015-06-05 22:57:47]

PTAL! =)

[bengr, 2015-06-09 16:32:28]

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

https://codereview.chromium.org/1162903008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:122:
std::string url = DataReductionProxyParams::GetConfigServiceURL();
Why not just have GetConfigServiceURL return a GURL?

https://codereview.chromium.org/1162903008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:123:
if (url.empty()) {
Remove curly braces.

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

https://codereview.chromium.org/1162903008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:336:
"Enabled", "http://enabled.config-service",
I find it slightly weird here and below that the host is not followed by a
trailing slash.

https://codereview.chromium.org/1162903008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:404:
<< test.test_case;
Did git cl format do this?

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

https://codereview.chromium.org/1162903008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:27:
extern const char kConfigServiceFieldTrial[];
While are these in the public namespace? Can they be put in an anonymous
namespace in the .cc?

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

https://codereview.chromium.org/1162903008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc:581:
std::string test_case;
#include <string>

[jeremyim, 2015-06-09 18:32:51]

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

https://codereview.chromium.org/1162903008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:122:
std::string url = DataReductionProxyParams::GetConfigServiceURL();
On 2015/06/09 16:32:28, bengr wrote:
> Why not just have GetConfigServiceURL return a GURL?

Done.

https://codereview.chromium.org/1162903008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:123:
if (url.empty()) {
On 2015/06/09 16:32:28, bengr wrote:
> Remove curly braces.

Done.

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

https://codereview.chromium.org/1162903008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:336:
"Enabled", "http://enabled.config-service",
On 2015/06/09 16:32:28, bengr wrote:
> I find it slightly weird here and below that the host is not followed by a
> trailing slash.

Done.

https://codereview.chromium.org/1162903008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:404:
<< test.test_case;
On 2015/06/09 16:32:28, bengr wrote:
> Did git cl format do this?

Yup!

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

https://codereview.chromium.org/1162903008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:27:
extern const char kConfigServiceFieldTrial[];
On 2015/06/09 16:32:28, bengr wrote:
> While are these in the public namespace? Can they be put in an anonymous
> namespace in the .cc?

Done.

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

https://codereview.chromium.org/1162903008/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc:581:
std::string test_case;
On 2015/06/09 16:32:28, bengr wrote:
> #include <string>

Done.

[bengr, 2015-06-09 22:58:16]

lgtm with nits

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

https://codereview.chromium.org/1162903008/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:182:
if (url.empty())
Probably worth adding a DCHECK somewhere to check that the
GURL(kClientConfigURL) is_valid().

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

https://codereview.chromium.org/1162903008/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc:23:
}
Either add a blank line before this or remove the one on line 21. I would remove
21.

[jeremyim, 2015-06-09 23:02:47]

Thanks for looking at this!

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

https://codereview.chromium.org/1162903008/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:182:
if (url.empty())
On 2015/06/09 22:58:16, bengr wrote:
> Probably worth adding a DCHECK somewhere to check that the
> GURL(kClientConfigURL) is_valid().

An empty URL is not considered valid (and kClientConfigURL is not yet set). The
behavior in DataReductionProxyConfigServiceClient is to generate a config based
on DataReductionProxyParams if an invalid URL is present.

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

https://codereview.chromium.org/1162903008/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc:23:
}
On 2015/06/09 22:58:16, bengr wrote:
> Either add a blank line before this or remove the one on line 21. I would
remove
> 21.

Done.

[jeremyim, 2015-06-09 23:02:58]

The CQ bit was checked by jeremyim@chromium.org

[jeremyim, 2015-06-09 23:02:58]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/1162903008/#ps40001
(title: "bengr CR comment")

[commit-bot: I haz the power, 2015-06-09 23:08:53]

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

[commit-bot: I haz the power, 2015-06-09 23:57:00]

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

[commit-bot: I haz the power, 2015-06-09 23:57:02]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[jeremyim, 2015-06-10 00:21:47]

The CQ bit was checked by jeremyim@chromium.org

[commit-bot: I haz the power, 2015-06-10 00:24:29]

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

[commit-bot: I haz the power, 2015-06-10 01:24:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-10 01:26:02]

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