Adding synthetic field trial for DataReductionProxyEnabled

Adding synthetic field trial for the trial name DataReductionProxyEnabled and groups |true| and |false| that indicate whether the data reduction proxy proxy is turned on or not. The field trial was added so we can see if the data reduction proxy is turned on or not for PLT.PT_*_DataReductionProxy histograms, but can be viewed on any histogram.

BUG=394125
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287133

[Alexei Svitkine (slow), 2014-07-30 17:27:05]

https://codereview.chromium.org/421653005/diff/1/components/data_reduction_pr...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/421653005/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:138:
IsDataReductionProxyEnabled() ? true : false);
I don't think you need the ternary operator here - just pass the result of the
function.

https://codereview.chromium.org/421653005/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:192:
IsDataReductionProxyEnabled() ? true : false);
Ditto.

https://codereview.chromium.org/421653005/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h
(right):

https://codereview.chromium.org/421653005/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h:167:
void RegisterSyntheticFieldTrialCallback(bool proxy_enabled) {
Functions should come before variables.

https://codereview.chromium.org/421653005/diff/1/components/metrics/metrics_s...
File components/metrics/metrics_service.h (right):

https://codereview.chromium.org/421653005/diff/1/components/metrics/metrics_s...
components/metrics/metrics_service.h:72: friend class
DataReductionProxyChromeSettings;
This is a layering violation, since the component shouldn't know anything about
things outside of the component.

Instead, please add the friend declaration to ChromeMetricsServiceAccessor in
chrome/browser/metrics and add a method on the accessor to register a synthetic
trial (the method can actually just be a static that takes two strings and
inside it, it could do the whole block that uses
g_browser_process->metrics_service() and passes the SyntheticTrial object.

[megjablon, 2014-07-30 18:50:49]

https://codereview.chromium.org/421653005/diff/1/components/data_reduction_pr...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/421653005/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:138:
IsDataReductionProxyEnabled() ? true : false);
On 2014/07/30 17:27:04, Alexei Svitkine wrote:
> I don't think you need the ternary operator here - just pass the result of the
> function.

Facepalm. Done.

https://codereview.chromium.org/421653005/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:192:
IsDataReductionProxyEnabled() ? true : false);
On 2014/07/30 17:27:04, Alexei Svitkine wrote:
> Ditto.

Done.

https://codereview.chromium.org/421653005/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h
(right):

