Push API: test that default notification is shown on Android when needed.

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 c26b3b516a26c702388a1f419c5111059c2671d2..387423d3951b66cec01a775ead6a95c6e05d1962 100644
--- a/chrome/browser/push_messaging/push_messaging_service_impl.cc
+++ b/chrome/browser/push_messaging/push_messaging_service_impl.cc
@@ -21,6 +21,7 @@
 #include "chrome/browser/push_messaging/push_messaging_app_identifier.h"
 #include "chrome/browser/push_messaging/push_messaging_constants.h"
 #include "chrome/browser/push_messaging/push_messaging_service_factory.h"
+#include "chrome/browser/push_messaging/push_messaging_service_observer.h"
 #include "chrome/browser/services/gcm/gcm_profile_service_factory.h"
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/common/chrome_switches.h"
@@ -121,6 +122,7 @@ PushMessagingServiceImpl::PushMessagingServiceImpl(Profile* profile)
 #if defined(ENABLE_NOTIFICATIONS)
       notification_manager_(profile),
 #endif
+      push_messaging_service_observer_(PushMessagingServiceObserver::Create()),
       weak_factory_(this) {
   DCHECK(profile);
   HostContentSettingsMapFactory::GetForProfile(profile_)->AddObserver(this);
@@ -242,13 +244,11 @@ void PushMessagingServiceImpl::DeliverMessageCallback(
     const gcm::IncomingMessage& message,
     const base::Closure& message_handled_closure,
     content::PushDeliveryStatus status) {
-  // Remove a single in-flight delivery for |app_id|. This has to be done using
-  // an iterator rather than by value, as the latter removes all entries.
-  DCHECK(in_flight_message_deliveries_.find(app_id) !=
-         in_flight_message_deliveries_.end());
+  DCHECK_GE(in_flight_message_deliveries_.count(app_id), 1u);
 
-  in_flight_message_deliveries_.erase(
-      in_flight_message_deliveries_.find(app_id));
+  base::Closure completion_closure =
+      base::Bind(&PushMessagingServiceImpl::DidHandleMessage,
+                 weak_factory_.GetWeakPtr(), app_id, message_handled_closure);

[Bernhard Bauer, 2015/12/11 12:28:41]

You may want to use ScopedClosureRunner (from base/callback_helpers.h) here.

[Michael van Ouwerkerk, 2015/12/11 16:05:55]

Wow, that's cool. Maybe a little too cool for me. That opens the door much wider
for code that does all kinds of interesting stuff when a function returns, in a
non-obvious way. I'd suggest that when someone feels the need to use a
ScopedClosureRunner, there might be a bigger need for cleaning up the function
that would potentially use it.

Reading through some examples in our current codebase that use
ScopedClosureRunner, I found stuff like a param of type
"scoped_ptr<base::ScopedClosureRunner>*" which is not encouraging either.

[Bernhard Bauer, 2015/12/11 16:35:57]

I'm not quite sure I get that argument -- wouldn't that imply that this function
requires cleanup? If so, go right ahead 😃

I also don't think there's that much non-obvious about it; this is not much
different from other objects that do things in their constructor, like freeing a
pointer or closing a file handle. In fact, I would argue this makes the intent
more obvious, because it's clear that the callback should run in all code paths
-- similar to how a scoped_ptr signifies ownership on a conceptual level. In
contrast, if you have Run() calls strewn all over the function, that is much
less clear, *and* there is the chance that you might miss a code path.
Again, I don't get this argument -- other people are using this in more
complicated ways, so we shouldn't use it here?

[Michael van Ouwerkerk, 2015/12/11 17:57:02]

I'm not a big fan of this one, but you seem to feel strongly about it. It's
really scope creep for the CL though as it is about refactoring existing code,
so as discussed I've added a TODO for this.

 
   // TODO(mvanouwerkerk): Show a warning in the developer console of the
   // Service Worker corresponding to app_id (and/or on an internals page).
@@ -261,35 +261,51 @@ void PushMessagingServiceImpl::DeliverMessageCallback(
     case content::PUSH_DELIVERY_STATUS_SUCCESS:
     case content::PUSH_DELIVERY_STATUS_EVENT_WAITUNTIL_REJECTED:
 #if defined(ENABLE_NOTIFICATIONS)
-      // Only enforce the user visible requirements after the entire queue of
-      // incoming messages for |app_id| has been flushed.
-      if (!in_flight_message_deliveries_.count(app_id)) {
+      // Only enforce the user visible requirements if this is currently running
+      // as the delivery callback for the last in-flight message.
+      if (in_flight_message_deliveries_.count(app_id) == 1) {
         notification_manager_.EnforceUserVisibleOnlyRequirements(
             requesting_origin, service_worker_registration_id,
-            message_handled_closure);
+            completion_closure);
       } else {
-        message_handled_closure.Run();
+        completion_closure.Run();
       }
 #else
-      message_handled_closure.Run();
+      completion_closure.Run();
 #endif
       break;
     case content::PUSH_DELIVERY_STATUS_INVALID_MESSAGE:
     case content::PUSH_DELIVERY_STATUS_SERVICE_WORKER_ERROR:
-      message_handled_closure.Run();
+      completion_closure.Run();
       break;
     case content::PUSH_DELIVERY_STATUS_UNKNOWN_APP_ID:
     case content::PUSH_DELIVERY_STATUS_PERMISSION_DENIED:
     case content::PUSH_DELIVERY_STATUS_NO_SERVICE_WORKER:
-      Unsubscribe(
-          app_id, message.sender_id,
-          base::Bind(&UnregisterCallbackToClosure, message_handled_closure));
+      Unsubscribe(app_id, message.sender_id,
+                  base::Bind(&UnregisterCallbackToClosure, completion_closure));
       break;
   }
 
   RecordDeliveryStatus(status);
 }
 
+void PushMessagingServiceImpl::DidHandleMessage(
+    const std::string& app_id,
+    const base::Closure& message_handled_closure) {
+  DCHECK(in_flight_message_deliveries_.find(app_id) !=

[Bernhard Bauer, 2015/12/11 12:28:41]

Nit: You could extract the iterator to a local variable so you can reuse it
below.

[Michael van Ouwerkerk, 2015/12/11 16:05:56]

Done.

+         in_flight_message_deliveries_.end());
+
+  // Remove a single in-flight delivery for |app_id|. This has to be done using
+  // an iterator rather than by value, as the latter removes all entries.
+  in_flight_message_deliveries_.erase(
+      in_flight_message_deliveries_.find(app_id));
+
+  message_handled_closure.Run();
+
+  if (push_messaging_service_observer_)
+    push_messaging_service_observer_->OnMessageHandled();

[johnme, 2015/12/11 14:42:56]

Consider setting the message_handled_closure in OnMessage instead of adding this
call here, to avoid some duplication.

[Michael van Ouwerkerk, 2015/12/11 16:05:56]

Acknowledged.

+}
+
 void PushMessagingServiceImpl::SetMessageCallbackForTesting(
     const base::Closure& callback) {
   message_callback_for_testing_ = callback;