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..c7ee61cf546d1cefa59438c4ea479e38e256609f 100644
--- a/chrome/browser/metrics/metrics_reporting_state.cc
+++ b/chrome/browser/metrics/metrics_reporting_state.cc
@@ -4,33 +4,90 @@
 
 #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"

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

Do you need the CrOS includes (this one and below) with your current change?

[gayane -on leave until 09-2017, 2014/09/08 18:32:46]

Done.

+#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 {
 
-  GoogleUpdateSettings::SetCollectStatsConsent(enabled);
-  bool update_pref = GoogleUpdateSettings::GetCollectStatsConsent();
+enum MetricsReportingChangeHistogramValue {
+  METRICS_REPORTING_DISABLED,
+  METRICS_REPORTING_ENABLED,
+  METRICS_REPORTING_MAX
+};
+
+void RecordMetricsReportingHistogramValue(
+    MetricsReportingChangeHistogramValue value) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "UMA.MetricsReporting", value, METRICS_REPORTING_MAX);
+}
 
-  if (enabled != update_pref)
+// Tries to set crash stats upload consent to |enabled|.
+bool SetGoogleUpdateSettings(bool enabled) {
+  GoogleUpdateSettings::SetCollectStatsConsent(enabled);
+  bool updated_pref = GoogleUpdateSettings::GetCollectStatsConsent();
+  bool success = (enabled == updated_pref);
+  if (!success)
     DVLOG(1) << "Unable to set crash report status to " << enabled;

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

Nit: Update this message while you're changing this. "crash report" -> "metrics
reporting".

Also, I suggest adding another enum entry to the histogram to indicate this
error and logging it.

[gayane -on leave until 09-2017, 2014/09/08 18:32:46]

Done.

 
+  return success;
+}
+
+// Does the necessary changes for MetricsReportingEnabled changes which needs
+// to be done in the main thread.
+void SetMetricsReporting(bool enabled,
+                         base::Callback<void(bool)> callback_fn,

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

Nit: pass the callback by const ref.

[gayane -on leave until 09-2017, 2014/09/08 18:32:46]

Done.

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

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

Nit: pass the callback by const ref.

[gayane -on leave until 09-2017, 2014/09/08 18:32:46]

Done.

+  if (!IsMetricsReportingUserChangable()) {
+    if (!callback_fn.is_null())
+      callback_fn.Run(false);
+    return;
+  }
+
+  // Posts on FILE thread as SetGoogleUpdateSettings modifies the local state.

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

This comment is incorrect. "Local State" in fact needs to be modified on the UI
thread (a background task actually does the write to disk) and you're doing that
in the correct place. The other function does IO, which is why it needs to be
done on the file thread.

[gayane -on leave until 09-2017, 2014/09/08 18:32:46]

Done.

On 2014/09/05 20:50:24, Alexei Svitkine wrote:

+  content::BrowserThread::PostTaskAndReplyWithResult(
+      content::BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&SetGoogleUpdateSettings, enabled),
+      base::Bind(&SetMetricsReporting, enabled, callback_fn));
+}
+
+bool IsMetricsReportingUserChangable() {

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

This isn't correct for CrOS or Android.

1. We shouldn't use it above for those platforms.
2. We should document this in the .h file.

[gayane -on leave until 09-2017, 2014/09/08 18:32:46]

- Comment added in the .h file for this function and the one which is calling
it.
- The call of the function which calls this one (SetupMetricsReportingCheckbox()
in BrowserOptionHandler) moved under #if !defined(OS_CHROMEOS).

On 2014/09/05 20:50:24, Alexei Svitkine wrote:

+  const PrefService* pref_service = g_browser_process->local_state();
 
-  return enabled;
+  const PrefService::Preference* pref =
+      pref_service->FindPreference(prefs::kMetricsReportingEnabled);
+  return pref && pref->IsManaged();
 }