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..ed8f4bc9406133dbead14d2e0b7e4e0c1bc58aaf 100644
--- a/chrome/browser/notifications/notification_platform_bridge_linux.cc
+++ b/chrome/browser/notifications/notification_platform_bridge_linux.cc
@@ -6,10 +6,12 @@
 
 #include <algorithm>
 
+#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 "base/task_scheduler/post_task.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/notifications/native_notification_display_service.h"
 #include "chrome/browser/notifications/notification.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
+    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,39 @@ void ProfileLoadedCallback(NotificationCommon::Operation operation,
                                      notification_id, action_index, reply);
 }
 
+// Writes |image| to a new temporary file and return its path.
+// Returns true on success.
+base::FilePath WriteImageToTmpFile(const gfx::Image& image) {
+  base::FilePath file_path;
+  if (image.IsEmpty() || !base::CreateTemporaryFile(&file_path))

[Peter Beverloo, 2017/04/12 00:35:16]

One thing that we discussed for the Windows Action Center (I don't know to
what extent this applies to Linux) is using a pool of files as opposed to
an arbitrary number of temporary files. If Chrome crashes or doesn't perform
a clean exit ResetResourceFiles() wouldn't be called, and the files may
be left behind.

The idea would be to have icon-[0-10].png or similar and iterate through
them. This is possible because the icon data is read by the OS when the
notification is shown, and is cached internally after. It sounds like
we're not sure of Linux' semantics in this area, is there a way to
verify?

[Tom (Use chromium acct), 2017/04/13 02:16:28]

The spec doesn't make any guarantees about this, but with the hundreds of DEs
out there, I'm sure at least one of them wouldn't cache the icon

+    return base::FilePath();
+  auto image_data = image.As1xPNGBytes();

[Peter Beverloo, 2017/04/12 00:35:16]

micro nit: I'd spell out the type (scoped_refptr<base::RefCountedMemory>). It
fits on a single line and it's not obvious from the call what the type is.

[Tom (Use chromium acct), 2017/04/13 02:16:28]

Done.

+  int data_len = image_data->size();
+  if (data_len == 0 || base::WriteFile(file_path, image_data->front_as<char>(),

[Lei Zhang, 2017/04/12 01:01:41]

Can we fetch the image data on the UI thread, and then pass only the data to
WriteImageToTmpFile()? I have no idea if any of the gfx classes are thread safe
and the ui/gfx/image/image.cc change looks slightly suspicious.

[Tom (Use chromium acct), 2017/04/13 02:16:29]

Done.

+                                       data_len) != data_len) {
+    base::DeleteFile(file_path, false);
+    return base::FilePath();
+  }
+  return file_path;
+}
+
+void DeleteFile(const base::FilePath& file_path) {

[Lei Zhang, 2017/04/13 01:12:17]

BTW, name this DeleteNotificationImage() so it's easier to distinguish vs other
functions named DeleteFile.

[Tom (Use chromium acct), 2017/04/13 02:16:28]

Done.

+  if (file_path.empty())
+    return;
+  base::PostTaskWithTraits(
+      FROM_HERE,
+      base::TaskTraits()
+          .MayBlock()
+          .WithPriority(base::TaskPriority::BACKGROUND)
+          .WithShutdownBehavior(
+              base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN),
+      base::Bind(

[Lei Zhang, 2017/04/12 01:03:54]

Isn't this just:

base::Bind(&base::DeleteFile, file_path, false) ?

[Tom (Use chromium acct), 2017/04/13 02:16:28]

Done.  Turns out base::IgnoreResult() was the missing ingredient

+          [](const base::FilePath& file_path) {
+            base::DeleteFile(file_path, false);
+          },
+          file_path));
+}
+
 }  // namespace
 
 // static
