Remove NotificationCenter from AlertDispatcher

This refactors the macOS AlertDispatcher interface to no longer require
an instance of NSUserNotificationCenter to be passed in. We expect the
caller to query their local NotificationCenter instead and merge the
results. This allows us to use the same interface in a future CL for the
UNNotification API.

Bug: 1127306
Change-Id: I207eaf454ed86a7636725949269de5fee780167b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2640119Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Commit-Queue: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845609}

chrome/browser/notifications/alert_dispatcher_mac.h

@@ -7,7 +7,6 @@
 
 #import <Foundation/Foundation.h>
 
-#include "base/callback_forward.h"
 #include "chrome/browser/notifications/displayed_notifications_dispatch_callback.h"
 
 // Interface to communicate with the Alert XPC service.
@@ -27,7 +26,6 @@
 - (void)
 getDisplayedAlertsForProfileId:(NSString*)profileId
                      incognito:(BOOL)incognito
-            notificationCenter:(NSUserNotificationCenter*)notificationCenter
                       callback:(GetDisplayedNotificationsCallback)callback;
 @end
 

chrome/browser/notifications/notification_platform_bridge_mac.h

@@ -47,9 +47,6 @@ class NotificationPlatformBridgeMac : public NotificationPlatformBridge {
   void SetReadyCallback(NotificationBridgeReadyCallback callback) override;
   void DisplayServiceShutDown(Profile* profile) override;
 
-  // Returns if alerts are supported on this machine.
-  static bool SupportsAlerts();
-
  private:
   // Cocoa class that receives callbacks from the NSUserNotificationCenter.
   base::scoped_nsobject<NotificationCenterDelegate> delegate_;

chrome/browser/notifications/notification_platform_bridge_mac.mm

@@ -23,7 +23,6 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/system/sys_info.h"
 #include "chrome/browser/browser_features.h"
 #include "chrome/browser/notifications/notification_common.h"
 #include "chrome/browser/notifications/notification_display_service_impl.h"
@@ -77,27 +76,6 @@ void RecordXPCEvent(XPCConnectionEvent event) {
                             XPC_CONNECTION_EVENT_COUNT);
 }
 
-bool IsPersistentNotification(
-    const message_center::Notification& notification) {
-  if (!NotificationPlatformBridgeMac::SupportsAlerts())
-    return false;
-
-  return notification.never_timeout() ||
-         notification.type() == message_center::NOTIFICATION_TYPE_PROGRESS;
-}
-
-// Implements the version check to determine if alerts are supported. Do not
-// call this method directly as SysInfo::OperatingSystemVersionNumbers might be
-// an expensive call. Instead use NotificationPlatformBridgeMac::SupportsAlerts
-// which caches this value.
-bool SupportsAlertsImpl() {
-  int32_t major, minor, bugfix;
-  base::SysInfo::OperatingSystemVersionNumbers(&major, &minor, &bugfix);
-  // Allow alerts on all versions except 10.15.0, 10.15.1 & 10.15.2.
-  // See crbug.com/1007418 for details.
-  return major != 10 || minor != 15 || bugfix > 2;
-}
-
 }  // namespace
 
 // A Cocoa class that represents the delegate of NSUserNotificationCenter and
