Refactor NotificationPlatformBridgeLinux

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/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/task_scheduler/post_task.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/chrome_notification_types.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"
 #include "chrome/browser/shell_integration_linux.h"
-#include "content/public/browser/notification_service.h"
+#include "content/public/browser/browser_thread.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_proxy.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.
-    return;
-  }
-  auto* platform_bridge_linux = static_cast<NotificationPlatformBridgeLinux*>(
-      g_browser_process->notification_platform_bridge());
-  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,
                            NotificationCommon::Type notification_type,
                            const std::string& origin,
                            const std::string& notification_id,
                            int action_index,
                            const base::NullableString16& reply,
                            Profile* profile) {
   if (!profile)
@@ skipping 18 matching lines @@
   if (!base::CreateTemporaryFile(&file_path))
     return base::FilePath();
   if (base::WriteFile(file_path, data->front_as<char>(), data_len) !=
       data_len) {
     base::DeleteFile(file_path, false);
     return base::FilePath();
   }
   return file_path;
 }
 
-void DeleteNotificationResourceFile(const base::FilePath& file_path) {
-  if (file_path.empty())
-    return;
-  base::PostTaskWithTraits(
-      FROM_HERE,
-      base::TaskTraits()
-          .MayBlock()
-          .WithPriority(base::TaskPriority::BACKGROUND)
-          .WithShutdownBehavior(base::TaskShutdownBehavior::BLOCK_SHUTDOWN),
-      base::Bind(base::IgnoreResult(base::DeleteFile), file_path, false));
-}
-
 }  // 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)
+  auto npbl = base::MakeUnique<NotificationPlatformBridgeLinux>();
+  if (!npbl->BlockUntilReady())

[Peter Beverloo, 2017/04/18 18:09:28]

We pretty much have to assume that the NotificationDisplayService is created on
startup, which then creates the NotificationPlatformBridge. That means that this
call would block start-up performance.

That's super unfortunate, but we need to know whether the object proxy is
available synchronously :/. Without refactoring the rest of the notification
system to be asynchronous too (which will be a *huge* amount of work) I don't
see a way around this.

Could we check whether a proxy could be provided with the
g_dbus_proxy_new_for_bus_sync() API? Or does that also (synchronously) create a
thread under the hood?

[Lei Zhang, 2017/04/18 22:31:05]

Is it really created at start up, or at first use? Or is first use during
startup?
So what about having a NotificationDisplayServiceProxy to queue pending
notifications? Code Search says 3 methods with 8 callers. It's not that hard,
right? ;-)

[Peter Beverloo, 2017/04/18 22:42:06]

On start-up for Android, but we're moving in that direction for Mac too. It's a
requirement when we want the notifications to be able to outlive the browser
process.
Yeah, Tom mentioned this. We'd have to queue all actions until after
initialisation and execute them in order, but that's doable indeed. 

What is unfortunate is that we'd have two ways of ending up in the
MessageCenterDisplayService, and I'm wondering if we can avoid that duplication
by baking this in the NotificationDisplayService instead, but I haven't yet had
time to think this through properly.

I don't want to block you though. In either solution we'd need a queueing
mechanism to enable asynchronous initialisation, so it's a safe path to head
into :).

[Tom (Use chromium acct), 2017/04/18 23:07:43]

It doesn't create a thread but it does to synchronous IO.  fwiw creating a
thread takes a fraction of the time of IO

Also, this would open up the possibility of the thread creation or the 2nd dbus
connection failing for whatever reason, leaving us with a broken NPBL.

[Tom (Use chromium acct), 2017/04/18 23:07:43]

I'll try creating a followup patch that does this just to ground the discussion

     return nullptr;
-  return new NotificationPlatformBridgeLinux(notification_proxy);
+  return npbl.release();
 }
 
