Push API: Require user visible notification, else show auto notification

chrome/browser/services/gcm/push_messaging_service_impl.cc

Index: chrome/browser/services/gcm/push_messaging_service_impl.cc
diff --git a/chrome/browser/services/gcm/push_messaging_service_impl.cc b/chrome/browser/services/gcm/push_messaging_service_impl.cc
index 1444176cfacc5b1fa1a2aabe05fb526dd3e1e1f0..5d95fcc8aa074b660784eaeb6b9b59a117ba7ed3 100644
--- a/chrome/browser/services/gcm/push_messaging_service_impl.cc
+++ b/chrome/browser/services/gcm/push_messaging_service_impl.cc
@@ -10,6 +10,10 @@
 #include "base/command_line.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/notifications/notification_ui_manager.h"
+#include "chrome/browser/notifications/platform_notification_service_impl.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/services/gcm/gcm_profile_service.h"
 #include "chrome/browser/services/gcm/gcm_profile_service_factory.h"
@@ -19,12 +23,17 @@
 #include "chrome/browser/services/gcm/push_messaging_permission_context_factory.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
+#include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/content_settings/core/common/content_settings_types.h"
 #include "components/content_settings/core/common/permission_request_id.h"
 #include "components/gcm_driver/gcm_driver.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/child_process_host.h"
+#include "content/public/common/platform_notification_data.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 
 namespace gcm {
 
@@ -45,6 +54,12 @@ blink::WebPushPermissionStatus ToPushPermission(ContentSetting setting) {
   }
 }
 
+int CountNotifications(Profile* profile, const GURL& origin) {

[Peter Beverloo, 2015/01/09 17:58:21]

CountNotificationsForOrigin.

[johnme, 2015/01/09 18:28:47]

Deleted.

+  return g_browser_process->notification_ui_manager()
+                          ->GetAllIdsByProfileAndSourceOrigin(profile, origin)
+                          .size();
+}
+
 }  // namespace
 
 // static
@@ -152,7 +167,6 @@ void PushMessagingServiceImpl::OnMessage(
   // TODO(johnme): Make sure this is clearly documented for developers.
   PushMessagingApplicationId application_id =
       PushMessagingApplicationId::Parse(app_id);
-  DCHECK(application_id.IsValid());

[Peter Beverloo, 2015/01/09 17:58:20]

Why are you removing this DCHECK? You're using the value all throughout.

[johnme, 2015/01/09 18:28:47]

After discussing with Michael, we concluded that malformed input shouldn't crash
the browser even in debug. The existing if condition two lines below covers this
case correctly already.

It's somewhat orthogonal; happy to split it into its own change if you'd rather.

[Peter Beverloo, 2015/01/09 18:46:25]

No, that's OK. Thanks for the info!

   GCMClient::MessageData::const_iterator it = message.data.find("data");
   if (application_id.IsValid() && it != message.data.end()) {
     if (!HasPermission(application_id.origin)) {
@@ -171,10 +185,12 @@ void PushMessagingServiceImpl::OnMessage(
         base::Bind(&PushMessagingServiceImpl::DeliverMessageCallback,
                    weak_factory_.GetWeakPtr(),
                    application_id,
-                   message));
+                   message,
+                   CountNotifications(profile_, application_id.origin)));
   } else {
     // Drop the message, as it is invalid.
     DeliverMessageCallback(application_id, message,
+                           0 /* notification_count_before_event */,
                            content::PUSH_DELIVERY_STATUS_INVALID_MESSAGE);
   }
 }
