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..f5359c383227d93c8d6cd107e82b7c28f2606613 100644
--- a/chrome/browser/metrics/metrics_reporting_state.cc
+++ b/chrome/browser/metrics/metrics_reporting_state.cc
@@ -4,33 +4,115 @@
 
 #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/policy/browser_policy_connector_chromeos.h"
+#include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/installer/util/google_update_settings.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 {
 
-  GoogleUpdateSettings::SetCollectStatsConsent(enabled);
-  bool update_pref = GoogleUpdateSettings::GetCollectStatsConsent();
+enum MetricsReportingChangeHistogramValue {
+  METRICS_REPORTING_ERROR,
+  METRICS_REPORTING_DISABLED,
+  METRICS_REPORTING_ENABLED,
+  METRICS_REPORTING_MAX
+};
 
-  if (enabled != update_pref)
-    DVLOG(1) << "Unable to set crash report status to " << enabled;
+void RecordMetricsReportingHistogramValue(
+    MetricsReportingChangeHistogramValue value) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "UMA.MetricsReporting.Toggle", value, METRICS_REPORTING_MAX);
+}
 
-  // Only change the pref if GoogleUpdateSettings::GetCollectStatsConsent
-  // succeeds.
-  enabled = update_pref;
+// Tries to set metrics reporting status to |enabled| and returns whatever is
+// the result of the update.
+bool SetGoogleUpdateSettings(bool enabled) {
+  GoogleUpdateSettings::SetCollectStatsConsent(enabled);
+  bool updated_pref = GoogleUpdateSettings::GetCollectStatsConsent();
+  if (enabled != updated_pref)
+    DVLOG(1) << "Unable to set metrics reporting status to " << enabled;
 
+  return updated_pref;
+}
+
+// Does the necessary changes for MetricsReportingEnabled changes which needs
+// to be done in the main thread.
+// As arguments this function gets:
+//  |to_update_pref| which indicates what the desired update should be,
+//  |callback_fn| is the callback function to be called in the end
+//  |updated_pref| is the result of attempted update.
+// Update considers to be successful if |to_update_pref| and |updated_pref| are
+// the same.
+void SetMetricsReporting(bool to_update_pref,
+                         const OnMetricsReportingCallbackType& callback_fn,
+                         bool updated_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)

[Alexei Svitkine (slow), 2014/09/15 21:36:44]

I would also expand this ifdef to be !ANDROID too.

[gayane -on leave until 09-2017, 2014/09/16 17:28:52]

Done.

+  g_browser_process->local_state()->SetBoolean(
+      prefs::kMetricsReportingEnabled, updated_pref);
+#endif
+  if (to_update_pref == updated_pref) {
+    RecordMetricsReportingHistogramValue(updated_pref ?
+        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 OnMetricsReportingCallbackType& callback_fn) {
+  if (!IsMetricsReportingUserChangable()) {
+    if (!callback_fn.is_null()) {
+      callback_fn.Run(
+          ChromeMetricsServiceAccessor::IsMetricsReportingEnabled());
+    }
+    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() {
+#if defined(GOOGLE_CHROME_BUILD)
+#if defined(OS_CHROMEOS)
+  policy::BrowserPolicyConnectorChromeOS* connector =
+        g_browser_process->platform_part()->browser_policy_connector_chromeos();
+  return connector->IsEnterpriseManaged();
+#else
+  const PrefService* pref_service = g_browser_process->local_state();
+#if defined(OS_ANDROID)

[Alexei Svitkine (slow), 2014/09/15 20:59:43]

Why is all of this needed?

I thought this function only gets called on non-CrOS non-Android platforms
currently (and your comment for this function mentions that it shouldn't be used
on those).

Is that not the case? (If so, can you clarify when it gets called on those other
platforms).

[gayane -on leave until 09-2017, 2014/09/15 21:09:17]

So the thing is that initially the plan was to use this function for Checkbox
setup for non-Chrome non-Andriod, but then we added a check in
InitiateMetricsReportingChange. This function can be called and is called from
ChromeOS right now. I am not sure about Andrioid. I added it just in case.

Changed the comment for this function.

[Alexei Svitkine (slow), 2014/09/15 21:36:44]

I see. But InitiateMetricsReportingChange() doesn't touch the pref on those
platforms yet. So I suggest just ifdefing the code in
InitiateMetricsReportingChange() to correspond to the ifdef around setting
prefs::kMetricsReportingEnabled and revert this function back to the simpler
version.

(In future CLs when we make this code support those platforms completely, we can
expand this - but I'm hoping that can be done with fewer ifdefs.)

[gayane -on leave until 09-2017, 2014/09/16 17:28:52]

Done.

+  const PrefService::Preference* pref =
+      pref_service->FindPreference(prefs::kCrashReportingEnabled);
+#else
+  const PrefService::Preference* pref =
+      pref_service->FindPreference(prefs::kMetricsReportingEnabled);
+#endif
+  return pref && !pref->IsManaged();
+#endif
+#else
+  return true;
+#endif
 
-  return enabled;
 }