-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),
-        weak_factory(this) {}
-
-  ~NotificationData() {
-    if (cancellable)
-      g_cancellable_cancel(cancellable);
-    ResetResourceFiles();
-  }
-
-  void ResetResourceFiles() {
-    for (const base::FilePath& file : resource_files)
-      DeleteNotificationResourceFile(file);
-    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().
-  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 keep track of the IDs of the buttons currently displayed
-  // on this notification.  The valid range of action IDs is
-  // [action_start, action_end).
-  size_t action_start = 0;
-  size_t action_end = 0;
-
-  // Temporary resource files associated with the notification that
-  // should be cleaned up when the notification is closed or on
-  // shutdown.
-  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;
-
-  base::WeakPtrFactory<NotificationData> weak_factory;
-};
-
-struct NotificationPlatformBridgeLinux::ResourceFiles {
-  explicit ResourceFiles(const base::FilePath& icon_file)
-      : icon_file(icon_file) {}
-  ~ResourceFiles() { DeleteNotificationResourceFile(icon_file); }
-  base::FilePath icon_file;
-};
-
-NotificationPlatformBridgeLinux::NotificationPlatformBridgeLinux(
-    GDBusProxy* notification_proxy)
-    : notification_proxy_(notification_proxy), weak_factory_(this) {
-  proxy_signal_handler_ = g_signal_connect(
-      notification_proxy_, "g-signal", G_CALLBACK(GSignalReceiverThunk), this);
-  registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
-                 content::NotificationService::AllSources());
-}
-
-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.
+class NotificationPlatformBridgeLinux::NativeNotificationThread
+    : public base::Thread {
+ public:
+  NativeNotificationThread()
+      : base::Thread("FDO Notifications D-Bus thread"),
+        event_(base::WaitableEvent::ResetPolicy::MANUAL,
+               base::WaitableEvent::InitialState::NOT_SIGNALED) {
+    base::Thread::Options thread_options(base::MessageLoop::TYPE_IO, 0);
+    if (!StartWithOptions(thread_options))
+      event_.Signal();
+  }
+
+  ~NativeNotificationThread() override {
+    Stop();
+    // All of the resources should have been cleaned up on the
+    // notification thread.
+    DCHECK(notifications_.empty());
+  }
+
+  void 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.
+      data->notification_type = notification_type;
+      data->resource_files.clear();
+    } else {
+      // Send the notification for the first time.
+      data =
+          new NotificationData(notification_type, notification_id, profile_id,
+                               is_incognito, notification.origin_url());
+      notifications_.emplace(data, base::WrapUnique(data));
+    }
+
+    dbus::MethodCall method_call(kFreedesktopNotificationsName, "Notify");
+    dbus::MessageWriter writer(&method_call);
+
+    // app_name passed implicitly via desktop-entry.
+    writer.AppendString("");

[Peter Beverloo, 2017/04/18 18:09:28]

One thing to consider for testing is that it may make sense to have a
DBusNotificationBuilder of sorts that wraps this machinery. (At some point we
should start adding some for sure.)

[Tom (Use chromium acct), 2017/04/18 23:07:43]

Acknowledged.

