Linux native notifications: Support icons

chrome/browser/notifications/notification_platform_bridge_linux.cc

Index: chrome/browser/notifications/notification_platform_bridge_linux.cc
diff --git a/chrome/browser/notifications/notification_platform_bridge_linux.cc b/chrome/browser/notifications/notification_platform_bridge_linux.cc
index 4eb07fdd2f154f463ebcd9a51f144cae81008876..8ed75bcb119f0106163ef97a199942d27e6692a2 100644
--- a/chrome/browser/notifications/notification_platform_bridge_linux.cc
+++ b/chrome/browser/notifications/notification_platform_bridge_linux.cc
@@ -6,6 +6,8 @@
 
 #include <algorithm>
 
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 #include "base/strings/nullable_string16.h"
@@ -28,6 +30,27 @@ void AddActionToNotification(GVariantBuilder* actions_builder,
   g_variant_builder_add(actions_builder, "s", button_label);
 }
 
+int NotificationPriorityToFdoUrgency(int priority) {
+  enum FdoUrgency {
+    LOW = 0,
+    NORMAL = 1,
+    CRITICAL = 2,
+  };
+  switch (priority) {
+    case message_center::MIN_PRIORITY:
+    case message_center::LOW_PRIORITY:
+      return LOW;
+    case message_center::HIGH_PRIORITY:
+    case message_center::MAX_PRIORITY:
+      return CRITICAL;
+    default:
+      NOTREACHED();
+    // fallthrough

[Lei Zhang, 2017/04/11 01:27:05]

super nitty: Intent 2 more because it's the NOTREACHED() that's falling through,
rather than the next case.

[Tom (Use chromium acct), 2017/04/11 23:21:08]

Done.

+    case message_center::DEFAULT_PRIORITY:
+      return NORMAL;
+  }
+}
+
 // Callback used by GLib when the "Notify" message completes for the
 // first time.
 void NotifyCompleteReceiver(GObject* source_object,
@@ -65,6 +88,28 @@ void ProfileLoadedCallback(NotificationCommon::Operation operation,
                                      notification_id, action_index, reply);
 }
 
+// Writes |image| to a new temporary file whose path will be returned
+// in |file_path|.  The file will be memory-mapped, so this function
+// shouldn't block on disk IO.  Returns true on success.
+bool WriteImageToTmpFile(const gfx::Image& image, base::FilePath* file_path) {
+  // Writing to a memory-mapped file doesn't require disk IO.

[Lei Zhang, 2017/04/11 01:27:05]

/dev/shm is backed by tmpfs. If there is enough memory pressure, tmpfs will use
swap, which can potentially hit disk. Yes, the images are small, but let's try
to limit ScopedAllowIO usage to where we have to.

The bottom line is we want to try to keep (potentially) janky operations off the
UI thread. It doesn't look that hard to use PostTaskAndReply to offload this
from the UI thread. Do we want to try doing that and then not have to use
/dev/shm? The deletion can be a straight up PostTask.

+  base::ThreadRestrictions::ScopedAllowIO allow_io;
+
+  base::FilePath shmem_temp_dir;
+  if (image.IsEmpty() || !GetShmemTempDir(false, &shmem_temp_dir) ||

[Lei Zhang, 2017/04/11 01:27:05]

Assuming we want to go with PostTask, don't bother even calling this if we
already know the image is empty? We'd want to get |image_data| first and then
post that over to a file-access capable thread.

[Tom (Use chromium acct), 2017/04/11 23:21:08]

Done.

+      !base::CreateTemporaryFileInDir(shmem_temp_dir, file_path)) {
+    return false;
+  }
+  auto image_data = image.As1xPNGBytes();
+  int data_len = image_data->size();
+  if (data_len == 0 || base::WriteFile(*file_path, image_data->front_as<char>(),
+                                       data_len) != data_len) {
+    base::DeleteFile(*file_path, false);
+    return false;
+  }
+  return true;
+}
+
 }  // namespace
 
 // static
