Refactor NotificationPlatformBridgeLinux

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 68cfe87579c73cfbdc9e90b2019d4573550fd005..363531505629d279912f9ce634933c3f85c9f902 100644
--- a/chrome/browser/notifications/notification_platform_bridge_linux.cc
+++ b/chrome/browser/notifications/notification_platform_bridge_linux.cc
@@ -7,12 +7,12 @@
 #include <algorithm>
 
 #include "base/files/file_util.h"
-#include "base/memory/ptr_util.h"
-#include "base/stl_util.h"
+#include "base/run_loop.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/lock.h"
 #include "base/task_scheduler/post_task.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ -21,18 +21,21 @@
 #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/browser_thread.h"
 #include "content/public/browser/notification_service.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_proxy.h"
+#include "ui/gfx/image/image_skia.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);
+gfx::Image DeepCopyImage(const gfx::Image& image) {
+  std::unique_ptr<gfx::ImageSkia> image_skia(image.CopyImageSkia());
+  return gfx::Image(*image_skia);
 }
 
 int NotificationPriorityToFdoUrgency(int priority) {
@@ -50,29 +53,11 @@ int NotificationPriorityToFdoUrgency(int 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,
@@ -111,33 +96,20 @@ base::FilePath WriteDataToTmpFile(
   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::BindOnce(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)
-    return nullptr;
-  return new NotificationPlatformBridgeLinux(notification_proxy);
+  return new NotificationPlatformBridgeLinux();
 }
 
+struct NotificationPlatformBridgeLinux::ResourceFile {
+  explicit ResourceFile(const base::FilePath& file_path)
+      : file_path(file_path) {}
+  ~ResourceFile() { base::DeleteFile(file_path, false); }
+  const base::FilePath file_path;
+};
+
 struct NotificationPlatformBridgeLinux::NotificationData {
   NotificationData(NotificationCommon::Type notification_type,
                    const std::string& notification_id,
@@ -148,20 +120,7 @@ struct NotificationPlatformBridgeLinux::NotificationData {
         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();
-  }
+        origin_url(origin_url) {}
 
   // The ID used by the notification server.  Will be 0 until the
   // first "Notify" message completes.
@@ -187,45 +146,18 @@ struct NotificationPlatformBridgeLinux::NotificationData {
   // 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;
+  std::vector<std::unique_ptr<ResourceFile>> resource_files;
 };
 
-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() {
+  content::BrowserThread::PostTask(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::BindOnce(&NotificationPlatformBridgeLinux::Init,
+                     base::Unretained(this)));

[Lei Zhang, 2017/04/21 22:45:18]

Do we know for sure if |this| will still be valid when Init() get called?

[Tom (Use chromium acct), 2017/04/24 20:46:14]

Made NPBLI ref counted, so we can be sure it's still alive when Init() (and more
importantly CleanUp()) is called

 }
 
 NotificationPlatformBridgeLinux::~NotificationPlatformBridgeLinux() {
-  if (proxy_signal_handler_)
-    g_signal_handler_disconnect(notification_proxy_, proxy_signal_handler_);
+  CleanUp();

[Lei Zhang, 2017/04/21 22:45:17]

Since NotificationPlatformBridgeLinux hangs off of g_browser_process, does it
have time to actually clean up on shutdown?

[Tom (Use chromium acct), 2017/04/24 20:46:14]

No.  Changed this to DCHECK instead

 }
 
 void NotificationPlatformBridgeLinux::Display(
@@ -234,74 +166,49 @@ void NotificationPlatformBridgeLinux::Display(
     const std::string& profile_id,
     bool is_incognito,
     const Notification& notification) {

[Lei Zhang, 2017/04/21 22:45:18]

Can we check we are on the UI thread here? Ditto below. Really, try to
DCHECK_CURRENTLY_ON() in all the methods when a class lives on multiple threads.

[Tom (Use chromium acct), 2017/04/24 20:46:14]

Done.

-  NotificationData* data =
-      FindNotificationData(notification_id, profile_id, is_incognito);
-  if (data) {
-    // Update an existing notification.
-    if (data->dbus_id) {
-      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));
-    Notify(notification, data, NotifyCompleteReceiver, data);
-  }
+  auto notification_copy = base::MakeUnique<Notification>(notification);
+  notification_copy->set_icon(DeepCopyImage(notification_copy->icon()));
+  notification_copy->set_image(gfx::Image());
+  notification_copy->set_small_image(gfx::Image());
+  for (size_t i = 0; i < notification_copy->buttons().size(); i++)
+    notification_copy->SetButtonIcon(i, gfx::Image());
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::Bind(&NotificationPlatformBridgeLinux::DisplayNow,
+                 base::Unretained(this), notification_type, notification_id,

[Lei Zhang, 2017/04/21 22:45:17]

Similar question re: base::Unretained() usage. Ditto below.

[Tom (Use chromium acct), 2017/04/24 20:46:14]

Done.

+                 profile_id, is_incognito, base::Passed(&notification_copy)));
 }
 
 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;
-      }
-    }
-  }
-  for (NotificationData* data : to_erase)
-    notifications_.erase(data);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::Bind(&NotificationPlatformBridgeLinux::CloseNow,
+                 base::Unretained(this), profile_id, notification_id));
 }
 
 void NotificationPlatformBridgeLinux::GetDisplayed(
     const std::string& profile_id,
     bool incognito,
     const GetDisplayedNotificationsCallback& callback) const {
-  // TODO(thomasanderson): implement.
-  callback.Run(base::MakeUnique<std::set<std::string>>(), false);
+  content::BrowserThread::PostTask(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::Bind(&NotificationPlatformBridgeLinux::GetDisplayedNow,
+                 base::Unretained(this), profile_id, incognito, callback));
 }
 