+
+    writer.AppendUint32(data->dbus_id);
+
+    // app_icon passed implicitly via desktop-entry.
+    writer.AppendString("");
+
+    const std::string title = base::UTF16ToUTF8(notification.title());
+    writer.AppendString(title);
+
+    const std::string message = base::UTF16ToUTF8(notification.message());
+    writer.AppendString(message);
+
+    // Even-indexed elements in this vector are action IDs passed back to
+    // us in OnActionInvoked().  Odd-indexed ones contain the button text.
+    std::vector<std::string> actions;
+    data->action_start = data->action_end;
+    for (const auto& button_info : notification.buttons()) {
+      // FDO notification buttons can contain either an icon or a label,
+      // but not both, and the type of all buttons must be the same (all
+      // labels or all icons), so always use labels.
+      std::string id = base::SizeTToString(data->action_end++);

[Peter Beverloo, 2017/04/18 18:09:28]

micro nit: line is untouched, but consider marking this as const too. Or remove
const from line 164. The reader might now think that there's a difference
between these lines.

[Tom (Use chromium acct), 2017/04/18 23:07:43]

Done.

+      const std::string label = base::UTF16ToUTF8(button_info.title);
+      actions.push_back(id);
+      actions.push_back(label);
+    }
+    if (notification.clickable()) {
+      // Special case: the pair ("default", "") will not add a button,
+      // but instead makes the entire notification clickable.
+      actions.push_back("default");
+      actions.push_back("");
+    }
+    // Always add a settings button.
+    actions.push_back("settings");
+    actions.push_back("Settings");
+    writer.AppendArrayOfStrings(actions);
+
+    dbus::MessageWriter hints_writer(nullptr);
+    writer.OpenArray("{sv}", &hints_writer);
+    dbus::MessageWriter urgency_writer(nullptr);
+    hints_writer.OpenDictEntry(&urgency_writer);
+    urgency_writer.AppendString("urgency");
+    urgency_writer.AppendVariantOfUint32(
+        NotificationPriorityToFdoUrgency(notification.priority()));
+    hints_writer.CloseContainer(&urgency_writer);
+
+    std::unique_ptr<base::Environment> env = base::Environment::Create();
+    base::FilePath desktop_file(
+        shell_integration_linux::GetDesktopName(env.get()));
+    const char kDesktopFileSuffix[] = ".desktop";
+    DCHECK(base::EndsWith(desktop_file.value(), kDesktopFileSuffix,
+                          base::CompareCase::SENSITIVE));
+    desktop_file = desktop_file.RemoveFinalExtension();
+    dbus::MessageWriter desktop_entry_writer(nullptr);
+    hints_writer.OpenDictEntry(&desktop_entry_writer);
+    desktop_entry_writer.AppendString("desktop-entry");
+    desktop_entry_writer.AppendVariantOfString(desktop_file.value());
+    hints_writer.CloseContainer(&desktop_entry_writer);
+
+    base::FilePath icon_file =
+        WriteDataToTmpFile(notification.icon().As1xPNGBytes());

[Peter Beverloo, 2017/04/18 18:09:28]

Do we have to extract the underlying RefCountedMemory on the main thread? This
seems like the exact issue Robert commented on in
https://codereview.chromium.org/2806203003/.

[Tom (Use chromium acct), 2017/04/18 23:07:43]

I thought we didn't have to (see my previous comment about this), but it
couldn't hurt to do a deep copy as the images are pretty small.  done

+    if (!icon_file.empty()) {
+      dbus::MessageWriter image_path_writer(nullptr);
+      hints_writer.OpenDictEntry(&image_path_writer);
+      image_path_writer.AppendString("image-path");
+      image_path_writer.AppendVariantOfString(icon_file.value());
+      hints_writer.CloseContainer(&image_path_writer);
+      data->resource_files.push_back(base::MakeUnique<ResourceFile>(icon_file));
+    }
+
+    writer.CloseContainer(&hints_writer);
+
+    const int32_t kExpireTimeoutDefault = -1;
+    writer.AppendInt32(kExpireTimeoutDefault);
+
     if (data->dbus_id) {
-      data->notification_type = notification_type;
-      Notify(notification, data, nullptr, nullptr);
+      notification_proxy_->CallMethod(&method_call,
+                                      dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+                                      base::Bind([](dbus::Response*) {}));

[Peter Beverloo, 2017/04/18 18:09:28]

Is there any benefit in using async calls if we're on a worker thread already?
It seems like a potential footgun - what happens if (for whatever reason)
Close() has to become synchronous and is called while an asynchronous Display()
call is in progress - does the synchronous call flush any pending async calls
too?

[Tom (Use chromium acct), 2017/04/18 23:07:43]

Didn't think about that, but it's better safe than sorry.  done

     } 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));
