Consolidates accessing and setting the UMA pref to be within metrics code

chrome/browser/metrics/metrics_reporting_state.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/metrics/metrics_reporting_state.h"
 
-#include "base/threading/thread_restrictions.h"
+#include "base/callback.h"
+#include "base/metrics/histogram.h"
+#include "base/prefs/pref_service.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/installer/util/google_update_settings.h"
+#include "chromeos/settings/cros_settings_names.h"
 #include "components/metrics/metrics_service.h"
+#include "content/public/browser/browser_thread.h"
 
-bool ResolveMetricsReportingEnabled(bool enabled) {
-  // GoogleUpdateSettings touches the disk from the UI thread. MetricsService
-  // also calls GoogleUpdateSettings below. http://crbug/62626
-  base::ThreadRestrictions::ScopedAllowIO allow_io;
+namespace {

[Alexei Svitkine (slow), 2014/09/04 13:58:03]

Nit: Add a blank line after this.

[gayane -on leave until 09-2017, 2014/09/04 18:35:47]

Done.

+enum MetricsReportingChangeHistogramValue {
+  METRICS_REPORTING_ENABLED,
+  METRICS_REPORTING_DISABLED,
+  METRICS_REPORTING_MAX,
+};
 
+void RecordMetricsReportingHistogramValue(
+    MetricsReportingChangeHistogramValue value) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "MetricsReporting", value, METRICS_REPORTING_MAX);
+}
+
+// Tries to set crash stats upload consent to |enabled|.
+bool SetGoogleUpdateSettings(bool enabled) {
   GoogleUpdateSettings::SetCollectStatsConsent(enabled);
-  bool update_pref = GoogleUpdateSettings::GetCollectStatsConsent();
-
-  if (enabled != update_pref)
+  bool updated_pref = GoogleUpdateSettings::GetCollectStatsConsent();
+  bool success = (enabled == updated_pref);
+  if (!success)
     DVLOG(1) << "Unable to set crash report status to " << enabled;
 
+  return success;
+}
+// Does the necessary changes for MetricsReportingEnabled changes which needs

[Alexei Svitkine (slow), 2014/09/04 13:58:03]

Nit: Add a blank line before this.

[gayane -on leave until 09-2017, 2014/09/04 18:35:46]

Done.

+// to be done in the main thread.
+void SetMetricsReporting(base::Callback<void(bool)> callback_fn,
+                                    bool enabled, bool success) {

[Alexei Svitkine (slow), 2014/09/04 13:58:03]

Nit: 1 param per line. Align params.

[gayane -on leave until 09-2017, 2014/09/04 18:35:47]

Done.

   // Only change the pref if GoogleUpdateSettings::GetCollectStatsConsent
   // succeeds.
-  enabled = update_pref;
+  metrics::MetricsService* metrics = g_browser_process->metrics_service();
+  if (success) {
+    if (metrics) {
+      if (enabled)
+        metrics->Start();
+      else
+        metrics->Stop();
+    }
+#if !defined(OS_CHROMEOS)
+    g_browser_process->local_state()->SetBoolean(
+      prefs::kMetricsReportingEnabled, enabled);

[Alexei Svitkine (slow), 2014/09/04 13:58:03]

Nit: Indent 2 more.

But also, I think before this is reached, there should be a check somewhere (in
this file) to ensure IsMetricsReportingUserChangable() is true.

[gayane -on leave until 09-2017, 2014/09/04 18:35:47]

Added a check in InitiateMetricsReportingChange


On 2014/09/04 13:58:03, Alexei Svitkine wrote:

+#endif
+    RecordMetricsReportingHistogramValue(enabled? METRICS_REPORTING_ENABLED
+        :METRICS_REPORTING_DISABLED);
+  }
+  if (!callback_fn.is_null())
+    callback_fn.Run(success);
+}
+} // namespace

[Alexei Svitkine (slow), 2014/09/04 13:58:03]

Nit: Add a blank line before this.

[gayane -on leave until 09-2017, 2014/09/04 18:35:47]

Done.

 
-  metrics::MetricsService* metrics = g_browser_process->metrics_service();
-  if (metrics) {
-    if (enabled)
-      metrics->Start();
-    else
-      metrics->Stop();
-  }
+void InitiateMetricsReportingChange(
+    base::Callback<void(bool)> callback_fn, bool enabled) {

[Alexei Svitkine (slow), 2014/09/04 13:58:03]

Nit: 1 param per line.

[gayane -on leave until 09-2017, 2014/09/04 18:35:47]

Done.

+  content::BrowserThread::PostTaskAndReplyWithResult(

[Alexei Svitkine (slow), 2014/09/04 13:58:03]

Add a comment why this is posted to the FILE thread.

[gayane -on leave until 09-2017, 2014/09/04 18:35:47]

Done.

+      content::BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&SetGoogleUpdateSettings, enabled),
+      base::Bind(&SetMetricsReporting, callback_fn, enabled));
+}
 
-  return enabled;
+bool IsMetricsReportingEnabled() {
+  const PrefService* pref_service = g_browser_process->local_state();
+  return pref_service->GetBoolean(prefs::kMetricsReportingEnabled);
 }
+
+bool IsMetricsReportingUserChangable() {
+  const PrefService* pref_service = g_browser_process->local_state();
+
+  const PrefService::Preference* pref = pref_service->FindPreference(
+      prefs::kMetricsReportingEnabled);
+  return pref && pref->IsManaged();
+}