-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::IsConnected(
+    base::OnceCallback<void(bool)> callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  base::AutoLock lock(connected_lock_);
+  if (connected_.has_value()) {
+    content::BrowserThread::PostTask(

[Lei Zhang, 2017/04/21 22:45:17]

If you PostTaskAndReply(WithResult) the you may be able to avoid locking.

[Tom (Use chromium acct), 2017/04/24 20:46:14]

Not sure I understand.  The lock protects connected_ which is used before the
PostTask

[Lei Zhang, 2017/04/25 18:55:57]

To be clear, I was thinking if you always accessed |connected_| on a single
thread, then you won't need locks.

[Tom (Use chromium acct), 2017/04/26 03:12:28]

Oh I see what you mean now.  Done.

+        content::BrowserThread::UI, FROM_HERE,
+        base::BindOnce(std::move(callback), connected_.value()));
+  } else {
+    on_connected_callbacks_.push_back(std::move(callback));
   }
 }
 
@@ -310,79 +217,120 @@ void NotificationPlatformBridgeLinux::Observe(
     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.
+  // The browser process is about to exit.  Post the CleanUp() task
+  // while we still can.
+  CleanUp();
+}
+
+void NotificationPlatformBridgeLinux::Init() {
+  dbus::Bus::Options bus_options;
+  bus_options.bus_type = dbus::Bus::SESSION;
+  bus_options.connection_type = dbus::Bus::PRIVATE;
+  bus_ = make_scoped_refptr(new dbus::Bus(bus_options));
+
+  notification_proxy_ =
+      bus_->GetObjectProxy(kFreedesktopNotificationsName,
+                           dbus::ObjectPath(kFreedesktopNotificationsPath));
+  OnConnected(notification_proxy_);

[Lei Zhang, 2017/04/21 22:45:17]

So if |notification_proxy_| is NULL, then we definitely want to call
OnConnected(false), but is this the right place to call OnConnected(true), or
should we wait until both ConnectToSignal() calls below succeeds as well?

[Tom (Use chromium acct), 2017/04/24 20:46:14]

Done.

+  if (!notification_proxy_)
+    return;
+
+  auto on_signal_connected = [](const std::string&, const std::string&,
+                                bool success) { DCHECK(success); };
+  notification_proxy_->ConnectToSignal(
+      kFreedesktopNotificationsName, "ActionInvoked",
+      base::Bind(&NotificationPlatformBridgeLinux::OnActionInvoked,
+                 base::Unretained(this)),
+      base::Bind(on_signal_connected));
+  notification_proxy_->ConnectToSignal(
+      kFreedesktopNotificationsName, "NotificationClosed",
+      base::Bind(&NotificationPlatformBridgeLinux::OnNotificationClosed,
+                 base::Unretained(this)),
+      base::Bind(on_signal_connected));
+}
+
+void NotificationPlatformBridgeLinux::CleanUp() {
+  content::BrowserThread::PostTask(
+      content::BrowserThread::FILE, FROM_HERE,
+      base::BindOnce(&NotificationPlatformBridgeLinux::CleanUpInternal,
+                     base::Unretained(this)));
+}
+
+void NotificationPlatformBridgeLinux::CleanUpInternal() {
+  bus_->ShutdownAndBlock();
+  bus_ = nullptr;
+  notification_proxy_ = nullptr;
   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()));
