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

chrome/browser/push_messaging/background_budget_service.h

 // 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.
 
 #ifndef CHROME_BROWSER_PUSH_MESSAGING_BACKGROUND_BUDGET_SERVICE_H_
 #define CHROME_BROWSER_PUSH_MESSAGING_BACKGROUND_BUDGET_SERVICE_H_
 
 #include <string>

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

IWYU: #include <memory>

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

Done.

 
+#include "base/gtest_prod_util.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "url/gurl.h"
 
 class Profile;
 
+namespace base {
+class Clock;
+}
+
 namespace user_prefs {
 class PrefRegistrySyncable;
 }
 
 // A budget service to help Chrome decide how much background work a service
-// worker should be able to do on behalf of the user. The budget currently
-// implements a grace period of 1 non-visual notification in 10.
+// worker should be able to do on behalf of the user. The budget is calculated
+// based on the Site Engagment Score and is consumed when a service worker
+// doesn't show a notification when it needed to.

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

I've noticed that you're very diligent in keeping comments up-to-date, but in
the longer term it may not be sustainable to keep mentioning the consumers.

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

I'll generify the comment.

 class BackgroundBudgetService : public KeyedService {
  public:
   explicit BackgroundBudgetService(Profile* profile);
-  ~BackgroundBudgetService() override {}
+  ~BackgroundBudgetService() override;
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
-  // Get the budget associated with the origin. This currently tracks a string
-  // which holds 0 and 1 for the last 10 push messages and whether they
-  // triggered a visual notification.
-  std::string GetBudget(const GURL& origin);
+  // Get the budget associated with the origin. This is returned as the double
+  // budget. Budget will be a value between 0.0 and
+  // SiteEngagementScore::kMaxPoints.
+  double GetBudget(const GURL& origin);
 
-  // Store the budget associated with the origin. notifications_shown is
-  // expected to be a string encoding whether the last 10 push messages
-  // triggered
-  // a visual notification.
-  void StoreBudget(const GURL& origin, const std::string& notifications_shown);
+  // Store the budget associated with the origin. Budget should be a value
+  // between 0.0 and SiteEngagementScore::kMaxPoints.
+  void StoreBudget(const GURL& origin, double budget);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(BackgroundBudgetServiceTest, GetBudgetNoElapsedTime);
+  FRIEND_TEST_ALL_PREFIXES(BackgroundBudgetServiceTest, GetBudgetElapsedTime);
+  FRIEND_TEST_ALL_PREFIXES(BackgroundBudgetServiceTest,
+                           GetBudgetConsumedOverTime);
+
+  // Only called directly by tests.
+  BackgroundBudgetService(Profile* profile, std::unique_ptr<base::Clock> clock);

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

You could move some of the complexity from the BackgroundBudgetService to tests
by doing the following:

  - Only have one BBS constructor: (Profile*, std::unique_ptr<base::Clock>)
  - BackgroundBudgetServiceFactory::BuildServiceInstanceFor() to create an
instance with base::DefaultClock.
  - BackgroundBudgetServiceTest to override the BBSF's factory
(BBSF::SetTestingFactory()) to create an instance with a custom clock, to which
the test keeps holding a weak reference.

This has the following benefits:
  (1) We can remove the second constructor.
  (2) You don't need the FRIEND_TEST_ALL_PREFIXES declarations above.
  (3) The BackgroundBudgetServiceTest can just *always* use a custom clock,
avoiding the need for second instances in the new tests that you're introducing.

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

As discussed in person, I've combined the two constructors, but instead of using
the SetTestingFactory, I've created a private method on BackgroundBudgetService
that allows the test to override the default assigned clock.

+
+  // The clock used to vend times.
+  std::unique_ptr<base::Clock> clock_;
+
   Profile* profile_;
   DISALLOW_COPY_AND_ASSIGN(BackgroundBudgetService);
 };
 
 #endif  // CHROME_BROWSER_PUSH_MESSAGING_BACKGROUND_BUDGET_SERVICE_H_