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

chrome/browser/push_messaging/push_messaging_notification_manager.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/push_messaging/push_messaging_notification_manager.h"
 
 #include <stddef.h>
 
 #include <bitset>
 
@@ skipping 168 matching lines @@
       platform_notification_service->ClosePersistentNotification(
           profile_, notification_database_data.notification_id);
       platform_notification_service->OnPersistentNotificationClose(
           profile_, notification_database_data.notification_id,
           notification_database_data.origin, false /* by_user */);
 
       break;
     }
   }
 
-  // Don't track push messages that didn't show a notification but were exempt
-  // from needing to do so.
-  if (notification_shown || notification_needed) {
+  if (notification_needed && !notification_shown) {
+    // Only track budget for messages that needed to show a notification but
+    // did not.
     BackgroundBudgetService* service =
         BackgroundBudgetServiceFactory::GetForProfile(profile_);
-    std::string notification_history = service->GetBudget(origin);
-    DidGetBudget(origin, service_worker_registration_id, notification_shown,
-                 notification_needed, message_handled_closure,
-                 notification_history);
+    double budget = service->GetBudget(origin);
+    DidGetBudget(origin, service_worker_registration_id,
+                 message_handled_closure, budget);
+    return;
+  }
+
+  if (notification_needed && notification_shown) {
+    RecordUserVisibleStatus(
+        content::PUSH_USER_VISIBLE_STATUS_REQUIRED_AND_SHOWN);
+  } else if (!notification_needed && !notification_shown) {
+    RecordUserVisibleStatus(
+        content::PUSH_USER_VISIBLE_STATUS_NOT_REQUIRED_AND_NOT_SHOWN);
   } else {
     RecordUserVisibleStatus(
-        content::PUSH_USER_VISIBLE_STATUS_NOT_REQUIRED_AND_NOT_SHOWN);
-    message_handled_closure.Run();
+        content::PUSH_USER_VISIBLE_STATUS_NOT_REQUIRED_BUT_SHOWN);
   }
+
+  message_handled_closure.Run();
 }
 
 bool PushMessagingNotificationManager::IsTabVisible(
     Profile* profile,
     WebContents* active_web_contents,
     const GURL& origin) {
   if (!active_web_contents || !active_web_contents->GetMainFrame())
     return false;
 
   // Don't leak information from other profiles.
@@ skipping 18 matching lines @@
   // prefix has to be removed before the origins can be compared.
   if (visible_url.SchemeIs(content::kViewSourceScheme))
     visible_url = GURL(visible_url.GetContent());
 
   return visible_url.GetOrigin() == origin;
 }
 
 void PushMessagingNotificationManager::DidGetBudget(
     const GURL& origin,
     int64_t service_worker_registration_id,
-    bool notification_shown,
-    bool notification_needed,
     const base::Closure& message_handled_closure,
-    const std::string& data) {
+    const double budget) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  // We remember whether the last (up to) 10 pushes showed notifications.
-  const size_t MISSED_NOTIFICATIONS_LENGTH = 10;
-  // data is a string like "0001000", where '0' means shown, and '1' means
-  // needed but not shown. We manipulate it in bitset form.
-  std::bitset<MISSED_NOTIFICATIONS_LENGTH> missed_notifications(data);
+  // If the service needed to show a notification but did not, update the
+  // budget. Currently this just subtracts 1.0, but we may expand the cost to
+  // include things like whether wifi is available in the mobile case.
+  double cost = 1.0;

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

nit: this should be a constant at the top of this file

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

Done.

+  if (budget >= cost) {
+    // Update the stored budget.
+    BackgroundBudgetService* service =
+        BackgroundBudgetServiceFactory::GetForProfile(profile_);
+    service->StoreBudget(origin, budget - cost);
 
-  DCHECK(notification_shown || notification_needed);  // Caller must ensure this
-  bool needed_but_not_shown = notification_needed && !notification_shown;
-
-  // New entries go at the end, and old ones are shifted off the beginning once
-  // the history length is exceeded.
-  missed_notifications <<= 1;
-  missed_notifications[0] = needed_but_not_shown;
-  std::string updated_data(missed_notifications.
-      to_string<char, std::string::traits_type, std::string::allocator_type>());
-  BackgroundBudgetService* service =
-      BackgroundBudgetServiceFactory::GetForProfile(profile_);
-  service->StoreBudget(origin, updated_data);
-
-  if (notification_shown) {
-    RecordUserVisibleStatus(
-        notification_needed
-            ? content::PUSH_USER_VISIBLE_STATUS_REQUIRED_AND_SHOWN
-            : content::PUSH_USER_VISIBLE_STATUS_NOT_REQUIRED_BUT_SHOWN);
-    message_handled_closure.Run();
-    return;
-  }
-  DCHECK(needed_but_not_shown);
-  if (missed_notifications.count() <= 1) {  // Apply grace.
     RecordUserVisibleStatus(
         content::PUSH_USER_VISIBLE_STATUS_REQUIRED_BUT_NOT_SHOWN_USED_GRACE);

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

I think we should introduce a new enum value for budget, so that we can
distinguish patterns more easily from grace uses.

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

Do you mind if I do that in my next CL, when I do all my uma changes, since I'll
be touching all those files for that already?

[Peter Beverloo, 2016/05/09 17:12:06]

OK.

     message_handled_closure.Run();
     return;
   }
+
   RecordUserVisibleStatus(
       content::PUSH_USER_VISIBLE_STATUS_REQUIRED_BUT_NOT_SHOWN_GRACE_EXCEEDED);
   rappor::SampleDomainAndRegistryFromGURL(
       g_browser_process->rappor_service(),
       "PushMessaging.GenericNotificationShown.Origin", origin);
 
   // The site failed to show a notification when one was needed, and they have
   // already failed once in the previous 10 push messages, so we will show a
   // generic notification. See https://crbug.com/437277.
   NotificationDatabaseData database_data =
@@ skipping 39 matching lines @@
     message_handled_closure.Run();
     return;
   }
 
   PlatformNotificationServiceImpl::GetInstance()->DisplayPersistentNotification(
       profile_, persistent_notification_id, origin, notification_data,
       NotificationResources());
 
   message_handled_closure.Run();
 }