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

chrome/browser/services/gcm/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/services/gcm/push_messaging_service_impl.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #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"
 #include "chrome/browser/services/gcm/push_messaging_application_id.h"
 #include "chrome/browser/services/gcm/push_messaging_constants.h"
 #include "chrome/browser/services/gcm/push_messaging_permission_context.h"
 #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 {
 
 namespace {
 const int kMaxRegistrations = 1000000;
 
 blink::WebPushPermissionStatus ToPushPermission(ContentSetting setting) {
   switch (setting) {
     case CONTENT_SETTING_ALLOW:
       return blink::WebPushPermissionStatusGranted;
     case CONTENT_SETTING_BLOCK:
       return blink::WebPushPermissionStatusDenied;
     case CONTENT_SETTING_ASK:
       return blink::WebPushPermissionStatusDefault;
     default:
       NOTREACHED();
       return blink::WebPushPermissionStatusDenied;
   }
 }
 
+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
 void PushMessagingServiceImpl::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterIntegerPref(
       prefs::kPushMessagingRegistrationCount,
       0,
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
@@ skipping 87 matching lines @@
   // {
   //     "registration_ids": ["FOO", "BAR"],
   //     "data": {
   //         "data": "BAZ",
   //     },
   //     "delay_while_idle": true,
   // }
   // 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)) {
       // The |origin| lost push permission. We need to unregister and drop this
       // message.
       Unregister(application_id, UnregisterCallback());
       return;
     }
 
     const std::string& data = it->second;
     content::BrowserContext::DeliverPushMessage(
         profile_,
         application_id.origin,
         application_id.service_worker_registration_id,
         data,
         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);
   }
 }
 
 void PushMessagingServiceImpl::SetProfileForTesting(Profile* profile) {
   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());
       break;
   }
 }
 
+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.
 }
 
 void PushMessagingServiceImpl::OnSendError(
     const std::string& app_id,
     const GCMClient::SendErrorDetails& send_error_details) {
   NOTREACHED() << "The Push API shouldn't have sent messages upstream";
 }
@@ skipping 242 matching lines @@
 }
 
 void PushMessagingServiceImpl::AddAppHandlerIfNecessary() {
   if (gcm_profile_service_->driver()->GetAppHandler(
           kPushMessagingApplicationIdPrefix) != this)
     gcm_profile_service_->driver()->AddAppHandler(
         kPushMessagingApplicationIdPrefix, this);
 }
 
 }  // namespace gcm