Small cleanup of SiteEngagementService.

chrome/browser/engagement/site_engagement_service.cc

 // 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.
 
 #include "chrome/browser/engagement/site_engagement_service.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <cmath>
@@ skipping 253 matching lines @@
   double to_add = std::min(kMaxPoints - raw_score_,
                            GetMaxPointsPerDay() - points_added_today_);
   to_add = std::min(to_add, points);
 
   points_added_today_ += to_add;
   raw_score_ += to_add;
 
   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 {
   if (!last_engagement_time_.is_null() &&
       clock_->Now().LocalMidnight() != last_engagement_time_.LocalMidnight()) {
     return false;
   }
 
   return points_added_today_ == GetMaxPointsPerDay();
 }
@@ skipping 152 matching lines @@
                                                bool is_hidden) {
   SiteEngagementMetrics::RecordEngagement(
       is_hidden ? SiteEngagementMetrics::ENGAGEMENT_MEDIA_HIDDEN
                 : SiteEngagementMetrics::ENGAGEMENT_MEDIA_VISIBLE);
   AddPoints(url, is_hidden ? SiteEngagementScore::GetHiddenMediaPoints()
                            : SiteEngagementScore::GetVisibleMediaPoints());
   RecordMetrics();
 }
 
 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(
     history::HistoryService* history_service,
     bool all_history,
     bool expired,
     const history::URLRows& deleted_rows,
     const std::set<GURL>& favicon_urls) {
   std::multiset<GURL> origins;
   for (const history::URLRow& row : deleted_rows)
     origins.insert(row.url().GetOrigin());
 
   history::HistoryService* hs = HistoryServiceFactory::GetForProfile(
       profile_, ServiceAccessType::EXPLICIT_ACCESS);
   hs->GetCountsAndLastVisitForOrigins(
       std::set<GURL>(origins.begin(), origins.end()),
       base::Bind(
           &SiteEngagementService::GetCountsAndLastVisitForOriginsComplete,
           weak_factory_.GetWeakPtr(), hs, origins, expired));
 }
 
 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()));
   }
   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 {
   std::map<GURL, double> score_map = GetScoreMap();
 
   double total_score = 0;
   for (const auto& value : score_map)
     total_score += value.second;
 
   return total_score;
 }
 
 std::map<GURL, double> SiteEngagementService::GetScoreMap() const {
   HostContentSettingsMap* settings_map =
       HostContentSettingsMapFactory::GetForProfile(profile_);
   std::unique_ptr<ContentSettingsForOneType> engagement_settings =
       GetEngagementContentSettings(settings_map);
 
   std::map<GURL, double> score_map;
   for (const auto& site : *engagement_settings) {
     GURL origin(site.primary_pattern.ToString());
     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;
 }
 
 bool SiteEngagementService::IsBootstrapped() {
   return GetTotalEngagementPoints() >=
          SiteEngagementScore::GetBootstrapPoints();
 }
 
@@ skipping 32 matching lines @@
       return score >= SiteEngagementScore::GetMediumEngagementBoundary();
     case ENGAGEMENT_LEVEL_HIGH:
       return score >= SiteEngagementScore::GetHighEngagementBoundary();
     case ENGAGEMENT_LEVEL_MAX:
       return score == SiteEngagementScore::kMaxPoints;
   }
   NOTREACHED();
   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) {
   // May be null in tests.
   history::HistoryService* history = HistoryServiceFactory::GetForProfile(
       profile, ServiceAccessType::IMPLICIT_ACCESS);
   if (history)
     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() {
   CleanupEngagementScores();
   RecordMetrics();
 }
 
 void SiteEngagementService::CleanupEngagementScores() {
   HostContentSettingsMap* settings_map =
     HostContentSettingsMapFactory::GetForProfile(profile_);
   std::unique_ptr<ContentSettingsForOneType> engagement_settings =
       GetEngagementContentSettings(settings_map);
 
   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);
   }
 }
 
 void SiteEngagementService::RecordMetrics() {
   base::Time now = clock_->Now();
   if (last_metrics_time_.is_null() ||
       (now - last_metrics_time_).InMinutes() >= kMetricsIntervalInMinutes) {
     last_metrics_time_ = now;
     std::map<GURL, double> score_map = GetScoreMap();
 
@@ skipping 51 matching lines @@
       GetEngagementContentSettings(settings_map);
 
   int total_origins = 0;
 
   // We cannot call GetScoreMap as we need the score objects, not raw scores.
   for (const auto& site : *engagement_settings) {
     GURL origin(site.primary_pattern.ToString());
     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;
   }
 
   return total_origins;
 }
 
 int SiteEngagementService::OriginsWithMaxEngagement(
     const std::map<GURL, double>& score_map) const {
   int total_origins = 0;
 
@@ skipping 49 matching lines @@
     int days_since_engagement = (now - last_visit).InDays();
     if (days_since_engagement > 0) {
       int periods = days_since_engagement /
                     SiteEngagementScore::GetDecayPeriodInDays();
       undecay = periods * SiteEngagementScore::GetDecayPoints();
     }
 
     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);
 }