Plumb site engagement to the renderer process.

chrome/browser/engagement/site_engagement_service.h

Index: chrome/browser/engagement/site_engagement_service.h
diff --git a/chrome/browser/engagement/site_engagement_service.h b/chrome/browser/engagement/site_engagement_service.h
index 65e22334d8f10e947e2d42a9d5624716a9a79459..bfb4c29e40513e01c6da379d2d84340f0afbddd1 100644
--- a/chrome/browser/engagement/site_engagement_service.h
+++ b/chrome/browser/engagement/site_engagement_service.h
@@ -18,6 +18,7 @@
 #include "chrome/browser/engagement/site_engagement_observer.h"
 #include "components/history/core/browser/history_service_observer.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "third_party/WebKit/public/platform/site_engagement.mojom.h"
 #include "ui/base/page_transition_types.h"
 
 namespace base {
@@ -70,14 +71,6 @@ class SiteEngagementService : public KeyedService,
   // the service of them.
   class Helper;
 
-  enum EngagementLevel {
-    ENGAGEMENT_LEVEL_NONE,
-    ENGAGEMENT_LEVEL_LOW,
-    ENGAGEMENT_LEVEL_MEDIUM,
-    ENGAGEMENT_LEVEL_HIGH,
-    ENGAGEMENT_LEVEL_MAX,
-  };
-
   // The name of the site engagement variation field trial.
   static const char kEngagementParams[];
 
@@ -108,7 +101,7 @@ class SiteEngagementService : public KeyedService,
   ~SiteEngagementService() override;
 
   // Returns the engagement level of |url|.
-  EngagementLevel GetEngagementLevel(const GURL& url) const;
+  blink::mojom::EngagementLevel GetEngagementLevel(const GURL& url) const;
 
   // Returns a map of all stored origins and their engagement scores.
   std::map<GURL, double> GetScoreMap() const;
@@ -119,7 +112,8 @@ class SiteEngagementService : public KeyedService,
   bool IsBootstrapped() const;
 
   // Returns whether |url| has at least the given |level| of engagement.
-  bool IsEngagementAtLeast(const GURL& url, EngagementLevel level) const;
+  bool IsEngagementAtLeast(const GURL& url,
+                           blink::mojom::EngagementLevel level) const;
 
   // Resets the engagement score |url| to |score|, clearing daily limits.
   void ResetScoreForURL(const GURL& url, double score);
@@ -128,6 +122,9 @@ class SiteEngagementService : public KeyedService,
   // clock_->Now().
   void SetLastShortcutLaunchTime(const GURL& url);
 
+  void HelperCreated(SiteEngagementService::Helper* helper);

[benwells, 2016/12/09 06:11:24]

This seems like an observer now. Apart from this, there doesn't appear to be any
reason for the Helper to be a nested class. Can you:
- remove the Helper nested class
- add an Observer nested class
- change SiteEnagementService::Helper to be SiteEngagementHelper
- make the SiteEngagementHelper implement the SiteEngagementService::Observer
interface
- use the normal AddObserver, ObserverList etc. patterns

[dominickn, 2016/12/13 06:27:37]

I've revamped the Helper to make it more of a private implementation detail of
the service. WDYT?

[benwells, 2016/12/14 06:17:53]

Hrmph. I think it is now more obscure. Not only is the observer not called an
observer or managed like other observers, the creation feels more convoluted. 

I won't block you on this. I think the weirdness is all private to this class'
implementation and tests, and you pretty much own all of it, so I'll defer to
you. But I'd prefer the previous patch set as the helper creation was more
straightforward, so will lg if you go back to that.

[dominickn, 2016/12/15 01:57:08]

Done.

I think the main thing here is that we have what really is a private
implementation detail (the helper) in a separate .h/.cc file for clarity
(otherwise the service .h/.cc files would be enormous). As much as possible, we
want to keep the helper -> service interface as a private detail.

+  void HelperDeleted(SiteEngagementService::Helper* helper);
+
   // Overridden from SiteEngagementScoreProvider.
   double GetScore(const GURL& url) const override;
   double GetTotalEngagementPoints() const override;
@@ -218,6 +215,11 @@ class SiteEngagementService : public KeyedService,
   void HandleUserInput(content::WebContents* web_contents,
                        SiteEngagementMetrics::EngagementType type);
 
+  // Called if |url| changes to |level| engagement, and informs every Helper of
+  // the change.
+  void SendLevelChangeToHelpers(const GURL& url,
+                                blink::mojom::EngagementLevel level);
+
   // Returns true if the last engagement increasing event seen by the site
   // engagement service was sufficiently long ago that we need to reset all
   // scores to be relative to now. This ensures that users who do not use the
@@ -260,6 +262,9 @@ class SiteEngagementService : public KeyedService,
   // upload.
   base::Time last_metrics_time_;
 
+  // All helpers currently attached to a WebContents.
+  std::set<SiteEngagementService::Helper*> helpers_;
+
   // A list of observers. When any origin registers an engagement-increasing
   // event, each observer's OnEngagementIncreased method will be called.
   base::ObserverList<SiteEngagementObserver> observer_list_;