+void NotificationPlatformBridgeLinux::DisplayNow(
+    NotificationCommon::Type notification_type,
+    const std::string& notification_id,
+    const std::string& profile_id,
+    bool is_incognito,
+    std::unique_ptr<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 {
-    base::PostTaskWithTraitsAndReplyWithResult(
-        FROM_HERE,
-        base::TaskTraits()
-            .MayBlock()
-            .WithPriority(base::TaskPriority::USER_BLOCKING)
-            .WithShutdownBehavior(base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN),
-        base::Bind(
-            [](scoped_refptr<base::RefCountedMemory> icon) {
-              return base::MakeUnique<ResourceFiles>(WriteDataToTmpFile(icon));
-            },
-            icon_data),
-        base::Bind(&NotificationPlatformBridgeLinux::NotifyNow,
-                   weak_factory_.GetWeakPtr(), notification,
-                   data->weak_factory.GetWeakPtr(), callback, user_data));
+    // 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));
   }
-}
 
-void NotificationPlatformBridgeLinux::NotifyNow(
-    const Notification& notification,
-    base::WeakPtr<NotificationData> data,
-    GAsyncReadyCallback callback,
-    gpointer user_data,
-    std::unique_ptr<ResourceFiles> resource_files) {
-  if (!data)
-    return;
+  dbus::MethodCall method_call(kFreedesktopNotificationsName, "Notify");
+  dbus::MessageWriter writer(&method_call);
 
-  if (data->dbus_id)
-    DCHECK(!data->cancellable);
+  // app_name passed implicitly via desktop-entry.
+  writer.AppendString("");
 
-  data->ResetResourceFiles();
+  writer.AppendUint32(data->dbus_id);
 
-  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"));
+  // 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()) {
+  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 id = base::SizeTToString(data->action_end++);
     const std::string label = base::UTF16ToUTF8(button_info.title);
-    AddActionToNotification(&actions_builder, id.c_str(), label.c_str());
+    actions.push_back(id);
+    actions.push_back(label);
   }
-  if (notification.clickable()) {
+  if (notification->clickable()) {
     // Special case: the pair ("default", "") will not add a button,
     // but instead makes the entire notification clickable.
-    AddActionToNotification(&actions_builder, "default", "");
+    actions.push_back("default");
+    actions.push_back("");
   }
   // Always add a settings button.
-  AddActionToNotification(&actions_builder, "settings", "Settings");
+  actions.push_back("settings");
+  actions.push_back("Settings");
+  writer.AppendArrayOfStrings(actions);
 
-  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())));
+  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(
@@ -390,35 +338,72 @@ void NotificationPlatformBridgeLinux::NotifyNow(
   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()));
