Small cleanup of SiteEngagementService.

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 77a1c5f0a86193f0255ee64f5b5d89d3743e6bd6..be9587503423b1ebd1b33e03f48c9ec38f65fdab 100644
--- a/chrome/browser/engagement/site_engagement_service.h
+++ b/chrome/browser/engagement/site_engagement_service.h
@@ -27,6 +27,7 @@ class HistoryService;
 }
 
 class GURL;
+class HostContentSettingsMap;
 class Profile;
 
 class SiteEngagementScore {
@@ -105,16 +106,9 @@ class SiteEngagementScore {
   double Score() const;
   void AddPoints(double points);
 
-  // Resets the score to |points| and resets the daily point limit. If
-  // |updated_time| is non-null, sets the last engagement time and last
-  // shortcut launch time (if it is non-null) to |updated_time|. Otherwise, last
-  // engagement time is set to the current time and last shortcut launch time is
-  // left unchanged.
-  // TODO(calamity): Ideally, all SiteEngagementScore methods should take a
-  // base::Time argument like this one does rather than each Score hold a
-  // pointer to a base::Clock. Then SiteEngagementScore doesn't need to worry
-  // about clock vending. See crbug.com/604305
-  void Reset(double points, const base::Time* updated_time);
+  // Resets the score to |points|, resets the daily point limit and sets the
+  // last engagement time to |last_engagement_time|.
+  void Reset(double points, const base::Time last_engagement_time);
 
   // Returns true if the maximum number of points today has been added.
   bool MaxPointsPerDayAdded() const;
@@ -291,9 +285,36 @@ class SiteEngagementService : public KeyedService,
   FRIEND_TEST_ALL_PREFIXES(AppBannerSettingsHelperTest, SiteEngagementTrigger);
   FRIEND_TEST_ALL_PREFIXES(ImportantSitesUtilTest, NotificationsThenEngagement);
 
+  class ScopedEngagementScore {
+   public:
+    ScopedEngagementScore(HostContentSettingsMap* settings_map,
+                          const GURL& url,
+                          base::Clock* clock,
+                          bool update);
+    ~ScopedEngagementScore();
+
+    SiteEngagementScore* get() { return &score_; }
+
+   private:
+    std::unique_ptr<base::DictionaryValue> score_dict_;
+    SiteEngagementScore score_;
+    bool update_;
+    GURL url_;
+
+    HostContentSettingsMap* settings_map_;
+
+    DISALLOW_COPY_AND_ASSIGN(ScopedEngagementScore);
+  };
+
   // Only used in tests.
   SiteEngagementService(Profile* profile, std::unique_ptr<base::Clock> clock);
 
+  std::unique_ptr<ScopedEngagementScore> GetEngagementScore(const GURL& url,
+                                                            bool update);

[dominickn, 2016/05/02 21:13:49]

I don't really like this ScopedEngagementScore.
 - bool in the interface is sad. It's not obvious that |update| means "update
the content setting when this goes out of scope"
 - everything in here could just be moved into SiteEngagementScore, with the
destructor stuff in an explicit Save() method
 - we should also make GetSiteEngagementScore() as as private method in
SiteEngagementService, with tests blessed to access it. Then we can test
properties of SiteEngagementScore when testing SiteEngagementService (e.g. last
shortcut launch time)

+
+  std::unique_ptr<ScopedEngagementScore> GetEngagementScore(
+      const GURL& url) const;
+
   // Adds the specified number of points to the given origin, respecting the
   // maximum limits for the day and overall.
   void AddPoints(const GURL& url, double points);
@@ -313,17 +334,19 @@ class SiteEngagementService : public KeyedService,
   int OriginsWithMaxEngagement(const std::map<GURL, double>& score_map) const;
 
   void GetCountsAndLastVisitForOriginsComplete(
-    history::HistoryService* history_service,
-    const std::multiset<GURL>& deleted_url_origins,
-    bool expired,
-    const history::OriginCountAndLastVisitMap& remaining_origin_counts);
+      history::HistoryService* history_service,
+      const std::multiset<GURL>& deleted_url_origins,
+      bool expired,
+      const history::OriginCountAndLastVisitMap& remaining_origin_counts);
 
   // Resets the engagement score for |url| to |score|, and sets the last
-  // engagement time and last shortcut launch time (if it is non-null) to
-  // |updated_time|. Clears daily limits.
-  void ResetScoreAndAccessTimesForURL(const GURL& url,
-                                      double score,
-                                      const base::Time* updated_time);
+  // engagement time and last shortcut launch time (if it is non-null). Clears
+  // daily maximum point limits.
+  void ResetScoreAndAccessTimesForURL(
+      const GURL& url,
+      double score,
+      const base::Time last_engagement_time,
+      const base::Time* last_shortcut_launch_time);

[dominickn, 2016/05/02 21:13:49]

Having one argument be the raw time and a second argument being a pointer is
kind of awful and I'm desperately trying to think of a way we can avoid it.

 
   Profile* profile_;