Store serialized notification data as an Intent extra on Android.

chrome/browser/notifications/notification_ui_manager_android.cc

Index: chrome/browser/notifications/notification_ui_manager_android.cc
diff --git a/chrome/browser/notifications/notification_ui_manager_android.cc b/chrome/browser/notifications/notification_ui_manager_android.cc
index a3a339c15ad21ae533bceea3e9c581b6c020829d..da03af7bfd6f1cbd298139e190ad4daea9d17c6c 100644
--- a/chrome/browser/notifications/notification_ui_manager_android.cc
+++ b/chrome/browser/notifications/notification_ui_manager_android.cc
@@ -4,11 +4,18 @@
 
 #include "chrome/browser/notifications/notification_ui_manager_android.h"
 
+#include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/logging.h"
+#include "base/pickle.h"
 #include "base/strings/string16.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/notifications/persistent_notification_delegate.h"
+#include "chrome/browser/notifications/platform_notification_service_impl.h"
 #include "chrome/browser/notifications/profile_notification.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "content/public/common/persistent_notification_status.h"
+#include "content/public/common/platform_notification_data.h"
 #include "jni/NotificationUIManager_jni.h"
 #include "ui/gfx/android/java_bitmap.h"
 #include "ui/gfx/image/image.h"
@@ -18,6 +25,93 @@ using base::android::ConvertJavaStringToUTF8;
 using base::android::ConvertUTF16ToJavaString;
 using base::android::ConvertUTF8ToJavaString;
 
+namespace {
+
+// Persistent notifications are likely to outlive the browser process they were
+// created by on Android. In order to be able to re-surface the notification
+// when the user interacts with them, all relevant notification data needs to
+// be serialized with the notification itself.
+//
+// In the near future, as more features get added to Chrome's notification
+// implementation, this will be done by storing the persistent notification data
+// in a database. However, to support launching Chrome for Android from a
+// notification event until that exists, serialize the data in the Intent.

[Miguel Garcia, 2015/01/07 11:07:40]

Is there a size limit we should be enforcing here? I worry that it would be
subject to abuse by sending a huge notification body that we then need to
serialize/deserialize.

[Peter Beverloo, 2015/01/07 15:16:30]

The reason that we eventually need a database is exactly that. In a future
release we'll also support developer-provided data to be tagged along with a
Notification, which makes this problem much more prevailing.

For now, however, since the feature is not enabled by default yet, I don't
consider this to be a major problem.

+//
+// TODO(peter): Move towards storing notification data in a database rather than
+//              as a serialized Intent extra.
+
+scoped_ptr<Pickle> SerializePersistentNotification(
+    const content::PlatformNotificationData& notification_data,
+    const GURL& origin,
+    int64 service_worker_registration_id) {
+  scoped_ptr<Pickle> pickle(new Pickle);
+
+  // content::PlatformNotificationData
+  pickle->WriteString16(notification_data.title);
+  pickle->WriteInt(static_cast<int>(notification_data.direction));
+  pickle->WriteString(notification_data.lang);
+  pickle->WriteString16(notification_data.body);
+  pickle->WriteString16(notification_data.tag);
+  pickle->WriteString(notification_data.icon.spec());
+
+  // The origin which is displaying the notification.
+  pickle->WriteString(origin.spec());
+
+  // The Service Worker registration this notification is associated with.
+  pickle->WriteInt64(service_worker_registration_id);
+
+  return pickle.Pass();
+}
+
+bool UnserializePersistentNotification(
+    const Pickle& pickle,
+    content::PlatformNotificationData* notification_data,
+    GURL* origin,
+    int64* service_worker_registration_id) {
+  DCHECK(notification_data && origin && service_worker_registration_id);
+  PickleIterator iterator(pickle);
+
+  std::string icon_url, origin_url;
+  int direction_value;
+
+  // Unpack content::PlatformNotificationData.
+  if (!iterator.ReadString16(&notification_data->title) ||
+      !iterator.ReadInt(&direction_value) ||
+      !iterator.ReadString(&notification_data->lang) ||
+      !iterator.ReadString16(&notification_data->body) ||
+      !iterator.ReadString16(&notification_data->tag) ||
+      !iterator.ReadString(&icon_url))

[Michael van Ouwerkerk, 2015/01/07 13:39:10]

Add curly braces when the condition is wrapped.

[Peter Beverloo, 2015/01/07 15:16:30]

Done.

+    return false;
+
+  notification_data->direction =
+      static_cast<content::PlatformNotificationData::NotificationDirection>(
+          direction_value);
+  notification_data->icon = GURL(icon_url);
+
+  // Unpack the origin which displayed this notification.
+  if (!iterator.ReadString(&origin_url))
+    return false;
+
+  *origin = GURL(origin_url);
+
+  // Unpack the Service Worker registration id.
+  if (!iterator.ReadInt64(service_worker_registration_id))
+    return false;
+
+  return true;
+}
+
+// Called when the "notificationclick" event in the Service Worker has finished
+// executing for a notification that was created in a previous instance of the
+// browser.
+void OnEventDispatchComplete(content::PersistentNotificationStatus status) {
+  // TODO(peter): Add UMA statistics based on |status|.
+  // TODO(peter): Decide if we want to forcefully shut down the browser process
+  // if we're confident it was created for delivering this event.
+}
+
+}  // namespace
+
 // Called by the Java side when a notification event has been received, but the
 // NotificationUIManager has not been initialized yet. Enforce initialization of
 // the class.
