Small cleanup of SiteEngagementService.

chrome/browser/engagement/site_engagement_service.h

 // 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.
 
 #ifndef CHROME_BROWSER_ENGAGEMENT_SITE_ENGAGEMENT_SERVICE_H_
 #define CHROME_BROWSER_ENGAGEMENT_SITE_ENGAGEMENT_SERVICE_H_
 
 #include <map>
 #include <set>
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "chrome/browser/engagement/site_engagement_metrics.h"
 #include "components/history/core/browser/history_service_observer.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "ui/base/page_transition_types.h"
 
 namespace base {
 class DictionaryValue;
 class Clock;
 }
 
 namespace history {
 class HistoryService;
 }
 
 class GURL;
+class HostContentSettingsMap;
 class Profile;
 
 class SiteEngagementScore {
  public:
   // The parameters which can be varied via field trial. All "points" values
   // should be appended to the end of the enum prior to MAX_VARIATION.
   enum Variation {
     // The maximum number of points that can be accrued in one day.
     MAX_POINTS_PER_DAY = 0,
 
@@ skipping 58 matching lines @@
   // The SiteEngagementService does not take ownership of |clock|. It is the
   // responsibility of the caller to make sure |clock| outlives this
   // SiteEngagementScore.
   SiteEngagementScore(base::Clock* clock,
                       const base::DictionaryValue& score_dict);
   ~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;
 
   // Get/set the last time this origin was launched from an installed shortcut.
   base::Time last_shortcut_launch_time() const {
     return last_shortcut_launch_time_;
   }
   void set_last_shortcut_launch_time(const base::Time& time) {
     last_shortcut_launch_time_ = time;
@@ skipping 156 matching lines @@
   FRIEND_TEST_ALL_PREFIXES(SiteEngagementServiceTest, GetTotalUserInputPoints);
   FRIEND_TEST_ALL_PREFIXES(SiteEngagementServiceTest, LastShortcutLaunch);
   FRIEND_TEST_ALL_PREFIXES(SiteEngagementServiceTest,
                            CleanupOriginsOnHistoryDeletion);
   FRIEND_TEST_ALL_PREFIXES(SiteEngagementServiceTest, IsBootstrapped);
   FRIEND_TEST_ALL_PREFIXES(SiteEngagementServiceTest, EngagementLevel);
   FRIEND_TEST_ALL_PREFIXES(SiteEngagementServiceTest, ScoreDecayHistograms);
   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);
 
   // Post startup tasks: cleaning up origins which have decayed to 0, and
   // logging UMA statistics.
   void AfterStartupTask();
   void CleanupEngagementScores();
   void RecordMetrics();
 
   // Returns the median engagement score of all recorded origins.
   double GetMedianEngagement(const std::map<GURL, double>& score_map) const;
 
   // Returns the number of origins with maximum daily and total engagement
   // respectively.
   int OriginsWithMaxDailyEngagement() const;
   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_;
 
   // The clock used to vend times.
   std::unique_ptr<base::Clock> clock_;
 
   // Metrics are recorded at non-incognito browser startup, and then
   // approximately once per hour thereafter. Store the local time at which
   // metrics were previously uploaded: the first event which affects any
   // origin's engagement score after an hour has elapsed triggers the next
   // upload.
   base::Time last_metrics_time_;
 
   base::WeakPtrFactory<SiteEngagementService> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SiteEngagementService);
 };
 
 #endif  // CHROME_BROWSER_ENGAGEMENT_SITE_ENGAGEMENT_SERVICE_H_