@@ -186,16 +202,22 @@ void PushMessagingServiceImpl::SetProfileForTesting(Profile* profile) {
 void PushMessagingServiceImpl::DeliverMessageCallback(
     const PushMessagingApplicationId& application_id,
     const GCMClient::IncomingMessage& message,
+    int notification_count_before_event,
     content::PushDeliveryStatus status) {
   // TODO(mvanouwerkerk): UMA logging.
   // TODO(mvanouwerkerk): Show a warning in the developer console of the
   // Service Worker corresponding to app_id (and/or on an internals page).
   // TODO(mvanouwerkerk): Is there a way to recover from failure?
   switch (status) {
+    // Call RequireUserVisibleUX if the message was delivered to the Service
+    // Worker JS, even if the website's event handler failed (to prevent sites
+    // deliberately failing to avoid having to show notifications).
     case content::PUSH_DELIVERY_STATUS_SUCCESS:
+    case content::PUSH_DELIVERY_STATUS_EVENT_WAITUNTIL_REJECTED:
+      RequireUserVisibleUX(application_id, notification_count_before_event);
+      break;
     case content::PUSH_DELIVERY_STATUS_INVALID_MESSAGE:
     case content::PUSH_DELIVERY_STATUS_SERVICE_WORKER_ERROR:
-    case content::PUSH_DELIVERY_STATUS_EVENT_WAITUNTIL_REJECTED:
       break;
     case content::PUSH_DELIVERY_STATUS_NO_SERVICE_WORKER:
       Unregister(application_id, UnregisterCallback());
@@ -203,6 +225,41 @@ void PushMessagingServiceImpl::DeliverMessageCallback(
   }
 }
 
+void PushMessagingServiceImpl::RequireUserVisibleUX(
+    const PushMessagingApplicationId& application_id,
+    int notification_count_before_event) {
+#if defined(ENABLE_NOTIFICATIONS)
+  // Require the event handler to have shown a new notification, or left a
+  // pre-existing one showing (possibly updated), or closed a pre-existing one.
+  // Showing then immediately closing a notification is not sufficient.

[Peter Beverloo, 2015/01/09 17:58:20]

This is a comment that should live in the header file. All these conditions
don't really matter either, what you care about is that, at the time of this
call, N>1.

[johnme, 2015/01/09 18:28:47]

Done.

On 2015/01/09 17:58:20, Peter Beverloo wrote:
Disagree, but done - we shall tweak the heuristic later anyway.

+  if (notification_count_before_event > 0)
+    return;  // The requirement is met by a pre-existing notification.

[Peter Beverloo, 2015/01/09 17:58:21]

Really? That would allow me to do a cycle like:

[push 1] show a notification (N=1)
[push 2] close the notification (N=0)
[push 3] show a notification (N=1)
...

Which would give me, for users that ignore notifications, 50% free background
push messages. I'm not sure that we should care about the notification count
*before* event execution at all.

[johnme, 2015/01/09 18:28:47]

We want to support e.g. dismissing a notification on other devices when the user
reads it on one device.

I guess it might be a problem if people send pairs of push messages, where the
first shows a notification and then the second one immediately hides it again.
We could fix that by requiring that the pre-existing notification has been
showing for some minimum duration in order to qualify.

But sure, I've removed this for now for simplicity, and we'll tweak this
heuristic before shipping (I added a TODO).

[Peter Beverloo, 2015/01/09 18:46:25]

Smells like a cat-and-mouse game. We should consider such heuristics very
carefully.

+  if (CountNotifications(profile_, application_id.origin) > 0)
+    return;  // The requirement is met by a newly shown notification.
+
+  // If we haven't returned yet, the site failed to show a notification, so we
+  // show a generic notification. See https://github.com/w3c/push-api/pull/87

[Peter Beverloo, 2015/01/09 17:58:21]

I would very much prefer not to refer to pull requests in a code comment.
Referring to a bug, which then refers to the pull request would be better.

[johnme, 2015/01/09 18:28:47]

Done.

+  // TODO(johnme): i18n.
+  content::PlatformNotificationData notification_data;
+  notification_data.title = base::ASCIIToUTF16("Updates available");
+  notification_data.direction =
+      content::PlatformNotificationData::NotificationDirectionLeftToRight;
+  notification_data.lang = "";
+  notification_data.body =
+      base::ASCIIToUTF16("This site updated in the background");
+  notification_data.tag =
+      base::ASCIIToUTF16("user_visible_auto_notification");
+  notification_data.icon = GURL("about:blank");

[Peter Beverloo, 2015/01/09 17:58:20]

GURL()

[johnme, 2015/01/09 18:28:47]

Done.

+  PlatformNotificationServiceImpl::GetInstance()->DisplayPersistentNotification(
+      profile_,
+      application_id.service_worker_registration_id,
+      application_id.origin,
+      SkBitmap() /* icon */,
+      notification_data,
+      content::ChildProcessHost::kInvalidUniqueID /* render_process_id */);

[Peter Beverloo, 2015/01/09 17:58:20]

This is a bit awkward, but OK.

[johnme, 2015/01/09 18:28:47]

Acknowledged.

+#endif
+}
+
 void PushMessagingServiceImpl::OnMessagesDeleted(const std::string& app_id) {
   // TODO(mvanouwerkerk): Fire push error event on the Service Worker
   // corresponding to app_id.