Initial client side implementation of the XPC service

chrome/browser/notifications/notification_platform_bridge_mac.mm

Index: chrome/browser/notifications/notification_platform_bridge_mac.mm
diff --git a/chrome/browser/notifications/notification_platform_bridge_mac.mm b/chrome/browser/notifications/notification_platform_bridge_mac.mm
index 08fefce8bcc7e130114c3ca7f3e03986004b5f58..273a5f3a14ee411709f6dd57cca3a72723e89d00 100644
--- a/chrome/browser/notifications/notification_platform_bridge_mac.mm
+++ b/chrome/browser/notifications/notification_platform_bridge_mac.mm
@@ -6,6 +6,10 @@
 
 #include <utility>
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/command_line.h"
+#include "base/mac/bundle_locations.h"
 #include "base/mac/foundation_util.h"
 #include "base/mac/mac_util.h"
 #include "base/strings/string_number_conversions.h"
@@ -20,10 +24,14 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/cocoa/notifications/notification_builder_mac.h"
+#import "chrome/browser/ui/cocoa/notifications/notification_delivery.h"
 #include "chrome/browser/ui/cocoa/notifications/notification_constants_mac.h"
 #import "chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/grit/generated_resources.h"
 #include "components/url_formatter/elide_url.h"
 #include "third_party/WebKit/public/platform/modules/notifications/WebNotificationConstants.h"
+#include "ui/base/l10n/l10n_util_mac.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
@@ -71,6 +79,23 @@ void ProfileLoadedCallback(NotificationCommon::Operation operation,
                                      notification_id, action_index);
 }
 
+// Loads the profile and process the Notification response
+void DoProcessNotificationResponse(NotificationCommon::Operation operation,
+                                   NotificationCommon::Type type,
+                                   const std::string& profile_id,
+                                   bool incognito,
+                                   const std::string& origin,
+                                   const std::string& notification_id,
+                                   int32_t button_index) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  ProfileManager* profileManager = g_browser_process->profile_manager();
+  DCHECK(profileManager);
+
+  profileManager->LoadProfile(
+      profile_id, incognito, base::Bind(&ProfileLoadedCallback, operation, type,
+                                        origin, notification_id, button_index));
+}
+
 }  // namespace
 
 // static
