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 <utility>
@@ skipping 33 matching lines @@
 
 std::unique_ptr<ContentSettingsForOneType> GetEngagementContentSettings(
     HostContentSettingsMap* settings_map) {
   std::unique_ptr<ContentSettingsForOneType> engagement_settings(
       new ContentSettingsForOneType);
   settings_map->GetSettingsForOneType(CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT,
                                       std::string(), engagement_settings.get());
   return engagement_settings;
 }
 
-std::unique_ptr<base::DictionaryValue> GetScoreDictForOrigin(
-    HostContentSettingsMap* settings,
-    const GURL& origin_url) {
-  if (!settings)
-    return std::unique_ptr<base::DictionaryValue>();
-
-  std::unique_ptr<base::Value> value = settings->GetWebsiteSetting(
-      origin_url, origin_url, CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT,
-      std::string(), NULL);
-  if (!value.get())
-    return base::WrapUnique(new base::DictionaryValue());
-
-  if (!value->IsType(base::Value::TYPE_DICTIONARY))
-    return base::WrapUnique(new base::DictionaryValue());
-
-  return base::WrapUnique(static_cast<base::DictionaryValue*>(value.release()));
-}
-
 // Only accept a navigation event for engagement if it is one of:
 //  a. direct typed navigation
 //  b. clicking on an omnibox suggestion brought up by typing a keyword
 //  c. clicking on a bookmark or opening a bookmark app
 //  d. a custom search engine keyword search (e.g. Wikipedia search box added as
 //  search engine).
 bool IsEngagementNavigation(ui::PageTransition transition) {
   return ui::PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_TYPED) ||
          ui::PageTransitionCoreTypeIs(transition,
                                       ui::PAGE_TRANSITION_GENERATED) ||
@@ skipping 84 matching lines @@
       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);
+    const SiteEngagementScore score(GetEngagementScore(origin));
     score_map[origin] = score.GetScore();

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

score_map[origin] = GetScore(origin);

Delete the line above.

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

Done.

   }
 
   return score_map;
 }
 
 bool SiteEngagementService::IsBootstrapped() {
   return GetTotalEngagementPoints() >=
          SiteEngagementScore::GetBootstrapPoints();
 }
 
@@ skipping 13 matching lines @@
     case ENGAGEMENT_LEVEL_HIGH:
       return score >= SiteEngagementScore::GetHighEngagementBoundary();
     case ENGAGEMENT_LEVEL_MAX:
       return score == SiteEngagementScore::kMaxPoints;
   }
   NOTREACHED();
   return false;
 }
 
 void SiteEngagementService::ResetScoreForURL(const GURL& url, double score) {
-  ResetScoreAndAccessTimesForURL(url, score, nullptr);
+  SiteEngagementScore engagement_score = GetEngagementScore(url);
+  engagement_score.Reset(score, clock_->Now());
+  engagement_score.Commit();
 }
 
 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);
+  SiteEngagementScore score = GetEngagementScore(url);
 
   // 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();
   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.Commit();
 }
 
 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.GetScore();
+  return GetEngagementScore(url).GetScore();
 }
 
 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;
 }
 
 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);
 }
 
 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);
+  SiteEngagementScore score = GetEngagementScore(url);
 
   score.AddPoints(points);
-  if (score.UpdateScoreDict(score_dict.get())) {
-    settings_map->SetWebsiteSettingDefaultScope(
-        url, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
-        score_dict.release());
-  }
+  score.Commit();
 }
 
 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);
+      const SiteEngagementScore score = GetEngagementScore(origin);
       if (score.GetScore() != 0)

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

if (GetScore(origin) != 0)

Delete the line above.

Actually, this could just use GetScoreMap(). I don't really mind either way.

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

Done.

         continue;
     }
 
     settings_map->SetWebsiteSettingDefaultScope(
         origin, GURL(), CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, std::string(),
         nullptr);
   }
 }
 
 void SiteEngagementService::RecordMetrics() {
@@ skipping 90 matching lines @@
 
   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));
 }
 
+SiteEngagementScore SiteEngagementService::GetEngagementScore(
+    const GURL& origin) const {
+  return SiteEngagementScore(
+      clock_.get(), origin,
+      HostContentSettingsMapFactory::GetForProfile(profile_));
+}
+
 int SiteEngagementService::OriginsWithMaxDailyEngagement() const {
   HostContentSettingsMap* settings_map =
       HostContentSettingsMapFactory::GetForProfile(profile_);
   std::unique_ptr<ContentSettingsForOneType> engagement_settings =
       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);
+    const SiteEngagementScore score = GetEngagementScore(origin);
     if (score.MaxPointsPerDayAdded())

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

if (GetEngagementScore(origin).MaxPointsPerDayAdded())

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

Done.

       ++total_origins;
   }
 
   return total_origins;
 }
 
 int SiteEngagementService::OriginsWithMaxEngagement(
     const std::map<GURL, double>& score_map) const {
   int total_origins = 0;
 
@@ skipping 12 matching lines @@
 
   // The most in-the-past option in the Clear Browsing Dialog aside from "all
   // time" is 4 weeks ago. Set the last updated date to 4 weeks ago for origins
   // where we can't find a valid last visit date.
   base::Time now = clock_->Now();
   base::Time four_weeks_ago =
       now - base::TimeDelta::FromDays(FOUR_WEEKS_IN_DAYS);
 
   for (const auto& origin_to_count : remaining_origins) {
     GURL origin = origin_to_count.first;
+    // It appears that the history service occasionally sends bad URLs to us.
+    // See crbug.com/612881.
+    if (!origin.is_valid())
+      return;

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

continue?

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

Done.

+
     int remaining = origin_to_count.second.first;
     base::Time last_visit = origin_to_count.second.second;
     int deleted = deleted_origins.count(origin);
 
     // Do not update engagement scores if the deletion was an expiry, but the
     // URL still has entries in history.
     if ((expired && remaining != 0) || deleted == 0)
       continue;
 
     // Remove engagement proportional to the urls expired from the origin's
@@ skipping 14 matching lines @@
     // reduced value rather than being decayed once here, and then once again
     // when it is next accessed.
     int undecay = 0;
     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);
+    SiteEngagementScore engagement_score = GetEngagementScore(origin);
+
+    double score = std::min(
+        SiteEngagementScore::kMaxPoints,
+        (proportion_remaining * engagement_score.GetScore()) + undecay);
+    engagement_score.Reset(score, last_visit);
+    if (!engagement_score.last_shortcut_launch_time().is_null())
+      engagement_score.set_last_shortcut_launch_time(last_visit);

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

if (!engagement_score.last_shortcut_launch_time().is_null() &&
engagement_score.last_shortcut_launch_time() > last_visit)

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

Done.

+
+    engagement_score.Commit();
   }
 }
-
-void SiteEngagementService::ResetScoreAndAccessTimesForURL(
-    const GURL& url, double score, const base::Time* updated_time) {
-  // It appears that the history service occassionally sends bad URLs to us.
-  // See crbug.com/612881.
-  if (!url.is_valid())
-    return;
-
-  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());
-  }
-}