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 121 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) {

[dominickn, 2017/04/06 01:47:16]

A nuance here is that sites with only a notification bonus score (and no actual
engagement) won't appear in the engagement_settings list. We'll need to also
iterate through the notification content setting. 

For now, this method can fetch a list of all URLs with notification permission,
and then iterate over the engagement content setting, removing origins from the
permission lists as they are seen. Then at the end we add all the remaining
origins in the permission list to the vector with their bonus engagement. WDYT?

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

IIUC correctly what you're saying is basically:

set<URL> notification_permission_urls = ...
set<URL> engagement_content_settings_urls = ...

set<URL> unengaged_urls = engagement_content_settings_urls -
notification_permission_urls;

for (engagement_content_settings_urls)
  result.insert(GetURLScore(url).GetDetails())

for (unengaged_urls)
  result.insert(SomehowGetBonusEngagement)

Two concerns:
1. What is the mechanism for getting the "bonus" for un-engaged URLs? If we call
GetScoreDetails() then that'll implicitly create a new SiteEngagementScore
in-memory - is that desirable?
2. This seems a lot of logic (most importantly a lot of memory-churn) to
calculate a result. :-/

Ideally we'd have a single internal API that gets the set of URLs to process,
and then have GetScoreMap, GetScoreDetails etc call that to determine what to
do; I'll see if that will work.

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

This is what I had in mind (slight difference to yours):

set<URL> notification_permission_urls = ...
set<URL> engagement_content_settings_urls = ...

for (engagement_content_settings_urls) {
  result.insert(GetURLScore(url).GetDetails())
  notification_permission_urls -= url
}

for (notification_permission_urls) {
  // if the URL had any non-bonus engagement it's caught in the first for loop
  result.insert(GetURLScore(url).GetDetails())
}
That should be fine. GetScoreDetails will be infrequently called:

1. when the important sites dialog is triggered (user clears browsing data)
2. when UMA stats are calculated (on startup and once per hour after that)
3. chrome://site-engagement page on load
4. tests
Ideally, yes, there would be a single internal API that gets the set of URLs to
process. But the only consumer of that method should be GetScoreDetails (we
should remove GetScoreMap because it should be supplanted completed by
GetScoreDetails). There shouldn't be any other uses that don't require scores
attached.

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

That makes sense; what I've actually done in the latest patch is collate a
set<GURL>, since all we use those content-settings fetches for is gathering the
URLs to process, so that we basically insert URLs from the two sources into a
single set and iterate that.
OK, the startup-time enumeration means we should be a little careful about cost.
The page refreshes every second or two, though.
There are several other calls that iterate over the URLs - the latest patch
replaces all the call-sites with the new set-collating implementation.

WDYT?

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

It's done using content::PostAfterStartupTask, so this should avoid any startup
critical paths and get called eventually.
Yep, but it's an internal / dev-focused page rather than being obviously
end-user exposed, so I'm comfortable with this.

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

Acknowledged.

     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.score;

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

Note to self; this also leads to loads of memory churn; see if we can
std::move(info.origin), otherwise revert to calling GetTotalScore(url) rather
than going via Details.

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

Even better: we should be able to remove this method once important sites and
tests are ported to use GetScoreDetails. :)

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

Acknowledged.

   }
 
   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).score;
+}
+
+mojom::SiteEngagementDetails SiteEngagementService::GetScoreDetails(

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

Note to self: This clashes with GetScoreDetails() above; rename one or the
other.

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

Renamed the two in the latest patch.

+    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 404 matching lines @@
     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);
 }