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 <memory>
 #include <set>
@@ skipping 11 matching lines @@
 class DictionaryValue;
 class Clock;
 }
 
 namespace history {
 class HistoryService;
 }
 
 class GURL;
 class Profile;
+class SiteEngagementScore;
 
 class SiteEngagementScoreProvider {
  public:
   // Returns a non-negative integer representing the engagement score of the
   // origin for this URL.
   virtual double GetScore(const GURL& url) const = 0;
 
   // Returns the sum of engagement points awarded to all sites.
   virtual double GetTotalEngagementPoints() const = 0;
 };
@@ skipping 43 matching lines @@
   // Returns the engagement level of |url|. This is the recommended API for
   // clients
   EngagementLevel GetEngagementLevel(const GURL& url) const;
 
   // Returns a map of all stored origins and their engagement scores.
   std::map<GURL, double> GetScoreMap() const;
 
   // Returns whether the engagement service has enough data to make meaningful
   // decisions. Clients should avoid using engagement in their heuristic until
   // this is true.
   bool IsBootstrapped();

[dominickn, 2016/05/25 06:52:43]

const

[calamity, 2016/05/26 08:26:20]

Done.

 
   // Returns whether |url| has at least the given |level| of engagement.
   bool IsEngagementAtLeast(const GURL& url, EngagementLevel level) const;
 
   // Resets the engagement score |url| to |score|, clearing daily limits.
   void ResetScoreForURL(const GURL& url, double score);

[dominickn, 2016/05/25 06:52:43]

const

[calamity, 2016/05/26 08:26:20]

So.... I don't want this to be const because it's bananas for setters to be
const. So I added a const override for GetEngagementScore so that this is no
longer const-able.

[dominickn, 2016/05/26 09:33:20]

Acknowledged.

 
   // Update the last time |url| was opened from an installed shortcut to be
   // clock_->Now().
   void SetLastShortcutLaunchTime(const GURL& url);

[dominickn, 2016/05/25 06:52:43]

const

 
   // Overridden from SiteEngagementScoreProvider.
   double GetScore(const GURL& url) const override;
   double GetTotalEngagementPoints() const override;
 
  private:
   FRIEND_TEST_ALL_PREFIXES(SiteEngagementServiceTest, CheckHistograms);
   FRIEND_TEST_ALL_PREFIXES(SiteEngagementServiceTest, CleanupEngagementScores);
   FRIEND_TEST_ALL_PREFIXES(SiteEngagementServiceTest, ClearHistoryForURLs);
   FRIEND_TEST_ALL_PREFIXES(SiteEngagementServiceTest, GetMedianEngagement);
   FRIEND_TEST_ALL_PREFIXES(SiteEngagementServiceTest, GetTotalNavigationPoints);
   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);
 
   // Only used in tests.
   SiteEngagementService(Profile* profile, std::unique_ptr<base::Clock> clock);
 
   // 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);

[dominickn, 2016/05/25 06:52:43]

const

 
+  // Retrieves the SiteEngagementScore object for |origin|.
+  SiteEngagementScore GetEngagementScore(const GURL& origin) const;

[dominickn, 2016/05/25 06:52:43]

This could be in the anonymous namespace in site_engagement_sevice.cc, taking a
clock pointer and origin argument. Then you don't need the forward declaration
of SiteEngagementScore.

[calamity, 2016/05/26 08:26:20]

I'd rather not do that for two reasons:
- I'd have to pass clock_ and profile_ at every call which is a bit verbose for
no reason.
- It would allow const setters again. Blech.

[dominickn, 2016/05/26 09:33:20]

Acknowledged.

+
   // 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;
 
   // Update the engagement score of the origin matching |url| for media playing.
@@ skipping 20 matching lines @@
   // respectively.
   int OriginsWithMaxDailyEngagement() const;
   int OriginsWithMaxEngagement(const std::map<GURL, double>& score_map) const;
 
   // Callback for the history service when it is asked for a map of origins to
   // how many URLs corresponding to that origin remain in history.
   void GetCountsAndLastVisitForOriginsComplete(
     history::HistoryService* history_service,
     const std::multiset<GURL>& deleted_url_origins,
     bool expired,
     const history::OriginCountAndLastVisitMap& remaining_origin_counts);

[dominickn, 2016/05/25 06:52:43]

Can this method be const now?

[calamity, 2016/05/26 08:26:20]

Not anymore ;)

[dominickn, 2016/05/26 09:33:20]

Acknowledged.

 
-  // 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);
-
   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_