+  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);
 
-  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();
+  base::FilePath icon_file =
+      WriteDataToTmpFile(notification->icon().As1xPNGBytes());
+  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));
   }
 
-  const std::string title = base::UTF16ToUTF8(notification.title());
-  const std::string message = base::UTF16ToUTF8(notification.message());
+  writer.CloseContainer(&hints_writer);
 
-  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);
+  const int32_t kExpireTimeoutDefault = -1;
+  writer.AppendInt32(kExpireTimeoutDefault);
+
+  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);
+  }
+  if (!data->dbus_id) {
+    // There was some sort of error with creating the notification.
+    notifications_.erase(data);
+  }
+}
+
+void NotificationPlatformBridgeLinux::CloseNow(
+    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_->CallMethodAndBlock(
+          &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT);
+    }
+  }
+  for (NotificationData* data : to_erase)
+    notifications_.erase(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);
+void NotificationPlatformBridgeLinux::GetDisplayedNow(
+    const std::string& profile_id,
+    bool incognito,
+    const GetDisplayedNotificationsCallback& callback) const {
+  // TODO(thomasanderson): Implement.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(callback,
+                 base::Passed(base::MakeUnique<std::set<std::string>>()),
+                 false));
 }
 
 NotificationPlatformBridgeLinux::NotificationData*
@@ -449,62 +434,89 @@ NotificationPlatformBridgeLinux::FindNotificationData(uint32_t dbus_id) {
 }
 
 void NotificationPlatformBridgeLinux::ForwardNotificationOperation(
-    uint32_t dbus_id,
+    NotificationData* data,
     NotificationCommon::Operation operation,
     int action_index) {
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(
+          [](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()));
+          },
+          operation, data->notification_type, data->origin_url.spec(),
+          data->notification_id, action_index, data->profile_id,
+          data->is_incognito));
+}
+
+void NotificationPlatformBridgeLinux::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;
+
   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.
+  if (!data)
     return;
+
+  if (action == "default") {
+    ForwardNotificationOperation(data, NotificationCommon::CLICK, -1);
+  } else if (action == "settings") {
+    ForwardNotificationOperation(data, NotificationCommon::SETTINGS, -1);
+  } else {
+    size_t id;
+    if (!base::StringToSizeT(action, &id))
+      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(data, NotificationCommon::CLICK,
+                                 id_zero_based);
   }
+}
 
-  ProfileManager* profile_manager = g_browser_process->profile_manager();
-  DCHECK(profile_manager);
+void NotificationPlatformBridgeLinux::OnNotificationClosed(
+    dbus::Signal* signal) {
+  dbus::MessageReader reader(signal);
+  uint32_t dbus_id;
+  if (!reader.PopUint32(&dbus_id))
+    return;
 
-  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()));
-}
+  NotificationData* data = FindNotificationData(dbus_id);
+  if (!data)
+    return;
 
-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);
+  ForwardNotificationOperation(data, NotificationCommon::CLOSE, -1);
+  notifications_.erase(data);
+}
 
-    if (strcmp(action, "default") == 0) {
-      ForwardNotificationOperation(dbus_id, NotificationCommon::CLICK, -1);
-    } else if (strcmp(action, "settings") == 0) {
-      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 NotificationPlatformBridgeLinux::OnConnected(bool success) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);
+  {
+    base::AutoLock lock(connected_lock_);
+    connected_ = success;
+    for (auto& callback : on_connected_callbacks_) {
+      content::BrowserThread::PostTask(
+          content::BrowserThread::UI, FROM_HERE,
+          base::BindOnce(std::move(callback), success));
     }
+    on_connected_callbacks_.clear();
   }
+  if (!success)
+    CleanUp();

[Lei Zhang, 2017/04/21 22:45:17]

Can this be CleanUpInternal() since it's already on the FILE thread?

[Tom (Use chromium acct), 2017/04/24 20:46:14]

Done.

 }