Set site engagement timestamps to privacy-respectful values when history is cleared.

chrome/browser/engagement/site_engagement_service.cc

Index: chrome/browser/engagement/site_engagement_service.cc
diff --git a/chrome/browser/engagement/site_engagement_service.cc b/chrome/browser/engagement/site_engagement_service.cc
index b336708446b92eed4f5abd471451282c781efd75..a16ccc586b9ea5ac435e7dfc66d57d7b7381ef4a 100644
--- a/chrome/browser/engagement/site_engagement_service.cc
+++ b/chrome/browser/engagement/site_engagement_service.cc
@@ -267,12 +267,18 @@ void SiteEngagementScore::AddPoints(double points) {
   last_engagement_time_ = now;
 }
 
-void SiteEngagementScore::Reset(double points) {
+void SiteEngagementScore::Reset(double points, const base::Time* updated_time) {
   raw_score_ = points;
   points_added_today_ = 0;
 
   // This must be set in order to prevent the score from decaying when read.
-  last_engagement_time_ = clock_->Now();
+  if (updated_time == nullptr) {

[benwells, 2016/04/18 07:01:42]

Nit: Can you rearrange to be 

if (updated_time) {
} else{
}?

[dominickn, 2016/04/18 23:27:52]

Done.

+    last_engagement_time_ = clock_->Now();
+  } else {
+    last_engagement_time_ = *updated_time;
+    if (!last_shortcut_launch_time_.is_null())
+      last_shortcut_launch_time_ = *updated_time;
+  }
 }
 
 bool SiteEngagementScore::MaxPointsPerDayAdded() const {
@@ -445,29 +451,7 @@ void SiteEngagementService::HandleMediaPlaying(const GURL& url,
 }
 
 void SiteEngagementService::ResetScoreForURL(const GURL& url, double score) {

[benwells, 2016/04/18 07:01:42]

Is this still used?

[dominickn, 2016/04/18 23:27:52]

Yes, in the WebUI code for changing engagement scores. I disliked the idea of
exposing an internal site engagement detail (last updated times) to the public
interface, which is why I had the public method without the time detail.

-  DCHECK(url.is_valid());
-  DCHECK_GE(score, 0);
-  DCHECK_LE(score, SiteEngagementScore::kMaxPoints);
-
-  HostContentSettingsMap* settings_map =
-    HostContentSettingsMapFactory::GetForProfile(profile_);
-  scoped_ptr<base::DictionaryValue> score_dict =
-      GetScoreDictForOrigin(settings_map, url);
-  SiteEngagementScore engagement_score(clock_.get(), *score_dict);
-
-  engagement_score.Reset(score);
-  if (score == 0) {
-    settings_map->SetWebsiteSettingDefaultScope(
-        url, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
-        nullptr);
-    return;
-  }
-
-  if (engagement_score.UpdateScoreDict(score_dict.get())) {
-    settings_map->SetWebsiteSettingDefaultScope(
-        url, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
-        score_dict.release());
-  }
+  ResetScoreAndAccessTimesForURL(url, score, nullptr);
 }
 
 void SiteEngagementService::OnURLsDeleted(
@@ -485,7 +469,7 @@ void SiteEngagementService::OnURLsDeleted(
   hs->GetCountsForOrigins(
       std::set<GURL>(origins.begin(), origins.end()),
       base::Bind(&SiteEngagementService::GetCountsForOriginsComplete,
-                 weak_factory_.GetWeakPtr(), origins, expired));
+                 weak_factory_.GetWeakPtr(), hs, origins, expired));
 }
 
 void SiteEngagementService::SetLastShortcutLaunchTime(const GURL& url) {
@@ -744,6 +728,7 @@ int SiteEngagementService::OriginsWithMaxEngagement(
 }
 
 void SiteEngagementService::GetCountsForOriginsComplete(
+    history::HistoryService* history_service,
     const std::multiset<GURL>& deleted_origins,
     bool expired,
     const history::OriginCountMap& remaining_origins) {
@@ -757,10 +742,47 @@ void SiteEngagementService::GetCountsForOriginsComplete(
     if ((expired && remaining != 0) || deleted == 0)
       continue;
 
+    // Retrieve the last visit time for this origin, and use this for the last
+    // engagement time and last shortcut launch time (if that is non-null).
+    // TODO(calamity): ensure that we have sufficient data to lookup the last
+    // visit to any URL under the origin, rather than just the origin.
+    base::Time updated_time;
+    if (!history_service->GetLastVisitTimeForURL(origin, &updated_time))
+      updated_time = clock_->Now();

[benwells, 2016/04/18 07:01:42]

IIUC this will be fairly common:
- usually people don't just visit the origin but visit some URL under the
origin, as noted in the TODO
- I think only a short amound of history is generally present.

Is that right?

[dominickn, 2016/04/18 23:27:52]

Yes, I agree with you here. 

As it turns out, HistoryService::GetLastVisitTimeForURL doesn't seem to work.
The failing tests are because this method returns blank for everything, even
origins which are in the history. After a lot of digging, I think it's because
GetLastVisitTimeForURL queries an in-memory cache which holds only part of the
history information - crucially, only URLs which have been searched for/typed
will be returned here.

This means I'll need to use a different method here, using a callback. Sigh.

[benwells, 2016/04/19 00:18:53]

Ack.

Can you also move this code to below where proportion_remaining is set? That
keeps the code that finds the proportion all together.

[dominickn, 2016/04/19 07:01:58]

Done.

+
     // Remove engagement proportional to the urls expired from the origin's
     // entire history.
     double proportion_remaining =
         static_cast<double>(remaining) / (remaining + deleted);
-    ResetScoreForURL(origin, proportion_remaining * GetScore(origin));
+    ResetScoreAndAccessTimesForURL(origin,
+                                   proportion_remaining * GetScore(origin),
+                                   &updated_time);
+  }
+}
+
+void SiteEngagementService::ResetScoreAndAccessTimesForURL(
+    const GURL& url, double score, const base::Time* updated_time) {

[benwells, 2016/04/18 07:01:42]

Should this score by undecayed? That is, what happens if the updated time was a
long time ago - won't the score be decayed next time its read?

[dominickn, 2016/04/18 23:27:52]

Having thought about this more, it seems counter-intuitive that *removing*
history would lead to an *increase* in engagement, which is what undecaying
would do as I understand it. Plus, the way the procedure currently works, the
next time the score is read, it would be decayed straight back down to what the
score was before deletion started.

[benwells, 2016/04/19 00:18:53]

Undecaying should not lead to any increase in engagement in today's values.

And yes, it would be decayed straight back down, that's the point. But it
wouldn't be decayed back down to what the score was before the deletion started,
it would be decayed back down to the new calculated proportional score.

[dominickn, 2016/04/19 07:01:57]

I finally understand what's going on here. Done, with tests modified to ensure
the right thing is going on.

+  DCHECK(url.is_valid());
+  DCHECK_GE(score, 0);
+  DCHECK_LE(score, SiteEngagementScore::kMaxPoints);
+
+  HostContentSettingsMap* settings_map =
+      HostContentSettingsMapFactory::GetForProfile(profile_);
+  scoped_ptr<base::DictionaryValue> score_dict =
+      GetScoreDictForOrigin(settings_map, url);
+  SiteEngagementScore engagement_score(clock_.get(), *score_dict);
+
+  engagement_score.Reset(score, updated_time);
+  if (score == 0) {
+    settings_map->SetWebsiteSettingDefaultScope(
+        url, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
+        nullptr);
+    return;
+  }
+
+  if (engagement_score.UpdateScoreDict(score_dict.get())) {
+    settings_map->SetWebsiteSettingDefaultScope(
+        url, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
+        score_dict.release());
   }
 }