Add SiteEngagementService::GetAllDetails(), to return detailed scores.

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 <utility>
@@ skipping 86 matching lines @@
       base::FieldTrialList::FindFullName(kEngagementParams);
   return !base::StartsWith(group_name, "Disabled",
                            base::CompareCase::SENSITIVE);
 }
 
 // static
 double SiteEngagementService::GetScoreFromSettings(
     HostContentSettingsMap* settings,
     const GURL& origin) {
   auto clock = base::MakeUnique<base::DefaultClock>();
-  return SiteEngagementScore(clock.get(), origin, settings)
-      .GetScore();
+  return SiteEngagementScore(clock.get(), origin, settings).GetTotal();
 }
 
 SiteEngagementService::SiteEngagementService(Profile* profile)
     : SiteEngagementService(profile, base::MakeUnique<base::DefaultClock>()) {
   content::BrowserThread::PostAfterStartupTask(
       FROM_HERE, content::BrowserThread::GetTaskRunnerForThread(
                      content::BrowserThread::UI),
       base::Bind(&SiteEngagementService::AfterStartupTask,
                  weak_factory_.GetWeakPtr()));
 
@@ skipping 13 matching lines @@
 }
 
 blink::mojom::EngagementLevel
 SiteEngagementService::GetEngagementLevel(const GURL& url) const {
   if (IsLastEngagementStale())
     CleanupEngagementScores(true);
 
   return CreateEngagementScore(url).GetEngagementLevel();
 }
 
-std::map<GURL, double> SiteEngagementService::GetScoreMap() const {
+std::vector<mojom::SiteEngagementDetails>
+SiteEngagementService::GetScoreDetails() const {
   HostContentSettingsMap* settings_map =
       HostContentSettingsMapFactory::GetForProfile(profile_);
   std::unique_ptr<ContentSettingsForOneType> engagement_settings =
       GetEngagementContentSettings(settings_map);
 
-  std::map<GURL, double> score_map;
+  std::vector<mojom::SiteEngagementDetails> scores;
+  scores.reserve(engagement_settings->size());
   for (const auto& site : *engagement_settings) {
     GURL origin(site.primary_pattern.ToString());
     if (!origin.is_valid())
       continue;
+    scores.push_back(GetScoreDetails(origin));
+  }
 
-    score_map[origin] = GetScore(origin);
+  return scores;
+}
+
+std::map<GURL, double> SiteEngagementService::GetScoreMap() const {
+  std::vector<mojom::SiteEngagementDetails> scores = GetScoreDetails();
+
+  std::map<GURL, double> score_map;
+  for (const auto& info : scores) {
+    score_map[info.origin] = info.total_score;
   }
 
   return score_map;
 }
 
 void SiteEngagementService::HandleNotificationInteraction(const GURL& url) {
   if (!ShouldRecordEngagement(url))
     return;
 
   SiteEngagementMetrics::RecordEngagement(
@@ skipping 72 matching lines @@
     SiteEngagementService::Helper* helper) {
   helpers_.insert(helper);
 }
 
 void SiteEngagementService::HelperDeleted(
     SiteEngagementService::Helper* helper) {
   helpers_.erase(helper);
 }
 
 double SiteEngagementService::GetScore(const GURL& url) const {
+  return GetScoreDetails(url).total_score;
+}
+
+mojom::SiteEngagementDetails SiteEngagementService::GetScoreDetails(
+    const GURL& url) const {
   // Ensure that if engagement is stale, we clean things up before fetching the
   // score.
   if (IsLastEngagementStale())
     CleanupEngagementScores(true);
 
-  return CreateEngagementScore(url).GetScore();
+  return CreateEngagementScore(url).GetDetails();
 }
 
 double SiteEngagementService::GetTotalEngagementPoints() const {
   std::map<GURL, double> score_map = GetScoreMap();
 
   double total_score = 0;
   for (const auto& value : score_map)
     total_score += value.second;
 
   return total_score;
@@ skipping 106 matching lines @@
               last_engagement_time - score.last_engagement_time();
           base::Time rebase_score_time = rebase_time - offset;
           score.set_last_engagement_time(rebase_score_time);
         }
 
         if (score.last_engagement_time() > new_last_engagement_time)
           new_last_engagement_time = score.last_engagement_time();
         score.Commit();
       }
 
-      if (score.GetScore() > SiteEngagementScore::GetScoreCleanupThreshold())
+      if (score.GetTotal() > SiteEngagementScore::GetScoreCleanupThreshold())
         continue;
     }
 
     // This origin has a score of 0. Wipe it from content settings.
     settings_map->SetWebsiteSettingDefaultScope(
         origin, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
         nullptr);
   }
 
   // Set the last engagement time to be consistent with the scores. This will