@@ -95,6 +140,17 @@ struct NotificationPlatformBridgeLinux::NotificationData {
   ~NotificationData() {
     if (cancellable)
       g_cancellable_cancel(cancellable);
+    ResetResourceFiles();
+  }
+
+  void ResetResourceFiles() {
+    // Resource files are memory-mapped, so deletion shouldn't require
+    // disk IO.
+    base::ThreadRestrictions::ScopedAllowIO allow_io;
+
+    for (const base::FilePath& file : resource_files)
+      base::DeleteFile(file, false);
+    resource_files.clear();
   }
 
   // The ID used by the notification server.  Will be 0 until the
@@ -102,7 +158,7 @@ struct NotificationPlatformBridgeLinux::NotificationData {
   uint32_t dbus_id = 0;
 
   // Same parameters used by NotificationPlatformBridge::Display().
-  const NotificationCommon::Type notification_type;
+  NotificationCommon::Type notification_type;
   const std::string notification_id;
   const std::string profile_id;
   const bool is_incognito;
@@ -112,6 +168,10 @@ struct NotificationPlatformBridgeLinux::NotificationData {
   // NativeNotificationDisplayService.
   const GURL origin_url;
 
+  // Temporary resource files associated with the notification that
+  // should be cleaned up when the notification is closed.
+  std::vector<base::FilePath> resource_files;
+
   // Used to cancel the initial "Notify" message so we don't call
   // NotificationPlatformBridgeLinux::NotifyCompleteInternal() with a
   // destroyed Notification.
@@ -120,6 +180,8 @@ struct NotificationPlatformBridgeLinux::NotificationData {
   // If not null, the data to update the notification with once
   // |dbus_id| becomes available.
   std::unique_ptr<Notification> update_data;
+  NotificationCommon::Type update_notification_type =
+      NotificationCommon::TYPE_MAX;
 
   // If true, indicates the notification should be closed once
   // |dbus_id| becomes available.
@@ -148,11 +210,11 @@ void NotificationPlatformBridgeLinux::Display(
       FindNotificationData(notification_id, profile_id, is_incognito);
   if (data) {
     // Update an existing notification.
-    DCHECK_EQ(data->notification_type, notification_type);
     if (data->dbus_id) {
-      NotifyNow(data->dbus_id, notification_type, notification, nullptr,
-                nullptr, nullptr);
+      data->notification_type = notification_type;
+      NotifyNow(notification, data, nullptr, nullptr);
     } else {
+      data->update_notification_type = notification_type;
       data->update_data = base::MakeUnique<Notification>(notification);
     }
   } else {
@@ -161,8 +223,7 @@ void NotificationPlatformBridgeLinux::Display(
                                 is_incognito, notification.origin_url());
     data->cancellable.reset(g_cancellable_new());
     notifications_.emplace(data, base::WrapUnique(data));
-    NotifyNow(0, notification_type, notification, data->cancellable,
-              NotifyCompleteReceiver, data);
+    NotifyNow(notification, data, NotifyCompleteReceiver, data);
   }
 }
 
@@ -209,20 +270,21 @@ void NotificationPlatformBridgeLinux::NotifyCompleteInternal(gpointer user_data,
     CloseNow(data->dbus_id);
     notifications_.erase(data);
   } else if (data->update_data) {
-    NotifyNow(data->dbus_id, data->notification_type, *data->update_data,
-              nullptr, nullptr, nullptr);
+    data->notification_type = data->update_notification_type;
+    NotifyNow(*data->update_data, data, nullptr, nullptr);
     data->update_data.reset();
   }
 }
 
 void NotificationPlatformBridgeLinux::NotifyNow(
-    uint32_t dbus_id,
-    NotificationCommon::Type notification_type,
     const Notification& notification,
-    GCancellable* cancellable,
+    NotificationData* data,
     GAsyncReadyCallback callback,
     gpointer user_data) {
-  // TODO(thomasanderson): Add a complete implementation.
+  if (data->dbus_id)
+    DCHECK(!data->cancellable);
+
+  data->ResetResourceFiles();
 
   GVariantBuilder actions_builder;
   // Even-indexed elements in this array are action IDs passed back to
@@ -236,14 +298,27 @@ void NotificationPlatformBridgeLinux::NotifyNow(
   // Always add a settings button.
   AddActionToNotification(&actions_builder, "settings", "Settings");
 
+  GVariantBuilder hints_builder;
+  g_variant_builder_init(&hints_builder, G_VARIANT_TYPE("a{sv}"));
+  g_variant_builder_add(&hints_builder, "{sv}", "urgency",
+                        g_variant_new_byte(NotificationPriorityToFdoUrgency(
+                            notification.priority())));
+
+  base::FilePath icon_file;
+  if (WriteImageToTmpFile(notification.icon(), &icon_file)) {
+    g_variant_builder_add(&hints_builder, "{sv}", "image-path",

[Tom (Use chromium acct), 2017/04/10 23:49:12]

Initially, this used "image-data" so we didn't have to do the temp file thing,
but for some reason this was really slow (~0.5s for the notification to show). 
Writing to the file and using image-path takes just a few ms.

I guess we would have needed to do this anyway when image support is added
because images cannot be inlined like icons.  You always need to use a file.

[Lei Zhang, 2017/04/11 01:04:32]

Any idea where that time is going? Maybe it's not worth investigating if we have
to support "image-path" anyway?
The spec says "Implementations are not required to support images." Which also
makes me wonder - do we have to support images? Does it better translate the Web
Notifications API if we do?


Side note: It's too bad earler versions of DBus did not support file handles.
Many things built on top of DBus has to use file paths as a result. Which means
we can't do the open file, delete file technique to avoid potentially leaking
files. If Chrome crashes while we have pending files to send, then we litter
/dev/shm.

+                          g_variant_new_string(icon_file.value().c_str()));
+    data->resource_files.push_back(icon_file);
+  }
+
   const std::string title = base::UTF16ToUTF8(notification.title());
   const std::string message = base::UTF16ToUTF8(notification.message());
 
   GVariant* parameters =
-      g_variant_new("(susssasa{sv}i)", "", dbus_id, "", title.c_str(),
-                    message.c_str(), &actions_builder, nullptr, -1);
+      g_variant_new("(susssasa{sv}i)", "", data->dbus_id, "", title.c_str(),
+                    message.c_str(), &actions_builder, &hints_builder, -1);
   g_dbus_proxy_call(notification_proxy_, "Notify", parameters,
-                    G_DBUS_CALL_FLAGS_NONE, -1, cancellable, callback,
+                    G_DBUS_CALL_FLAGS_NONE, -1, data->cancellable, callback,
                     user_data);
 }