Prevent site engagement scores from decaying when Chrome isn't in use.

chrome/browser/engagement/site_engagement_service_unittest.cc

Index: chrome/browser/engagement/site_engagement_service_unittest.cc
diff --git a/chrome/browser/engagement/site_engagement_service_unittest.cc b/chrome/browser/engagement/site_engagement_service_unittest.cc
index 85e709da56e3038918725d5fd517713441c19ae2..f0a0f79e2c601dc3262afa07741d8ca58cdbba78 100644
--- a/chrome/browser/engagement/site_engagement_service_unittest.cc
+++ b/chrome/browser/engagement/site_engagement_service_unittest.cc
@@ -20,6 +20,7 @@
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/content_settings/core/browser/content_settings_observer.h"
@@ -27,6 +28,7 @@
 #include "components/history/core/browser/history_database_params.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/test/test_history_database.h"
+#include "components/prefs/pref_service.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/page_navigator.h"
 #include "content/public/browser/web_contents.h"
@@ -39,7 +41,6 @@ base::FilePath g_temp_history_dir;
 
 const int kMoreAccumulationsThanNeededToMaxDailyEngagement = 40;
 const int kMoreDaysThanNeededToMaxTotalEngagement = 40;
-const int kLessPeriodsThanNeededToDecayMaxScore = 2;
 const int kMorePeriodsThanNeededToDecayMaxScore = 40;
 
 // Waits until a change is observed in site engagement content settings.
@@ -666,67 +667,140 @@ TEST_F(SiteEngagementServiceTest, CheckHistograms) {
   histograms.ExpectBucketCount(engagement_bucket_histogram_names[1], 66, 1);
 }
 
