Route newly created notifications to notification provider API.

chrome/browser/notifications/message_center_notification_manager.cc

Index: chrome/browser/notifications/message_center_notification_manager.cc
diff --git a/chrome/browser/notifications/message_center_notification_manager.cc b/chrome/browser/notifications/message_center_notification_manager.cc
index 8afa5a1714d82d9df32ebe9b709082e145d3dd02..11e7ee1a2d0eff9e80eee3990b7e421afe425d94 100644
--- a/chrome/browser/notifications/message_center_notification_manager.cc
+++ b/chrome/browser/notifications/message_center_notification_manager.cc
@@ -9,7 +9,9 @@
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/stl_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/api/notification_provider/notification_provider_api.h"
 #include "chrome/browser/notifications/desktop_notification_service.h"
 #include "chrome/browser/notifications/desktop_notification_service_factory.h"
 #include "chrome/browser/notifications/fullscreen_notification_blocker.h"
@@ -20,13 +22,16 @@
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/host_desktop.h"
+#include "chrome/common/extensions/api/notification_provider.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/info_map.h"
 #include "extensions/common/extension_set.h"
+#include "extensions/common/permissions/permissions_data.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/message_center/message_center_style.h"
 #include "ui/message_center/message_center_tray.h"
@@ -49,6 +54,163 @@
 const int kFirstRunIdleDelaySeconds = 1;
 #endif
 
