Refactor budget computation to be more tuneable.

chrome/browser/budget_service/budget_database.cc

Index: chrome/browser/budget_service/budget_database.cc
diff --git a/chrome/browser/budget_service/budget_database.cc b/chrome/browser/budget_service/budget_database.cc
index 28e3f7ebfdf055cf9e2a31e964860a2ce558f267..86824122b5e42c4c42ce2272274edbf578d69fcd 100644
--- a/chrome/browser/budget_service/budget_database.cc
+++ b/chrome/browser/budget_service/budget_database.cc
@@ -27,8 +27,11 @@ namespace {
 const char kDatabaseUMAName[] = "BudgetManager";
 
 // The default amount of time during which a budget will be valid.
-// This is 4 days = 96 hours.
-constexpr double kBudgetDurationInHours = 96;
+constexpr int kBudgetDurationInDays = 4;
+
+// The amount of budget that a maximally engaged site should receive per hour.
+// This allows 6 silent push messages a day for a maximal site.

[Peter Beverloo, 2017/01/12 14:40:04]

Can we make this comment slightly clearer, since it assumes knowledge that the
cost of a silent push is 2.0, which is defined in another file?

// The amount of budget that a maximally engaged site should receive per hour.
// This value allows for up to 6 silent push messages, which have a cost of 2,
// per day for a fully engaged site.

(Even better if these values could be defined in the same file.)

[harkness, 2017/01/12 17:44:14]

I updated the comment here. I'd like to keep the cost values in BudgetManager,
since the database shouldn't have to know how much various actions cost.

+constexpr double kMaximumHourlyBudget = 12.0 / 24.0;
 
 }  // namespace
 
@@ -164,8 +167,6 @@ void BudgetDatabase::GetBudgetAfterSync(const url::Origin& origin,
     predictions.push_back(std::move(prediction));
   }
 
-  DCHECK_EQ(0, total);
-
   callback.Run(blink::mojom::BudgetServiceErrorType::NONE,
                std::move(predictions));
 }
@@ -304,35 +305,34 @@ void BudgetDatabase::SyncLoadedCache(const url::Origin& origin,
 }
 
 void BudgetDatabase::AddEngagementBudget(const url::Origin& origin) {
-  // Get the current SES score, which we'll use to set a new budget.
-  SiteEngagementService* service = SiteEngagementService::Get(profile_);
-  double score = service->GetScore(origin.GetURL());
-
-  // By default we award the "full" award. Then that ratio is decreased if
-  // there have been other awards recently.
-  double ratio = 1.0;
-
-  // Calculate how much budget should be awarded. If there is no entry in the
-  // cache then we award a full amount.
+  // Calculate how much budget should be awarded. The award depends on the
+  // time elapsed since the last award and the SES score.
+  base::TimeDelta elapsed = base::TimeDelta::FromDays(kBudgetDurationInDays);

[Peter Beverloo, 2017/01/12 14:40:04]

nit: I think the "By default..." comment would still be very useful to explain
why we use kBudgetDurationInDays for uncached origins.

[harkness, 2017/01/12 17:44:14]

Done.

   if (IsCached(origin)) {
-    base::TimeDelta elapsed =
-        clock_->Now() - budget_map_[origin].last_engagement_award;
-    int elapsed_hours = elapsed.InHours();
+    elapsed = clock_->Now() - budget_map_[origin].last_engagement_award;
     // Don't give engagement awards for periods less than an hour.
-    if (elapsed_hours < 1)
+    if (elapsed.InHours() < 1)
       return;
-    if (elapsed_hours < kBudgetDurationInHours)
-      ratio = elapsed_hours / kBudgetDurationInHours;
+    // Cap elapsed time to the budget duration.
+    if (elapsed.InDays() > kBudgetDurationInDays)
+      elapsed = base::TimeDelta::FromDays(kBudgetDurationInDays);
   }
 
+  // Get the current SES score, and calculate the hourly budget for that score.
+  SiteEngagementService* service = SiteEngagementService::Get(profile_);
+  double hourly_budget = kMaximumHourlyBudget *
+                         service->GetScore(origin.GetURL()) /
+                         service->GetMaxPoints();
+
   // Update the last_engagement_award to the current time. If the origin wasn't
   // already in the map, this adds a new entry for it.
   budget_map_[origin].last_engagement_award = clock_->Now();
 
   // Add a new chunk of budget for the origin at the default expiration time.
   base::Time expiration =
-      clock_->Now() + base::TimeDelta::FromHours(kBudgetDurationInHours);
-  budget_map_[origin].chunks.emplace_back(ratio * score, expiration);
+      clock_->Now() + base::TimeDelta::FromDays(kBudgetDurationInDays);
+  budget_map_[origin].chunks.emplace_back(elapsed.InHours() * hourly_budget,
+                                          expiration);
 
   // Any time we award engagement budget, which is done at most once an hour
   // whenever any budget action is taken, record the budget.
@@ -355,10 +355,10 @@ bool BudgetDatabase::CleanupExpiredBudget(const url::Origin& origin) {
     cleanup_iter = chunks.erase(cleanup_iter);
 
   // If the entire budget is empty now AND there have been no engagements
-  // in the last kBudgetDurationInHours hours, remove this from the cache.
+  // in the last kBudgetDurationInDays days, remove this from the cache.
   if (chunks.empty() &&
       budget_map_[origin].last_engagement_award <
-          clock_->Now() - base::TimeDelta::FromHours(kBudgetDurationInHours)) {
+          clock_->Now() - base::TimeDelta::FromDays(kBudgetDurationInDays)) {
     budget_map_.erase(origin);
     return true;
   }