[OsSettingsMetrics] Add new RecordSettingChange and new metric

Adds a new version of the RecordSettingChange function that takes a
mojom::Setting and the value of the setting change. Marks the old
function for deprecation and uses the new function in the user action
tracker. Adds in a top level metric to record which settings were
changed. Also adds in metrics for the Files Page.

Bug: 1133553
Change-Id: Ie232d6375c66dabfd8493d4657c8bacea915fc1e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2441950Reviewed-by: Regan Hsu <hsuregan@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Daniel Classon <dclasson@google.com>
Cr-Commit-Position: refs/heads/master@{#815748}

chrome/browser/chromeos/BUILD.gn

@@ -3749,6 +3749,7 @@ source_set("unit_tests") {
     "../ui/webui/settings/chromeos/search/per_session_settings_user_action_tracker_unittest.cc",
     "../ui/webui/settings/chromeos/search/search_handler_unittest.cc",
     "../ui/webui/settings/chromeos/search/search_tag_registry_unittest.cc",
+    "../ui/webui/settings/chromeos/settings_user_action_tracker_unittest.cc",
   ]
   if (use_cups) {
     sources += [

chrome/browser/resources/settings/chromeos/bluetooth_page/BUILD.gn

@@ -17,6 +17,7 @@ js_type_check("closure_compile") {
 js_library("bluetooth_page") {
   deps = [
     "..:deep_linking_behavior",
+    "..:metrics_recorder",
     "..:os_route",
     "../..:router",
     "../../prefs:prefs_behavior",
@@ -94,6 +95,7 @@ js_library("bluetooth_page.m") {
   deps = [
     ":bluetooth_subpage.m",
     "..:deep_linking_behavior.m",
+    "..:metrics_recorder.m",
     "..:os_route.m",
     "../..:router.m",
     "../../prefs:prefs_behavior.m",

chrome/browser/resources/settings/chromeos/bluetooth_page/bluetooth_page.html

@@ -10,6 +10,7 @@
 <link rel="import" href="../../prefs/prefs.html">
 <link rel="import" href="../../prefs/prefs_behavior.html">
 <link rel="import" href="../deep_linking_behavior.html">
+<link rel="import" href="../metrics_recorder.html">
 <link rel="import" href="../os_route.html">
 <link rel="import" href="../../router.html">
 <link rel="import" href="../../settings_page/settings_animated_pages.html">
@@ -55,7 +56,8 @@
                 disabled$="[[!isToggleEnabled_(
                     adapterState_, stateChangeInProgress_)]]"
                 aria-label="$i18n{bluetoothToggleA11yLabel}"
-                deep-link-focus-id$="[[Setting.kBluetoothOnOff]]">
+                deep-link-focus-id$="[[Setting.kBluetoothOnOff]]"
+                on-change="onBluetoothToggledByUser_">
             </cr-toggle>
           </div>
         </template>

chrome/browser/resources/settings/chromeos/bluetooth_page/bluetooth_page.js

@@ -238,6 +238,18 @@ Polymer({
     }
   },
 
+  /**
+   * Listens for toggle change events (rather than state changes) to handle
+   * just user-triggered changes to the bluetoothToggleState_.
+   * @private
+   */
+  onBluetoothToggledByUser_() {
+    // Record that the user manually enabled/disabled Bluetooth.
+    settings.recordSettingChange(
+        chromeos.settings.mojom.Setting.kBluetoothOnOff,
+        {boolValue: this.bluetoothToggleState_});
+  },
+
   /** @private */
   onTap_() {
     if (!this.isToggleEnabled_()) {

chrome/browser/resources/settings/chromeos/metrics_recorder.html

 <link rel="import" href="chrome://resources/html/cr.html">
 <link rel="import" href="chrome://resources/mojo/mojo/public/js/mojo_bindings_lite.html">
 
+<script src="chrome://os-settings/constants/setting.mojom-lite.js"></script>
 <script src="chrome://os-settings/search/user_action_recorder.mojom-lite.js"></script>
 <script src="metrics_recorder.js"></script>

chrome/browser/resources/settings/chromeos/metrics_recorder.js

@@ -57,8 +57,22 @@ cr.define('settings', function() {
     getRecorder().recordSearch();
   }
 
-  /* #export */ function recordSettingChange() {
-    getRecorder().recordSettingChange();
+  /**
+   * All new code should pass a value for |opt_setting| and, if applicable,
+   * |opt_value|. The zero-parameter version of this function is reserved for
+   * legacy code which has not yet been converted.
+   * TODO(https://crbug.com/1133553): make |opt_setting| non-optional when
+   * migration is complete.
+   * @param {!chromeos.settings.mojom.Setting=} opt_setting
+   * @param {!chromeos.settings.mojom.SettingChangeValue=} opt_value
+   */
+  /* #export */ function recordSettingChange(opt_setting, opt_value) {
+    if (opt_setting === undefined) {
+      getRecorder().recordSettingChange();
+    } else {
+      getRecorder().recordSettingChangeWithDetails(
+          opt_setting, opt_value || null);
+    }
   }
 
   // #cr_define_end

chrome/browser/ui/webui/settings/chromeos/bluetooth_section.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/webui/settings/chromeos/bluetooth_section.h"
 
 #include "base/bind.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/no_destructor.h"
 #include "chrome/browser/ui/webui/chromeos/bluetooth_dialog_localized_strings_provider.h"
 #include "chrome/browser/ui/webui/settings/chromeos/constants/routes.mojom.h"
@@ -209,8 +210,15 @@ std::string BluetoothSection::GetSectionPath() const {
 
 bool BluetoothSection::LogMetric(mojom::Setting setting,
                                  base::Value& value) const {
-  // Unimplemented.
-  return false;
+  switch (setting) {
+    case mojom::Setting::kBluetoothOnOff:
+      base::UmaHistogramBoolean("ChromeOS.Settings.Bluetooth.BluetoothOnOff",
+                                value.GetBool());
+      return true;
+
+    default:
+      return false;
+  }
 }
 
 void BluetoothSection::RegisterHierarchy(HierarchyGenerator* generator) const {

chrome/browser/ui/webui/settings/chromeos/fake_os_settings_section.cc

@@ -34,7 +34,8 @@ std::string FakeOsSettingsSection::GetSectionPath() const {
 
 bool FakeOsSettingsSection::LogMetric(mojom::Setting setting,
                                       base::Value& value) const {
-  return false;
+  logged_metrics_.push_back(setting);
+  return true;
 }
 
 std::string FakeOsSettingsSection::ModifySearchResultUrl(

chrome/browser/ui/webui/settings/chromeos/fake_os_settings_section.h

@@ -5,6 +5,9 @@
 #ifndef CHROME_BROWSER_UI_WEBUI_SETTINGS_CHROMEOS_FAKE_OS_SETTINGS_SECTION_H_
 #define CHROME_BROWSER_UI_WEBUI_SETTINGS_CHROMEOS_FAKE_OS_SETTINGS_SECTION_H_
 
+#include <utility>
+#include <vector>
+
 #include "base/values.h"
 #include "chrome/browser/ui/webui/settings/chromeos/os_settings_section.h"
 
@@ -21,6 +24,9 @@ class FakeOsSettingsSection : public OsSettingsSection {
   FakeOsSettingsSection& operator=(const FakeOsSettingsSection& other) = delete;
 
   mojom::Section section() { return section_; }
+  const std::vector<mojom::Setting>& logged_metrics() const {
+    return logged_metrics_;
+  }
 
   // OsSettingsSection:
   void AddLoadTimeData(content::WebUIDataSource* html_source) override {}
@@ -50,6 +56,8 @@ class FakeOsSettingsSection : public OsSettingsSection {
 
  private:
   const mojom::Section section_;
+  // Use mutable to modify this vector within the overridden const LogMetric.
+  mutable std::vector<mojom::Setting> logged_metrics_;
 };
 
 }  // namespace settings

chrome/browser/ui/webui/settings/chromeos/os_settings_manager.cc

@@ -45,9 +45,10 @@ OsSettingsManager::OsSettingsManager(
                                                identity_manager,
                                                android_sms_service,
                                                printers_manager)),
-      settings_user_action_tracker_(
-          std::make_unique<SettingsUserActionTracker>()),
       hierarchy_(std::make_unique<Hierarchy>(sections_.get())),
+      settings_user_action_tracker_(
+          std::make_unique<SettingsUserActionTracker>(hierarchy_.get(),
+                                                      sections_.get())),
       search_handler_(
           std::make_unique<SearchHandler>(search_tag_registry_.get(),
                                           sections_.get(),
@@ -71,6 +72,7 @@ void OsSettingsManager::Shutdown() {
   // Note: These must be deleted in the opposite order of their creation to
   // prevent against UAF violations.
   search_handler_.reset();
+  settings_user_action_tracker_.reset();
   hierarchy_.reset();
   sections_.reset();
   search_tag_registry_.reset();

chrome/browser/ui/webui/settings/chromeos/os_settings_manager.h

@@ -124,8 +124,8 @@ class OsSettingsManager : public KeyedService {
 
   std::unique_ptr<SearchTagRegistry> search_tag_registry_;
   std::unique_ptr<OsSettingsSections> sections_;
-  std::unique_ptr<SettingsUserActionTracker> settings_user_action_tracker_;
   std::unique_ptr<Hierarchy> hierarchy_;
+  std::unique_ptr<SettingsUserActionTracker> settings_user_action_tracker_;
   std::unique_ptr<SearchHandler> search_handler_;
 };
 

chrome/browser/ui/webui/settings/chromeos/search/user_action_recorder.mojom

@@ -4,6 +4,15 @@
 
 module chromeos.settings.mojom;
 
+import "chrome/browser/ui/webui/settings/chromeos/constants/setting.mojom";
+
+// Value for the setting change; can be a bool, int, or string.
+union SettingChangeValue {
+  bool bool_value;
+  int32 int_value;
+  string string_value;
+};
+
 // Records user actions within OS settings. Implemented in the browser process;
 // intended to be called from settings JS.
 interface UserActionRecorder {
@@ -23,6 +32,13 @@ interface UserActionRecorder {
   // Records that the user has completed a search attempt.
   RecordSearch();
 
-  // Records that the user has changed a setting.
+  // Deprecated; all new usages should use the other RecordSettingChangeValue.
+  // TODO(https://crbug.com/1133553): remove once migration is complete.
   RecordSettingChange();
+
+  // Records that the user has changed a |setting| and the |value| that the
+  // setting was changed to. |value| can be null when the corresponding
+  // setting doesn't actually change a value, such as the open wallpaper
+  // setting.
+  RecordSettingChangeWithDetails(Setting setting, SettingChangeValue? value);
 };

chrome/browser/ui/webui/settings/chromeos/settings_user_action_tracker.cc

@@ -8,11 +8,17 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/metrics/histogram_functions.h"
+#include "chrome/browser/ui/webui/settings/chromeos/hierarchy.h"
+#include "chrome/browser/ui/webui/settings/chromeos/os_settings_sections.h"
 
 namespace chromeos {
 namespace settings {
 
-SettingsUserActionTracker::SettingsUserActionTracker() = default;
+SettingsUserActionTracker::SettingsUserActionTracker(
+    Hierarchy* hierarchy,
+    OsSettingsSections* sections)
+    : hierarchy_(hierarchy), sections_(sections) {}
 
 SettingsUserActionTracker::~SettingsUserActionTracker() = default;
 
@@ -60,9 +66,35 @@ void SettingsUserActionTracker::RecordSearch() {
   per_session_tracker_->RecordSearch();
 }
 
+// TODO(https://crbug.com/1133553): remove this once migration is complete.
 void SettingsUserActionTracker::RecordSettingChange() {
   per_session_tracker_->RecordSettingChange();
 }
 
+void SettingsUserActionTracker::RecordSettingChangeWithDetails(
+    mojom::Setting setting,
+    mojom::SettingChangeValuePtr value) {
+  per_session_tracker_->RecordSettingChange();
+
+  // Get the primary section location of the changed setting and log the metric.
+  mojom::Section section_id =
+      hierarchy_->GetSettingMetadata(setting).primary.first;
+  const OsSettingsSection* section = sections_->GetSection(section_id);
+  // new_value is initialized as null. Null value is used in cases that don't
+  // require extra metadata.
+  base::Value new_value;
+  if (value->is_bool_value()) {
+    new_value = base::Value(value->get_bool_value());
+  } else if (value->is_int_value()) {
+    new_value = base::Value(value->get_int_value());
+  } else if (value->is_string_value()) {
+    new_value = base::Value(value->get_string_value());
+  }
+  section->LogMetric(setting, new_value);
+
+  base::UmaHistogramSparse("ChromeOS.Settings.SettingChanged",
+                           static_cast<int>(setting));
+}
+
 }  // namespace settings
 }  // namespace chromeos

chrome/browser/ui/webui/settings/chromeos/settings_user_action_tracker.h

@@ -7,6 +7,8 @@
 
 #include <memory>
 
+#include "base/optional.h"
+#include "chrome/browser/ui/webui/settings/chromeos/constants/setting.mojom.h"
 #include "chrome/browser/ui/webui/settings/chromeos/search/per_session_settings_user_action_tracker.h"
 #include "chrome/browser/ui/webui/settings/chromeos/search/user_action_recorder.mojom.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
@@ -15,12 +17,15 @@
 namespace chromeos {
 namespace settings {
 
+class Hierarchy;
+class OsSettingsSections;
+
 // Records user actions within Settings. Utilizes a per session tracker that
 // measures the user's effort required to change a setting. Eventually uses
 // a per section tracker to record metrics in each section.
 class SettingsUserActionTracker : public mojom::UserActionRecorder {
  public:
-  SettingsUserActionTracker();
+  SettingsUserActionTracker(Hierarchy* hierarchy, OsSettingsSections* sections);
   SettingsUserActionTracker(const SettingsUserActionTracker& other) = delete;
   SettingsUserActionTracker& operator=(const SettingsUserActionTracker& other) =
       delete;
@@ -30,6 +35,12 @@ class SettingsUserActionTracker : public mojom::UserActionRecorder {
       mojo::PendingReceiver<mojom::UserActionRecorder> pending_receiver);
 
  private:
+  // For unit tests.
+  // SettingsUserActionTracker();
+  friend class SettingsUserActionTrackerTest;
+  FRIEND_TEST_ALL_PREFIXES(SettingsUserActionTrackerTest,
+                           TestRecordSettingChanged);
+
   // mojom::UserActionRecorder:
   void RecordPageFocus() override;
   void RecordPageBlur() override;
@@ -37,12 +48,17 @@ class SettingsUserActionTracker : public mojom::UserActionRecorder {
   void RecordNavigation() override;
   void RecordSearch() override;
   void RecordSettingChange() override;
+  void RecordSettingChangeWithDetails(
+      mojom::Setting setting,
+      mojom::SettingChangeValuePtr value) override;
 
   void EndCurrentSession();
   void OnBindingDisconnected();
 
-  std::unique_ptr<PerSessionSettingsUserActionTracker> per_session_tracker_;
+  Hierarchy* hierarchy_;
+  OsSettingsSections* sections_;
 
+  std::unique_ptr<PerSessionSettingsUserActionTracker> per_session_tracker_;
   mojo::Receiver<mojom::UserActionRecorder> receiver_{this};
 };
 

chrome/browser/ui/webui/settings/chromeos/settings_user_action_tracker_unittest.cc

+// Copyright 2020 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/ui/webui/settings/chromeos/settings_user_action_tracker.h"
+
+#include "base/test/metrics/histogram_tester.h"
+#include "base/values.h"
+#include "chrome/browser/ui/webui/settings/chromeos/constants/setting.mojom.h"
+#include "chrome/browser/ui/webui/settings/chromeos/fake_hierarchy.h"
+#include "chrome/browser/ui/webui/settings/chromeos/fake_os_settings_section.h"
+#include "chrome/browser/ui/webui/settings/chromeos/fake_os_settings_sections.h"
+#include "chrome/browser/ui/webui/settings/chromeos/search/per_session_settings_user_action_tracker.h"
+#include "chrome/browser/ui/webui/settings/chromeos/search/user_action_recorder.mojom.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace settings {
+
+class SettingsUserActionTrackerTest : public testing::Test {
+ protected:
+  SettingsUserActionTrackerTest()
+      : fake_hierarchy_(&fake_sections_),
+        tracker_(&fake_hierarchy_, &fake_sections_) {
+    // Initialize per_session_tracker_ manually since BindInterface is never
+    // called on tracker_.
+    tracker_.per_session_tracker_ =
+        std::make_unique<PerSessionSettingsUserActionTracker>();
+  }
+  ~SettingsUserActionTrackerTest() override = default;
+
+  // testing::Test:
+  void SetUp() override {
+    fake_hierarchy_.AddSettingMetadata(mojom::Section::kBluetooth,
+                                       mojom::Setting::kBluetoothOnOff);
+  }
+
+  base::HistogramTester histogram_tester_;
+  FakeOsSettingsSections fake_sections_;
+  FakeHierarchy fake_hierarchy_;
+  SettingsUserActionTracker tracker_;
+};
+
+TEST_F(SettingsUserActionTrackerTest, TestRecordSettingChanged) {
+  // Record that the bluetooth enabled setting was toggled off.
+  tracker_.RecordSettingChangeWithDetails(
+      mojom::Setting::kBluetoothOnOff,
+      mojom::SettingChangeValue::NewBoolValue(false));
+
+  // The umbrella metric for which setting was changed should be updated. Note
+  // that kBluetoothOnOff has enum value of 100.
+  histogram_tester_.ExpectTotalCount("ChromeOS.Settings.SettingChanged",
+                                     /*count=*/1);
+  histogram_tester_.ExpectBucketCount("ChromeOS.Settings.SettingChanged",
+                                      /*sample=*/100,
+                                      /*count=*/1);
+
+  // The LogMetric fn in the Blutooth section should have been called.
+  const FakeOsSettingsSection* bluetooth_section =
+      static_cast<const FakeOsSettingsSection*>(
+          fake_sections_.GetSection(mojom::Section::kBluetooth));
+  EXPECT_TRUE(bluetooth_section->logged_metrics().back() ==
+              mojom::Setting::kBluetoothOnOff);
+}
+
+}  // namespace settings.
+}  // namespace chromeos.

chrome/test/data/webui/settings/chromeos/fake_user_action_recorder.js

@@ -46,6 +46,10 @@ cr.define('settings', function() {
     recordSettingChange() {
       ++this.settingChangeCount;
     }
+
+    recordSettingChangeWithDetails() {
+      ++this.settingChangeCount;
+    }
   }
 
   // #cr_define_end

tools/metrics/histograms/histograms_xml/chromeos/histograms.xml

@@ -528,6 +528,16 @@ incoming reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
+<histogram name="ChromeOS.Settings.Bluetooth.BluetoothOnOff"
+    enum="BooleanToggled" expires_after="2021-09-30">
+  <owner>khorimoto@chromium.org</owner>
+  <owner>hsuregan@chromium.org</owner>
+  <owner>cros-customization@google.com</owner>
+  <summary>
+    Records when users change the kBluetoothOnOff setting on the Bluetooth page.
+  </summary>
+</histogram>
+
 <histogram name="ChromeOS.Settings.BlurredWindowDuration" units="ms"
     expires_after="2020-12-01">
   <owner>khorimoto@chromium.org</owner>
@@ -778,6 +788,19 @@ incoming reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
+<histogram name="ChromeOS.Settings.SettingChanged" enum="OsSetting"
+    expires_after="2021-09-29">
+  <owner>khorimoto@chromium.org</owner>
+  <owner>hsuregan@chromium.org</owner>
+  <owner>cros-customization@google.com</owner>
+  <summary>
+    Records the the setting id when the user changes a setting. Note that some
+    of the values of the OsSetting enum aren't strictly settings (such as
+    kKeyboardShortcuts, which is just a hyperlink). All of the recorded values
+    appear in the settings page.
+  </summary>
+</histogram>
+
 <histogram base="true" name="ChromeOS.Settings.TimeUntilChange" units="ms"
     expires_after="2021-04-04">
 <!-- Name completed by histogram_suffixes name="OsSettingsChangeType" -->