@@ -90,11 +146,19 @@ struct NotificationPlatformBridgeLinux::NotificationData {
         notification_id(notification_id),
         profile_id(profile_id),
         is_incognito(is_incognito),
-        origin_url(origin_url) {}
+        origin_url(origin_url),
+        weak_factory(this) {}
 
   ~NotificationData() {
     if (cancellable)
       g_cancellable_cancel(cancellable);
+    ResetResourceFiles();

[Peter Beverloo, 2017/04/12 00:35:16]

Since the NotificationPlatformBridgeLinux is owned by the BrowserProcessImpl
we're already very far in the shutdown process by the time we schedule this
task. Will this call do anything at all?

[Tom (Use chromium acct), 2017/04/13 02:16:28]

Turns out it will not.  Added NOTIFICATION_APP_TERMINATING to handle this.

+  }
+
+  void ResetResourceFiles() {
+    for (const base::FilePath& file : resource_files)
+      DeleteFile(file);

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

So all of the resource files get cleaned up when chrome closes, even if there
are still notifications displayed.  Not sure if this is a problem

+    resource_files.clear();
   }
 
   // The ID used by the notification server.  Will be 0 until the
@@ -102,7 +166,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 +176,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;

[Peter Beverloo, 2017/04/12 00:35:16]

Out of interest, are you aware of base::FileProxy? It's scoped, avoids some of
the task complexity and removes the ScopedAllowUI.

[Tom (Use chromium acct), 2017/04/13 02:16:29]

Didn't know about that.  The only thing is, it would complicate Notify() because
it would require 4 chained callbacks (open icon file, write to icon file, open
image file, write to image file).

+
   // Used to cancel the initial "Notify" message so we don't call
   // NotificationPlatformBridgeLinux::NotifyCompleteInternal() with a
   // destroyed Notification.
@@ -120,15 +188,26 @@ 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.
   bool should_close = false;
+
+  base::WeakPtrFactory<NotificationData> weak_factory;
+};
+
+struct NotificationPlatformBridgeLinux::ResourceFiles {
+  explicit ResourceFiles(const base::FilePath& icon_file)
+      : icon_file(icon_file) {}
+  ~ResourceFiles() { DeleteFile(icon_file); }
+  base::FilePath icon_file;
 };
 
 NotificationPlatformBridgeLinux::NotificationPlatformBridgeLinux(
     GDBusProxy* notification_proxy)
-    : notification_proxy_(notification_proxy) {
+    : notification_proxy_(notification_proxy), weak_factory_(this) {
   proxy_signal_handler_ = g_signal_connect(
       notification_proxy_, "g-signal", G_CALLBACK(GSignalReceiverThunk), this);
 }
@@ -148,11 +227,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;
+      Notify(notification, data, nullptr, nullptr);
     } else {
+      data->update_notification_type = notification_type;
       data->update_data = base::MakeUnique<Notification>(notification);
     }
   } else {
@@ -161,8 +240,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);
+    Notify(notification, data, NotifyCompleteReceiver, data);
   }
 }
 
@@ -209,20 +287,45 @@ 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;
+    Notify(*data->update_data, data, nullptr, nullptr);
     data->update_data.reset();
   }
 }
 
+void NotificationPlatformBridgeLinux::Notify(const Notification& notification,
+                                             NotificationData* data,
+                                             GAsyncReadyCallback callback,
+                                             gpointer user_data) {
+  base::PostTaskWithTraitsAndReplyWithResult(
+      FROM_HERE,
+      base::TaskTraits()
+          .MayBlock()
+          .WithPriority(base::TaskPriority::USER_BLOCKING)
+          .WithShutdownBehavior(base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN),
+      base::Bind(
+          [](const gfx::Image& icon) {
+            return base::MakeUnique<ResourceFiles>(WriteImageToTmpFile(icon));

[Lei Zhang, 2017/04/12 01:03:54]

If NotifyNow() can take the FilePath as a result and wrap it in a ResourceFiles
there, then this becomes:

base::Bind(&WriteImageToTmpFile, notification.icon())

[Tom (Use chromium acct), 2017/04/13 02:16:28]

I want to keep it as a unique_ptr so that the resource files get deleted if
NotifyNow() is not called (this happens when NPBL goes away and resets the
WeakPtr)

+          },
+          notification.icon()),
+      base::Bind(&NotificationPlatformBridgeLinux::NotifyNow,
+                 weak_factory_.GetWeakPtr(), notification,
+                 data->weak_factory.GetWeakPtr(), callback, user_data));
+}
+
 void NotificationPlatformBridgeLinux::NotifyNow(
-    uint32_t dbus_id,
-    NotificationCommon::Type notification_type,
     const Notification& notification,
-    GCancellable* cancellable,
+    base::WeakPtr<NotificationData> data,
     GAsyncReadyCallback callback,
-    gpointer user_data) {
-  // TODO(thomasanderson): Add a complete implementation.
+    gpointer user_data,
+    std::unique_ptr<ResourceFiles> resource_files) {
+  if (!data)
+    return;
+
+  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 +339,28 @@ 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())));
+
+  if (!resource_files->icon_file.empty()) {
+    g_variant_builder_add(
+        &hints_builder, "{sv}", "image-path",
+        g_variant_new_string(resource_files->icon_file.value().c_str()));
+    data->resource_files.push_back(resource_files->icon_file);
+    resource_files->icon_file.clear();
+  }
+
   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);
 }