-    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 &&
-        data->profile_id == profile_id) {
-      if (data->dbus_id) {
-        CloseNow(data->dbus_id);
-        to_erase.push_back(data);
-      } else {
-        data->should_close = true;
+      std::unique_ptr<dbus::Response> response =
+          notification_proxy_->CallMethodAndBlock(
+              &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT);
+      if (response) {
+        dbus::MessageReader reader(response.get());
+        reader.PopUint32(&data->dbus_id);
       }
-    }
-  }
-  for (NotificationData* data : to_erase)
-    notifications_.erase(data);
-}
-
-void NotificationPlatformBridgeLinux::GetDisplayed(
-    const std::string& profile_id,
-    bool incognito,
-    const DisplayedNotificationsCallback& callback) const {
-  callback.Run(base::MakeUnique<std::set<std::string>>(), false);
-}
-
-void NotificationPlatformBridgeLinux::NotifyCompleteInternal(gpointer user_data,
-                                                             GVariant* value) {
-  NotificationData* data = reinterpret_cast<NotificationData*>(user_data);
-  if (!base::ContainsKey(notifications_, data))
-    return;
-  data->cancellable.reset();
-  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) {
-    data->notification_type = data->update_notification_type;
-    Notify(*data->update_data, data, nullptr, nullptr);
-    data->update_data.reset();
-  }
-}
-
-void NotificationPlatformBridgeLinux::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  DCHECK_EQ(chrome::NOTIFICATION_APP_TERMINATING, type);
-  // The browser process is about to exit.  Clean up all notification
-  // resource files.
-  notifications_.clear();
-}
-
-void NotificationPlatformBridgeLinux::Notify(const Notification& notification,
-                                             NotificationData* data,
-                                             GAsyncReadyCallback callback,
-                                             gpointer user_data) {
-  const scoped_refptr<base::RefCountedMemory> icon_data =
-      notification.icon().As1xPNGBytes();
-  if (!icon_data->size()) {
-    NotifyNow(notification, data->weak_factory.GetWeakPtr(), callback,
-              user_data, base::MakeUnique<ResourceFiles>(base::FilePath()));
-  } else {
-    base::PostTaskWithTraitsAndReplyWithResult(
-        FROM_HERE,
-        base::TaskTraits()
-            .MayBlock()
-            .WithPriority(base::TaskPriority::USER_BLOCKING)
-            .WithShutdownBehavior(base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN),
+      if (!data->dbus_id) {
+        // There was some sort of error with creating the notification.
+        notifications_.erase(data);
+      }
+    }
+  }
+
+  void 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 &&
+          data->profile_id == profile_id) {
+        dbus::MethodCall method_call(kFreedesktopNotificationsName,
+                                     "CloseNotification");
+        dbus::MessageWriter writer(&method_call);
+        writer.AppendUint32(data->dbus_id);
+        notification_proxy_->CallMethod(&method_call,
+                                        dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+                                        base::Bind([](dbus::Response*) {}));
+      }
+    }
+    for (NotificationData* data : to_erase)
+      notifications_.erase(data);
+  }
+
+  void GetDisplayed(const std::string& profile_id,
+                    bool incognito,
+                    const DisplayedNotificationsCallback& callback) const {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI, FROM_HERE,
+        base::Bind(callback,
+                   base::Passed(base::MakeUnique<std::set<std::string>>()),
+                   false));
+  }
+
+  bool BlockUntilReady() {

[Peter Beverloo, 2017/04/18 18:09:28]

nit: document that it returns whether the proxy is available

[Tom (Use chromium acct), 2017/04/18 23:07:43]

Done.

+    event_.Wait();
+    return notification_proxy_initialized_.IsSet();
+  }
+
+ private:
+  struct ResourceFile {
+    explicit ResourceFile(const base::FilePath& file_path)
+        : file_path(file_path) {}
+    ~ResourceFile() { base::DeleteFile(file_path, false); }
+    const base::FilePath file_path;
+  };
+
+  struct 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) {}
+
+    // 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().
+    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 keep track of the IDs of the buttons currently displayed
+    // on this notification.  The valid range of action IDs is
+    // [action_start, action_end).
+    size_t action_start = 0;
+    size_t action_end = 0;
+
+    // Temporary resource files associated with the notification that
+    // should be cleaned up when the notification is closed or on
+    // shutdown.
+    std::vector<std::unique_ptr<ResourceFile>> resource_files;
+  };
+
+  void Init() override {
+    dbus::Bus::Options bus_options;
+    bus_options.bus_type = dbus::Bus::SESSION;
+    bus_options.connection_type = dbus::Bus::PRIVATE;
+    bus_ = new dbus::Bus(bus_options);

[Peter Beverloo, 2017/04/18 18:09:28]

nit: make_scoped_refptr(new dbus::Bus(bus_options))

That'd clarify the fact that it's a refptr at call-site.

[Tom (Use chromium acct), 2017/04/18 23:07:43]

Done.

+
+    notification_proxy_ =
+        bus_->GetObjectProxy(kFreedesktopNotificationsName,
+                             dbus::ObjectPath(kFreedesktopNotificationsPath));
+    if (notification_proxy_)
+      notification_proxy_initialized_.Set();
+    event_.Signal();
+    if (!notification_proxy_)
+      return;
+
+    auto on_signal_connected = [](const std::string&, const std::string&,

[Peter Beverloo, 2017/04/18 18:09:28]

question: suppose that we hit this DCHECK, what would we do about it? It's a
broken assumption in the freedesktop notification service that we're connected
to, so from that point of view it works for me.

[Tom (Use chromium acct), 2017/04/18 23:07:43]

I think the only way this could happen is if the notification server goes down
while Chrome is running.  I suppose if we had the proxy we could switch to using
Chrome notifications, but I don't think this is a common enough case to worry
about.  At least Chrome won't crash if this happens.

+                                  bool success) { DCHECK(success); };
+    notification_proxy_->ConnectToSignal(
+        kFreedesktopNotificationsName, "ActionInvoked",
+        base::Bind(&NativeNotificationThread::OnActionInvoked,
+                   base::Unretained(this)),
+        base::Bind(on_signal_connected));
+    notification_proxy_->ConnectToSignal(
+        kFreedesktopNotificationsName, "NotificationClosed",
+        base::Bind(&NativeNotificationThread::OnNotificationClosed,
+                   base::Unretained(this)),
+        base::Bind(on_signal_connected));
+  }
+
+  void CleanUp() override {
+    bus_->ShutdownAndBlock();
+    notifications_.clear();

[Peter Beverloo, 2017/04/18 18:09:27]

nit: consider setting |notification_proxy_| to a nullptr here, so make it more
explicit that line 348 closed it.

[Tom (Use chromium acct), 2017/04/18 23:07:43]

Done.

+  }
+
+  NotificationData* FindNotificationData(const std::string& notification_id,
+                                         const std::string& profile_id,
+                                         bool is_incognito) {
+    for (const auto& pair : notifications_) {
+      NotificationData* data = pair.first;
+      if (data->notification_id == notification_id &&
+          data->profile_id == profile_id &&
+          data->is_incognito == is_incognito) {
+        return data;
+      }
+    }
+
+    return nullptr;
+  }
+
+  NotificationData* 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 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
+      // already removed the NotificationData after sending a
+      // "CloseNotification" message.
+      return;
+    }
+
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI, FROM_HERE,
         base::Bind(
-            [](scoped_refptr<base::RefCountedMemory> icon) {
-              return base::MakeUnique<ResourceFiles>(WriteDataToTmpFile(icon));
+            [](NotificationCommon::Operation operation,
+               NotificationCommon::Type notification_type,
+               const std::string& origin, const std::string& notification_id,
+               int action_index, const std::string& profile_id,
+               bool is_incognito) {
+              ProfileManager* profile_manager =
+                  g_browser_process->profile_manager();
+              DCHECK(profile_manager);
+
+              profile_manager->LoadProfile(
+                  profile_id, is_incognito,
+                  base::Bind(&ProfileLoadedCallback, operation,
+                             notification_type, origin, notification_id,
+                             action_index, base::NullableString16()));
             },
-            icon_data),
-        base::Bind(&NotificationPlatformBridgeLinux::NotifyNow,
-                   weak_factory_.GetWeakPtr(), notification,
-                   data->weak_factory.GetWeakPtr(), callback, user_data));
-  }
-}
-
-void NotificationPlatformBridgeLinux::NotifyNow(
-    const Notification& notification,
-    base::WeakPtr<NotificationData> data,
-    GAsyncReadyCallback callback,
-    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"));
-  data->action_start = data->action_end;
-  for (const auto& button_info : notification.buttons()) {
-    // FDO notification buttons can contain either an icon or a label,
-    // but not both, and the type of all buttons must be the same (all
-    // labels or all icons), so always use labels.
-    std::string id = base::SizeTToString(data->action_end++);
-    const std::string label = base::UTF16ToUTF8(button_info.title);
-    AddActionToNotification(&actions_builder, id.c_str(), label.c_str());
-  }
-  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())));
-
-  std::unique_ptr<base::Environment> env = base::Environment::Create();
-  base::FilePath desktop_file(
-      shell_integration_linux::GetDesktopName(env.get()));
-  const char kDesktopFileSuffix[] = ".desktop";
-  DCHECK(base::EndsWith(desktop_file.value(), kDesktopFileSuffix,
-                        base::CompareCase::SENSITIVE));
-
-  desktop_file = desktop_file.RemoveFinalExtension();
-  g_variant_builder_add(&hints_builder, "{sv}", "desktop-entry",
-                        g_variant_new_string(desktop_file.value().c_str()));
-
-  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)", "" /* app_name passed implicitly via desktop-entry */,
-      data->dbus_id, "" /* app_icon passed implicitly via desktop-entry */,
-      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, 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*
-NotificationPlatformBridgeLinux::FindNotificationData(
-    const std::string& notification_id,
-    const std::string& profile_id,
-    bool is_incognito) {
-  for (const auto& pair : notifications_) {
-    NotificationData* data = pair.first;
-    if (data->notification_id == notification_id &&
-        data->profile_id == profile_id && data->is_incognito == is_incognito) {
-      return data;
-    }
-  }
-
-  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
-    // 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);
-    // std::unordered_map::erase(nullptr) is safe here.
-    notifications_.erase(FindNotificationData(dbus_id));
-  } else if (strcmp("ActionInvoked", sender_signal) == 0 &&
-             g_variant_is_of_type(parameters, G_VARIANT_TYPE("(us)"))) {
-    const gchar* action = nullptr;
-    g_variant_get(parameters, "(u&s)", &dbus_id, &action);
-    DCHECK(action);
-
-    if (strcmp(action, "default") == 0) {
+            operation, data->notification_type, data->origin_url.spec(),
+            data->notification_id, action_index, data->profile_id,
+            data->is_incognito));
+  }
+
+  void OnActionInvoked(dbus::Signal* signal) {
+    dbus::MessageReader reader(signal);
+    uint32_t dbus_id;
+    if (!reader.PopUint32(&dbus_id))
+      return;
+    std::string action;
+    if (!reader.PopString(&action))
+      return;
+
+    if (action == "default") {
       ForwardNotificationOperation(dbus_id, NotificationCommon::CLICK, -1);
-    } else if (strcmp(action, "settings") == 0) {
+    } else if (action == "settings") {
       ForwardNotificationOperation(dbus_id, NotificationCommon::SETTINGS, -1);
     } else {
       size_t id;
       if (!base::StringToSizeT(action, &id))
         return;
       NotificationData* data = FindNotificationData(dbus_id);
       if (!data)
         return;
       size_t n_buttons = data->action_end - data->action_start;
       size_t id_zero_based = id - data->action_start;
       if (id_zero_based >= n_buttons)
         return;
       ForwardNotificationOperation(dbus_id, NotificationCommon::CLICK,
                                    id_zero_based);
     }
   }
