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

chrome/browser/metrics/metrics_reporting_state.cc

Index: chrome/browser/metrics/metrics_reporting_state.cc
diff --git a/chrome/browser/metrics/metrics_reporting_state.cc b/chrome/browser/metrics/metrics_reporting_state.cc
index 6c72ba66de08962bcf8ecef206143fdbd2e00fe4..ad182719f70a980815008cf997a62f63f9d6c4e3 100644
--- a/chrome/browser/metrics/metrics_reporting_state.cc
+++ b/chrome/browser/metrics/metrics_reporting_state.cc
@@ -4,33 +4,92 @@
 
 #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/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 {
 
+enum MetricsReportingChangeHistogramValue {
+  METRICS_REPORTING_ERROR,
+  METRICS_REPORTING_DISABLED,
+  METRICS_REPORTING_ENABLED,
+  METRICS_REPORTING_MAX
+};
+
+void RecordMetricsReportingHistogramValue(
+    MetricsReportingChangeHistogramValue value) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "UMA.MetricsReporting.Toggle", 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();
+  bool updated_pref = GoogleUpdateSettings::GetCollectStatsConsent();
+  if (enabled != updated_pref)
+    DVLOG(1) << "Unable to set metrics reporting status to " << enabled;
 
-  if (enabled != update_pref)
-    DVLOG(1) << "Unable to set crash report status to " << enabled;
+  return updated_pref;
+}
 
+// Does the necessary changes for MetricsReportingEnabled changes which needs
+// to be done in the main thread.
+void SetMetricsReporting(bool enabled,
+                         const base::Callback<void(bool)> callback_fn,
+                         bool updated_pref) {

[Alexei Svitkine (slow), 2014/09/09 20:30:04]

Do we need both updated_pref and enabled params?

I think we should just use the last param and remove the first one.

[gayane -on leave until 09-2017, 2014/09/10 16:58:27]

We need it for understanding the success of the change to record to histogram.
Other option would be to record the error in the previous function and here
record whatever is the new value.

   // Only change the pref if GoogleUpdateSettings::GetCollectStatsConsent
   // succeeds.
-  enabled = update_pref;
-
   metrics::MetricsService* metrics = g_browser_process->metrics_service();
   if (metrics) {
-    if (enabled)
+    if (updated_pref)
       metrics->Start();
     else
       metrics->Stop();
   }
+#if !defined(OS_CHROMEOS)
+  g_browser_process->local_state()->SetBoolean(
+      prefs::kMetricsReportingEnabled, enabled);
+#endif
+  if (enabled == updated_pref) {
+    RecordMetricsReportingHistogramValue(enabled ? METRICS_REPORTING_ENABLED :
+        METRICS_REPORTING_DISABLED);
+  } else {
+    RecordMetricsReportingHistogramValue(METRICS_REPORTING_ERROR);
+  }
+  if (!callback_fn.is_null())
+    callback_fn.Run(updated_pref);
+}
+
+} // namespace
+
+void InitiateMetricsReportingChange(
+    bool enabled,
+    const base::Callback<void(bool)> callback_fn) {
+  if (!IsMetricsReportingUserChangable()) {
+    if (!callback_fn.is_null())
+      callback_fn.Run(false);

[Alexei Svitkine (slow), 2014/09/09 20:30:05]

This isn't correct anymore since the value is now the uma_enabled state, not
success. Please fix.

[gayane -on leave until 09-2017, 2014/09/10 16:58:27]

Done.

+    return;
+  }
+
+  // Posts to FILE thread as SetGoogleUpdateSettings does IO operations.
+  content::BrowserThread::PostTaskAndReplyWithResult(
+      content::BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&SetGoogleUpdateSettings, enabled),
+      base::Bind(&SetMetricsReporting, enabled, callback_fn));
+}
+
+bool IsMetricsReportingUserChangable() {
+  const PrefService* pref_service = g_browser_process->local_state();
 
-  return enabled;
+  const PrefService::Preference* pref =
+      pref_service->FindPreference(prefs::kMetricsReportingEnabled);
+  return pref && pref->IsManaged();
 }