@@ skipping 258 matching lines @@
     // under the origin in history. If this new last visit date is long enough
     // in the past, the next time the origin's engagement is accessed the
     // automatic decay will kick in - i.e. a double decay will have occurred.
     // To prevent this, compute the decay that would have taken place since the
     // new last visit and add it to the engagement at this point. When the
     // engagement is next accessed, it will decay back to the proportionally
     // reduced value rather than being decayed once here, and then once again
     // when it is next accessed.
     SiteEngagementScore engagement_score = CreateEngagementScore(origin);
 
-    double new_score = proportion_remaining * engagement_score.GetScore();
+    double new_score = proportion_remaining * engagement_score.GetTotal();

[Wez, 2017/04/08 01:52:53]

This logic looks wrong; we're grabbing the *total* score, decaying it and
Reset()ing based on that - should we actually be grabbing the underlying
raw_score? or the decayed score? Or the base score?

Why do we even have decay-handling logic in here, rather than in
SiteEngagementScore itself?

[dominickn, 2017/04/10 00:58:11]

You're right, this should be the underlying raw_score that's decayed. The bonus
concept has become more prominent since it now includes origins with
notification permission (way more than previously, which was just whether or not
a site was installed). When this was first implemented, the bonus wasn't really
hit that often so it wasn't important.

Using the raw_score will stay true to the original intention of the proportion
decay.
Legacy holdover from before SiteEngagementScore existed (and when the decay was
much simpler - a linear decay of 5 per week). :)

Makes sense to me to move this into a SiteEngagementScore::Decay(base::Time now)
method, which does everything except commit.

[Wez, 2017/04/10 03:45:28]

That sounds great, but reading through the comment above, I wasn't able to grok
why we have both this "explicit" decay logic and separate "automatic" logic (as
per the comment), rather than just letting the automatic decay do its job.

It almost sounds like |raw_score_| should be replaced with direct calculation
based on the remaining URLs for the origin, in the history.

For now I've added a ::raw_score() getter to Score and used it here - let's get
the Important Sites issue fixed and come back to clean this up.

[dominickn, 2017/04/10 04:41:26]

Privacy required us to explicitly decay the engagement score when the user
cleared their history. This means that you avoid the situation where a user has
cleared their history, but the engagement remains high (i.e. "revealing"
information they've cleared) So this decay is proportionate to how many history
items they removed, and is separate to the automatic time-based decay we already
do.

We don't want to decay based on URLs remaining in the history all the time
though, since there are single-page apps and the like that will only ever
generate one history entry (and vice-versa multi-page sites which generate
oodles of history entries). Time-based decay is what we want in general.

Anyway, circling back to this later SGTM. 

[Wez, 2017/04/10 21:18:09]

The double-decay stuff was cofusing; I realised it was just that in starting
from GetScore() there is already decay included, hence the need for the complex
compensation logic.  So using raw_score() here is only correct if you remove the
compensation.
Yup, I'll switch back to GetTotalScore() and add a TODO to cleanup.

     int hours_since_engagement = (now - last_visit).InHours();
     int periods =
         hours_since_engagement / SiteEngagementScore::GetDecayPeriodInHours();
     new_score += periods * SiteEngagementScore::GetDecayPoints();
     new_score *= pow(1.0 / SiteEngagementScore::GetDecayProportion(), periods);
 
     double score = std::min(SiteEngagementScore::kMaxPoints, new_score);
     engagement_score.Reset(score, last_visit);
     if (!engagement_score.last_shortcut_launch_time().is_null() &&
         engagement_score.last_shortcut_launch_time() > last_visit) {
       engagement_score.set_last_shortcut_launch_time(last_visit);
     }
 
     engagement_score.Commit();
   }
 
   SetLastEngagementTime(now);
 }