+
+  void OnNotificationClosed(dbus::Signal* signal) {
+    dbus::MessageReader reader(signal);
+    uint32_t dbus_id;
+    if (!reader.PopUint32(&dbus_id))
+      return;
+    ForwardNotificationOperation(dbus_id, NotificationCommon::CLOSE, -1);
+    // std::unordered_map::erase(nullptr) is safe here.
+    notifications_.erase(FindNotificationData(dbus_id));
+  }
+
+  scoped_refptr<dbus::Bus> bus_;
+
+  dbus::ObjectProxy* notification_proxy_ = nullptr;
+  // Event that will be signaled once |notification_proxy_| is done
+  // connecting, or has failed to connect.
+  base::WaitableEvent event_;
+
+  // Set if the |notification_proxy_| has initialized without error.
+  // Only valid once |event_| has been signaled.
+  base::AtomicFlag notification_proxy_initialized_;
+
+  // A std::set<std::unique_ptr<T>> doesn't work well because
+  // eg. std::set::erase(T) would require a std::unique_ptr<T>
+  // argument, so the data would get double-destructed.
+  template <typename T>
+  using UnorderedUniqueSet = std::unordered_map<T*, std::unique_ptr<T>>;
+
+  UnorderedUniqueSet<NotificationData> notifications_;
+};
+
+NotificationPlatformBridgeLinux::NotificationPlatformBridgeLinux()
+    : thread_(new NativeNotificationThread()) {}

