LocalDB - Ignore events happening just after backgrounding a tab

Also rename some of the existing params while I'm here to make them more
explicit.

Bug: 1014976
Change-Id: I6fade2127172343f22084b762a500309a418920b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1865599
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706960}

chrome/browser/resource_coordinator/local_site_characteristics_database_browsertest.cc

@@ -68,8 +68,8 @@ base::TimeDelta GetLongestObservationWindow() {
 base::TimeDelta GetLongestGracePeriod() {
   const SiteCharacteristicsDatabaseParams& params =
       GetStaticSiteCharacteristicsDatabaseParams();
-  return std::max({params.title_or_favicon_change_grace_period,
-                   params.audio_usage_grace_period});
+  return std::max({params.title_or_favicon_change_post_load_grace_period,
+                   params.feature_usage_post_background_grace_period});
 }
 
 // Returns the LocalSiteCharacteristicsDataImpl that backs |reader|.
@@ -226,8 +226,8 @@ class LocalSiteCharacteristicsDatabaseTest : public InProcessBrowserTest {
     // Background the tab and reload it so the audio will stop playing if it's
     // still playing.
     GetActiveWebContents()->WasHidden();
-    test_clock_.Advance(
-        GetStaticSiteCharacteristicsDatabaseParams().audio_usage_grace_period);
+    test_clock_.Advance(GetStaticSiteCharacteristicsDatabaseParams()
+                            .feature_usage_post_background_grace_period);
   }
 
   // Ensure that the current tab is allowed to display non-persistent
@@ -243,7 +243,7 @@ class LocalSiteCharacteristicsDatabaseTest : public InProcessBrowserTest {
 
   void ExpireTitleOrFaviconGracePeriod() {
     test_clock_.Advance(GetStaticSiteCharacteristicsDatabaseParams()
-                            .title_or_favicon_change_grace_period);
+                            .title_or_favicon_change_post_load_grace_period);
   }
 
   base::SimpleTestTickClock& test_clock() { return test_clock_; }

chrome/browser/resource_coordinator/local_site_characteristics_webcontents_observer.cc

@@ -259,17 +259,21 @@ bool LocalSiteCharacteristicsWebContentsObserver::ShouldIgnoreFeatureUsageEvent(
   if (feature_type == FeatureType::kTitleChange ||
       feature_type == FeatureType::kFaviconChange) {
     DCHECK(!loaded_time_.is_null());
-    if (NowTicks() - loaded_time_ < GetStaticSiteCharacteristicsDatabaseParams()
-                                        .title_or_favicon_change_grace_period) {
+    if (NowTicks() - loaded_time_ <
+        GetStaticSiteCharacteristicsDatabaseParams()
+            .title_or_favicon_change_post_load_grace_period) {
       return true;
     }
-  } else if (feature_type == FeatureType::kAudioUsage) {
+  }
+
+  // Ignore events happening shortly after the tab being backgrounded, they're
+  // usually false positives.
   DCHECK(!backgrounded_time_.is_null());
   if (NowTicks() - backgrounded_time_ <
-        GetStaticSiteCharacteristicsDatabaseParams().audio_usage_grace_period) {
+      GetStaticSiteCharacteristicsDatabaseParams()
+          .feature_usage_post_background_grace_period) {
     return true;
   }
-  }
 
   return false;
 }

chrome/browser/resource_coordinator/local_site_characteristics_webcontents_observer_unittest.cc

@@ -211,18 +211,13 @@ TEST_F(LocalSiteCharacteristicsWebContentsObserverTest,
   web_contents()->WasHidden();
   ::testing::Mock::VerifyAndClear(mock_writer);
 
-  // Notification usage events always get forwarded.
-  EXPECT_CALL(*mock_writer, NotifyUsesNotificationsInBackground());
-  observer()->OnNonPersistentNotificationCreated();
-  ::testing::Mock::VerifyAndClear(mock_writer);
-
   auto params = GetStaticSiteCharacteristicsDatabaseParams();
   // Title and Favicon should be ignored during the post-loading grace period.
   observer()->DidUpdateFaviconURL({});
   observer()->TitleWasSet(nullptr);
   ::testing::Mock::VerifyAndClear(mock_writer);
 
-  test_clock().Advance(params.title_or_favicon_change_grace_period);
+  test_clock().Advance(params.title_or_favicon_change_post_load_grace_period);
 
   EXPECT_CALL(*mock_writer, NotifyUpdatesFaviconInBackground());
   observer()->DidUpdateFaviconURL({});
@@ -242,14 +237,22 @@ TEST_F(LocalSiteCharacteristicsWebContentsObserverTest,
   web_contents()->WasHidden();
   ::testing::Mock::VerifyAndClear(mock_writer);
 
-  // Audio usage events should be ignored during the post-background grace
-  // period.
+  // Events should be ignored during the post-background grace period.
   observer()->OnAudioStateChanged(true);
+  observer()->DidUpdateFaviconURL({});
+  observer()->TitleWasSet(nullptr);
+  observer()->OnNonPersistentNotificationCreated();
   ::testing::Mock::VerifyAndClear(mock_writer);
 
-  test_clock().Advance(params.audio_usage_grace_period);
+  test_clock().Advance(params.feature_usage_post_background_grace_period);
   EXPECT_CALL(*mock_writer, NotifyUsesAudioInBackground());
+  EXPECT_CALL(*mock_writer, NotifyUpdatesFaviconInBackground());
+  EXPECT_CALL(*mock_writer, NotifyUpdatesTitleInBackground());
+  EXPECT_CALL(*mock_writer, NotifyUsesNotificationsInBackground());
   observer()->OnAudioStateChanged(true);
+  observer()->DidUpdateFaviconURL({});
+  observer()->TitleWasSet(nullptr);
+  observer()->OnNonPersistentNotificationCreated();
   ::testing::Mock::VerifyAndClear(mock_writer);
 
   EXPECT_CALL(*mock_writer, OnDestroy());
@@ -296,6 +299,12 @@ TEST_F(LocalSiteCharacteristicsWebContentsObserverTest,
   web_contents()->WasHidden();
   ::testing::Mock::VerifyAndClear(mock_writer);
 
+  // Events should be ignored during the post-background grace period.
+  observer()->OnNonPersistentNotificationCreated();
+  ::testing::Mock::VerifyAndClear(mock_writer);
+  test_clock().Advance(GetStaticSiteCharacteristicsDatabaseParams()
+                           .feature_usage_post_background_grace_period);
+
   EXPECT_CALL(*mock_writer, NotifyUsesNotificationsInBackground());
   observer()->OnNonPersistentNotificationCreated();
   ::testing::Mock::VerifyAndClear(mock_writer);

chrome/browser/resource_coordinator/tab_manager_features.cc

@@ -121,9 +121,9 @@ constexpr base::FeatureParam<int>
 constexpr base::FeatureParam<int>
     SiteCharacteristicsDatabaseParams::kNotificationsUsageObservationWindow;
 constexpr base::FeatureParam<int>
-    SiteCharacteristicsDatabaseParams::kTitleOrFaviconChangeGracePeriod;
+    SiteCharacteristicsDatabaseParams::kTitleOrFaviconChangePostLoadGracePeriod;
 constexpr base::FeatureParam<int>
-    SiteCharacteristicsDatabaseParams::kAudioUsageGracePeriod;
+    SiteCharacteristicsDatabaseParams::kFeatureUsagePostBackgroundGracePeriod;
 
 ProactiveTabFreezeAndDiscardParams::ProactiveTabFreezeAndDiscardParams() =
     default;
@@ -235,12 +235,15 @@ SiteCharacteristicsDatabaseParams GetSiteCharacteristicsDatabaseParams() {
       SiteCharacteristicsDatabaseParams::kNotificationsUsageObservationWindow
           .Get());
 
-  params.title_or_favicon_change_grace_period = base::TimeDelta::FromSeconds(
-      SiteCharacteristicsDatabaseParams::kTitleOrFaviconChangeGracePeriod
-          .Get());
+  params.title_or_favicon_change_post_load_grace_period =
+      base::TimeDelta::FromSeconds(
+          SiteCharacteristicsDatabaseParams::
+              kTitleOrFaviconChangePostLoadGracePeriod.Get());
 
-  params.audio_usage_grace_period = base::TimeDelta::FromSeconds(
-      SiteCharacteristicsDatabaseParams::kAudioUsageGracePeriod.Get());
+  params.feature_usage_post_background_grace_period =
+      base::TimeDelta::FromSeconds(
+          SiteCharacteristicsDatabaseParams::
+              kFeatureUsagePostBackgroundGracePeriod.Get());
 
   return params;
 }

chrome/browser/resource_coordinator/tab_manager_features.h

@@ -210,12 +210,14 @@ struct SiteCharacteristicsDatabaseParams {
   static constexpr base::FeatureParam<int> kNotificationsUsageObservationWindow{
       &features::kSiteCharacteristicsDatabase,
       "NotificationsUsageObservationWindow", 2 * base::Time::kSecondsPerHour};
-  static constexpr base::FeatureParam<int> kTitleOrFaviconChangeGracePeriod{
+  static constexpr base::FeatureParam<int>
+      kTitleOrFaviconChangePostLoadGracePeriod{
           &features::kSiteCharacteristicsDatabase,
-      "TitleOrFaviconChangeGracePeriod", 20 /* 20 seconds */};
-  static constexpr base::FeatureParam<int> kAudioUsageGracePeriod{
-      &features::kSiteCharacteristicsDatabase, "AudioUsageGracePeriod",
-      10 /* 10 seconds */};
+          "TitleOrFaviconChangePostLoadGracePeriod", 20 /* 20 seconds */};
+  static constexpr base::FeatureParam<int>
+      kFeatureUsagePostBackgroundGracePeriod{
+          &features::kSiteCharacteristicsDatabase,
+          "FeatureUsagePostBackgroundGracePeriod", 10 /* 10 seconds */};
 
   // Minimum observation window before considering that this website doesn't
   // update its favicon while in background.
@@ -233,12 +235,18 @@ struct SiteCharacteristicsDatabaseParams {
   // change events. It's possible for some site that are loaded in background to
   // use some of these features without this being an attempt to communicate
   // with the user (e.g. the tab is just really finishing to load).
-  base::TimeDelta title_or_favicon_change_grace_period;
-  // The period of time during which we ignore audio usage gets ignored after a
-  // tab gets backgrounded. It's necessary because there might be a delay
-  // between a media request gets initiated and the time the audio actually
-  // starts.
-  base::TimeDelta audio_usage_grace_period;
+  base::TimeDelta title_or_favicon_change_post_load_grace_period;
+  // The period of time during which we ignore events after a tab gets
+  // backgrounded. It's necessary because some events might happen shortly after
+  // backgrounding a tab without this being an attempt to communicate with the
+  // user:
+  //    - There might be a delay between a media request gets initiated and the
+  //      time the audio actually starts.
+  //    - Same-document navigation can cause the title or favicon to change, if
+  //      the user switch tab before this completes this will be recorded as a
+  //      background communication event while in reality it's just a navigation
+  //      event.
+  base::TimeDelta feature_usage_post_background_grace_period;
 };
 
 // Gets parameters for the proactive tab discarding feature. This does no

chrome/browser/resource_coordinator/tab_manager_features_unittest.cc

@@ -85,8 +85,8 @@ class TabManagerFeaturesTest : public testing::Test {
       base::TimeDelta title_update_observation_window,
       base::TimeDelta audio_usage_observation_window,
       base::TimeDelta notifications_usage_observation_window,
-      base::TimeDelta title_or_favicon_change_grace_period,
-      base::TimeDelta audio_usage_grace_period) {
+      base::TimeDelta title_or_favicon_change_post_load_grace_period,
+      base::TimeDelta feature_usage_post_background_grace_period) {
     SiteCharacteristicsDatabaseParams params =
         GetSiteCharacteristicsDatabaseParams();
 
@@ -98,9 +98,10 @@ class TabManagerFeaturesTest : public testing::Test {
               params.audio_usage_observation_window);
     EXPECT_EQ(notifications_usage_observation_window,
               params.notifications_usage_observation_window);
-    EXPECT_EQ(title_or_favicon_change_grace_period,
-              params.title_or_favicon_change_grace_period);
-    EXPECT_EQ(audio_usage_grace_period, params.audio_usage_grace_period);
+    EXPECT_EQ(title_or_favicon_change_post_load_grace_period,
+              params.title_or_favicon_change_post_load_grace_period);
+    EXPECT_EQ(feature_usage_post_background_grace_period,
+              params.feature_usage_post_background_grace_period);
   }
 
   void ExpectDefaultProactiveTabFreezeAndDiscardParams() {
@@ -152,11 +153,11 @@ class TabManagerFeaturesTest : public testing::Test {
             SiteCharacteristicsDatabaseParams::
                 kNotificationsUsageObservationWindow.default_value),
         base::TimeDelta::FromSeconds(
-            SiteCharacteristicsDatabaseParams::kTitleOrFaviconChangeGracePeriod
-                .default_value),
+            SiteCharacteristicsDatabaseParams::
+                kTitleOrFaviconChangePostLoadGracePeriod.default_value),
         base::TimeDelta::FromSeconds(
-            SiteCharacteristicsDatabaseParams::kAudioUsageGracePeriod
-                .default_value));
+            SiteCharacteristicsDatabaseParams::
+                kFeatureUsagePostBackgroundGracePeriod.default_value));
   }
 
  private:
@@ -286,10 +287,12 @@ TEST_F(TabManagerFeaturesTest,
       SiteCharacteristicsDatabaseParams::kNotificationsUsageObservationWindow
           .name,
       "abc");
-  SetParam(
-      SiteCharacteristicsDatabaseParams::kTitleOrFaviconChangeGracePeriod.name,
+  SetParam(SiteCharacteristicsDatabaseParams::
+               kTitleOrFaviconChangePostLoadGracePeriod.name,
            "bleh");
-  SetParam(SiteCharacteristicsDatabaseParams::kAudioUsageGracePeriod.name,
+  SetParam(
+      SiteCharacteristicsDatabaseParams::kFeatureUsagePostBackgroundGracePeriod
+          .name,
       "!!!");
   EnableSiteCharacteristicsDatabase();
   ExpectDefaultSiteCharacteristicsDatabaseParams();
@@ -308,10 +311,12 @@ TEST_F(TabManagerFeaturesTest, GetSiteCharacteristicsDatabaseParams) {
       SiteCharacteristicsDatabaseParams::kNotificationsUsageObservationWindow
           .name,
       "3600000");
-  SetParam(
-      SiteCharacteristicsDatabaseParams::kTitleOrFaviconChangeGracePeriod.name,
+  SetParam(SiteCharacteristicsDatabaseParams::
+               kTitleOrFaviconChangePostLoadGracePeriod.name,
            "42");
-  SetParam(SiteCharacteristicsDatabaseParams::kAudioUsageGracePeriod.name,
+  SetParam(
+      SiteCharacteristicsDatabaseParams::kFeatureUsagePostBackgroundGracePeriod
+          .name,
       "43");
 
   EnableSiteCharacteristicsDatabase();