Replace the 1 in 10 grace period with an accumulating budget based on SES.

chrome/browser/push_messaging/background_budget_service.cc

 // Copyright 2016 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/push_messaging/background_budget_service.h"
 
+#include <stdint.h>
+
 #include "base/callback.h"
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "base/strings/stringprintf.h"
+#include "base/time/clock.h"
+#include "base/time/default_clock.h"
+#include "base/time/time.h"
+#include "chrome/browser/engagement/site_engagement_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
 
 namespace {
 
-std::string GetBudgetStringForOrigin(Profile* profile, const GURL& origin) {
-  DCHECK_EQ(origin.GetOrigin(), origin);
+const char kSeparator = '#';
+const double kSecondsToAccumulate = 864000.0;  // 10 days of seconds.
+
+bool GetBudgetDataFromPrefs(Profile* profile,
+                            const GURL& origin,
+                            double* old_budget,
+                            double* old_ses,
+                            double* last_updated) {

[Peter Beverloo, 2016/05/04 15:41:51]

Something to consider for the longer term: store the budget in a structure that
has a static FromString method, and a non-static ToString method. It would allow
you to separate out storage from this file, as this is largely plumbing while
there's some important logic in here.

[harkness, 2016/05/09 15:33:27]

Acknowledged.

   const base::DictionaryValue* map =
       profile->GetPrefs()->GetDictionary(prefs::kBackgroundBudgetMap);
 
   std::string map_string;
   map->GetStringWithoutPathExpansion(origin.spec(), &map_string);
-  return map_string;
+
+  std::vector<base::StringPiece> parts =
+      base::SplitStringPiece(map_string, std::string(1, kSeparator),

[Peter Beverloo, 2016/05/04 15:41:52]

nit: StringPieceSplit takes a StringPiece as the separator, so you can avoid
this allocation by using StringPiece(&kSeparator, 1) instead.

[harkness, 2016/05/09 15:33:27]

Done.

+                             base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+  if (parts.size() != 3)
+    return false;
+
+  if (!base::StringToDouble(parts[0].as_string(), last_updated))
+    return false;
+
+  if (!base::StringToDouble(parts[1].as_string(), old_budget))
+    return false;
+
+  if (!base::StringToDouble(parts[2].as_string(), old_ses))
+    return false;
+
+  return true;
+}
+
+// The value stored in prefs is a concatenated string of last updated time, old
+// budget, and old ses.
+void SetBudgetDataInPrefs(Profile* profile,
+                          const GURL& origin,
+                          double last_updated,
+                          double budget,
+                          double ses) {
+  std::string s = base::StringPrintf("%f%c%f%c%f", last_updated, kSeparator,
+                                     budget, kSeparator, ses);
+  DictionaryPrefUpdate update(profile->GetPrefs(), prefs::kBackgroundBudgetMap);
+  base::DictionaryValue* map = update.Get();
+
+  map->SetStringWithoutPathExpansion(origin.spec(), s);
 }
 
 }  // namespace
 
 BackgroundBudgetService::BackgroundBudgetService(Profile* profile)
-    : profile_(profile) {
+    : BackgroundBudgetService(profile,
+                              base::WrapUnique(new base::DefaultClock)) {}
+
+BackgroundBudgetService::BackgroundBudgetService(
+    Profile* profile,
+    std::unique_ptr<base::Clock> clock)
+    : clock_(std::move(clock)), profile_(profile) {
   DCHECK(profile);
 }
 
+BackgroundBudgetService::~BackgroundBudgetService() {}
+
 // static
 void BackgroundBudgetService::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterDictionaryPref(prefs::kBackgroundBudgetMap);
 }
 
-std::string BackgroundBudgetService::GetBudget(const GURL& origin) {
-  return GetBudgetStringForOrigin(profile_, origin);
+double BackgroundBudgetService::GetBudget(const GURL& origin) {

[Peter Beverloo, 2016/05/04 15:41:52]

DCHECK_EQ(origin, origin.GetOrigin());
(also in StoreBudget)

This will guard against people accidentally passing a random URL to these
methods.

[harkness, 2016/05/09 15:33:27]

Done.

+  // Get the current SES score, which we'll use to set a new budget.
+  SiteEngagementService* service = SiteEngagementService::Get(profile_);
+  double ses_score = service->GetScore(origin);
+
+  // Get the last used budget data. This is a triple of last calculated time,
+  // budget at that time, and Site Engagement Score (ses) at that time.
+  double old_budget = 0.0, old_ses = 0.0, last_updated_msec = 0.0;
+  if (!GetBudgetDataFromPrefs(profile_, origin, &old_budget, &old_ses,
+                              &last_updated_msec)) {

[Peter Beverloo, 2016/05/04 15:41:52]

Is it OK to silently ignore parsing errors? (I.e. the "return false" statements
on lines 44, 47, 50 and 53.) Your comment on line 104 doesn't mention that case.

[harkness, 2016/05/09 15:33:27]

I've updated the comment.

+    // If there is no stored data, just return data based on the SES.
+    return ses_score;
+  }
+
+  base::Time now = clock_->Now();
+  base::TimeDelta elapsed = now - base::Time::FromDoubleT(last_updated_msec);
+
+  // Budget recalculation consists of a budget carryover component, which
+  // rewards sites that don't use all their budgets every day, and a ses
+  // component, which gives extra budget to sites that have a high ses score.

[Peter Beverloo, 2016/05/04 15:41:51]

This would really benefit from an explanation of the algorithm in English :)

[harkness, 2016/05/09 15:33:27]

I've updated it, which involved moving a few things around. Let me know if you
think it reads better.

+  double carryover_ratio = std::max(
+      0.0,
+      ((kSecondsToAccumulate - elapsed.InSeconds()) / kSecondsToAccumulate));
+  double budget_carryover = old_budget * carryover_ratio;
+  double ses_ratio =
+      std::min(1.0, (elapsed.InSeconds() / kSecondsToAccumulate));
+  double ses_component = (old_ses + ses_score) / 2 * ses_ratio;
+  double budget = budget_carryover + ses_component;
+  DCHECK(budget >= 0.0);

[Peter Beverloo, 2016/05/04 15:41:52]

DCHECK_GE

[harkness, 2016/05/09 15:33:27]

Done.

+  return budget;
 }
 
-void BackgroundBudgetService::StoreBudget(
-    const GURL& origin,
-    const std::string& notifications_shown) {
-  DictionaryPrefUpdate update(profile_->GetPrefs(),
-                              prefs::kBackgroundBudgetMap);
-  base::DictionaryValue* map = update.Get();
-  map->SetStringWithoutPathExpansion(origin.spec(), notifications_shown);
+void BackgroundBudgetService::StoreBudget(const GURL& origin, double budget) {
+  DCHECK(budget >= 0.0);

[Peter Beverloo, 2016/05/04 15:41:51]

DCHECK_GE

[harkness, 2016/05/09 15:33:27]

Done.

+  DCHECK(budget <= SiteEngagementScore::kMaxPoints);

[Peter Beverloo, 2016/05/04 15:41:52]

DCHECK_LE

[harkness, 2016/05/09 15:33:27]

Done.

+
+  // Get the current SES score to write into the prefs with the new budget.
+  SiteEngagementService* service = SiteEngagementService::Get(profile_);
+  double ses_score = service->GetScore(origin);
+
+  base::Time t = clock_->Now();
+  SetBudgetDataInPrefs(profile_, origin, t.ToDoubleT(), budget, ses_score);
 }