Linux native notifications: Handle clicks and closes

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 f19184ac8c78ff35c57e1baf256a8a3c2d51b8a0..e9c66ca64ea5c619dcf633df052fff9902cd741f 100644
--- a/chrome/browser/notifications/notification_platform_bridge_linux.cc
+++ b/chrome/browser/notifications/notification_platform_bridge_linux.cc
@@ -8,9 +8,13 @@
 
 #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 {
 
@@ -34,6 +38,26 @@ void NotifyCompleteReceiver(GObject* source_object,
   platform_bridge_linux->NotifyCompleteInternal(user_data, value);
 }
 
+// Runs once the profile has been loaded in order to perform a given
+// |operation| on a notification.
+void ProfileLoadedCallback(NotificationCommon::Operation operation,

[Peter Beverloo, 2017/04/07 01:50:48]

FYI: I've written down a task for me to remove the copies of this.

[Tom (Use chromium acct), 2017/04/07 18:59:51]

Acknowledged.

+                           NotificationCommon::Type notification_type,
+                           const std::string& origin,
+                           const std::string& notification_id,
+                           int action_index,
+                           const base::NullableString16& reply,
+                           Profile* profile) {
+  if (!profile)
+    return;
+
+  NotificationDisplayService* display_service =
+      NotificationDisplayServiceFactory::GetForProfile(profile);
+
+  static_cast<NativeNotificationDisplayService*>(display_service)
+      ->ProcessNotificationOperation(operation, notification_type, origin,
+                                     notification_id, action_index, reply);
+}
+
 }  // namespace
 
 // static
@@ -41,7 +65,6 @@ 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_CONNECT_SIGNALS |
                                    G_DBUS_PROXY_FLAGS_DO_NOT_AUTO_START),
       nullptr, kFreedesktopNotificationsName, kFreedesktopNotificationsPath,
       kFreedesktopNotificationsName, nullptr, nullptr);
