Linux native notifications: Support icons

chrome/browser/notifications/notification_platform_bridge_linux.cc

 // Copyright 2017 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/notifications/notification_platform_bridge_linux.h"
 
 #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"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/notifications/native_notification_display_service.h"
 #include "chrome/browser/notifications/notification.h"
 #include "chrome/browser/notifications/notification_display_service_factory.h"
 #include "chrome/browser/profiles/profile_manager.h"
 
 namespace {
 
 const char kFreedesktopNotificationsName[] = "org.freedesktop.Notifications";
 const char kFreedesktopNotificationsPath[] = "/org/freedesktop/Notifications";
 
 void AddActionToNotification(GVariantBuilder* actions_builder,
                              const char* action_id,
                              const char* button_label) {
   g_variant_builder_add(actions_builder, "s", action_id);
   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,
                             GAsyncResult* result,
                             gpointer user_data) {
   GDBusProxy* proxy = G_DBUS_PROXY(source_object);
   GVariant* value = g_dbus_proxy_call_finish(proxy, result, nullptr);
   if (!value) {
     // The message might have been cancelled, in which case
     // |user_data| points to a destroyed NotificationData.
@@ skipping 17 matching lines @@
     return;
 
   NotificationDisplayService* display_service =
       NotificationDisplayServiceFactory::GetForProfile(profile);
 
   static_cast<NativeNotificationDisplayService*>(display_service)
       ->ProcessNotificationOperation(operation, notification_type, origin,
                                      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
 NotificationPlatformBridge* NotificationPlatformBridge::Create() {
   GDBusProxy* notification_proxy = g_dbus_proxy_new_for_bus_sync(
       G_BUS_TYPE_SESSION,
       static_cast<GDBusProxyFlags>(G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES |
                                    G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START),
       nullptr, kFreedesktopNotificationsName, kFreedesktopNotificationsPath,
       kFreedesktopNotificationsName, nullptr, nullptr);
@@ skipping 10 matching lines @@
                    const GURL& origin_url)
       : notification_type(notification_type),
         notification_id(notification_id),
         profile_id(profile_id),
         is_incognito(is_incognito),
         origin_url(origin_url) {}
 
   ~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
   // first "Notify" message completes.
   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;
 
   // A copy of the origin_url from the underlying
   // message_center::Notification.  Used to pass back to
   // 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.
   ScopedGObject<GCancellable> cancellable;
 
   // 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.
   bool should_close = false;
 };
 
 NotificationPlatformBridgeLinux::NotificationPlatformBridgeLinux(
     GDBusProxy* notification_proxy)
     : notification_proxy_(notification_proxy) {
   proxy_signal_handler_ = g_signal_connect(
       notification_proxy_, "g-signal", G_CALLBACK(GSignalReceiverThunk), this);
 }
 
 NotificationPlatformBridgeLinux::~NotificationPlatformBridgeLinux() {
   if (proxy_signal_handler_)
     g_signal_handler_disconnect(notification_proxy_, proxy_signal_handler_);
 }
 
 void NotificationPlatformBridgeLinux::Display(
     NotificationCommon::Type notification_type,
     const std::string& notification_id,
     const std::string& profile_id,
     bool is_incognito,
     const Notification& notification) {
   NotificationData* data =
       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 {
     // Send the notification for the first time.
     data = new NotificationData(notification_type, notification_id, profile_id,
                                 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);
   }
 }
 
 void NotificationPlatformBridgeLinux::Close(
     const std::string& profile_id,
     const std::string& notification_id) {
   std::vector<NotificationData*> to_erase;
   for (const auto& pair : notifications_) {
     NotificationData* data = pair.first;
     if (data->notification_id == notification_id &&
@@ skipping 26 matching lines @@
   if (value && g_variant_is_of_type(value, G_VARIANT_TYPE("(u)")))
     g_variant_get(value, "(u)", &data->dbus_id);
 
   if (!data->dbus_id) {
     // There was some sort of error with creating the notification.
     notifications_.erase(data);
   } else if (data->should_close) {
     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
   // us in GSignalReceiver.  Odd-indexed ones contain the button text.
   g_variant_builder_init(&actions_builder, G_VARIANT_TYPE("as"));
   if (notification.clickable()) {
     // Special case: the pair ("default", "") will not add a button,
     // but instead makes the entire notification clickable.
     AddActionToNotification(&actions_builder, "default", "");
   }
   // 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);
 }
 
 void NotificationPlatformBridgeLinux::CloseNow(uint32_t dbus_id) {
   g_dbus_proxy_call(notification_proxy_, "CloseNotification",
                     g_variant_new("(u)", dbus_id), G_DBUS_CALL_FLAGS_NONE, -1,
                     nullptr, nullptr, nullptr);
 }
 
 NotificationPlatformBridgeLinux::NotificationData*
@@ skipping 65 matching lines @@
 
     if (strcmp(action, "default") == 0) {
       ForwardNotificationOperation(dbus_id, NotificationCommon::CLICK, 0);
     } else if (strcmp(action, "settings") == 0) {
       ForwardNotificationOperation(dbus_id, NotificationCommon::SETTINGS, -1);
     } else {
       NOTIMPLEMENTED() << "No custom buttons just yet!";
     }
   }
 }