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

chrome/browser/engagement/site_engagement_service.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/engagement/site_engagement_service.h"
 
 #include <stddef.h>
 #include <algorithm>
 #include <cmath>
 #include <utility>
@@ skipping 249 matching lines @@
   double to_add = std::min(kMaxPoints - raw_score_,
                            GetMaxPointsPerDay() - points_added_today_);
   to_add = std::min(to_add, points);
 
   points_added_today_ += to_add;
   raw_score_ += to_add;
 
   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 {
   if (!last_engagement_time_.is_null() &&
       clock_->Now().LocalMidnight() != last_engagement_time_.LocalMidnight()) {
     return false;
   }
 
   return points_added_today_ == GetMaxPointsPerDay();
 }
@@ skipping 151 matching lines @@
 void SiteEngagementService::HandleMediaPlaying(const GURL& url,
                                                bool is_hidden) {
   SiteEngagementMetrics::RecordEngagement(
       is_hidden ? SiteEngagementMetrics::ENGAGEMENT_MEDIA_HIDDEN
                 : SiteEngagementMetrics::ENGAGEMENT_MEDIA_VISIBLE);
   AddPoints(url, is_hidden ? SiteEngagementScore::GetHiddenMediaPoints()
                            : SiteEngagementScore::GetVisibleMediaPoints());
   RecordMetrics();
 }
 
 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(
     history::HistoryService* history_service,
     bool all_history,
     bool expired,
     const history::URLRows& deleted_rows,
     const std::set<GURL>& favicon_urls) {
   std::multiset<GURL> origins;
   for (const history::URLRow& row : deleted_rows)
     origins.insert(row.url().GetOrigin());
 
   history::HistoryService* hs = HistoryServiceFactory::GetForProfile(
       profile_, ServiceAccessType::EXPLICIT_ACCESS);
   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) {
   HostContentSettingsMap* settings_map =
     HostContentSettingsMapFactory::GetForProfile(profile_);
   scoped_ptr<base::DictionaryValue> score_dict =
       GetScoreDictForOrigin(settings_map, url);
   SiteEngagementScore score(clock_.get(), *score_dict);
 
   // Record the number of days since the last launch in UMA. If the user's clock
@@ skipping 238 matching lines @@
   int total_origins = 0;
 
   for (const auto& value : score_map)
     if (value.second == SiteEngagementScore::kMaxPoints)
       ++total_origins;
 
   return total_origins;
 }
 
 void SiteEngagementService::GetCountsForOriginsComplete(
+    history::HistoryService* history_service,
     const std::multiset<GURL>& deleted_origins,
     bool expired,
     const history::OriginCountMap& remaining_origins) {
   for (const auto& origin_to_count : remaining_origins) {
     GURL origin = origin_to_count.first;
     int remaining = origin_to_count.second;
     int deleted = deleted_origins.count(origin);
 
     // Do not update engagement scores if the deletion was an expiry, but the
     // URL still has entries in history.
     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());
+  }
+}