Push API: Clarify needed_but_not_shown logic

chrome/browser/push_messaging/push_messaging_service_impl.cc

 // Copyright 2014 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_service_impl.h"
 
 #include <bitset>
 #include <vector>
 
 #include "base/barrier_closure.h"
@@ skipping 365 matching lines @@
   content::ServiceWorkerContext* service_worker_context =
       content::BrowserContext::GetStoragePartitionForSite(
           profile_, requesting_origin)->GetServiceWorkerContext();
 
   // 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);
 
+  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
   // 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>());
   SetNotificationsShownByLastFewPushes(
       service_worker_context, service_worker_registration_id,
       requesting_origin, updated_data,
       base::Bind(&IgnoreResult));  // This is a heuristic; ignore failure.
 
   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;
   }
-  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(
       const base::Closure& callback) {
   message_callback_for_testing_ = callback;
 }
 
 // Other gcm::GCMAppHandler methods -------------------------------------------
 
 void PushMessagingServiceImpl::OnMessagesDeleted(const std::string& app_id) {
@@ skipping 364 matching lines @@
       CONTENT_SETTING_ALLOW;
 }
 
 gcm::GCMDriver* PushMessagingServiceImpl::GetGCMDriver() const {
   gcm::GCMProfileService* gcm_profile_service =
       gcm::GCMProfileServiceFactory::GetForProfile(profile_);
   CHECK(gcm_profile_service);
   CHECK(gcm_profile_service->driver());
   return gcm_profile_service->driver();
 }