Push API: Clarify needed_but_not_shown logic

chrome/browser/push_messaging/push_messaging_service_impl.cc

Index: chrome/browser/push_messaging/push_messaging_service_impl.cc
diff --git a/chrome/browser/push_messaging/push_messaging_service_impl.cc b/chrome/browser/push_messaging/push_messaging_service_impl.cc
index 2f205d3bdeed37547aae2dfb52a64f65ef8ca5c3..9953036baf09cee22c61f82415c82329241f8c4b 100644
--- a/chrome/browser/push_messaging/push_messaging_service_impl.cc
+++ b/chrome/browser/push_messaging/push_messaging_service_impl.cc
@@ -383,6 +383,7 @@ void PushMessagingServiceImpl::DidGetNotificationsShown(
   // needed but not shown. We manipulate it in bitset form.
   std::bitset<MISSED_NOTIFICATIONS_LENGTH> missed_notifications(data);
 
+  DCHECK(notification_shown || notification_needed);  // Caller must ensure this

[Peter Beverloo, 2015/04/26 19:42:22]

nit: I prefer to have DCHECKs validating the arguments as the very first thing
in a method, as they're essentially "entry conditions".

I don't think "Caller must ensure this" adds much value - there would not be a
DCHECK if this were not the case.

   bool needed_but_not_shown = notification_needed && !notification_shown;
 
   // New entries go at the end, and old ones are shifted off the beginning once
@@ -404,45 +405,44 @@ void PushMessagingServiceImpl::DidGetNotificationsShown(
     message_handled_closure.Run();
     return;
   }
-  if (needed_but_not_shown) {
-    if (missed_notifications.count() <= 1) {  // apply grace
-      RecordUserVisibleStatus(
-        content::PUSH_USER_VISIBLE_STATUS_REQUIRED_BUT_NOT_SHOWN_USED_GRACE);
-      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_GRACE_EXCEEDED);
-    rappor::SampleDomainAndRegistryFromGURL(
-        g_browser_process->rappor_service(),
-        "PushMessaging.GenericNotificationShown.Origin",
-        requesting_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.
-    // TODO(johnme): The generic notification should probably automatically
-    // close itself when the next push message arrives?
-    content::PlatformNotificationData notification_data;
-    // TODO(johnme): Switch to FormatOriginForDisplay from crbug.com/402698
-    notification_data.title = base::UTF8ToUTF16(requesting_origin.host());
-    notification_data.direction =
-        content::PlatformNotificationData::NotificationDirectionLeftToRight;
-    notification_data.body =
-        l10n_util::GetStringUTF16(IDS_PUSH_MESSAGING_GENERIC_NOTIFICATION_BODY);
-    notification_data.tag = kPushMessagingForcedNotificationTag;
-    notification_data.icon = GURL();  // TODO(johnme): Better icon?
-    notification_data.silent = true;
-    PlatformNotificationServiceImpl* notification_service =
-        PlatformNotificationServiceImpl::GetInstance();
-    notification_service->DisplayPersistentNotification(
-        profile_,
-        service_worker_registration_id,
-        requesting_origin,
-        SkBitmap() /* icon */,
-        notification_data);
+      content::PUSH_USER_VISIBLE_STATUS_REQUIRED_BUT_NOT_SHOWN_USED_GRACE);
     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",
+      requesting_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.
+  // TODO(johnme): The generic notification should probably automatically
+  // close itself when the next push message arrives?
+  content::PlatformNotificationData notification_data;
+  // TODO(johnme): Switch to FormatOriginForDisplay from crbug.com/402698
+  notification_data.title = base::UTF8ToUTF16(requesting_origin.host());
+  notification_data.direction =
+      content::PlatformNotificationData::NotificationDirectionLeftToRight;
+  notification_data.body =
+      l10n_util::GetStringUTF16(IDS_PUSH_MESSAGING_GENERIC_NOTIFICATION_BODY);
+  notification_data.tag = kPushMessagingForcedNotificationTag;
+  notification_data.icon = GURL();  // TODO(johnme): Better icon?
+  notification_data.silent = true;
+  PlatformNotificationServiceImpl* notification_service =
+      PlatformNotificationServiceImpl::GetInstance();
+  notification_service->DisplayPersistentNotification(
+      profile_,
+      service_worker_registration_id,
+      requesting_origin,
+      SkBitmap() /* icon */,
+      notification_data);
+  message_handled_closure.Run();
 }
 
 void PushMessagingServiceImpl::SetMessageCallbackForTesting(