Linux native notifications: Support closing and updating notifications

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 6a0f8974c804f7e3ef7e9dbbef6546c7fc14068c..0413a8808a434b4afad0e42c3736cc3d35d2bffa 100644
--- a/chrome/browser/notifications/notification_platform_bridge_linux.cc
+++ b/chrome/browser/notifications/notification_platform_bridge_linux.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/notifications/notification_platform_bridge_linux.h"
 
+#include <algorithm>
+
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/notifications/notification.h"
@@ -29,13 +31,42 @@ NotificationPlatformBridge* NotificationPlatformBridge::Create() {
   return new NotificationPlatformBridgeLinux(notification_proxy);
 }
 
+NotificationPlatformBridgeLinux::NotificationData::NotificationData(
+    const std::string& notification_id,
+    const std::string& profile_id,
+    bool is_incognito,
+    NotificationPlatformBridgeLinux* platform_bridge)
+    : notification_id(notification_id),
+      profile_id(profile_id),
+      is_incognito(is_incognito),
+      platform_bridge(platform_bridge) {}
+
+NotificationPlatformBridgeLinux::NotificationData::~NotificationData() {
+  if (cancellable)
+    g_cancellable_cancel(cancellable);
+}
+
+void OnNotifyComplete(GObject* source_object,

[Peter Beverloo, 2017/04/06 17:56:57]

nit: maybe move to the anonymous namespace around line 17? OnNotifyComplete is
the sort of generic name that could cause multiple symbol definitions when added
to the global scope.

[Tom (Use chromium acct), 2017/04/06 18:25:38]

Done.

+                      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.
+    return;
+  }
+  auto* data =
+      reinterpret_cast<NotificationPlatformBridgeLinux::NotificationData*>(
+          user_data);

[Peter Beverloo, 2017/04/06 17:56:57]

We're putting quite some trust here in the data that the libgio is giving back
to us.

Since we can get the platform bridge from the global scope, you could simplify
this by calling:

g_browser_process->notification_platform_bridge()->OnNotifyComplete(user_data,
value)

Then cast user_data to NotificationData* in there, and verify that it's actually
part of the UnorderedUniqueSet map.

That feels a lot safer and drops the pointer to the NPBridgeLinux from each of
the NotificationData instances. WDYT?

[Tom (Use chromium acct), 2017/04/06 18:25:37]

Done.

+  data->platform_bridge->OnNotifyComplete(data, value);
+}
+
 NotificationPlatformBridgeLinux::NotificationPlatformBridgeLinux(
     GDBusProxy* notification_proxy)
     : notification_proxy_(notification_proxy) {}
 
