Ensure last engagement is never regarded as stale in incognito.

This CL addresses a bug where engagement cleanup is continually run in
incognito mode because we do not write a pref value for the last
engagement. When in incognito, engagement is never regarded as going
stale, meaning that cleanup is never run. A test is added to ensure the
correct behaviour.

The frequency of calls to Clock::Now() is also reduced in this CL.

BUG=947835

Change-Id: I7c91caa98a3c7e7c6e39aec314682f382bb2581b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1895097
Auto-Submit: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712035}

chrome/browser/engagement/site_engagement_service.cc

@@ -484,7 +484,7 @@ void SiteEngagementService::MaybeRecordMetrics() {
       {base::ThreadPool(), base::TaskPriority::BEST_EFFORT,
        base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
       base::BindOnce(
-          &GetAllDetailsInBackground, clock_->Now(),
+          &GetAllDetailsInBackground, now,
           base::WrapRefCounted(
               HostContentSettingsMapFactory::GetForProfile(profile_))),
       base::BindOnce(&SiteEngagementService::RecordMetrics,
@@ -536,6 +536,9 @@ bool SiteEngagementService::ShouldRecordEngagement(const GURL& url) const {
 }
 
 base::Time SiteEngagementService::GetLastEngagementTime() const {
+  if (profile_->IsOffTheRecord())
+    return base::Time();
+
   return base::Time::FromInternalValue(
       profile_->GetPrefs()->GetInt64(prefs::kSiteEngagementLastUpdateTime));
 }
@@ -627,15 +630,18 @@ void SiteEngagementService::OnEngagementEvent(
 }
 
 bool SiteEngagementService::IsLastEngagementStale() const {
-  // Only happens on first run when no engagement has ever been recorded.
+  // |last_engagement_time| will be null when no engagement has been recorded
+  // (first run or post clearing site data), or if we are running in incognito.
+  // Do not regard these cases as stale.
   base::Time last_engagement_time = GetLastEngagementTime();
   if (last_engagement_time.is_null())
     return false;
 
   // Stale is either too *far* back, or any amount *forward* in time. This could
   // occur due to a changed clock, or extended non-use of the browser.
-  return (clock_->Now() - last_engagement_time) >= GetStalePeriod() ||
-         (clock_->Now() < last_engagement_time);
+  base::Time now = clock_->Now();
+  return (now - last_engagement_time) >= GetStalePeriod() ||
+         (now < last_engagement_time);
 }
 
 void SiteEngagementService::OnURLsDeleted(

chrome/browser/engagement/site_engagement_service_unittest.cc

@@ -1722,6 +1722,9 @@ TEST_F(SiteEngagementServiceTest, CleanupMovesScoreBackToRebase) {
 }
 
 TEST_F(SiteEngagementServiceTest, IncognitoEngagementService) {
+  base::Time current_day = GetReferenceTime();
+  clock_.SetNow(current_day);
+
   SiteEngagementService* service = SiteEngagementService::Get(profile());
   ASSERT_TRUE(service);
 
@@ -1733,8 +1736,8 @@ TEST_F(SiteEngagementServiceTest, IncognitoEngagementService) {
   service->AddPoints(url1, 1);
   service->AddPoints(url2, 2);
 
-  SiteEngagementService* incognito_service =
-      SiteEngagementService::Get(profile()->GetOffTheRecordProfile());
+  auto incognito_service = base::WrapUnique(
+      new SiteEngagementService(profile()->GetOffTheRecordProfile(), &clock_));
   EXPECT_EQ(1, incognito_service->GetScore(url1));
   EXPECT_EQ(2, incognito_service->GetScore(url2));
   EXPECT_EQ(0, incognito_service->GetScore(url3));
@@ -1755,6 +1758,16 @@ TEST_F(SiteEngagementServiceTest, IncognitoEngagementService) {
   service->AddPoints(url4, 2);
   EXPECT_EQ(2, incognito_service->GetScore(url4));
   EXPECT_EQ(2, service->GetScore(url4));
+
+  // Engagement should never become stale in incognito.
+  current_day += incognito_service->GetStalePeriod();
+  clock_.SetNow(current_day);
+  EXPECT_FALSE(incognito_service->IsLastEngagementStale());
+  current_day += incognito_service->GetStalePeriod();
+  clock_.SetNow(current_day);
+  EXPECT_FALSE(incognito_service->IsLastEngagementStale());
+
+  incognito_service->Shutdown();
 }
 
 TEST_F(SiteEngagementServiceTest, GetScoreFromSettings) {