[CrOS Settings] Fix recording setting change with no value

Opening Scan app from OS Settings does not need to pass an extra value
to recordSettingChangeWithDetails() so underlying C++ function should be
able to handle this case.

Fixed: 1155694
Change-Id: Iff2cafaf12c5294ebfb52d16ed3dc42858171c14
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2575386Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Gavin Williams <gavinwill@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833891}

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

@@ -83,6 +83,7 @@ void SettingsUserActionTracker::RecordSettingChangeWithDetails(
   // new_value is initialized as null. Null value is used in cases that don't
   // require extra metadata.
   base::Value new_value;
+  if (value) {
     if (value->is_bool_value()) {
       new_value = base::Value(value->get_bool_value());
     } else if (value->is_int_value()) {
@@ -90,6 +91,7 @@ void SettingsUserActionTracker::RecordSettingChangeWithDetails(
     } else if (value->is_string_value()) {
       new_value = base::Value(value->get_string_value());
     }
+  }
   section->LogMetric(setting, new_value);
 
   base::UmaHistogramSparse("ChromeOS.Settings.SettingChanged",

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

@@ -46,6 +46,8 @@ class SettingsUserActionTracker : public mojom::UserActionRecorder {
                            TestRecordSettingChangedBoolPref);
   FRIEND_TEST_ALL_PREFIXES(SettingsUserActionTrackerTest,
                            TestRecordSettingChangedIntPref);
+  FRIEND_TEST_ALL_PREFIXES(SettingsUserActionTrackerTest,
+                           TestRecordSettingChangedNullValue);
 
   // mojom::UserActionRecorder:
   void RecordPageFocus() override;

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

@@ -39,6 +39,8 @@ class SettingsUserActionTrackerTest : public testing::Test {
                                        mojom::Setting::kTouchpadSpeed);
     fake_hierarchy_.AddSettingMetadata(mojom::Section::kPeople,
                                        mojom::Setting::kAddAccount);
+    fake_hierarchy_.AddSettingMetadata(mojom::Section::kPrinting,
+                                       mojom::Setting::kScanningApp);
   }
 
   base::HistogramTester histogram_tester_;
@@ -138,5 +140,26 @@ TEST_F(SettingsUserActionTrackerTest, TestRecordSettingChangedIntPref) {
               mojom::Setting::kTouchpadSpeed);
 }
 
+TEST_F(SettingsUserActionTrackerTest, TestRecordSettingChangedNullValue) {
+  // Record that the Scan app is opened.
+  tracker_.RecordSettingChangeWithDetails(mojom::Setting::kScanningApp,
+                                          nullptr);
+
+  // The umbrella metric for which setting was changed should be updated. Note
+  // that kScanningApp has enum value of 1403.
+  histogram_tester_.ExpectTotalCount("ChromeOS.Settings.SettingChanged",
+                                     /*count=*/1);
+  histogram_tester_.ExpectBucketCount("ChromeOS.Settings.SettingChanged",
+                                      /*sample=*/1403,
+                                      /*count=*/1);
+
+  // The LogMetric fn in the Printing section should have been called.
+  const FakeOsSettingsSection* printing_section =
+      static_cast<const FakeOsSettingsSection*>(
+          fake_sections_.GetSection(mojom::Section::kPrinting));
+  EXPECT_TRUE(printing_section->logged_metrics().back() ==
+              mojom::Setting::kScanningApp);
+}
+
 }  // namespace settings.
 }  // namespace chromeos.