[CrOS Settings] Fix ChromeOS.Settings.TimeUntilChange.SubsequentChange

This CL ignores settings changes which are <200ms from each other, which
fixes false positives logged with the metric.

Bug: 1073714
Change-Id: I07f81a6243ccad85cd92b34e4b639ba2b6b87442
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2162126Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#762032}

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

@@ -15,6 +15,12 @@ namespace {
 // considered short enough for the "first change" metric.
 constexpr base::TimeDelta kShortBlurTimeLimit = base::TimeDelta::FromMinutes(1);
 
+// The minimum amount of time between a setting change and a subsequent setting
+// change. If two changes occur les than this amount of time from each other,
+// they are ignored by metrics. See https://crbug.com/1073714 for details.
+constexpr base::TimeDelta kMinSubsequentChange =
+    base::TimeDelta::FromMilliseconds(200);
+
 // Min/max values for the duration metrics. Note that these values are tied to
 // the metrics defined below; if these ever change, the metric names must also
 // be updated.
@@ -55,7 +61,7 @@ void SettingsUserActionTracker::RecordPageFocus() {
   // user coming back to the window a new session for the purpose of metrics.
   if (blurred_duration >= kShortBlurTimeLimit) {
     ResetMetricsCountersAndTimestamp();
-    has_changed_setting_ = false;
+    last_record_setting_changed_timestamp_ = base::TimeTicks();
   }
 }
 
@@ -76,7 +82,15 @@ void SettingsUserActionTracker::RecordSearch() {
 }
 
 void SettingsUserActionTracker::RecordSettingChange() {
-  if (has_changed_setting_) {
+  base::TimeTicks now = base::TimeTicks::Now();
+
+  if (!last_record_setting_changed_timestamp_.is_null()) {
+    // If it has been less than |kMinSubsequentChange| since the last recorded
+    // setting change, this change is discarded. See https://crbug.com/1073714
+    // for details.
+    if (now - last_record_setting_changed_timestamp_ < kMinSubsequentChange)
+      return;
+
     base::UmaHistogramCounts1000(
         "ChromeOS.Settings.NumClicksUntilChange.SubsequentChange",
         num_clicks_since_start_time_);
@@ -87,7 +101,7 @@ void SettingsUserActionTracker::RecordSettingChange() {
         "ChromeOS.Settings.NumSearchesUntilChange.SubsequentChange",
         num_searches_since_start_time_);
     LogDurationMetric("ChromeOS.Settings.TimeUntilChange.SubsequentChange",
-                      base::TimeTicks::Now() - metric_start_time_);
+                      now - metric_start_time_);
   } else {
     base::UmaHistogramCounts1000(
         "ChromeOS.Settings.NumClicksUntilChange.FirstChange",
@@ -99,11 +113,11 @@ void SettingsUserActionTracker::RecordSettingChange() {
         "ChromeOS.Settings.NumSearchesUntilChange.FirstChange",
         num_searches_since_start_time_);
     LogDurationMetric("ChromeOS.Settings.TimeUntilChange.FirstChange",
-                      base::TimeTicks::Now() - metric_start_time_);
+                      now - metric_start_time_);
   }
 
   ResetMetricsCountersAndTimestamp();
-  has_changed_setting_ = true;
+  last_record_setting_changed_timestamp_ = now;
 }
 
 void SettingsUserActionTracker::ResetMetricsCountersAndTimestamp() {

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

@@ -42,11 +42,13 @@ class SettingsUserActionTracker : public mojom::UserActionRecorder {
 
   void ResetMetricsCountersAndTimestamp();
 
-  // Whether a setting has been changed since the window has been focused. Note
-  // that if the user blurs the window, then refocuses it in less than a minute,
-  // this value remains true; i.e., it flips back to false only when the user
-  // has blurred the window for over a minute.
-  bool has_changed_setting_ = false;
+  // Time at which the last setting change metric was recorded since the window
+  // has been focused, or null if no setting change has been recorded since the
+  // window has been focused. Note that if the user blurs the window then
+  // refocuses it in less than a minute, this value remains non-null; i.e., it
+  // flips back to null only when the user has blurred the window for over a
+  // minute.
+  base::TimeTicks last_record_setting_changed_timestamp_;
 
   // Time at which recording the current metric has started. If
   // |has_changed_setting_| is true, we're currently measuring the "subsequent

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

@@ -70,6 +70,30 @@ TEST_F(SettingsUserActionTrackerTest, TestRecordMetrics) {
       /*sample=*/base::TimeDelta::FromSeconds(10),
       /*count=*/1);
 
+  // Repeat this, but only after 100ms. This is lower than the minimum value
+  // required for this metric, so it should be ignored.
+  tracker_.RecordClick();
+  tracker_.RecordNavigation();
+  tracker_.RecordSearch();
+  task_environment_.FastForwardBy(base::TimeDelta::FromMilliseconds(100));
+  tracker_.RecordSettingChange();
+
+  // No additional logging should have occurred, so make the same verifications
+  // as above.
+  histogram_tester_.ExpectTotalCount(
+      "ChromeOS.Settings.NumClicksUntilChange.SubsequentChange",
+      /*count=*/1);
+  histogram_tester_.ExpectTotalCount(
+      "ChromeOS.Settings.NumNavigationsUntilChange.SubsequentChange",
+      /*count=*/1);
+  histogram_tester_.ExpectTotalCount(
+      "ChromeOS.Settings.NumSearchesUntilChange.SubsequentChange",
+      /*count=*/1);
+  histogram_tester_.ExpectTimeBucketCount(
+      "ChromeOS.Settings.TimeUntilChange.SubsequentChange",
+      /*sample=*/base::TimeDelta::FromSeconds(10),
+      /*count=*/1);
+
   // Repeat this once more, and verify that the counts increased.
   tracker_.RecordClick();
   tracker_.RecordNavigation();