@@ -91,7 +116,9 @@ NotificationPlatformBridge* NotificationPlatformBridge::Create() {
 NotificationPlatformBridgeMac::NotificationPlatformBridgeMac(
     NSUserNotificationCenter* notification_center)
     : delegate_([NotificationCenterDelegate alloc]),
-      notification_center_(notification_center) {
+      notification_center_(notification_center),
+      notification_remote_dispatcher_(
+          [[NotificationRemoteDispatcher alloc] init]) {
   [notification_center_ setDelegate:delegate_.get()];
 }
 
@@ -99,6 +126,8 @@ NotificationPlatformBridgeMac::~NotificationPlatformBridgeMac() {
   [notification_center_ setDelegate:nil];
 
   // TODO(miguelg) lift this restriction if possible.
+  // But make sure the ones shown by the XPC service are actually
+  // removed.

[Peter Beverloo, 2016/09/09 14:56:19]

Is this comment separate from the TODO, or is it a continuation? Reformat if the
former.

[Miguel Garcia, 2016/09/12 13:49:00]

Formatted as 2 TODOs now

On 2016/09/09 14:56:19, Peter Beverloo wrote:

   [notification_center_ removeAllDeliveredNotifications];
 }
 
@@ -139,6 +168,7 @@ void NotificationPlatformBridgeMac::Display(
   // Tag
   if (!notification.tag().empty()) {
     [builder setTag:base::SysUTF8ToNSString(notification.tag())];
+
     // If renotify is needed, delete the notification with the same tag
     // from the notification center before displaying this one.
     // TODO(miguelg): This will need to work for alerts as well via XPC
@@ -165,9 +195,15 @@ void NotificationPlatformBridgeMac::Display(
   [builder setProfileId:base::SysUTF8ToNSString(profile_id)];
   [builder setIncognito:incognito];
   [builder setNotificationType:[NSNumber numberWithInteger:notification_type]];
-
-  NSUserNotification* toast = [builder buildUserNotification];
-  [notification_center_ deliverNotification:toast];
+  if (notification.never_timeout() &&
+      base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableNativeAlerts)) {

[Peter Beverloo, 2016/09/09 14:56:19]

As discussed, we'll want to switch this to a base::Feature instead.(See
//chrome/common/chrome_features.h.) That enables us to get Finch for our part of
the XPC roll-out for free.

This is orthogonal of the compile-time flag.

+    NSDictionary* dict = [builder buildDictionary];
+    [notification_remote_dispatcher_ dispatchNotification:dict];
+  } else {
+    NSUserNotification* toast = [builder buildUserNotification];
+    [notification_center_ deliverNotification:toast];
+  }
 }
 
 void NotificationPlatformBridgeMac::Close(const std::string& profile_id,
@@ -214,6 +250,41 @@ bool NotificationPlatformBridgeMac::SupportsNotificationCenter() const {
 }
 
 // static
+void NotificationPlatformBridgeMac::ProcessNotificationResponse(
+    NSDictionary* notificationResponseData) {

[Robert Sesek, 2016/09/08 21:10:08]

naming: under_scores (and maybe a shorter name to make for less wrapping).

[Peter Beverloo, 2016/09/09 14:56:19]

I *really* dislike the hacker_style vs. camelCase inconsistencies within a
single file :-(

[Robert Sesek, 2016/09/09 20:43:43]

The style rule isn't about the file, though. It's about the context (in C++ use
those naming conventions, in ObjC use those).

[Miguel Garcia, 2016/09/12 13:48:58]

Done.

[Miguel Garcia, 2016/09/12 13:49:00]

I re-named it to a single word to make both of you happy :) But yeah the rule is
clear hacker_style in C++ methods, camelCase in Obj-C methods. I just messed it
up

+  if (!NotificationPlatformBridgeMac::VerifyNotificationData(
+          notificationResponseData))
+    return;
+
+  NSNumber* buttonIndex = [notificationResponseData
+      objectForKey:notification_constants::kNotificationButtonIndex];
+  NSNumber* operation = [notificationResponseData
+      objectForKey:notification_constants::kNotificationOperation];
+
+  std::string notificationOrigin =
+      base::SysNSStringToUTF8([notificationResponseData
+          objectForKey:notification_constants::kNotificationOrigin]);
+  std::string notificationId = base::SysNSStringToUTF8([notificationResponseData
+      objectForKey:notification_constants::kNotificationId]);
+  std::string profileId = base::SysNSStringToUTF8([notificationResponseData
+      objectForKey:notification_constants::kNotificationProfileId]);
+  NSNumber* isIncognito = [notificationResponseData
+      objectForKey:notification_constants::kNotificationIncognito];
+  NSNumber* notificationType = [notificationResponseData
+      objectForKey:notification_constants::kNotificationType];
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(DoProcessNotificationResponse,
+                 static_cast<NotificationCommon::Operation>(
+                     operation.unsignedIntValue),
+                 static_cast<NotificationCommon::Type>(
+                     notificationType.unsignedIntValue),
+                 profileId, [isIncognito boolValue], notificationOrigin,
+                 notificationId, buttonIndex.intValue));
+}
+
+// static
 bool NotificationPlatformBridgeMac::VerifyNotificationData(
     NSDictionary* response) {
   if (![response
@@ -286,40 +357,10 @@ bool NotificationPlatformBridgeMac::VerifyNotificationData(
 @implementation NotificationCenterDelegate
 - (void)userNotificationCenter:(NSUserNotificationCenter*)center
        didActivateNotification:(NSUserNotification*)notification {
-  NSDictionary* response =
+  NSDictionary* notificationResponseData =
       [NotificationResponseBuilder buildDictionary:notification];
-  if (!NotificationPlatformBridgeMac::VerifyNotificationData(response))
-    return;
-
-  NSNumber* buttonIndex =
-      [response objectForKey:notification_constants::kNotificationButtonIndex];
-  NSNumber* operation =
-      [response objectForKey:notification_constants::kNotificationOperation];
-
-  std::string notificationOrigin = base::SysNSStringToUTF8(
-      [response objectForKey:notification_constants::kNotificationOrigin]);
-  NSString* notificationId =
-      [response objectForKey:notification_constants::kNotificationId];
-  std::string persistentNotificationId =
-      base::SysNSStringToUTF8(notificationId);
-  std::string profileId = base::SysNSStringToUTF8(
-      [response objectForKey:notification_constants::kNotificationProfileId]);
-  NSNumber* isIncognito =
-      [response objectForKey:notification_constants::kNotificationIncognito];
-  NSNumber* notificationType =
-      [response objectForKey:notification_constants::kNotificationType];
-
-  ProfileManager* profileManager = g_browser_process->profile_manager();
-  DCHECK(profileManager);
-
-  profileManager->LoadProfile(
-      profileId, [isIncognito boolValue],
-      base::Bind(
-          &ProfileLoadedCallback, static_cast<NotificationCommon::Operation>(
-                                      operation.unsignedIntValue),
-          static_cast<NotificationCommon::Type>(
-              notificationType.unsignedIntValue),
-          notificationOrigin, persistentNotificationId, buttonIndex.intValue));
+  NotificationPlatformBridgeMac::ProcessNotificationResponse(
+      notificationResponseData);
 }
 
 - (BOOL)userNotificationCenter:(NSUserNotificationCenter*)center
@@ -329,3 +370,51 @@ bool NotificationPlatformBridgeMac::VerifyNotificationData(
 }
 
 @end
+
+@implementation NotificationRemoteDispatcher
+
+@synthesize xpcConnection = _xpcConnection;

[Robert Sesek, 2016/09/08 21:10:08]

There's no need for this @property. It can just be a scoped_nsobject ivar.

[Robert Sesek, 2016/09/08 21:10:08]

Use trailing underscores.

[Miguel Garcia, 2016/09/12 13:49:00]

Done.

[Miguel Garcia, 2016/09/12 13:49:00]

Done.

+
+- (instancetype)init {

[Robert Sesek, 2016/09/08 21:10:08]

This needs to be wrapped in a 

if ((self = [super init])) {
  // your initialization here
}

[Miguel Garcia, 2016/09/12 13:49:00]

Done.

+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableNativeAlerts)) {
+    _xpcConnection = [[NSXPCConnection alloc]

[Robert Sesek, 2016/09/08 21:10:08]

This is leaked.

[Miguel Garcia, 2016/09/12 13:49:00]

Done.

+        initWithServiceName:
+            [NSString
+                stringWithFormat:notification_constants::kAlertXPCServiceName,
+                                 [base::mac::OuterBundle() bundleIdentifier]]];
+    _xpcConnection.remoteObjectInterface =
+        [NSXPCInterface interfaceWithProtocol:@protocol(NotificationDelivery)];
+    _xpcConnection.interruptionHandler = ^{
+      NSLog(@"connection interrupted: interruptionHandler: %@", _xpcConnection);

[Robert Sesek, 2016/09/08 21:10:08]

Use Chromium logging instead.

[Miguel Garcia, 2016/09/12 13:48:58]

Done.

+      // TODO(miguelg): perhaps add some UMA here.
+      // We will be getting this handler both when the XPC server crashes or
+      // when it decides to close the connection.
+    };
+    _xpcConnection.invalidationHandler = ^{
+      NSLog(@"conn invalidationHandler %@", _xpcConnection);
+      // This means that the connection should be recreated if it needs
+      // to be used again. It should not really happen.
+      DCHECK(false) << "XPC Connection invalidated";
+    };
+
+    _xpcConnection.exportedInterface =
+        [NSXPCInterface interfaceWithProtocol:@protocol(NotificationReply)];
+    _xpcConnection.exportedObject = self;
+    [_xpcConnection resume];
+  }
+
+  return self;
+}
+
+- (void)dispatchNotification:(NSDictionary*)data {
+  [[_xpcConnection remoteObjectProxy] deliverNotification:data];
+}
+
+// NotificationReply implementation
+- (void)notificationClick:(NSDictionary*)notificationResponseData {
+  NotificationPlatformBridgeMac::ProcessNotificationResponse(
+      notificationResponseData);
+}
+
+@end