@@ -178,10 +156,10 @@ void NotificationPlatformBridgeMac::Display(
       notification.context_message().empty() &&
       notification_type != NotificationHandler::Type::EXTENSION;
 
-  bool is_persistent = IsPersistentNotification(notification);
+  bool is_alert = IsAlertNotificationMac(notification);
 
   [builder setSubTitle:base::SysUTF16ToNSString(CreateMacNotificationContext(
-                           is_persistent, notification, requires_attribution))];
+                           is_alert, notification, requires_attribution))];
 
   if (!notification.icon().IsEmpty()) {
     // TODO(crbug/1138176): Resize images by adding a transparent border so that
@@ -230,10 +208,9 @@ void NotificationPlatformBridgeMac::Display(
       setNotificationType:[NSNumber numberWithInteger:static_cast<NSInteger>(
                                                           notification_type)]];
 
-  // Send persistent notifications to the XPC service so they
-  // can be displayed as alerts. Chrome itself can only display
+  // Send alert notifications to the XPC service. Chrome itself can only display
   // banners.
-  if (IsPersistentNotification(notification)) {
+  if (is_alert) {
     NSDictionary* dict = [builder buildDictionary];
     [alert_dispatcher_ dispatchNotification:dict];
   } else {
@@ -275,11 +252,38 @@ void NotificationPlatformBridgeMac::Close(Profile* profile,
 void NotificationPlatformBridgeMac::GetDisplayed(
     Profile* profile,
     GetDisplayedNotificationsCallback callback) const {
+  NSString* profileId = base::SysUTF8ToNSString(GetProfileId(profile));
+  bool incognito = profile->IsOffTheRecord();
+  std::set<std::string> banners;
+
+  for (NSUserNotification* toast in
+       [notification_center_ deliveredNotifications]) {
+    NSString* toastProfileId = [toast.userInfo
+        objectForKey:notification_constants::kNotificationProfileId];
+    BOOL toastIncognito = [[toast.userInfo
+        objectForKey:notification_constants::kNotificationIncognito] boolValue];
+
+    if ([profileId isEqualToString:toastProfileId] &&
+        incognito == toastIncognito) {
+      banners.insert(base::SysNSStringToUTF8([toast.userInfo
+          objectForKey:notification_constants::kNotificationId]));
+    }
+  }
+
+  GetDisplayedNotificationsCallback alerts_callback = base::BindOnce(
+      [](GetDisplayedNotificationsCallback callback,
+         std::set<std::string> banners, std::set<std::string> alerts,
+         bool supports_synchronization) {
+        // Merge banner and alert notification ids.
+        banners.insert(alerts.begin(), alerts.end());
+        std::move(callback).Run(std::move(banners), supports_synchronization);
+      },
+      std::move(callback), std::move(banners));
+
   [alert_dispatcher_ getDisplayedAlertsForProfileId:base::SysUTF8ToNSString(
                                                         GetProfileId(profile))
                                           incognito:profile->IsOffTheRecord()
-                                 notificationCenter:notification_center_
-                                           callback:std::move(callback)];
+                                           callback:std::move(alerts_callback)];
 }
 
 void NotificationPlatformBridgeMac::SetReadyCallback(
@@ -289,13 +293,6 @@ void NotificationPlatformBridgeMac::SetReadyCallback(
 
 void NotificationPlatformBridgeMac::DisplayServiceShutDown(Profile* profile) {}
 
-// static
-bool NotificationPlatformBridgeMac::SupportsAlerts() {
-  // Cache result as SysInfo::OperatingSystemVersionNumbers might be expensive.
-  static bool supports_alerts = SupportsAlertsImpl();
-  return supports_alerts;
-}
-
 // /////////////////////////////////////////////////////////////////////////////
 @implementation NotificationCenterDelegate
 - (void)userNotificationCenter:(NSUserNotificationCenter*)center
@@ -402,7 +399,6 @@ bool NotificationPlatformBridgeMac::SupportsAlerts() {
 - (void)
 getDisplayedAlertsForProfileId:(NSString*)profileId
                      incognito:(BOOL)incognito
-            notificationCenter:(NSUserNotificationCenter*)notificationCenter
                       callback:(GetDisplayedNotificationsCallback)callback {
   // Create a copyable version of the OnceCallback because ObjectiveC blocks
   // copy all referenced variables via copy constructor.
@@ -410,20 +406,6 @@ getDisplayedAlertsForProfileId:(NSString*)profileId
   auto reply = ^(NSArray* alerts) {
     std::set<std::string> displayedNotifications;
 
-    for (NSUserNotification* toast in
-         [notificationCenter deliveredNotifications]) {
-      NSString* toastProfileId = [toast.userInfo
-          objectForKey:notification_constants::kNotificationProfileId];
-      BOOL incognitoNotification = [[toast.userInfo
-          objectForKey:notification_constants::kNotificationIncognito]
-          boolValue];
-      if ([toastProfileId isEqualToString:profileId] &&
-          incognito == incognitoNotification) {
-        displayedNotifications.insert(base::SysNSStringToUTF8([toast.userInfo
-            objectForKey:notification_constants::kNotificationId]));
-      }
-    }
-
     for (NSString* alert in alerts)
       displayedNotifications.insert(base::SysNSStringToUTF8(alert));
 

chrome/browser/notifications/notification_platform_bridge_mac_unittest.mm

@@ -14,6 +14,7 @@
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/notifications/notification_platform_bridge_mac.h"
+#include "chrome/browser/notifications/notification_platform_bridge_mac_utils.h"
 #include "chrome/browser/notifications/notification_test_util.h"
 #include "chrome/browser/notifications/stub_alert_dispatcher_mac.h"
 #include "chrome/browser/notifications/stub_notification_center_mac.h"
@@ -191,7 +192,7 @@ TEST_F(NotificationPlatformBridgeMacTest, TestDisplayOneButton) {
 }
 
 TEST_F(NotificationPlatformBridgeMacTest, TestDisplayProgress) {
-  if (!NotificationPlatformBridgeMac::SupportsAlerts())
+  if (!MacOSSupportsXPCAlerts())
     return;
 
   std::unique_ptr<Notification> notification =
@@ -271,7 +272,7 @@ TEST_F(NotificationPlatformBridgeMacTest, TestQuitRemovesNotifications) {
 }
 
 TEST_F(NotificationPlatformBridgeMacTest, TestDisplayAlert) {
-  if (!NotificationPlatformBridgeMac::SupportsAlerts())
+  if (!MacOSSupportsXPCAlerts())
     return;
 
   std::unique_ptr<Notification> alert =
@@ -286,7 +287,7 @@ TEST_F(NotificationPlatformBridgeMacTest, TestDisplayAlert) {
 }
 
 TEST_F(NotificationPlatformBridgeMacTest, TestDisplayBannerAndAlert) {
-  if (!NotificationPlatformBridgeMac::SupportsAlerts())
+  if (!MacOSSupportsXPCAlerts())
     return;
 
   std::unique_ptr<Notification> alert =
@@ -305,7 +306,7 @@ TEST_F(NotificationPlatformBridgeMacTest, TestDisplayBannerAndAlert) {
 }
 
 TEST_F(NotificationPlatformBridgeMacTest, TestCloseAlert) {
-  if (!NotificationPlatformBridgeMac::SupportsAlerts())
+  if (!MacOSSupportsXPCAlerts())
     return;
 
   std::unique_ptr<Notification> alert =
@@ -323,7 +324,7 @@ TEST_F(NotificationPlatformBridgeMacTest, TestCloseAlert) {
 }
 
 TEST_F(NotificationPlatformBridgeMacTest, TestQuitRemovesBannersAndAlerts) {
-  if (!NotificationPlatformBridgeMac::SupportsAlerts())
+  if (!MacOSSupportsXPCAlerts())
     return;
 
   std::unique_ptr<Notification> notification = CreateBanner(

chrome/browser/notifications/notification_platform_bridge_mac_unnotification.mm

@@ -82,6 +82,7 @@ NotificationPlatformBridgeMacUNNotification::
     ~NotificationPlatformBridgeMacUNNotification() {
   [notification_center_ setDelegate:nil];
   [notification_center_ removeAllDeliveredNotifications];
+  // TODO(crbug/1134539): Close alerts.
 }
 
 void NotificationPlatformBridgeMacUNNotification::Display(
@@ -102,6 +103,8 @@ void NotificationPlatformBridgeMacUNNotification::Display(
           : (notification.items().at(0).title + base::UTF8ToUTF16(" - ") +
              notification.items().at(0).message);
 
+  bool is_alert = IsAlertNotificationMac(notification);
+
   bool requires_attribution =
       notification.context_message().empty() &&
       notification_type != NotificationHandler::Type::EXTENSION;
@@ -110,8 +113,7 @@ void NotificationPlatformBridgeMacUNNotification::Display(
                         CreateMacNotificationTitle(notification))];
   [builder setContextMessage:base::SysUTF16ToNSString(context_message)];
   [builder setSubTitle:base::SysUTF16ToNSString(CreateMacNotificationContext(
-                           /*is_persistent=*/false, notification,
-                           requires_attribution))];
+                           is_alert, notification, requires_attribution))];
 
   if (!notification.icon().IsEmpty()) {
     // TODO(crbug/1138176): Resize images by adding a transparent border so that
@@ -142,11 +144,16 @@ void NotificationPlatformBridgeMacUNNotification::Display(
   [builder setIncognito:profile->IsOffTheRecord()];
   [builder setCreatorPid:[NSNumber numberWithInteger:static_cast<NSInteger>(
                                                          getpid())]];
-
   [builder
       setNotificationType:[NSNumber numberWithInteger:static_cast<NSInteger>(
                                                           notification_type)]];
 
+  if (is_alert) {
+    // TODO(crbug.com/1134539): Send out an alert.
+    NOTIMPLEMENTED();
+  }
+
+  // Create a new category from the desired action buttons.
   UNNotificationCategory* category = [builder buildCategory];
 
   // Check if this notification had an already existing category from a previous

chrome/browser/notifications/notification_platform_bridge_mac_utils.h

@@ -32,4 +32,10 @@ bool VerifyMacNotificationData(NSDictionary* response) WARN_UNUSED_RESULT;
 // (click close, etc.).
 void ProcessMacNotificationResponse(NSDictionary* response);
 
+// Returns if alerts via XPC are supported on this machine.
+bool MacOSSupportsXPCAlerts();
+
+// Returns if the given |notification| should be shown as an alert.
+bool IsAlertNotificationMac(const message_center::Notification& notification);
+
 #endif  // CHROME_BROWSER_NOTIFICATIONS_NOTIFICATION_PLATFORM_BRIDGE_MAC_UTILS_H_

chrome/browser/notifications/notification_platform_bridge_mac_utils.mm

@@ -4,10 +4,13 @@
 
 #include "chrome/browser/notifications/notification_platform_bridge_mac_utils.h"
 
+#include "base/feature_list.h"
 #include "base/i18n/number_formatting.h"
 #include "base/optional.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/system/sys_info.h"
+#include "chrome/browser/browser_features.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/notifications/notification_display_service_impl.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -52,6 +55,17 @@ void DoProcessMacNotificationResponse(
                  byUser));
 }
 
+// Implements the version check to determine if alerts are supported. Do not
+// call this method directly as SysInfo::OperatingSystemVersionNumbers might be
+// an expensive call. Instead use SupportsAlerts which caches this value.
+bool MacOSSupportsXPCAlertsImpl() {
+  int32_t major, minor, bugfix;
+  base::SysInfo::OperatingSystemVersionNumbers(&major, &minor, &bugfix);
+  // Allow alerts on all versions except 10.15.0, 10.15.1 & 10.15.2.
+  // See crbug.com/1007418 for details.
+  return major != 10 || minor != 15 || bugfix > 2;
+}
+
 }  // namespace
 
 base::string16 CreateMacNotificationTitle(
@@ -222,3 +236,23 @@ void ProcessMacNotificationResponse(NSDictionary* response) {
                      GURL(notificationOrigin), notificationId, actionIndex,
                      base::nullopt /* reply */, true /* byUser */));
 }
+
+bool MacOSSupportsXPCAlerts() {
+  // Cache result as SysInfo::OperatingSystemVersionNumbers might be expensive.
+  static bool supportsAlerts = MacOSSupportsXPCAlertsImpl();
+  return supportsAlerts;
+}
+
+bool IsAlertNotificationMac(const message_center::Notification& notification) {
+  // TODO(crbug/1134539): Support alerts via UNNotification API.
+  if (base::FeatureList::IsEnabled(features::kNewMacNotificationAPI))
+    return false;
+
+  // We show alerts via an XPC service, check if that's possible.
+  if (!MacOSSupportsXPCAlerts())
+    return false;
+
+  // Check if the |notification| should be shown as alert.
+  return notification.never_timeout() ||
+         notification.type() == message_center::NOTIFICATION_TYPE_PROGRESS;
+}

chrome/browser/notifications/stub_alert_dispatcher_mac.h

@@ -7,28 +7,10 @@
 
 #import <Foundation/Foundation.h>
 
-#include <string>
-
 #include "chrome/browser/notifications/alert_dispatcher_mac.h"
 
 @interface StubAlertDispatcher : NSObject<AlertDispatcher>
-
-- (void)dispatchNotification:(NSDictionary*)data;
-
-- (void)closeNotificationWithId:(NSString*)notificationId
-                  withProfileId:(NSString*)profileId;
-
-- (void)closeAllNotifications;
-
-- (void)
-getDisplayedAlertsForProfileId:(NSString*)profileId
-                     incognito:(BOOL)incognito
-            notificationCenter:(NSUserNotificationCenter*)notificationCenter
-                      callback:(GetDisplayedNotificationsCallback)callback;
-
-// Stub specific methods.
 - (NSArray*)alerts;
-
 @end
 
 #endif  // CHROME_BROWSER_NOTIFICATIONS_STUB_ALERT_DISPATCHER_MAC_H_

chrome/browser/notifications/stub_alert_dispatcher_mac.mm

@@ -52,21 +52,24 @@
 - (void)
 getDisplayedAlertsForProfileId:(NSString*)profileId
                      incognito:(BOOL)incognito
-            notificationCenter:(NSUserNotificationCenter*)notificationCenter
                       callback:(GetDisplayedNotificationsCallback)callback {
-  std::set<std::string> displayedNotifications;
-  for (NSUserNotification* toast in
-       [notificationCenter deliveredNotifications]) {
-    NSString* toastProfileId = [toast.userInfo
-        objectForKey:notification_constants::kNotificationProfileId];
-    if ([toastProfileId isEqualToString:profileId]) {
-      displayedNotifications.insert(base::SysNSStringToUTF8([toast.userInfo
-          objectForKey:notification_constants::kNotificationId]));
+  std::set<std::string> alerts;
+
+  for (NSDictionary* toast in _alerts.get()) {
+    NSString* toastProfileId =
+        [toast objectForKey:notification_constants::kNotificationProfileId];
+    BOOL toastIncognito = [[toast
+        objectForKey:notification_constants::kNotificationIncognito] boolValue];
+
+    if ([profileId isEqualToString:toastProfileId] &&
+        incognito == toastIncognito) {
+      alerts.insert(base::SysNSStringToUTF8(
+          [toast objectForKey:notification_constants::kNotificationId]));
     }
   }
 
-  std::move(callback).Run(std::move(displayedNotifications),
-                          true /* supports_synchronization */);
+  std::move(callback).Run(std::move(alerts),
+                          /*supports_synchronization=*/true);
 }
 
 - (NSArray*)alerts {