Fix ChromeMetricsServiceAccessor::IsMetricsReportingEnabled on CrOs

Fix ChromeMetricsServiceAccessor::IsMetricsReportingEnabled on CrOs

Since MetricsServicesManager::IsMetricsReportingEnabled is correct, it is
called directly.

BUG=457430
Committed: https://crrev.com/40dd030f117e794b8e8919d4f2c6e079b77209d1
Cr-Commit-Position: refs/heads/master@{#317058}

[anthonyvd, 2015-02-11 21:50:28]

anthonyvd@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2015-02-11 21:58:26]

https://codereview.chromium.org/916133003/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_metrics_service_accessor.cc (left):

https://codereview.chromium.org/916133003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:37:
&reporting_enabled);
Why did you remove this?

https://codereview.chromium.org/916133003/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_metrics_service_accessor.cc (right):

https://codereview.chromium.org/916133003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:21:
IsMetricsReportingEnabled();
IsMetricsReportingEnabled() on MetricsServicesManager needs to be private.

It's an oversight that it currently isn't.

Can you make it private and adjust this code accordingly?

For example, you can actually move its implementation into this class and have
MetricsServicesManager call this version.

[Alexei Svitkine (slow), 2015-02-11 22:06:11]

https://codereview.chromium.org/916133003/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_metrics_service_accessor.cc (right):

https://codereview.chromium.org/916133003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:21:
IsMetricsReportingEnabled();
On 2015/02/11 21:58:26, Alexei Svitkine wrote:
> IsMetricsReportingEnabled() on MetricsServicesManager needs to be private.
> 
> It's an oversight that it currently isn't.
> 
> Can you make it private and adjust this code accordingly?
> 
> For example, you can actually move its implementation into this class and have
> MetricsServicesManager call this version.

I just took a glance, and only place that relies on IsMetricsReportingEnabled()
not being private is a call from chrome_browser_main.cc which ends up calling
MetricsServicesManager::UpdatePermissions() on the metrics services manager - so
you can add a new version of UpdatePermissions function that does that all
within MetricsServicesManager.

[anthonyvd, 2015-02-12 15:48:55]

https://codereview.chromium.org/916133003/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_metrics_service_accessor.cc (left):

https://codereview.chromium.org/916133003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:37:
&reporting_enabled);
On 2015/02/11 21:58:26, Alexei Svitkine wrote:
> Why did you remove this?

It was a fix for ChromeMetricsServiceAccessor::IsMetricsReportingEnabled on
ChromeOS.  Now that it's working properly, the special ChromeOS branch isn't
needed anymore (IsMetricsReportingEnabled has the exact same code).

https://codereview.chromium.org/916133003/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_metrics_service_accessor.cc (right):

https://codereview.chromium.org/916133003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:21:
IsMetricsReportingEnabled();
On 2015/02/11 21:58:26, Alexei Svitkine wrote:
> IsMetricsReportingEnabled() on MetricsServicesManager needs to be private.
> 
> It's an oversight that it currently isn't.
> 
> Can you make it private and adjust this code accordingly?
> 
> For example, you can actually move its implementation into this class and have
> MetricsServicesManager call this version.

Done.

https://codereview.chromium.org/916133003/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:21:
IsMetricsReportingEnabled();
On 2015/02/11 22:06:11, Alexei Svitkine wrote:
> On 2015/02/11 21:58:26, Alexei Svitkine wrote:
> > IsMetricsReportingEnabled() on MetricsServicesManager needs to be private.
> > 
> > It's an oversight that it currently isn't.
> > 
> > Can you make it private and adjust this code accordingly?
> > 
> > For example, you can actually move its implementation into this class and
have
> > MetricsServicesManager call this version.
> 
> I just took a glance, and only place that relies on
IsMetricsReportingEnabled()
> not being private is a call from chrome_browser_main.cc which ends up calling
> MetricsServicesManager::UpdatePermissions() on the metrics services manager -
so
> you can add a new version of UpdatePermissions function that does that all
> within MetricsServicesManager.

Done.

[Alexei Svitkine (slow), 2015-02-12 15:58:46]

Thanks! A couple more comments.

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

https://codereview.chromium.org/916133003/diff/20001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:26:
switches::kForceFieldTrials))
Nit: While you're touching this, add {}'s here.

https://codereview.chromium.org/916133003/diff/20001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:35: enabled =
local_state_->GetBoolean(prefs::kMetricsReportingEnabled);
I don't think this will compile, since there's no local_state_ member variable.

Use g_browser_process->local_state() to get it, like the previous code does.

https://codereview.chromium.org/916133003/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_services_manager.cc (left):

https://codereview.chromium.org/916133003/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:131: // see
http://crbug.com/362192.
Nit: Move this comment to the moved implementation.

https://codereview.chromium.org/916133003/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/916133003/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:137: bool
MetricsServicesManager::IsMetricsReportingEnabled() const {
Maybe we don't need this function on this wrapper anymore and the other one can
just be called directly?

[anthonyvd, 2015-02-12 16:29:20]

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

https://codereview.chromium.org/916133003/diff/20001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:26:
switches::kForceFieldTrials))
On 2015/02/12 15:58:46, Alexei Svitkine wrote:
> Nit: While you're touching this, add {}'s here.

Done.

https://codereview.chromium.org/916133003/diff/20001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_accessor.cc:35: enabled =
local_state_->GetBoolean(prefs::kMetricsReportingEnabled);
On 2015/02/12 15:58:46, Alexei Svitkine wrote:
> I don't think this will compile, since there's no local_state_ member
variable.
> 
> Use g_browser_process->local_state() to get it, like the previous code does.

Done.

https://codereview.chromium.org/916133003/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_services_manager.cc (left):

https://codereview.chromium.org/916133003/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:131: // see
http://crbug.com/362192.
On 2015/02/12 15:58:46, Alexei Svitkine wrote:
> Nit: Move this comment to the moved implementation.

Done.

https://codereview.chromium.org/916133003/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/916133003/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.cc:137: bool
MetricsServicesManager::IsMetricsReportingEnabled() const {
On 2015/02/12 15:58:46, Alexei Svitkine wrote:
> Maybe we don't need this function on this wrapper anymore and the other one
can
> just be called directly?

Done.

[Alexei Svitkine (slow), 2015-02-12 16:41:31]

lgtm

[anthonyvd, 2015-02-12 16:43:03]

anthonyvd@chromium.org changed reviewers:
+ sky@chromium.org

[anthonyvd, 2015-02-12 16:43:04]

sky@ can you please take a look at chrome_browser_main.cc? Thanks!

[sky, 2015-02-18 22:14:48]

chrome_browser_main LGTM

[anthonyvd, 2015-02-19 15:28:22]

New patchsets have been uploaded after l-g-t-m from
asvitkine@chromium.org,sky@chromium.org

[anthonyvd, 2015-02-19 15:28:50]

The CQ bit was checked by anthonyvd@chromium.org

[commit-bot: I haz the power, 2015-02-19 15:29:09]

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

[commit-bot: I haz the power, 2015-02-19 16:11:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-19 16:12:10]

Patchset 4 (id:??) landed as
https://crrev.com/40dd030f117e794b8e8919d4f2c6e079b77209d1
Cr-Commit-Position: refs/heads/master@{#317058}