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

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 43b83532572753245b9cc3450ba3c21d540158c9..7c0c534fd41c2c64bef40c2503ba21d0cdd05df9 100644
--- a/chrome/browser/engagement/site_engagement_service.cc
+++ b/chrome/browser/engagement/site_engagement_service.cc
@@ -8,7 +8,6 @@
 
 #include <algorithm>
 #include <utility>
-#include <vector>
 
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
@@ -52,13 +51,34 @@ bool g_updated_from_variations = false;
 // Length of time between metrics logging.
 const int kMetricsIntervalInMinutes = 60;
 
-std::unique_ptr<ContentSettingsForOneType> GetEngagementContentSettings(
-    HostContentSettingsMap* settings_map) {
-  std::unique_ptr<ContentSettingsForOneType> engagement_settings(
-      new ContentSettingsForOneType);
+// Returns the combined list of origins which either have site engagement
+// data stored, or have other settings that would provide a score bonus.
+std::set<GURL> GetEngagementOriginsFromContentSettings(Profile* profile) {
+  HostContentSettingsMap* settings_map =
+      HostContentSettingsMapFactory::GetForProfile(profile);
+
+  ContentSettingsForOneType content_settings_list;
+  std::set<GURL> urls;
+
+  // Fetch site engagement content settings.
   settings_map->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT,
-                                      std::string(), engagement_settings.get());
-  return engagement_settings;
+                                      content_settings::ResourceIdentifier(),
+                                      &content_settings_list);
+  for (const auto& site : content_settings_list) {

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

Nit: no braces

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

Done, though note that other areas of Chromium tend to use braces all the time,
to mitigate potential security issues due to typos. :)

+    urls.insert(GURL(site.primary_pattern.ToString()));
+  }
+
+  // Fetch notifications allowed content settings.
+  settings_map->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+                                      content_settings::ResourceIdentifier(),
+                                      &content_settings_list);
+  for (const auto& site : content_settings_list) {
+    if (site.setting == CONTENT_SETTING_ALLOW)

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

Should this be != ?

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

Yup; and of course my simple test wouldn't catch that... *facepalm*

+      continue;
+    urls.insert(GURL(site.primary_pattern.ToString()));
+  }
+
+  return urls;
 }
 
 // Only accept a navigation event for engagement if it is one of:
@@ -104,8 +124,7 @@ 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).GetTotalScore();
 }
 
 SiteEngagementService::SiteEngagementService(Profile* profile)
@@ -139,21 +158,28 @@ SiteEngagementService::GetEngagementLevel(const GURL& url) const {
   return CreateEngagementScore(url).GetEngagementLevel();
 }
 
-std::map<GURL, double> SiteEngagementService::GetScoreMap() const {
-  HostContentSettingsMap* settings_map =
-      HostContentSettingsMapFactory::GetForProfile(profile_);
-  std::unique_ptr<ContentSettingsForOneType> engagement_settings =
-      GetEngagementContentSettings(settings_map);
+std::vector<mojom::SiteEngagementDetails> SiteEngagementService::GetAllDetails()
+    const {
+  std::set<GURL> origins = GetEngagementOriginsFromContentSettings(profile_);
 
-  std::map<GURL, double> score_map;
-  for (const auto& site : *engagement_settings) {
-    GURL origin(site.primary_pattern.ToString());
+  std::vector<mojom::SiteEngagementDetails> details;
+  details.reserve(origins.size());
+  for (const GURL& origin : origins) {
     if (!origin.is_valid())
       continue;
+    details.push_back(GetDetails(origin));
+  }
+
+  return details;
+}
 
+std::map<GURL, double> SiteEngagementService::GetScoreMap() const {
+  std::map<GURL, double> score_map;
+  for (const GURL& origin : GetEngagementOriginsFromContentSettings(profile_)) {
+    if (!origin.is_valid())
+      continue;
     score_map[origin] = GetScore(origin);
   }
-
   return score_map;
 }
 
@@ -244,12 +270,17 @@ void SiteEngagementService::HelperDeleted(
 }
 
 double SiteEngagementService::GetScore(const GURL& url) const {
+  return GetDetails(url).total_score;
+}
+
+mojom::SiteEngagementDetails SiteEngagementService::GetDetails(
+    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 {
@@ -317,11 +348,6 @@ void SiteEngagementService::AfterStartupTask() {
 
 void SiteEngagementService::CleanupEngagementScores(

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

This cleanup method doesn't need to iterate over origins with notification
permission. Its only purpose is to clean up the engagement content setting of
sites with 0 raw scores, and rescale things in there if Chrome hasn't been used
in a while (so you don't get a huge loss if you go on holiday). Notifications is
always a flat boost, so it's not necessary to capture origins which only have
notification permission (and no raw engagement) in this method.

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

Done.

     bool update_last_engagement_time) const {
-  HostContentSettingsMap* settings_map =
-      HostContentSettingsMapFactory::GetForProfile(profile_);
-  std::unique_ptr<ContentSettingsForOneType> engagement_settings =
-      GetEngagementContentSettings(settings_map);
-
   // We want to rebase last engagement times relative to MaxDecaysPerScore
   // periods of decay in the past.
   base::Time now = clock_->Now();
@@ -341,9 +367,9 @@ void SiteEngagementService::CleanupEngagementScores(
   if (last_engagement_time > now)
     last_engagement_time = now;
 
-  for (const auto& site : *engagement_settings) {
-    GURL origin(site.primary_pattern.ToString());
-
+  HostContentSettingsMap* settings_map =
+      HostContentSettingsMapFactory::GetForProfile(profile_);
+  for (const GURL& origin : GetEngagementOriginsFromContentSettings(profile_)) {
     if (origin.is_valid()) {
       SiteEngagementScore score = CreateEngagementScore(origin);
       if (update_last_engagement_time) {
@@ -376,7 +402,8 @@ void SiteEngagementService::CleanupEngagementScores(
         score.Commit();
       }
 
-      if (score.GetScore() > SiteEngagementScore::GetScoreCleanupThreshold())
+      if (score.GetTotalScore() >
+          SiteEngagementScore::GetScoreCleanupThreshold())
         continue;
     }
 
@@ -577,16 +604,10 @@ SiteEngagementScore SiteEngagementService::CreateEngagementScore(
 }
 
 int SiteEngagementService::OriginsWithMaxDailyEngagement() const {
-  HostContentSettingsMap* settings_map =
-      HostContentSettingsMapFactory::GetForProfile(profile_);
-  std::unique_ptr<ContentSettingsForOneType> engagement_settings =
-      GetEngagementContentSettings(settings_map);
-
   int total_origins = 0;
 
   // We cannot call GetScoreMap as we need the score objects, not raw scores.
-  for (const auto& site : *engagement_settings) {
-    GURL origin(site.primary_pattern.ToString());
+  for (const GURL& origin : GetEngagementOriginsFromContentSettings(profile_)) {

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

An origin can't possibly get maximum daily engagement unless it has an entry in
the engagement content setting (max daily = 15), so it's probably not necessary
to do this here.

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

Done.

     if (!origin.is_valid())
       continue;
 
@@ -655,7 +676,7 @@ void SiteEngagementService::GetCountsAndLastVisitForOriginsComplete(
     // 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.raw_score();
     int hours_since_engagement = (now - last_visit).InHours();
     int periods =
         hours_since_engagement / SiteEngagementScore::GetDecayPeriodInHours();