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

chrome/browser/engagement/site_engagement_score.cc

Index: chrome/browser/engagement/site_engagement_score.cc
diff --git a/chrome/browser/engagement/site_engagement_score.cc b/chrome/browser/engagement/site_engagement_score.cc
index 10fe452045ed6ec5b34dee916337b8735971a19e..6b9d00de7d1b2409b8981913655309de3be33662 100644
--- a/chrome/browser/engagement/site_engagement_score.cc
+++ b/chrome/browser/engagement/site_engagement_score.cc
@@ -270,8 +270,20 @@ void SiteEngagementScore::AddPoints(double points) {
   last_engagement_time_ = now;
 }
 
-double SiteEngagementScore::GetScore() const {
-  return std::min(DecayedScore() + BonusScore(), kMaxPoints);
+mojom::SiteEngagementDetails SiteEngagementScore::GetDetails() const {
+  mojom::SiteEngagementDetails engagement;
+  engagement.origin = origin_;
+  engagement.base_score = DecayedScore();
+  engagement.installed_bonus = BonusIfShortcutLaunched();
+  engagement.notifications_bonus = BonusIfHasNotifications();
+  engagement.total_score = GetTotal();
+  return engagement;
+}
+
+double SiteEngagementScore::GetTotal() const {

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

Renamed this to GetTotal, for clarity, and rearranged things to avoid the churn
of calling GetDetails() under the hood.

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

I'm not sure that GetTotal is more clear than GetScore (plus the comment in the
header file can clarify). But I'm not overly fussy on this.

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

The class is SiteEngagementScore, so SiteEngagementScore::GetTotal() seems
reasonably intuitive, and more obvious than Site...::GetScore().  If you prefer
GetTotalScore() though then happy to update to that.

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

Not fussed, it can stay.

+  return std::min(
+      DecayedScore() + BonusIfShortcutLaunched() + BonusIfHasNotifications(),
+      kMaxPoints);
 }
 
 void SiteEngagementScore::Commit() {
@@ -287,7 +299,7 @@ void SiteEngagementScore::Commit() {
 blink::mojom::EngagementLevel SiteEngagementScore::GetEngagementLevel() const {
   DCHECK_LT(GetMediumEngagementBoundary(), GetHighEngagementBoundary());
 
-  double score = GetScore();
+  double score = GetTotal();
   if (score == 0)
     return blink::mojom::EngagementLevel::NONE;
 
@@ -400,20 +412,22 @@ double SiteEngagementScore::DecayedScore() const {
                            periods * GetDecayPoints());
 }
 
-double SiteEngagementScore::BonusScore() const {
-  double bonus = 0;
+double SiteEngagementScore::BonusIfShortcutLaunched() const {
   int days_since_shortcut_launch =
       (clock_->Now() - last_shortcut_launch_time_).InDays();
   if (days_since_shortcut_launch <= kMaxDaysSinceShortcutLaunch)

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

nit: While we'll never hit this in practice, this code would have had unexpected
behaviour in January 1970 (and it does behave unexpectedly in tests with
SimpleTestClock...).

-    bonus += GetWebAppInstalledPoints();
+    return GetWebAppInstalledPoints();
+  return 0;
+}
 
+double SiteEngagementScore::BonusIfHasNotifications() const {

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

Nit: I'd call this BonusIfCanShowNotifications()

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

Done.

   // TODO(dominickn, raymes): call PermissionManager::GetPermissionStatus when
   // the PermissionManager is thread-safe.
   if (settings_map_ && settings_map_->GetContentSetting(
                            origin_, GURL(), CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
                            std::string()) == CONTENT_SETTING_ALLOW) {
-    bonus += GetNotificationPermissionPoints();
+    return GetNotificationPermissionPoints();
   }
 
-  return bonus;
+  return 0;
 }