https://codereview.chromium.org/421653005/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/browser/data_reduction_proxy_settings_test_utils.h:167:
void RegisterSyntheticFieldTrialCallback(bool proxy_enabled) {
On 2014/07/30 17:27:04, Alexei Svitkine wrote:
> Functions should come before variables.

Done.

https://codereview.chromium.org/421653005/diff/1/components/metrics/metrics_s...
File components/metrics/metrics_service.h (right):

https://codereview.chromium.org/421653005/diff/1/components/metrics/metrics_s...
components/metrics/metrics_service.h:72: friend class
DataReductionProxyChromeSettings;
On 2014/07/30 17:27:04, Alexei Svitkine wrote:
> This is a layering violation, since the component shouldn't know anything
about
> things outside of the component.
> 
> Instead, please add the friend declaration to ChromeMetricsServiceAccessor in
> chrome/browser/metrics and add a method on the accessor to register a
synthetic
> trial (the method can actually just be a static that takes two strings and
> inside it, it could do the whole block that uses
> g_browser_process->metrics_service() and passes the SyntheticTrial object.

Done.

[Alexei Svitkine (slow), 2014-07-30 19:00:32]

https://codereview.chromium.org/421653005/diff/20001/components/metrics/metri...
File components/metrics/metrics_service.h (right):

https://codereview.chromium.org/421653005/diff/20001/components/metrics/metri...
components/metrics/metrics_service.h:71: friend class
ChromeMetricsServiceAccessor;
Can you change this to be MetricsServiceAccessor? (That is the class that is
already a friend of MetricsService and we're planning to move it to the
component, so it's OK to have it here.)

Then, a bit more plumbing unfortunately, but what you can then do is have
MetricsServiceAccessor have the code that uses this API in a protected function
that takes a MetricsService* param and then the public
ChromeMetricsServiceAccessor method simply call that method with
g_browser_process->metrics_service() as a param.

(Sorry for asking you to do this extra plumbing - but this is effectively the
first use of a synthetic trial and this bit of extra work is needed to expose
the API while whitelisting access to it through the component boundary.)

[bengr, 2014-07-30 19:08:22]

https://codereview.chromium.org/421653005/diff/20001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_accessor.cc (right):

https://codereview.chromium.org/421653005/diff/20001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:54: if
(g_browser_process->metrics_service()) {
Would we ever not have a metrics_service? It would be nice if the caller could
know that registration was a no-op.

https://codereview.chromium.org/421653005/diff/20001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_accessor.h (right):

https://codereview.chromium.org/421653005/diff/20001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_accessor.h:75: static void
RegisterSyntheticFieldTrial(std::string trial, std::string group);
Can these be const std::string&?

https://codereview.chromium.org/421653005/diff/20001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/421653005/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:125:
const base::Callback<void(bool)>& on_data_reduction_proxy_enabled) {
This is fine, but eventually we should move to adding an observer.

https://codereview.chromium.org/421653005/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:190:
on_data_reduction_proxy_enabled_.Run(IsDataReductionProxyEnabled());
I think this should be done in OnProxyEnabledPrefChange.

[megjablon, 2014-07-30 21:27:07]

https://codereview.chromium.org/421653005/diff/20001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_accessor.cc (right):

https://codereview.chromium.org/421653005/diff/20001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:54: if
(g_browser_process->metrics_service()) {
On 2014/07/30 19:08:22, bengr1 wrote:
> Would we ever not have a metrics_service? It would be nice if the caller could
> know that registration was a no-op.

g_browser_process->metrics_service() may return null

we can return false if the service is not available so the registration didn't
happen

https://codereview.chromium.org/421653005/diff/20001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_accessor.h (right):

https://codereview.chromium.org/421653005/diff/20001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_accessor.h:75: static void
RegisterSyntheticFieldTrial(std::string trial, std::string group);
On 2014/07/30 19:08:22, bengr1 wrote:
> Can these be const std::string&?

Yes. Done.

https://codereview.chromium.org/421653005/diff/20001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/421653005/diff/20001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:190:
on_data_reduction_proxy_enabled_.Run(IsDataReductionProxyEnabled());
On 2014/07/30 19:08:22, bengr1 wrote:
> I think this should be done in OnProxyEnabledPrefChange.

Done.

https://codereview.chromium.org/421653005/diff/20001/components/metrics/metri...
File components/metrics/metrics_service.h (right):

https://codereview.chromium.org/421653005/diff/20001/components/metrics/metri...
components/metrics/metrics_service.h:71: friend class
ChromeMetricsServiceAccessor;
On 2014/07/30 19:00:32, Alexei Svitkine wrote:
> Can you change this to be MetricsServiceAccessor? (That is the class that is
> already a friend of MetricsService and we're planning to move it to the
> component, so it's OK to have it here.)
> 
> Then, a bit more plumbing unfortunately, but what you can then do is have
> MetricsServiceAccessor have the code that uses this API in a protected
function
> that takes a MetricsService* param and then the public
> ChromeMetricsServiceAccessor method simply call that method with
> g_browser_process->metrics_service() as a param.
> 
> (Sorry for asking you to do this extra plumbing - but this is effectively the
> first use of a synthetic trial and this bit of extra work is needed to expose
> the API while whitelisting access to it through the component boundary.)

Does this looks like what you want?

[Alexei Svitkine (slow), 2014-07-30 21:33:31]

LGTM % comments.

Thanks!

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_accessor.cc (right):

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:9: #include
"chrome/browser/metrics/metrics_service_accessor.h"
Nit: This include is unnecessary.

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:53: const std::string&
trial, const std::string& group) {
Nit: 1 param per line if first param is on a new line in the function signature.

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service_accessor.cc (right):

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service_accessor.cc:32: if (metrics_service) {
Nit: Instead do an early return if the service is nil (reverse the if).

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service_accessor.h (right):

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service_accessor.h:37: const std::string& trial,
Nit: Align params.

[megjablon, 2014-07-30 21:42:51]

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_accessor.cc (right):

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:9: #include
"chrome/browser/metrics/metrics_service_accessor.h"
On 2014/07/30 21:33:31, Alexei Svitkine wrote:
> Nit: This include is unnecessary.

Done.

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:53: const std::string&
trial, const std::string& group) {
On 2014/07/30 21:33:31, Alexei Svitkine wrote:
> Nit: 1 param per line if first param is on a new line in the function
signature.

Done.

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service_accessor.cc (right):

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service_accessor.cc:32: if (metrics_service) {
On 2014/07/30 21:33:31, Alexei Svitkine wrote:
> Nit: Instead do an early return if the service is nil (reverse the if).

Done.

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_service_accessor.h (right):

https://codereview.chromium.org/421653005/diff/40001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_service_accessor.h:37: const std::string& trial,
On 2014/07/30 21:33:31, Alexei Svitkine wrote:
> Nit: Align params.

Done.

[bengr, 2014-07-30 23:42:07]

https://codereview.chromium.org/421653005/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_accessor.h (right):

https://codereview.chromium.org/421653005/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_accessor.h:74: const std::string&
group);
indentation

https://codereview.chromium.org/421653005/diff/100001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/421653005/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:160:
on_data_reduction_proxy_enabled_.Run(IsDataReductionProxyEnabled());
Is this needed?

[megjablon, 2014-07-30 23:57:07]

https://codereview.chromium.org/421653005/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_accessor.h (right):

https://codereview.chromium.org/421653005/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_accessor.h:74: const std::string&
group);
On 2014/07/30 23:42:07, bengr1 wrote:
> indentation

Done.

https://codereview.chromium.org/421653005/diff/100001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/421653005/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:160:
on_data_reduction_proxy_enabled_.Run(IsDataReductionProxyEnabled());
On 2014/07/30 23:42:07, bengr1 wrote:
> Is this needed?
Yes, because we want the synthetic field trial set as soon as we have access to
the callback and then it will be toggled every time thereafter by
OnProxyEnabledPrefChange.

[bengr, 2014-07-31 16:27:52]

lgtm

[bengr, 2014-07-31 16:28:35]

On 2014/07/31 16:27:52, bengr1 wrote:
> lgtm

I forgot one thing: Please update the CL description to go into more detail
about what this change does.

[megjablon, 2014-07-31 17:28:27]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2014-07-31 17:29:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/megjablon@chromium.org/421653005/100001

[commit-bot: I haz the power, 2014-07-31 18:26:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[megjablon, 2014-07-31 18:32:18]

The CQ bit was unchecked by megjablon@chromium.org

[megjablon, 2014-07-31 18:33:24]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2014-07-31 18:34:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/megjablon@chromium.org/421653005/110001

[commit-bot: I haz the power, 2014-07-31 21:53:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 10:29:57]

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

[commit-bot: I haz the power, 2014-08-01 10:29:58]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[megjablon, 2014-08-01 16:57:11]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2014-08-01 17:02:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/megjablon@chromium.org/421653005/110001

[commit-bot: I haz the power, 2014-08-02 00:35:22]

Change committed as 287133