+namespace {
+
+bool GfxImageToNotificationBitmapArgs(

[Pete Williamson, 2014/08/05 22:03:52]

Why return bool when this has no failure case?  Maybe we missed a return false,
or maybe this function should be void.

[liyanhou, 2014/08/08 22:40:25]

Done.

+    const gfx::Image* gfx_image,
+    base::DictionaryValue* return_image_args) {
+  SkBitmap sk_bitmap = gfx_image->AsBitmap();
+  sk_bitmap.lockPixels();
+
+  return_image_args->SetWithoutPathExpansion(
+      "width", new base::FundamentalValue(sk_bitmap.width()));
+  return_image_args->SetWithoutPathExpansion(
+      "height", new base::FundamentalValue(sk_bitmap.height()));
+
+  const int BYTES_PER_PIXEL = 4;
+  size_t data_area = sk_bitmap.width() * sk_bitmap.height();
+  size_t data_size = data_area * BYTES_PER_PIXEL;

[Pete Williamson, 2014/08/05 22:03:52]

Do we know for sure that all input bitmaps will be 4 bytes per pixel?

[liyanhou, 2014/08/08 22:40:25]

Yes. All input images are of gfx::Image type.

+
+  scoped_ptr<char[]> data(new char[data_size]);
+  uint32_t* bitmap_pixels = sk_bitmap.getAddr32(0, 0);
+
+  for (size_t t = 0; t < data_area; ++t) {

[Pete Williamson, 2014/08/05 22:03:52]

nit - "i" is normally used as an iterator variable in our code base, not "t".

[liyanhou, 2014/08/08 22:40:25]

Done.

+    // Bitmap_pixels (original image) is ARGB, data (converted) is RGBA
+    size_t rgba_index = t * BYTES_PER_PIXEL;
+
+    data[rgba_index + 0] = (((bitmap_pixels[t]) >> 16) & 0xFF);

[Pete Williamson, 2014/08/05 22:03:52]

Have you checked elsewhere in the code to see if there is a utility function to
do this?  It seems likely.

[liyanhou, 2014/08/08 22:40:24]

Done.

+    data[rgba_index + 1] = (((bitmap_pixels[t]) >> 8) & 0xFF);
+    data[rgba_index + 2] = (((bitmap_pixels[t]) >> 0) & 0xFF);
+    data[rgba_index + 3] = (((bitmap_pixels[t]) >> 24) & 0xFF);
+  }
+
+  scoped_ptr<base::BinaryValue> image_data(
+      new base::BinaryValue(data.Pass(), data_size));
+  return_image_args->SetWithoutPathExpansion("data", image_data.release());
+
+  return true;
+}
+
+std::string MapTypeToString(message_center::NotificationType type) {
+  switch (type) {
+    case message_center::NOTIFICATION_TYPE_BASE_FORMAT:
+      return "basic";
+    case message_center::NOTIFICATION_TYPE_IMAGE:
+      return "image";
+    case message_center::NOTIFICATION_TYPE_MULTIPLE:
+      return "list";
+    case message_center::NOTIFICATION_TYPE_PROGRESS:
+      return "progress";
+    default:
+      return "";
+  }
+}
+
+// This function converts Notification::Notification data to
+// base::DictionaryValue
+// for extensions::api::notifications::NotificationOptions object construction
+void NotificationToNotificationOptionsArgs(
+    const Notification& notification,
+    base::DictionaryValue* content_args) {
+  // Extract required fileds: type, title, message, and icon.

[Pete Williamson, 2014/08/05 22:03:52]

nit: fileds: fields

[liyanhou, 2014/08/08 22:40:25]

Done.

+  std::string type = MapTypeToString(notification.type());
+  content_args->SetWithoutPathExpansion("type", new base::StringValue(type));
+
+  content_args->SetWithoutPathExpansion(
+      "title", new base::StringValue(notification.title()));

[Pete Williamson, 2014/08/05 22:03:52]

can notification.title() (or any of the other methods we call on it) return
null?  If so, we can have a crash.  The notification may be coming from an
untrusted source, we don't want it to be able to crash chrome, which might be a
security hole.

[liyanhou, 2014/08/08 22:40:24]

id, title, message, and icon url are the required fields for creating
notifications through notifications API. Since Notifications is converted from
that, all notifications should have those four fields.

[Pete Williamson, 2014/08/09 00:38:19]

Thanks!  Just to be sure, can you check that the notification api is actually
enforcing (checking that they are not null) when the notification enters the
system?

[liyanhou, 2014/08/11 17:59:23]

Yes. Notifications API enforces it when creating a notification.

+
+  content_args->SetWithoutPathExpansion(
+      "message", new base::StringValue(notification.message()));
+
+  if (!notification.icon().IsEmpty()) {
+    scoped_ptr<base::DictionaryValue> icon_args(new base::DictionaryValue);
+    if (GfxImageToNotificationBitmapArgs(&notification.icon(), icon_args.get()))
+      content_args->SetWithoutPathExpansion("iconBitmap", icon_args.release());
+  }
+
+  // Handle optional data provided.
+  const message_center::RichNotificationData* rich_data =
+      &notification.rich_notification_data();
+
+  content_args->SetWithoutPathExpansion(
+      "priority", new base::FundamentalValue(rich_data->priority));
+
+  content_args->SetWithoutPathExpansion(
+      "isClickable", new base::FundamentalValue(rich_data->clickable));
+
+  content_args->SetWithoutPathExpansion(
+      "eventTime",
+      new base::FundamentalValue(rich_data->timestamp.ToDoubleT()));
+
+  if (!base::UTF16ToUTF8(rich_data->context_message).empty())

[Pete Williamson, 2014/08/05 22:03:52]

Can we check emptyness without doing a conversion?  It would be simpler and
faster.  (or maybe this function has a side effect of converting in place?  It
seems to be throwing away any return value)

[liyanhou, 2014/08/08 22:40:25]

Done.

+    content_args->SetWithoutPathExpansion(
+        "contextMessage", new base::StringValue(rich_data->context_message));
+
+  if (!rich_data->buttons.empty()) {
+    scoped_ptr<base::ListValue> button_list(new base::ListValue);
+    for (size_t i = 0; i < rich_data->buttons.size(); i++) {
+      scoped_ptr<base::DictionaryValue> button(new base::DictionaryValue());
+      button->SetWithoutPathExpansion(
+          "title", new base::StringValue(rich_data->buttons[i].title));
+
+      if (!rich_data->buttons[i].icon.IsEmpty()) {
+        scoped_ptr<base::DictionaryValue> icon_args(new base::DictionaryValue);
+        if (GfxImageToNotificationBitmapArgs(&rich_data->buttons[i].icon,
+                                             icon_args.get()))
+          button->SetWithoutPathExpansion("iconBitmap", icon_args.release());
+      }
+      button_list->Set(i, button.release());
+    }
+    content_args->SetWithoutPathExpansion("buttons", button_list.release());
+  }
+
+  // Only image type notifications should have images.
+  if (type == "image" && !rich_data->image.IsEmpty()) {

[Pete Williamson, 2014/08/05 22:03:52]

maybe log if we find one that violates this assumption (same for progress)

[liyanhou, 2014/08/08 22:40:25]

Done. 

+    scoped_ptr<base::DictionaryValue> image_args(new base::DictionaryValue);
+    if (GfxImageToNotificationBitmapArgs(&notification.image(),
+                                         image_args.get()))
+      content_args->SetWithoutPathExpansion("imageBitmap",
+                                            image_args.release());
+  }
+
+  // Only progress type notifications should have progress bars.
+  if (type == "progress")
+    content_args->SetWithoutPathExpansion(
+        "progress", new base::FundamentalValue(rich_data->progress));
+
+  // Only list type notifications should have lists.
+  if (type == "list" && !rich_data->items.empty()) {
+    scoped_ptr<base::ListValue> list(new base::ListValue);
+    for (size_t i = 0; i < rich_data->items.size(); i++) {

[Pete Williamson, 2014/08/05 22:03:52]

use "j" instead of "i" here to keep it distinct from the one using "i" above. 
It can also help in the debugger in some circumstances.

[liyanhou, 2014/08/08 22:40:24]

Done.

+      scoped_ptr<base::DictionaryValue> item(new base::DictionaryValue());
+      item->SetWithoutPathExpansion(
+          "title", new base::StringValue(rich_data->items[i].title));
+      item->SetWithoutPathExpansion(
+          "message", new base::StringValue(rich_data->items[i].message));
+      list->Set(i, item.release());
+    }
+    content_args->SetWithoutPathExpansion("items", list.release());
+  }
+}
+
+// TODO(liyanhou): When additional settings in Chrome Settings is implemented,
+// change choosing a random app with this permission to a user selected app.

[Pete Williamson, 2014/08/05 22:03:53]

nit:  We're not actually getting a random app here, we're getting the last one
to be returned from the search (which might actually be stable instead of
random, so it would return the same one every time).

[liyanhou, 2014/08/08 22:40:25]

Done.

+std::string GetExtensionTakingOverNotifications(Profile* profile) {
+  extensions::ExtensionRegistry* registry =

[Pete Williamson, 2014/08/05 22:03:52]

Can this return null (maybe early in setup before the extension registry is set
up).

[liyanhou, 2014/08/08 22:40:25]

I don't really know. I read cases where they check, and cases where they didn't
check. I'll just add a check in case.

+      extensions::ExtensionRegistry::Get(profile);
+  std::string app_id;
+  for (extensions::ExtensionSet::const_iterator it =
+           registry->enabled_extensions().begin();
+       it != registry->enabled_extensions().end();
+       ++it) {
+    if ((*it->get()).permissions_data()->HasAPIPermission(
+            extensions::APIPermission::ID::kNotificationProvider))
+      app_id = (*it->get()).id();
+  }
+  return app_id;
+}
+}  // namespace
+
 MessageCenterNotificationManager::MessageCenterNotificationManager(
     message_center::MessageCenter* message_center,
     PrefService* local_state,
@@ -493,6 +655,34 @@ void MessageCenterNotificationManager::AddProfileNotification(
       new message_center::Notification(notification));
   message_center_->AddNotification(message_center_notification.Pass());
 
+  // TODO(liyanhou): Change the logic to only send notifications to one party.
+  // Currently, if there's an app with notificationProvider permission,
+  // notifications will go to both Chrome Notification Center and that app.

[dewittj, 2014/08/07 22:49:08]

I don't believe this code should live in AddProfileNotification.  It should have
its own function, something like RouteNotificationToAlternateProvider() or
something.  I don't think that this patch forwards updates correctly, right?

[liyanhou, 2014/08/08 22:40:25]

Done. No, this patch doesn't forward updates.

+

[Pete Williamson, 2014/08/05 22:03:52]

We should not check this in turned on.  It is OK to check it in behind a flag,
or in trunk or canary only (not stable, beta, or dev).  We don't want to turn
the feature on for everybody until the settings story is figured out and turned
on.

[liyanhou, 2014/08/08 22:40:25]

Yes it is in trunk.

+  // If there exists apps/extensions that have notificationProvider permission,
+  // reroute notifications to one of the apps/extensions.
+  std::string app_id =
+      GetExtensionTakingOverNotifications(profile_notification->profile());
+  if (!app_id.empty()) {
+    const std::string sender_id = notification.notifier_id().id;
+
+    // Extract data from Notification.
+    scoped_ptr<base::DictionaryValue> content_args(new base::DictionaryValue);
+    NotificationToNotificationOptionsArgs(notification, content_args.get());
+
+    scoped_ptr<extensions::api::notifications::NotificationOptions> content(
+        new extensions::api::notifications::NotificationOptions());
+    extensions::api::notifications::NotificationOptions::Populate(
+        *(content_args.get()), content.get());
+
+    // Fire event to send the data to an extension/app.
+    scoped_ptr<extensions::NotificationProviderEventRouter> event_router(
+        new extensions::NotificationProviderEventRouter(
+            profile_notification->profile()));
+    event_router->CreateNotification(
+        app_id, sender_id, id, *(content.release()));
+  }
+
   profile_notification->StartDownloads();
 }