@@ -51,12 +74,16 @@ NotificationPlatformBridge* NotificationPlatformBridge::Create() {
 }
 
 struct NotificationPlatformBridgeLinux::NotificationData {
-  NotificationData(const std::string& notification_id,
+  NotificationData(NotificationCommon::Type notification_type,
+                   const std::string& notification_id,
                    const std::string& profile_id,
-                   bool is_incognito)
-      : notification_id(notification_id),
+                   bool is_incognito,
+                   const GURL& origin_url)
+      : notification_type(notification_type),
+        notification_id(notification_id),
         profile_id(profile_id),
-        is_incognito(is_incognito) {}
+        is_incognito(is_incognito),
+        origin_url(origin_url) {}
 
   ~NotificationData() {
     if (cancellable)
@@ -68,10 +95,16 @@ struct NotificationPlatformBridgeLinux::NotificationData {
   uint32_t dbus_id = 0;
 
   // Same parameters used by NotificationPlatformBridge::Display().
+  const 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;
+
   // Used to cancel the initial "Notify" message so we don't call
   // NotificationPlatformBridgeLinux::NotifyCompleteInternal() with a
   // destroyed Notification.
@@ -80,7 +113,6 @@ 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_type = NotificationCommon::TYPE_MAX;
 
   // If true, indicates the notification should be closed once
   // |dbus_id| becomes available.
@@ -89,9 +121,15 @@ struct NotificationPlatformBridgeLinux::NotificationData {
 
 NotificationPlatformBridgeLinux::NotificationPlatformBridgeLinux(
     GDBusProxy* notification_proxy)
-    : notification_proxy_(notification_proxy) {}
+    : notification_proxy_(notification_proxy) {
+  proxy_signal_handler_ = g_signal_connect(
+      notification_proxy_, "g-signal", G_CALLBACK(GSignalReceiverThunk), this);
+}
 
-NotificationPlatformBridgeLinux::~NotificationPlatformBridgeLinux() {}
+NotificationPlatformBridgeLinux::~NotificationPlatformBridgeLinux() {
+  if (proxy_signal_handler_)
+    g_signal_handler_disconnect(notification_proxy_, proxy_signal_handler_);
+}
 
 void NotificationPlatformBridgeLinux::Display(
     NotificationCommon::Type notification_type,
@@ -102,16 +140,18 @@ void NotificationPlatformBridgeLinux::Display(
   NotificationData* data = FindNotificationData(notification_id, profile_id);
   if (data) {
     // Update an existing notification.
+    DCHECK_EQ(data->notification_type, notification_type);
+    DCHECK_EQ(data->is_incognito, is_incognito);

[Peter Beverloo, 2017/04/07 01:50:47]

I think it'd be possible for an extension to create a notification with the same
|notification_id| for a given |profile_id| if the extension is enabled in
Incognito as well. Maybe FND() should include the boolean in its matching now
that we store it?

[Tom (Use chromium acct), 2017/04/07 18:59:51]

Done.  This changes the Close() impl though since it doesn't take |is_incognito|

     if (data->dbus_id) {
       NotifyNow(data->dbus_id, notification_type, notification, nullptr,
                 nullptr, nullptr);
     } else {
-      data->update_type = notification_type;
       data->update_data = base::MakeUnique<Notification>(notification);
     }
   } else {
     // Send the notification for the first time.
-    data = new NotificationData(notification_id, profile_id, is_incognito);
+    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,
@@ -156,8 +196,8 @@ void NotificationPlatformBridgeLinux::NotifyCompleteInternal(gpointer user_data,
     CloseNow(data->dbus_id);
     notifications_.erase(data);
   } else if (data->update_data) {
-    NotifyNow(data->dbus_id, data->update_type, *data->update_data, nullptr,
-              nullptr, nullptr);
+    NotifyNow(data->dbus_id, data->notification_type, *data->update_data,
+              nullptr, nullptr, nullptr);
     data->update_data.reset();
   }
 }
@@ -170,11 +210,27 @@ void NotificationPlatformBridgeLinux::NotifyNow(
     GAsyncReadyCallback callback,
     gpointer user_data) {
   // TODO(thomasanderson): Add a complete implementation.
+
+  // Even-indexed elements in this array are action IDs passed back to
+  // us in GSignalReceiver.  Odd-indexed ones contain the button text.
+  GVariantBuilder actions_builder;
+  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.
+    g_variant_builder_add(&actions_builder, "s", "default");
+    g_variant_builder_add(&actions_builder, "s", "");
+  }
+  // Always add a settings button.
+  g_variant_builder_add(&actions_builder, "s", "settings");
+  g_variant_builder_add(&actions_builder, "s", "Settings");

[Peter Beverloo, 2017/04/07 01:50:48]

Cool!

[Peter Beverloo, 2017/04/07 01:50:48]

In the mid-term it might be interesting to split functionality like this out in
smaller helper functions to make them unit testable. Out of interest, can we
somehow fake being the receiver of the DBus calls?

[Tom (Use chromium acct), 2017/04/07 18:59:51]

Done.
Yes.  We would have to create a private D-Bus server using
g_dbus_server_new(chromium_address).

Then, we use g_dbus_connection_new_for_address(chromium_address) to connect and
use g_dbus_connection_register_object() to register
/org/freedesktop/Notifications and install the callbacks on it.  This connection
acts as the notification server.

In client code, connect to chromium_address in the same way.  Except now when we
create the GDBusProxy, we use g_dbus_proxy_new() and give it the connection. 
Now we just pass it along to the NotificationPlatformBridgeLinux constructor as
normal.

+
   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(), nullptr, nullptr, -1);
+                    message.c_str(), &actions_builder, nullptr, -1);
   g_dbus_proxy_call(notification_proxy_, "Notify", parameters,
                     G_DBUS_CALL_FLAGS_NONE, -1, cancellable, callback,
                     user_data);
@@ -200,3 +256,62 @@ NotificationPlatformBridgeLinux::FindNotificationData(
 
   return nullptr;
 }
+
+NotificationPlatformBridgeLinux::NotificationData*
+NotificationPlatformBridgeLinux::FindNotificationData(uint32_t dbus_id) {
+  for (const auto& pair : notifications_) {
+    NotificationData* data = pair.first;
+    if (data->dbus_id == dbus_id)
+      return data;
+  }
+
+  return nullptr;
+}
+
+void NotificationPlatformBridgeLinux::ForwardNotificationOperation(
+    uint32_t dbus_id,
+    NotificationCommon::Operation operation,
+    int action_index) {
+  NotificationData* data = FindNotificationData(dbus_id);
+  if (!data) {
+    // This notification either belongs to a different app or we

[Peter Beverloo, 2017/04/07 01:50:48]

We can get notified about changes to notifications shown by other apps??

[Tom (Use chromium acct), 2017/04/07 18:59:51]

Yes, the linux system is anarchy :(

+    // already removed the NotificationData after sending a
+    // "CloseNotification" message.
+    return;
+  }
+
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  DCHECK(profile_manager);
+
+  profile_manager->LoadProfile(
+      data->profile_id, data->is_incognito,
+      base::Bind(&ProfileLoadedCallback, operation, data->notification_type,
+                 data->origin_url.spec(), data->notification_id, action_index,
+                 base::NullableString16()));
+}
+
+void NotificationPlatformBridgeLinux::GSignalReceiver(GDBusProxy* proxy,
+                                                      const char* sender_name,
+                                                      const char* sender_signal,
+                                                      GVariant* parameters) {
+  uint32_t dbus_id = 0;
+  if (strcmp("NotificationClosed", sender_signal) == 0 &&
+      g_variant_is_of_type(parameters, G_VARIANT_TYPE("(uu)"))) {
+    uint32_t reason;
+    g_variant_get(parameters, "(uu)", &dbus_id, &reason);
+    ForwardNotificationOperation(dbus_id, NotificationCommon::CLOSE, -1);
+    notifications_.erase(FindNotificationData(dbus_id));

[Peter Beverloo, 2017/04/07 01:50:48]

Mm. I guess std::unordered_map_.erase(nullptr) is safe enough. It's a bit
implicit, maybe a comment would help?

[Tom (Use chromium acct), 2017/04/07 18:59:51]

Done.

+  } else if (strcmp("ActionInvoked", sender_signal) == 0 &&
+             g_variant_is_of_type(parameters, G_VARIANT_TYPE("(us)"))) {
+    const gchar* action;
+    g_variant_get(parameters, "(u&s)", &dbus_id, &action);

[Peter Beverloo, 2017/04/07 01:50:48]

Should we DCHECK(action) here, or is that me being pedantic due to my ignorance
about these APIs?

[Tom (Use chromium acct), 2017/04/07 18:59:51]

Done.

+
+    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!";
+    }
+  }
+}