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"

[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 {
 
+enum MetricsReportingChangeHistogramValue {
+  METRICS_REPORTING_DISABLED,
+  METRICS_REPORTING_ENABLED,
+  METRICS_REPORTING_MAX
+};
+
+void RecordMetricsReportingHistogramValue(
+    MetricsReportingChangeHistogramValue value) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "UMA.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;

[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 (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);
+}
 
-  metrics::MetricsService* metrics = g_browser_process->metrics_service();
-  if (metrics) {
-    if (enabled)
-      metrics->Start();
-    else
-      metrics->Stop();
+} // 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;
   }
 
-  return enabled;
+  // 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();
+
+  const PrefService::Preference* pref =
+      pref_service->FindPreference(prefs::kMetricsReportingEnabled);
+  return pref && pref->IsManaged();
+}