Small cleanup of SiteEngagementService.

chrome/browser/engagement/site_engagement_service.cc

Index: chrome/browser/engagement/site_engagement_service.cc
diff --git a/chrome/browser/engagement/site_engagement_service.cc b/chrome/browser/engagement/site_engagement_service.cc
index df709189e6bfac5542aad42bf67fcacb6e6c1e24..64531715876c21ea6904e95c6ab57df98a6a34e3 100644
--- a/chrome/browser/engagement/site_engagement_service.cc
+++ b/chrome/browser/engagement/site_engagement_service.cc
@@ -271,18 +271,12 @@ void SiteEngagementScore::AddPoints(double points) {
   last_engagement_time_ = now;
 }
 
-void SiteEngagementScore::Reset(double points, const base::Time* updated_time) {
+void SiteEngagementScore::Reset(double points,
+                                const base::Time last_engagement_time) {
   raw_score_ = points;
   points_added_today_ = 0;
 
-  // This must be set in order to prevent the score from decaying when read.
-  if (updated_time) {
-    last_engagement_time_ = *updated_time;
-    if (!last_shortcut_launch_time_.is_null())
-      last_shortcut_launch_time_ = *updated_time;
-  } else {
-    last_engagement_time_ = clock_->Now();
-  }
+  last_engagement_time_ = last_engagement_time;
 }
 
 bool SiteEngagementScore::MaxPointsPerDayAdded() const {
@@ -455,7 +449,9 @@ void SiteEngagementService::HandleMediaPlaying(const GURL& url,
 }
 
 void SiteEngagementService::ResetScoreForURL(const GURL& url, double score) {
-  ResetScoreAndAccessTimesForURL(url, score, nullptr);
+  // Set the last engagement time to Now in order to prevent the newly set
+  // score from decaying when read.
+  ResetScoreAndAccessTimesForURL(url, score, clock_->Now(), nullptr);
 }
 
 void SiteEngagementService::OnURLsDeleted(
@@ -478,16 +474,12 @@ void SiteEngagementService::OnURLsDeleted(
 }
 
 void SiteEngagementService::SetLastShortcutLaunchTime(const GURL& url) {
-  HostContentSettingsMap* settings_map =
-    HostContentSettingsMapFactory::GetForProfile(profile_);
-  std::unique_ptr<base::DictionaryValue> score_dict =
-      GetScoreDictForOrigin(settings_map, url);
-  SiteEngagementScore score(clock_.get(), *score_dict);
+  std::unique_ptr<ScopedEngagementScore> score = GetEngagementScore(url, true);
 
   // Record the number of days since the last launch in UMA. If the user's clock
   // has changed back in time, set this to 0.
   base::Time now = clock_->Now();
-  base::Time last_launch = score.last_shortcut_launch_time();
+  base::Time last_launch = score->get()->last_shortcut_launch_time();
   if (!last_launch.is_null()) {
     SiteEngagementMetrics::RecordDaysSinceLastShortcutLaunch(
         std::max(0, (now - last_launch).InDays()));
@@ -495,22 +487,11 @@ void SiteEngagementService::SetLastShortcutLaunchTime(const GURL& url) {
   SiteEngagementMetrics::RecordEngagement(
       SiteEngagementMetrics::ENGAGEMENT_WEBAPP_SHORTCUT_LAUNCH);
 
-  score.set_last_shortcut_launch_time(now);
-  if (score.UpdateScoreDict(score_dict.get())) {
-    settings_map->SetWebsiteSettingDefaultScope(
-        url, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
-        score_dict.release());
-  }
+  score->get()->set_last_shortcut_launch_time(now);
 }
 
 double SiteEngagementService::GetScore(const GURL& url) const {
-  HostContentSettingsMap* settings_map =
-    HostContentSettingsMapFactory::GetForProfile(profile_);
-  std::unique_ptr<base::DictionaryValue> score_dict =
-      GetScoreDictForOrigin(settings_map, url);
-  SiteEngagementScore score(clock_.get(), *score_dict);
-
-  return score.Score();
+  return GetEngagementScore(url)->get()->Score();
 }
 
 double SiteEngagementService::GetTotalEngagementPoints() const {
@@ -535,10 +516,7 @@ std::map<GURL, double> SiteEngagementService::GetScoreMap() const {
     if (!origin.is_valid())
       continue;
 
-    std::unique_ptr<base::DictionaryValue> score_dict =
-        GetScoreDictForOrigin(settings_map, origin);
-    SiteEngagementScore score(clock_.get(), *score_dict);
-    score_map[origin] = score.Score();
+    score_map[origin] = GetEngagementScore(origin)->get()->Score();
   }
 
   return score_map;
@@ -591,6 +569,34 @@ bool SiteEngagementService::IsEngagementAtLeast(
   return false;
 }
 
+SiteEngagementService::ScopedEngagementScore::ScopedEngagementScore(
+    HostContentSettingsMap* settings_map,
+    const GURL& url,
+    base::Clock* clock,
+    bool update)
+    : score_dict_(GetScoreDictForOrigin(settings_map, url)),
+      score_(clock, *score_dict_),
+      update_(update),
+      url_(url),
+      settings_map_(settings_map) {}
+
+SiteEngagementService::ScopedEngagementScore::~ScopedEngagementScore() {
+  if (update_) {
+    if (score_.Score() == 0) {
+      settings_map_->SetWebsiteSettingDefaultScope(
+          url_, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
+          nullptr);
+      return;
+    }
+
+    if (score_.UpdateScoreDict(score_dict_.get())) {
+      settings_map_->SetWebsiteSettingDefaultScope(
+          url_, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
+          score_dict_.release());
+    }
+  }
+}
+
 SiteEngagementService::SiteEngagementService(Profile* profile,
                                              std::unique_ptr<base::Clock> clock)
     : profile_(profile), clock_(std::move(clock)), weak_factory_(this) {
@@ -601,19 +607,23 @@ SiteEngagementService::SiteEngagementService(Profile* profile,
     history->AddObserver(this);
 }
 
+std::unique_ptr<SiteEngagementService::ScopedEngagementScore>
+SiteEngagementService::GetEngagementScore(const GURL& url, bool update) {
+  return base::WrapUnique(new ScopedEngagementScore(
+      HostContentSettingsMapFactory::GetForProfile(profile_), url, clock_.get(),
+      update));
+}
+
+std::unique_ptr<SiteEngagementService::ScopedEngagementScore>
+SiteEngagementService::GetEngagementScore(const GURL& url) const {
+  return base::WrapUnique(new ScopedEngagementScore(
+      HostContentSettingsMapFactory::GetForProfile(profile_), url, clock_.get(),
+      false));
+}
+
 void SiteEngagementService::AddPoints(const GURL& url, double points) {
-  HostContentSettingsMap* settings_map =
-    HostContentSettingsMapFactory::GetForProfile(profile_);
-  std::unique_ptr<base::DictionaryValue> score_dict =
-      GetScoreDictForOrigin(settings_map, url);
-  SiteEngagementScore score(clock_.get(), *score_dict);
-
-  score.AddPoints(points);
-  if (score.UpdateScoreDict(score_dict.get())) {
-    settings_map->SetWebsiteSettingDefaultScope(
-        url, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
-        score_dict.release());
-  }
+  std::unique_ptr<ScopedEngagementScore> score = GetEngagementScore(url, true);
+  score->get()->AddPoints(points);

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

Nit: if you're not going to null check, then GetEngagementScore(url,
true)->get()->AddPoints(points);

 }
 
 void SiteEngagementService::AfterStartupTask() {
@@ -630,16 +640,9 @@ void SiteEngagementService::CleanupEngagementScores() {
   for (const auto& site : *engagement_settings) {
     GURL origin(site.primary_pattern.ToString());
     if (origin.is_valid()) {
-      std::unique_ptr<base::DictionaryValue> score_dict =
-          GetScoreDictForOrigin(settings_map, origin);
-      SiteEngagementScore score(clock_.get(), *score_dict);
-      if (score.Score() != 0)
-        continue;
+      // When this goes out of scope, the update will trigger a delete.
+      GetEngagementScore(origin, true);
     }
-
-    settings_map->SetWebsiteSettingDefaultScope(
-        origin, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
-        nullptr);
   }
 }
 
@@ -711,10 +714,7 @@ int SiteEngagementService::OriginsWithMaxDailyEngagement() const {
     if (!origin.is_valid())
       continue;
 
-    std::unique_ptr<base::DictionaryValue> score_dict =
-        GetScoreDictForOrigin(settings_map, origin);
-    SiteEngagementScore score(clock_.get(), *score_dict);
-    if (score.MaxPointsPerDayAdded())
+    if (GetEngagementScore(origin)->get()->MaxPointsPerDayAdded())
       ++total_origins;
   }
 
@@ -784,33 +784,29 @@ void SiteEngagementService::GetCountsAndLastVisitForOriginsComplete(
     double score =
         std::min(SiteEngagementScore::kMaxPoints,
                  (proportion_remaining * GetScore(origin)) + undecay);
-    ResetScoreAndAccessTimesForURL(origin, score, &last_visit);
+    base::Time last_shortcut_launch_time =
+        GetEngagementScore(origin, false)->get()->last_shortcut_launch_time();
+    std::unique_ptr<base::Time> new_last_shortcut_time;
+    if (last_shortcut_launch_time > last_visit)
+      new_last_shortcut_time.reset(new base::Time(last_visit));
+    ResetScoreAndAccessTimesForURL(origin, score, last_visit,
+                                   new_last_shortcut_time.get());
   }
 }
 
 void SiteEngagementService::ResetScoreAndAccessTimesForURL(
-    const GURL& url, double score, const base::Time* updated_time) {
+    const GURL& url,
+    double score,
+    const base::Time last_engagement_time,
+    const base::Time* last_shortcut_launch_time) {
   DCHECK(url.is_valid());
   DCHECK_GE(score, 0);
   DCHECK_LE(score, SiteEngagementScore::kMaxPoints);
 
-  HostContentSettingsMap* settings_map =
-      HostContentSettingsMapFactory::GetForProfile(profile_);
-  std::unique_ptr<base::DictionaryValue> score_dict =
-      GetScoreDictForOrigin(settings_map, url);
-  SiteEngagementScore engagement_score(clock_.get(), *score_dict);
-
-  engagement_score.Reset(score, updated_time);
-  if (score == 0) {
-    settings_map->SetWebsiteSettingDefaultScope(
-        url, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
-        nullptr);
-    return;
-  }
-
-  if (engagement_score.UpdateScoreDict(score_dict.get())) {
-    settings_map->SetWebsiteSettingDefaultScope(
-        url, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
-        score_dict.release());
-  }
+  std::unique_ptr<ScopedEngagementScore> engagement_score =
+      GetEngagementScore(url, true);
+  engagement_score->get()->Reset(score, last_engagement_time);
+  if (last_shortcut_launch_time)
+    engagement_score->get()->set_last_shortcut_launch_time(
+        *last_shortcut_launch_time);
 }