@@ -46,18 +140,55 @@ NotificationUIManagerAndroid::~NotificationUIManagerAndroid() {
 }
 
 bool NotificationUIManagerAndroid::OnNotificationClicked(
-    JNIEnv* env, jobject java_object, jstring notification_id) {
+    JNIEnv* env,
+    jobject java_object,
+    jstring notification_id,
+    jbyteArray serialized_notification_data) {
   std::string id = ConvertJavaStringToUTF8(env, notification_id);
 
   auto iter = profile_notifications_.find(id);
-  if (iter == profile_notifications_.end()) {
-    // TODO(peter): Handle the "click" event for notifications which have
-    // outlived the browser process that created them.
+  if (iter != profile_notifications_.end()) {

[Miguel Garcia, 2015/01/07 11:07:40]

Now that you have everything nicely stored in the notification serialized data
itself. Can we drop this altogether and save memory / simplify things?

[Peter Beverloo, 2015/01/07 15:16:30]

No. We will still use the delegates in the situation where Chrome did not have
to start up again, and we need this information to fulfill the
NotificationUIManager API contract.

I haven't thought through yet whether it makes sense for the database to be
Android-only, or shared on all platforms. It may be wise to have this live a
layer below the NotificationUIManager, in which case that argument will no
longer be relevant.

+    const Notification& notification = iter->second->notification();
+    notification.delegate()->Click();
+
+    return true;
+  }
+
+  // If the Notification were not found, it may be a persistent notification
+  // that was outlived the Chrome browser process. In this case, try to

[Michael van Ouwerkerk, 2015/01/07 13:39:10]

s/was/has/ or just delete it

[Peter Beverloo, 2015/01/07 15:16:30]

Done.

+  // unserialize the notification's serialized data and trigger the click
+  // event manually.
+
+  std::vector<uint8> bytes;
+  base::android::JavaByteArrayToByteVector(env, serialized_notification_data,
+                                           &bytes);
+  if (!bytes.size())
+    return false;
+
+  content::PlatformNotificationData notification_data;
+  GURL origin;
+  int64 service_worker_registration_id;
+
+  Pickle pickle(reinterpret_cast<const char*>(&bytes[0]), bytes.size());
+  if (!UnserializePersistentNotification(pickle, &notification_data, &origin,
+                                         &service_worker_registration_id)) {
     return false;
   }
 
-  const Notification& notification = iter->second->notification();
-  notification.delegate()->Click();
+  PlatformNotificationServiceImpl* service =
+      PlatformNotificationServiceImpl::GetInstance();
+
+  // TODO(peter): Rather than assuming that the last used profile is the
+  // appropriate one for this notification, the used profile should be
+  // stored as part of the notification's data. See https://crbug.com/437574.
+  service->OnPersistentNotificationClick(
+      ProfileManager::GetLastUsedProfile(),
+      service_worker_registration_id,
+      id,
+      origin,
+      notification_data,
+      base::Bind(&OnEventDispatchComplete));
+
   return true;
 }
 
@@ -102,9 +233,22 @@ void NotificationUIManagerAndroid::Add(const Notification& notification,
   if (!icon_bitmap.isNull())
     icon = gfx::ConvertToJavaBitmap(&icon_bitmap);
 
+  ScopedJavaLocalRef<jbyteArray> notification_data;
+  if (true /** is_persistent_notification **/) {
+    PersistentNotificationDelegate* delegate =
+        static_cast<PersistentNotificationDelegate*>(notification.delegate());
+    scoped_ptr<Pickle> pickle = SerializePersistentNotification(
+        delegate->notification_data(),
+        notification.origin_url(),
+        delegate->service_worker_registration_id());
+
+    notification_data = base::android::ToJavaByteArray(
+        env, static_cast<const uint8*>(pickle->data()), pickle->size());
+  }
+
   int platform_id = Java_NotificationUIManager_displayNotification(
       env, java_object_.obj(), id.obj(), title.obj(), body.obj(), icon.obj(),
-      origin.obj());
+      origin.obj(), notification_data.obj());
 
   std::string notification_id = profile_notification->notification().id();
   platform_notifications_[notification_id] = platform_id;