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_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"
 #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
+    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 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
 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);
   if (!notification_proxy)
     return nullptr;
   return new NotificationPlatformBridgeLinux(notification_proxy);
 }
 
 struct NotificationPlatformBridgeLinux::NotificationData {
   NotificationData(NotificationCommon::Type notification_type,
                    const std::string& notification_id,
                    const std::string& profile_id,
                    bool is_incognito,
                    const GURL& origin_url)
       : notification_type(notification_type),
         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
   // 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;

[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.
   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;
+
+  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);
 }
 
 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;
+      Notify(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);
+    Notify(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;
+    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
   // 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())));
+
+  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);
 }
 
 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!";
     }
   }
 }