-NotificationPlatformBridgeLinux::~NotificationPlatformBridgeLinux() {
-  g_object_unref(notification_proxy_);
-}
+NotificationPlatformBridgeLinux::~NotificationPlatformBridgeLinux() {}
 
 void NotificationPlatformBridgeLinux::Display(
     NotificationCommon::Type notification_type,
@@ -43,20 +74,39 @@ void NotificationPlatformBridgeLinux::Display(
     const std::string& profile_id,
     bool is_incognito,
     const Notification& notification) {
-  // TODO(thomasanderson): Add a complete implementation.
-  g_dbus_proxy_call(
-      notification_proxy_, "Notify",
-      g_variant_new("(susssasa{sv}i)", "", 0, "",
-                    base::UTF16ToUTF8(notification.title()).c_str(),
-                    base::UTF16ToUTF8(notification.message()).c_str(), nullptr,
-                    nullptr, -1),
-      G_DBUS_CALL_FLAGS_NONE, -1, nullptr, nullptr, nullptr);
+  NotificationData* data = FindNotificationData(notification_id, profile_id);
+  if (data) {
+    // Update an existing notification.
+    if (data->dbus_id) {
+      NotifyNow(data->dbus_id, notification_type, notification, nullptr,
+                nullptr, nullptr);
+    } else {
+      data->update_type = notification_type;
+      data->update_data.reset(new Notification(notification));

[Peter Beverloo, 2017/04/06 17:56:57]

nit (from base/memory/ptr_util.h):

data->update_data = base::MakeUnique<Notification>(notification);

[Tom (Use chromium acct), 2017/04/06 18:25:37]

Done.

+    }
+  } else {
+    // Send the notification for the first time.
+    data =
+        new NotificationData(notification_id, profile_id, is_incognito, this);
+    data->cancellable.reset(g_cancellable_new());
+    notifications_.emplace(data, std::unique_ptr<NotificationData>(data));

[Peter Beverloo, 2017/04/06 17:56:57]

base::WrapUnique(data)

[Tom (Use chromium acct), 2017/04/06 18:25:37]

Done.

+    NotifyNow(0, notification_type, notification, data->cancellable,
+              ::OnNotifyComplete, data);

[Peter Beverloo, 2017/04/06 17:56:57]

It's rather confusing that we have two OnNotifyComplete methods in this file.
Maybe rename the global scope one to NotifyCompleteReceiver?

[Tom (Use chromium acct), 2017/04/06 18:25:37]

Done.

+  }
 }
 
 void NotificationPlatformBridgeLinux::Close(
     const std::string& profile_id,
     const std::string& notification_id) {
-  NOTIMPLEMENTED();
+  NotificationData* data = FindNotificationData(notification_id, profile_id);
+  if (!data)
+    return;
+  if (data->dbus_id) {
+    CloseNow(data->dbus_id);
+    notifications_.erase(data);
+  } else {
+    data->should_close = true;
+  }
 }
 
 void NotificationPlatformBridgeLinux::GetDisplayed(
@@ -65,3 +115,62 @@ void NotificationPlatformBridgeLinux::GetDisplayed(
     const DisplayedNotificationsCallback& callback) const {
   callback.Run(base::MakeUnique<std::set<std::string>>(), false);
 }
+
+void NotificationPlatformBridgeLinux::OnNotifyComplete(NotificationData* data,
+                                                       GVariant* value) {
+  data->cancellable.reset(nullptr);

[Peter Beverloo, 2017/04/06 17:56:57]

nit: no need for `nullptr`

[Tom (Use chromium acct), 2017/04/06 18:25:38]

Done.

[Lei Zhang, 2017/04/06 19:59:33]

Well, only with the newest ScopedGObject.

+  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->update_type, *data->update_data, nullptr,
+              nullptr, nullptr);
+    data->update_data.reset();
+  }
+}
+
+void NotificationPlatformBridgeLinux::NotifyNow(
+    uint32_t dbus_id,
+    NotificationCommon::Type notification_type,
+    const Notification& notification,
+    GCancellable* cancellable,
+    GAsyncReadyCallback callback,
+    gpointer user_data) {
+  // TODO(thomasanderson): Add a complete implementation.
+  GVariant* parameters = g_variant_new(
+      "(susssasa{sv}i)", "", dbus_id, "",
+      base::UTF16ToUTF8(notification.title()).c_str(),
+      base::UTF16ToUTF8(notification.message()).c_str(), nullptr, nullptr, -1);

[Peter Beverloo, 2017/04/06 17:56:57]

I know that you're just moving this, but are these safe?

The std::string temporary variable to hold the UTF8 version of
notification.title() might end up being destroyed before the call is made,
because we're not holding on to a reference and rather only use the c_str().

Maybe extract to locals to be safe?

const std::string title = base::UTF16ToUTF8(notification.title());
--> title.c_str() (in the call)

[Tom (Use chromium acct), 2017/04/06 18:25:37]

Done.

+  g_dbus_proxy_call(notification_proxy_, "Notify", parameters,
+                    G_DBUS_CALL_FLAGS_NONE, -1, 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*
+NotificationPlatformBridgeLinux::FindNotificationData(
+    const std::string& notification_id,
+    const std::string& profile_id) {
+  auto it = std::find_if(
+      notifications_.begin(), notifications_.end(),
+      [&](std::pair<NotificationData* const, std::unique_ptr<NotificationData>>&
+              data) {
+        return data.first->notification_id == notification_id &&
+               data.first->profile_id == profile_id;
+
+      });

[Peter Beverloo, 2017/04/06 17:56:57]

nit: while I'm a fan of STL, we're tied to a lookup performance of O(n) here
anyway, so consider simplifying as:

=========================
for (const auto& pair : notifications_) {
  NotificationData* data = pair.first;
  if (data->notification_id == notification_id &&
      data->profile_id == profile_id) {
    return data;
  }
}

return nullptr;
=========================

[Tom (Use chromium acct), 2017/04/06 18:25:38]

Done.

+  if (it == notifications_.end())
+    return nullptr;
+  return it->first;
+}