Consolidates accessing and setting the UMA pref

to be within metrics code

MetricsReportingEnabled checkbox changed code refactored for not-chromeos:
-prefs removed from HTML
-Handler and callback functions added to core_options_handler
-Major refactoring in metrics_reporting_state
-Simple histogram added in metrics_reporting_state without distinguishing the source of the change
-browser_options_handler changed to handle new way of forcing company policies of disabling the checkbox

BUG=401233

NOPRESUBMIT=true
R=asvitkine@chromium.org

Review URL: https://codereview.chromium.org/506663003

Patch from Gayane Petrosyan <gayane@chromium.org>.

Cr-Commit-Position: refs/heads/master@{#295124}

chrome/browser/chromeos/login/wizard_controller.cc

@@ -591,17 +591,10 @@ void WizardController::OnUpdateCompleted() {
 void WizardController::OnEulaAccepted() {
   time_eula_accepted_ = base::Time::Now();
   StartupUtils::MarkEulaAccepted();
-  bool uma_enabled =
-      ResolveMetricsReportingEnabled(usage_statistics_reporting_);
-
-  CrosSettings::Get()->SetBoolean(kStatsReportingPref, uma_enabled);
-  if (uma_enabled) {
-#if defined(GOOGLE_CHROME_BUILD)
-    // The crash reporter initialization needs IO to complete.
-    base::ThreadRestrictions::ScopedAllowIO allow_io;
-    breakpad::InitCrashReporter(std::string());
-#endif
-  }
+  InitiateMetricsReportingChange(
+      usage_statistics_reporting_,
+      base::Bind(&WizardController::InitiateMetricsReportingChangeCallback,
+                 weak_factory_.GetWeakPtr()));
 
   if (skip_update_enroll_after_eula_) {
     PerformPostEulaActions();
@@ -611,6 +604,18 @@ void WizardController::OnEulaAccepted() {
   }
 }
 
+void WizardController::InitiateMetricsReportingChangeCallback(bool enabled) {
+  CrosSettings::Get()->SetBoolean(kStatsReportingPref, enabled);
+  if (!enabled)
+    return;
+#if defined(GOOGLE_CHROME_BUILD)
+  // The crash reporter initialization needs IO to complete.
+  base::ThreadRestrictions::ScopedAllowIO allow_io;
+  breakpad::InitCrashReporter(std::string());
+#endif
+
+}
+
 void WizardController::OnUpdateErrorCheckingForUpdate() {
   // TODO(nkostylev): Update should be required during OOBE.
   // We do not want to block users from being able to proceed to the login

chrome/browser/chromeos/login/wizard_controller.h

@@ -215,6 +215,9 @@ class WizardController : public ScreenObserver, public ScreenManager {
   void OnControllerPairingFinished();
   void OnHostPairingFinished();
 
+  // Callback function after setting MetricsReporting.
+  void InitiateMetricsReportingChangeCallback(bool enabled);
+
   // Loads brand code on I/O enabled thread and stores to Local State.
   void LoadBrandCodeFromFile();
 

chrome/browser/chromeos/settings/device_settings_provider.cc

@@ -848,7 +848,7 @@ void DeviceSettingsProvider::ApplyMetricsSetting(bool use_file,
           << "(use file : " << use_file << ")";
   // TODO(pastarmovj): Remove this once we don't need to regenerate the
   // consent file for the GUID anymore.
-  ResolveMetricsReportingEnabled(new_value);
+  InitiateMetricsReportingChange(new_value, OnMetricsReportingCallbackType());
 }
 
 void DeviceSettingsProvider::ApplySideEffects(

chrome/browser/metrics/chrome_metrics_service_accessor.h

@@ -10,6 +10,7 @@
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "chrome/browser/metrics/metrics_reporting_state.h"
 #include "chrome/browser/metrics/metrics_service_accessor.h"
 
 class PrefService;
@@ -40,6 +41,10 @@ namespace system_logs {
 class ChromeInternalLogSource;
 }
 
+namespace options {
+class BrowserOptionsHandler;
+}
+
 // This class limits and documents access to metrics service helper methods.
 // Since these methods are private, each user has to be explicitly declared
 // as a 'friend' below.
@@ -58,6 +63,9 @@ class ChromeMetricsServiceAccessor : public MetricsServiceAccessor {
   friend class ::FlashDOMHandler;
   friend class system_logs::ChromeInternalLogSource;
   friend class UmaSessionStats;
+  friend class options::BrowserOptionsHandler;
+  friend void InitiateMetricsReportingChange(
+      bool, const OnMetricsReportingCallbackType&);
 
   FRIEND_TEST_ALL_PREFIXES(ChromeMetricsServiceAccessorTest,
                            MetricsReportingEnabled);

chrome/browser/metrics/metrics_reporting_state.cc

@@ -4,33 +4,99 @@
 
 #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/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
+};
+
+void RecordMetricsReportingHistogramValue(
+    MetricsReportingChangeHistogramValue value) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "UMA.MetricsReporting.Toggle", value, METRICS_REPORTING_MAX);
+}
 
-  if (enabled != update_pref)
-    DVLOG(1) << "Unable to set crash report status to " << enabled;
+// 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;
 
-  // Only change the pref if GoogleUpdateSettings::GetCollectStatsConsent
-  // succeeds.
-  enabled = update_pref;
+  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) && !defined(OS_ANDROID)
+  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 !defined(OS_CHROMEOS) && !defined(OS_ANDROID)
+  if (!IsMetricsReportingUserChangable()) {
+    if (!callback_fn.is_null()) {
+      callback_fn.Run(
+          ChromeMetricsServiceAccessor::IsMetricsReportingEnabled());
+    }
+    return;
+  }
+#endif
+  // 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));
+}
 
-  return enabled;
+bool IsMetricsReportingUserChangable() {
+  const PrefService* pref_service = g_browser_process->local_state();
+  const PrefService::Preference* pref =
+      pref_service->FindPreference(prefs::kMetricsReportingEnabled);
+  return pref && !pref->IsManaged();
 }

chrome/browser/metrics/metrics_reporting_state.h

@@ -5,12 +5,22 @@
 #ifndef CHROME_BROWSER_METRICS_METRICS_REPORTING_STATE_H_
 #define CHROME_BROWSER_METRICS_METRICS_REPORTING_STATE_H_
 
-// Tries to set crash stats upload consent to |enabled|. Regardless of success,
-// if crash stats uploading is enabled, starts the MetricsService; otherwise
-// stops it.
-// Returns whether crash stats uploading is enabled.
-// TODO(tfarina): This method is pretty confusing. Clean this up and rename it
-// to something that makes more sense.
-bool ResolveMetricsReportingEnabled(bool enabled);
+#include "base/callback.h"
+
+typedef base::Callback<void(bool)> OnMetricsReportingCallbackType;
+
+// Initiates a change to metrics reporting state to the new value of |enabled|.
+// Starts or stops the metrics service based on the new state and then runs
+// |callback_fn| (which can be null) with the updated state (as the operation
+// may fail). On platforms other than CrOS and Android, also updates the
+// underlying pref.
+// TODO(gayane): Support setting the pref on all platforms.
+void InitiateMetricsReportingChange(
+    bool enabled,
+    const OnMetricsReportingCallbackType& callback_fn);
+
+// Returns whether MetricsReporting can be modified by the user (except CrOS and
+// Android).
+bool IsMetricsReportingUserChangable();
 
 #endif  // CHROME_BROWSER_METRICS_METRICS_REPORTING_STATE_H_

chrome/browser/resources/options/browser_options.html

@@ -367,12 +367,11 @@
 </if>
 <if expr="not chromeos">
         <label>
-          <input id="metricsReportingEnabled"
-              pref="user_experience_metrics.reporting_enabled" type="checkbox">
+          <input id="metricsReportingEnabled" type="checkbox">
           <span i18n-content="enableLogging">
         </label>
-        <span class="controlled-setting-indicator"
-            pref="user_experience_metrics.reporting_enabled">
+        <span id="metrics-reporting-disabled-icon"
+            class="controlled-setting-indicator">
         </span>
         <span id="metrics-reporting-reset-restart">
           <!-- Text filled by JavaScript -->

chrome/browser/resources/options/browser_options.js

@@ -375,14 +375,19 @@ cr.define('options', function() {
         restartElements[1].onclick = function(event) {
           chrome.send('restartBrowser');
         };
+        // Attach the listener for updating the checkbox and restart button.
         var updateMetricsRestartButton = function() {
           $('metrics-reporting-reset-restart').hidden =
               loadTimeData.getBoolean('metricsReportingEnabledAtStart') ==
                   $('metricsReportingEnabled').checked;
         };
-        Preferences.getInstance().addEventListener(
-            $('metricsReportingEnabled').getAttribute('pref'),
-            updateMetricsRestartButton);
+        $('metricsReportingEnabled').onclick = function(event) {
+          chrome.send('metricsReportingCheckboxChanged',
+              [Boolean(event.currentTarget.checked)]);
+          updateMetricsRestartButton();
+        };
+        $('metricsReportingEnabled').checked =
+            loadTimeData.getBoolean('metricsReportingEnabledAtStart');
         updateMetricsRestartButton();
       }
       $('networkPredictionOptions').onchange = function(event) {
@@ -1498,6 +1503,13 @@ cr.define('options', function() {
     setMetricsReportingCheckboxState_: function(checked, disabled) {
       $('metricsReportingEnabled').checked = checked;
       $('metricsReportingEnabled').disabled = disabled;
+
+      // If checkbox gets disabled then add an attribute for displaying the
+      // special icon. The opposite shouldn't be possible to do.
+      if (disabled) {
+        $('metrics-reporting-disabled-icon').setAttribute('controlled-by',
+                                                          'policy');
+      }
     },
 
     /**

chrome/browser/ui/views/session_crashed_bubble_view.cc

@@ -423,11 +423,7 @@ void SessionCrashedBubbleView::RestorePreviousSession(views::Button* sender) {
   // Record user's choice for opting in to UMA.
   // There's no opting-out choice in the crash restore bubble.
   if (uma_option_ && uma_option_->checked()) {
-    // TODO: Clean up function ResolveMetricsReportingEnabled so that user pref
-    // is stored automatically.
-    ResolveMetricsReportingEnabled(true);
-    g_browser_process->local_state()->SetBoolean(
-        prefs::kMetricsReportingEnabled, true);
+    InitiateMetricsReportingChange(true, OnMetricsReportingCallbackType());
     RecordBubbleHistogramValue(SESSION_CRASHED_BUBBLE_UMA_OPTIN);
   }
   CloseBubble();

chrome/browser/ui/webui/options/browser_options_handler.cc

@@ -30,6 +30,8 @@
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/gpu/gpu_mode_manager.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
+#include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
+#include "chrome/browser/metrics/metrics_reporting_state.h"
 #include "chrome/browser/net/prediction_options.h"
 #include "chrome/browser/prefs/session_startup_pref.h"
 #include "chrome/browser/printing/cloud_print/cloud_print_proxy_service.h"
@@ -543,9 +545,8 @@ void BrowserOptionsHandler::GetLocalizedValues(base::DictionaryValue* values) {
   values->SetString("doNotTrackLearnMoreURL", chrome::kDoNotTrackLearnMoreURL);
 
 #if !defined(OS_CHROMEOS)
-  PrefService* pref_service = g_browser_process->local_state();
-  values->SetBoolean("metricsReportingEnabledAtStart", pref_service->GetBoolean(
-      prefs::kMetricsReportingEnabled));
+  values->SetBoolean("metricsReportingEnabledAtStart",
+       ChromeMetricsServiceAccessor::IsMetricsReportingEnabled());
 #endif
 
 #if defined(OS_CHROMEOS)
@@ -756,6 +757,9 @@ void BrowserOptionsHandler::RegisterMessages() {
           &BrowserOptionsHandler::HandleRefreshExtensionControlIndicators,
           base::Unretained(this)));
 #endif  // defined(OS_WIN)
+  web_ui()->RegisterMessageCallback("metricsReportingCheckboxChanged",
+      base::Bind(&BrowserOptionsHandler::HandleMetricsReportingChange,
+                 base::Unretained(this)));
 }
 
 void BrowserOptionsHandler::Uninitialize() {
@@ -924,6 +928,7 @@ void BrowserOptionsHandler::InitializePage() {
   UpdateDefaultBrowserState();
 
   SetupMetricsReportingSettingVisibility();
+  SetupMetricsReportingCheckbox();
   SetupNetworkPredictionControl();
   SetupFontSizeSelector();
   SetupPageZoomSelector();
@@ -1887,4 +1892,38 @@ void BrowserOptionsHandler::SetupExtensionControlledIndicators() {
 #endif  // defined(OS_WIN)
 }
 
+void BrowserOptionsHandler::SetupMetricsReportingCheckbox() {
+  // This function does not work for ChromeOS and non-official builds.
+#if !defined(OS_CHROMEOS) && defined(GOOGLE_CHROME_BUILD)
+  bool checked = ChromeMetricsServiceAccessor::IsMetricsReportingEnabled();
+  bool disabled = !IsMetricsReportingUserChangable();
+
+  SetMetricsReportingCheckbox(checked, disabled);
+#endif
+}
+
+void BrowserOptionsHandler::HandleMetricsReportingChange(
+    const base::ListValue* args) {
+  bool enable;
+  if (!args->GetBoolean(0, &enable))
+    return;
+
+  InitiateMetricsReportingChange(
+      enable,
+      base::Bind(&BrowserOptionsHandler::MetricsReportingChangeCallback,
+                 base::Unretained(this)));
+}
+
+void BrowserOptionsHandler::MetricsReportingChangeCallback(bool enabled) {
+  SetMetricsReportingCheckbox(enabled, !IsMetricsReportingUserChangable());
+}
+
+void BrowserOptionsHandler::SetMetricsReportingCheckbox(bool checked,
+                                                        bool disabled) {
+  web_ui()->CallJavascriptFunction(
+      "BrowserOptions.setMetricsReportingCheckboxState",
+      base::FundamentalValue(checked),
+      base::FundamentalValue(disabled));
+}
+
 }  // namespace options

chrome/browser/ui/webui/options/browser_options_handler.h

@@ -332,6 +332,20 @@ class BrowserOptionsHandler
   // Setup the UI for showing which settings are extension controlled.
   void SetupExtensionControlledIndicators();
 
+  // Setup the value and the disabled property for metrics reporting for (except
+  // CrOS and Android).
+  void SetupMetricsReportingCheckbox();
+
+  // Called when the MetricsReportingEnabled checkbox values are changed.
+  // |args| will contain the checkbox checked state as a boolean.
+  void HandleMetricsReportingChange(const base::ListValue* args);
+
+  // Notifies the result of MetricsReportingEnabled change to Javascript layer.
+  void MetricsReportingChangeCallback(bool enabled);
+
+  // Calls a Javascript function to set the state of MetricsReporting checkbox.
+  void SetMetricsReportingCheckbox(bool checked, bool disabled);
+
 #if defined(OS_CHROMEOS)
   // Setup the accessibility features for ChromeOS.
   void SetupAccessibilityFeatures();

chrome/browser/ui/webui/options/core_options_handler.cc

@@ -16,7 +16,6 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_util.h"
-#include "chrome/browser/metrics/metrics_reporting_state.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
@@ -43,18 +42,6 @@ namespace options {
 
 namespace {
 
-// Only allow changes to the metrics reporting checkbox if we were succesfully
-// able to change the service.
-bool AllowMetricsReportingChange(const base::Value* to_value) {
-  bool enable;
-  if (!to_value->GetAsBoolean(&enable)) {
-    NOTREACHED();
-    return false;
-  }
-
-  return enable == ResolveMetricsReportingEnabled(enable);
-}
-
 // Whether "controlledBy" property of pref value sent to options web UI needs to
 // be set to "extension" when the preference is controlled by an extension.
 bool CanSetExtensionControlledPrefValue(
@@ -90,8 +77,6 @@ void CoreOptionsHandler::InitializeHandler() {
                  base::Unretained(this),
                  profile->GetPrefs()));
 
-  pref_change_filters_[prefs::kMetricsReportingEnabled] =
-      base::Bind(&AllowMetricsReportingChange);
   pref_change_filters_[prefs::kBrowserGuestModeEnabled] =
       base::Bind(&CoreOptionsHandler::IsUserUnsupervised,
                  base::Unretained(this));

components/metrics/metrics_state_manager.cc

@@ -12,6 +12,7 @@
 #include "base/prefs/pref_service.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "components/metrics/cloned_install_detector.h"
 #include "components/metrics/machine_id_provider.h"
@@ -218,6 +219,9 @@ void MetricsStateManager::RegisterPrefs(PrefRegistrySimple* registry) {
 }
 
 void MetricsStateManager::BackUpCurrentClientInfo() {
+  // TODO(gayane): Eliminate use of ScopedAllowIO. crbug.com/413783
+  base::ThreadRestrictions::ScopedAllowIO allow_io;
+
   ClientInfo client_info;
   client_info.client_id = client_id_;
   client_info.installation_date = local_state_->GetInt64(prefs::kInstallDate);

tools/metrics/histograms/histograms.xml

@@ -35110,6 +35110,14 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   </summary>
 </histogram>
 
+<histogram name="UMA.MetricsReporting.Toggle" enum="MetricsReportingChange">
+  <owner>asvitkine@chromium.org</owner>
+  <summary>
+    Logged when user successfully enables/disables MetricsReporting or when an
+    error occurs.
+  </summary>
+</histogram>
+
 <histogram name="UMA.Perf.GetData" enum="GetPerfDataOutcome">
   <owner>asvitkine@chromium.org</owner>
   <summary>
@@ -45728,6 +45736,14 @@ To add a new entry, add it with any value and run test to compute valid value.
   <int value="6" label="XHTML-MP document type"/>
 </enum>
 
+<enum name="MetricsReportingChange" type="int">
+  <int value="0" label="Error">
+    Error occurred while updating MetricsReporting
+  </int>
+  <int value="1" label="Disabled successfully"/>
+  <int value="2" label="Enabled successfully"/>
+</enum>
+
 <enum name="MigrationNssToPemNetworkTypes" type="int">
   <int value="0" label="EAP"/>
   <int value="1" label="OpenVPN"/>