[Peter Beverloo, 2017/04/18 18:09:28]

style nit: base::MakeUnique<NativeNotificationThread>()

[Tom (Use chromium acct), 2017/04/18 23:07:43]

Done.

+
+NotificationPlatformBridgeLinux::~NotificationPlatformBridgeLinux() {}

[Peter Beverloo, 2017/04/18 18:09:27]

style nit: {} -> "= default;"

[Tom (Use chromium acct), 2017/04/18 23:07:43]

Done.

+
+void NotificationPlatformBridgeLinux::Display(
+    NotificationCommon::Type notification_type,
+    const std::string& notification_id,
+    const std::string& profile_id,
+    bool is_incognito,
+    const Notification& notification) {
+  thread_->task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&NativeNotificationThread::Display,
+                 base::Unretained(thread_.get()), notification_type,
+                 notification_id, profile_id, is_incognito, notification));
 }
+
+void NotificationPlatformBridgeLinux::Close(
+    const std::string& profile_id,
+    const std::string& notification_id) {
+  thread_->task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&NativeNotificationThread::Close,
+                 base::Unretained(thread_.get()), profile_id, notification_id));
+}
+
+void NotificationPlatformBridgeLinux::GetDisplayed(
+    const std::string& profile_id,
+    bool incognito,
+    const DisplayedNotificationsCallback& callback) const {
+  thread_->task_runner()->PostTask(
+      FROM_HERE, base::Bind(&NativeNotificationThread::GetDisplayed,
+                            base::Unretained(thread_.get()), profile_id,
+                            incognito, callback));

[Peter Beverloo, 2017/04/18 18:09:28]

Will we be able to support this eventually, or not at all? Right now it's making
a thread-jump to the thread where we post back saying "no", which is a bit
unnecessary.

[Tom (Use chromium acct), 2017/04/18 23:07:43]

I've been putting off implementing this for a while and I'm not sure I
understand how it's meant to work.  My first impl involved iterating over
|notifications_| and filtering the ones that matched |profile_id| and
|incognito|.  But when I clicked on "Get displayed notifications" in your
notification generator, it listed ones from previous runs as well.

[Peter Beverloo, 2017/04/18 23:16:13]

Assuming the parameters of Display(), it should return a vector of the
|notification_id| values displayed for the {profile_id, incognito} profile that
are still being available to the user, either on-screen or in a notification
center.

+}
+
+bool NotificationPlatformBridgeLinux::BlockUntilReady() {
+  return thread_->BlockUntilReady();
+}