-// Expect that sites that have reached zero engagement are cleaned up.
+// Expect that sites that have reached zero engagement are cleaned up. Expect
+// engagement times to be reset if too much time has passed since the last
+// engagement.
 TEST_F(SiteEngagementServiceTest, CleanupEngagementScores) {
   base::SimpleTestClock* clock = new base::SimpleTestClock();
   std::unique_ptr<SiteEngagementService> service(
       new SiteEngagementService(profile(), base::WrapUnique(clock)));
 
-  base::Time current_day = GetReferenceTime();
-  clock->SetNow(current_day);
+  // Set the base time to be (obsoletion period + 2 * (decay period - 1 second))
+  // in the past. The one second offset ensures that engagement won't decay
+  // (yet).
+  base::TimeDelta one_second = base::TimeDelta::FromSeconds(1);
+  base::TimeDelta obsoletion_period = base::TimeDelta::FromHours(
+      SiteEngagementScore::GetObsoleteLastEngagementPeriodInHours());
+  base::TimeDelta decay_period =
+      base::TimeDelta::FromDays(SiteEngagementScore::GetDecayPeriodInDays());
+  base::TimeDelta shorter_than_decay_period = decay_period - one_second;
+  base::Time base_time =
+      GetReferenceTime() - obsoletion_period - shorter_than_decay_period * 2;
+  clock->SetNow(base_time);
 
   // The https and http versions of www.google.com should be separate.
   GURL url1("https://www.google.com/");
   GURL url2("http://www.google.com/");
+  GURL url3("http://maps.google.com/");
+  GURL url4("http://drive.google.com/");
 
   EXPECT_EQ(0, service->GetScore(url1));
   EXPECT_EQ(0, service->GetScore(url2));
+  EXPECT_EQ(0, service->GetScore(url3));
+  EXPECT_EQ(0, service->GetScore(url4));
 
-  // Add the maximum number of points for one day.
+  // Add the maximum number of points for one day for two origins. Leave url2
+  // alone after this.
   service->AddPoints(url1, 5.0);
-  EXPECT_EQ(5.0, service->GetScore(url1));
   service->AddPoints(url2, 5.0);
+  EXPECT_EQ(5.0, service->GetScore(url1));
   EXPECT_EQ(5.0, service->GetScore(url2));
 
-  // Add more points by moving to another day.
-  clock->SetNow(GetReferenceTime() + base::TimeDelta::FromDays(1));
-
-  service->AddPoints(url1, 5.0);
+  // Add more points by moving to another day. This is (obsoletion_period
+  // + shorter_than_decay_period) in the past.
+  base_time += shorter_than_decay_period;
+  clock->SetNow(base_time);
+  service->AddPoints(url1, 2.5);
+  service->AddPoints(url3, 5.0);
+  EXPECT_EQ(7.5, service->GetScore(url1));
+  EXPECT_EQ(5.0, service->GetScore(url3));
+
+  // Add more points by moving to another day. This is obsoletion_period in
+  // the past.
+  base_time += shorter_than_decay_period;
+  clock->SetNow(base_time);
+  service->AddPoints(url1, 2.5);
+  service->AddPoints(url4, 5.0);
   EXPECT_EQ(10.0, service->GetScore(url1));
+  EXPECT_EQ(5.0, service->GetScore(url4));
 
   {
-    // Decay one origin to zero by advancing time and expect the engagement
-    // score to be cleaned up. The other score was changed a day later so it
-    // will not have decayed at all.
-    clock->SetNow(
-        GetReferenceTime() +
-        base::TimeDelta::FromDays(SiteEngagementScore::GetDecayPeriodInDays()));
+    clock->SetNow(GetReferenceTime());
+    ASSERT_TRUE(service->IsLastEngagementObsolete());
+
+    // Run a cleanup. Last engagement times will be reset relative to today.
+    // After the reset, url2 was last accessed > decay_period ago, so it will
+    // still decay. Everything else will be within a decay period and not decay.
+    service->CleanupEngagementScores(true);
 
     std::map<GURL, double> score_map = service->GetScoreMap();
-    EXPECT_EQ(2u, score_map.size());
+    EXPECT_EQ(3u, score_map.size());
     EXPECT_EQ(10, score_map[url1]);
-    EXPECT_EQ(0, score_map[url2]);
+    EXPECT_EQ(5.0, score_map[url3]);
+    EXPECT_EQ(5.0, score_map[url4]);
+    EXPECT_EQ(0, service->GetScore(url2));
+
+    EXPECT_EQ(clock->Now(),
+              service->CreateEngagementScore(url1).last_engagement_time());
+    EXPECT_EQ(clock->Now() - shorter_than_decay_period,
+              service->CreateEngagementScore(url3).last_engagement_time());
+    EXPECT_EQ(clock->Now(),
+              service->CreateEngagementScore(url4).last_engagement_time());
+  }
 
-    service->CleanupEngagementScores();
+  {
+    // Advance time by a second and expect url3 to be decayed to zero. Run a
+    // cleanup without adjusting times and expect url3 to be purged.
+    clock->SetNow(GetReferenceTime() + one_second);
+    ASSERT_FALSE(service->IsLastEngagementObsolete());
+
+    std::map<GURL, double> score_map = service->GetScoreMap();
+    EXPECT_EQ(3u, score_map.size());
+    EXPECT_EQ(10, score_map[url1]);
+    EXPECT_EQ(0, score_map[url3]);
+    EXPECT_EQ(5.0, score_map[url4]);
+
+    service->CleanupEngagementScores(false);
 
     score_map = service->GetScoreMap();
-    EXPECT_EQ(1u, score_map.size());
+    EXPECT_EQ(2u, score_map.size());
     EXPECT_EQ(10, score_map[url1]);
-    EXPECT_EQ(0, service->GetScore(url2));
+    EXPECT_EQ(5.0, score_map[url4]);
+    EXPECT_EQ(0, service->GetScore(url3));
   }
 
   {
-    // Decay the other origin to zero by advancing time and expect the
-    // engagement score to be cleaned up.
-    clock->SetNow(GetReferenceTime() +
-                  base::TimeDelta::FromDays(
-                      3 * SiteEngagementScore::GetDecayPeriodInDays()));
+    // Decay the final two origins to zero and expect them to be cleaned up.
+    // This needs to be done in two calls or else the score will be obsolete.
+    clock->SetNow(
+        GetReferenceTime() +
+        base::TimeDelta::FromDays(SiteEngagementScore::GetDecayPeriodInDays()) +
+        one_second);
+    ASSERT_FALSE(service->IsLastEngagementObsolete());
 
     std::map<GURL, double> score_map = service->GetScoreMap();
+    EXPECT_EQ(2u, score_map.size());
+    EXPECT_EQ(5, score_map[url1]);
+    EXPECT_EQ(0, score_map[url4]);
+
+    service->CleanupEngagementScores(false);
+
+    score_map = service->GetScoreMap();
+    EXPECT_EQ(1u, score_map.size());
+    EXPECT_EQ(5, score_map[url1]);
+    EXPECT_EQ(0, service->GetScore(url4));
+
+    clock->SetNow(GetReferenceTime() +
+                  2 * base::TimeDelta::FromDays(
+                          SiteEngagementScore::GetDecayPeriodInDays()) +
+                  one_second);
+    ASSERT_FALSE(service->IsLastEngagementObsolete());
+
+    score_map = service->GetScoreMap();
     EXPECT_EQ(1u, score_map.size());
     EXPECT_EQ(0, score_map[url1]);
 
-    service->CleanupEngagementScores();
+    service->CleanupEngagementScores(false);
 
     score_map = service->GetScoreMap();
     EXPECT_EQ(0u, score_map.size());
@@ -1063,10 +1137,10 @@ TEST_F(SiteEngagementServiceTest, ScoreDecayHistograms) {
       SiteEngagementScore::kMaxPoints - SiteEngagementScore::GetDecayPoints(),
       1);
 
-  // Check histograms after a few decay periods.
-  clock->SetNow(current_day + base::TimeDelta::FromDays(
-                                  kLessPeriodsThanNeededToDecayMaxScore *
-                                  SiteEngagementScore::GetDecayPeriodInDays()));
+  // Check histograms after another decay period.
+  clock->SetNow(current_day +
+                base::TimeDelta::FromDays(
+                    2 * SiteEngagementScore::GetDecayPeriodInDays()));
   // Trigger decay and histogram hit.
   service->AddPoints(origin1, 0.01);
   histograms.ExpectTotalCount(SiteEngagementMetrics::kScoreDecayedFromHistogram,
@@ -1074,34 +1148,107 @@ TEST_F(SiteEngagementServiceTest, ScoreDecayHistograms) {
   histograms.ExpectTotalCount(SiteEngagementMetrics::kScoreDecayedToHistogram,
                               2);
 
-  // Check decay to zero.
-  clock->SetNow(current_day + base::TimeDelta::FromDays(
-                                  kMorePeriodsThanNeededToDecayMaxScore *
-                                  SiteEngagementScore::GetDecayPeriodInDays()));
-  // Trigger decay and histogram hit.
-  service->AddPoints(origin1, 0.01);
+  // Check decay to zero. Start at the 3rd decay period (we have had two
+  // already). This will be 40 decays in total.
+  for (int i = 3; i <= kMorePeriodsThanNeededToDecayMaxScore; ++i) {
+    clock->SetNow(current_day +
+                  base::TimeDelta::FromDays(
+                      i * SiteEngagementScore::GetDecayPeriodInDays()));
+    // Trigger decay and histogram hit.
+    service->AddPoints(origin1, 0.01);
+  }
   histograms.ExpectTotalCount(SiteEngagementMetrics::kScoreDecayedFromHistogram,
-                              3);
+                              kMorePeriodsThanNeededToDecayMaxScore);
   histograms.ExpectTotalCount(SiteEngagementMetrics::kScoreDecayedToHistogram,
-                              3);
+                              kMorePeriodsThanNeededToDecayMaxScore);
+  // It should have taken (20 - 3) = 17 of the 38 decays to get to zero, since
+  // we started from 95. Expect the remaining 21 decays to be to bucket 0 (and
+  // hence 20 from bucket 0).
+  histograms.ExpectBucketCount(
+      SiteEngagementMetrics::kScoreDecayedFromHistogram, 0, 20);
   histograms.ExpectBucketCount(SiteEngagementMetrics::kScoreDecayedToHistogram,
-                               0, 1);
+                               0, 21);
   // Trigger decay and histogram hit for origin2, checking an independent decay.
   service->AddPoints(origin2, 0.01);
   histograms.ExpectTotalCount(SiteEngagementMetrics::kScoreDecayedFromHistogram,
-                              4);
+                              kMorePeriodsThanNeededToDecayMaxScore + 1);
   histograms.ExpectTotalCount(SiteEngagementMetrics::kScoreDecayedToHistogram,
-                              4);
+                              kMorePeriodsThanNeededToDecayMaxScore + 1);
   histograms.ExpectBucketCount(
-      SiteEngagementMetrics::kScoreDecayedFromHistogram, 0, 1);
+      SiteEngagementMetrics::kScoreDecayedFromHistogram, 0, 21);
   histograms.ExpectBucketCount(SiteEngagementMetrics::kScoreDecayedToHistogram,
-                               0, 2);
+                               0, 22);
 
   // Add more points and ensure no more samples are present.
   service->AddPoints(origin1, 0.01);
   service->AddPoints(origin2, 0.01);
   histograms.ExpectTotalCount(SiteEngagementMetrics::kScoreDecayedFromHistogram,
-                              4);
+                              kMorePeriodsThanNeededToDecayMaxScore + 1);
   histograms.ExpectTotalCount(SiteEngagementMetrics::kScoreDecayedToHistogram,
-                              4);
+                              kMorePeriodsThanNeededToDecayMaxScore + 1);
+}
+
+TEST_F(SiteEngagementServiceTest, PersistLastEngagementTime) {
+  // The last engagement time should start off null in prefs and in the service.
+  base::Time last_engagement_time = base::Time::FromInternalValue(
+      profile()->GetPrefs()->GetInt64(prefs::kSiteEngagementLastUpdateTime));
+
+  ASSERT_TRUE(last_engagement_time.is_null());
+
+  base::SimpleTestClock* clock = new base::SimpleTestClock();
+  std::unique_ptr<SiteEngagementService> service(
+      new SiteEngagementService(profile(), base::WrapUnique(clock)));
+
+  ASSERT_TRUE(service->last_engagement_time_.is_null());
+
+  base::Time current_day = GetReferenceTime();
+  clock->SetNow(current_day);
+
+  // Add points should set the last engagement time in the service, but not
+  // persist it to disk.
+  GURL origin("http://www.google.com/");
+  service->AddPoints(origin, 1);
+
+  last_engagement_time = base::Time::FromInternalValue(
+      profile()->GetPrefs()->GetInt64(prefs::kSiteEngagementLastUpdateTime));
+
+  ASSERT_TRUE(last_engagement_time.is_null());
+  ASSERT_FALSE(service->last_engagement_time_.is_null());
+  EXPECT_EQ(service->last_engagement_time_, current_day);
+
+  // Running a cleanup and updating last engagement times should persist the
+  // last engagement time to disk.
+  current_day +=
+      base::TimeDelta::FromHours(
+          SiteEngagementScore::GetObsoleteLastEngagementPeriodInHours());
+  clock->SetNow(current_day);
+  service->CleanupEngagementScores(true);
+
+  last_engagement_time = base::Time::FromInternalValue(
+      profile()->GetPrefs()->GetInt64(prefs::kSiteEngagementLastUpdateTime));
+
+  ASSERT_FALSE(last_engagement_time.is_null());
+  EXPECT_EQ(last_engagement_time, current_day);
+  EXPECT_EQ(service->last_engagement_time_, current_day);
+
+  // Add some more points and ensure the value isn't persisted.
+  base::Time later_in_day = current_day + base::TimeDelta::FromSeconds(30);
+  clock->SetNow(later_in_day);
+  service->AddPoints(origin, 3);
+
+  last_engagement_time = base::Time::FromInternalValue(
+      profile()->GetPrefs()->GetInt64(prefs::kSiteEngagementLastUpdateTime));
+
+  ASSERT_FALSE(last_engagement_time.is_null());
+  EXPECT_EQ(last_engagement_time, current_day);
+  ASSERT_FALSE(service->last_engagement_time_.is_null());
+  EXPECT_EQ(service->last_engagement_time_, later_in_day);
+
+  // When the service is destroyed, the time should be persisted to disk.
+  service.reset(nullptr);
+  last_engagement_time = base::Time::FromInternalValue(
+      profile()->GetPrefs()->GetInt64(prefs::kSiteEngagementLastUpdateTime));
+
+  ASSERT_FALSE(last_engagement_time.is_null());
+  EXPECT_EQ(last_engagement_time, later_in_day);
 }