Add NotificationDisplayServiceProxy

chrome/browser/notifications/notification_display_service_proxy.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_display_service_proxy.h"
+
+#include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/notifications/message_center_display_service.h"
+#include "chrome/browser/notifications/native_notification_display_service.h"
+#include "chrome/browser/notifications/notification.h"
+#include "chrome/browser/notifications/notification_platform_bridge_linux.h"
+
+NotificationDisplayServiceProxy::NotificationDisplayServiceProxy(
+    Profile* profile)
+    : profile_(profile) {
+  // TODO(thomasanderson): Add interface to avoid this cast?
+  auto* npbl = static_cast<NotificationPlatformBridgeLinux*>(

[Peter Beverloo, 2017/04/19 13:26:41]

Here and elsewhere (impls, nds, etc) - Chromium style has a reasonably strong
dislike towards using acronyms for identifiers. Maybe substitute like:

npbl -> platform_bridge_linux
impls -> implementations
nds -> display_service

[Tom (Use chromium acct), 2017/04/20 01:43:46]

Done.

+      g_browser_process->notification_platform_bridge());
+  npbl->IsConnected(base::Bind(
+      &NotificationDisplayServiceProxy::OnNotificationPlatformBridgeInitialized,
+      base::Unretained(this)));

[Peter Beverloo, 2017/04/19 13:26:41]

There's two race conditions here:

  (1) We can hit the DCHECK on notification_platform_bridge_linux.cc:269 when
multiple profiles are being initialised at once.
  (2) Super unlikely, but |this| may not outlive the |npbl|. Use a WeakPtr?

[Tom (Use chromium acct), 2017/04/20 01:43:46]

Good catch, fixed!
Done.

+}
+
+NotificationDisplayServiceProxy::~NotificationDisplayServiceProxy() = default;
+
+NotificationDisplayService*
+NotificationDisplayServiceProxy::GetNotificationDisplayService() const {
+  // Can't have both impls at once.
+  DCHECK(!native_notification_display_service_ ||
+         !message_center_display_service_);
+
+  if (native_notification_display_service_) {
+    return native_notification_display_service_.get();
+  } else if (message_center_display_service_) {
+    return message_center_display_service_.get();
+  }
+
+  return nullptr;
+}
+
+void NotificationDisplayServiceProxy::OnNotificationPlatformBridgeInitialized(
+    bool success) {
+  if (success) {
+    native_notification_display_service_ =
+        base::MakeUnique<NativeNotificationDisplayService>(
+            profile_, g_browser_process->notification_platform_bridge());
+  } else {
+    // TODO(thomasanderson): clean up |g_browser_process->notification_bridge_|?

[Peter Beverloo, 2017/04/19 13:26:41]

Can the NPBL just stop its own thread when it identifies that the object proxy
cannot be obtained?

[Tom (Use chromium acct), 2017/04/20 01:43:46]

Done.

+    message_center_display_service_ =
+        base::MakeUnique<MessageCenterDisplayService>(
+            profile_, g_browser_process->notification_ui_manager());
+  }
+
+  NotificationDisplayService* nds = GetNotificationDisplayService();
+  while (!actions_.empty()) {
+    actions_.front().Run(nds);
+    actions_.pop();
+  }
+}
+
+void NotificationDisplayServiceProxy::RunAction(
+    base::Callback<void(NotificationDisplayService*)> action) {
+  NotificationDisplayService* nds = GetNotificationDisplayService();
+  if (nds)
+    action.Run(nds);
+  else
+    actions_.push(action);
+}
+
+void NotificationDisplayServiceProxy::Display(
+    NotificationCommon::Type notification_type,
+    const std::string& notification_id,
+    const Notification& notification) {
+  RunAction(base::Bind(
+      [](NotificationCommon::Type notification_type,
+         const std::string& notification_id, const Notification& notification,
+         NotificationDisplayService* nds) {
+        nds->Display(notification_type, notification_id, notification);
+      },
+      notification_type, notification_id, notification));

[Peter Beverloo, 2017/04/19 13:26:41]

Instead of this pattern, consider something like:

=========================
if (initialized) {
  GetNotificationDisplayService()->Display(..);
  return;
}

QueueTask(base::Bind(&NotificationDisplayServiceProxy::Display,
                     base::Unretained(this), ...));
=========================

That'll avoid having to copy all the arguments in the common case, as well as
the closures. See e.g. the GCMDelayedTaskController for an application of this.

[Tom (Use chromium acct), 2017/04/20 01:43:46]

Done.

+}
+
+void NotificationDisplayServiceProxy::Close(
+    NotificationCommon::Type notification_type,
+    const std::string& notification_id) {
+  RunAction(base::Bind(
+      [](NotificationCommon::Type notification_type,
+         const std::string& notification_id, NotificationDisplayService* nds) {
+        nds->Close(notification_type, notification_id);
+      },
+      notification_type, notification_id));
+}
+
+void NotificationDisplayServiceProxy::GetDisplayed(
+    const DisplayedNotificationsCallback& callback) {
+  RunAction(base::Bind(
+      [](const DisplayedNotificationsCallback& callback,
+         NotificationDisplayService* nds) { nds->GetDisplayed(callback); },
+      callback));
+}