Abstract notification clicks in its own dictionary

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 c1ac1e8b659387c89e797788440d0c399560fe5a..0ab7ec108f02e9ebbe09182d83cb9e433ededbc8 100644
--- a/chrome/browser/notifications/notification_platform_bridge_mac.mm
+++ b/chrome/browser/notifications/notification_platform_bridge_mac.mm
@@ -18,6 +18,8 @@
 #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_constants_mac.h"
+#import "chrome/browser/ui/cocoa/notifications/notification_response_builder_mac.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/url_formatter/elide_url.h"
 #include "third_party/WebKit/public/platform/modules/notifications/WebNotificationConstants.h"
@@ -146,10 +148,10 @@ void NotificationPlatformBridgeMac::Close(const std::string& profile_id,
   for (NSUserNotification* toast in
        [notification_center_ deliveredNotifications]) {
     NSString* toast_id =
-        [toast.userInfo objectForKey:notification_builder::kNotificationId];
+        [toast.userInfo objectForKey:notification_constants::kNotificationId];
 
     NSString* persistent_profile_id = [toast.userInfo
-        objectForKey:notification_builder::kNotificationProfileId];
+        objectForKey:notification_constants::kNotificationProfileId];
 
     if (toast_id == candidate_id &&
         persistent_profile_id == current_profile_id) {
@@ -167,10 +169,10 @@ bool NotificationPlatformBridgeMac::GetDisplayed(
   for (NSUserNotification* toast in
        [notification_center_ deliveredNotifications]) {
     NSString* toast_profile_id = [toast.userInfo
-        objectForKey:notification_builder::kNotificationProfileId];
+        objectForKey:notification_constants::kNotificationProfileId];
     if (toast_profile_id == current_profile_id) {
-      notifications->insert(base::SysNSStringToUTF8(
-          [toast.userInfo objectForKey:notification_builder::kNotificationId]));
+      notifications->insert(base::SysNSStringToUTF8([toast.userInfo
+          objectForKey:notification_constants::kNotificationId]));
     }
   }
   return true;
@@ -185,63 +187,38 @@ bool NotificationPlatformBridgeMac::SupportsNotificationCenter() const {
 @implementation NotificationCenterDelegate
 - (void)userNotificationCenter:(NSUserNotificationCenter*)center
        didActivateNotification:(NSUserNotification*)notification {
-  std::string notificationOrigin =
-      base::SysNSStringToUTF8([notification.userInfo
-          objectForKey:notification_builder::kNotificationOrigin]);
-  NSNumber* notificationId = [notification.userInfo
-      objectForKey:notification_builder::kNotificationId];
-  NSString* profileId = [notification.userInfo
-      objectForKey:notification_builder::kNotificationProfileId];
-  NSNumber* isIncognito = [notification.userInfo
-      objectForKey:notification_builder::kNotificationIncognito];
+  NSDictionary* response =
+      [NotificationResponseBuilder buildDictionary:notification];

[Peter Beverloo, 2016/06/16 17:02:43]

Could be pedantic and add a whole lot of DCHECKS to this method, since we
control both sides of the usage.

+
+  NSNumber* buttonIndex =
+      [response objectForKey:notification_constants::kNotificationButtonIndex];
+  NSNumber* operation =
+      [response objectForKey:notification_constants::kNotificationOperation];
+
+  std::string notificationOrigin = base::SysNSStringToUTF8(
+      [response objectForKey:notification_constants::kNotificationOrigin]);

[Peter Beverloo, 2016/06/16 17:02:43]

nit: would be good to consistently use (or not use) the `notification` prefix in
the local variable names. Right now it is:

kNotificationButtonIndex -> buttonIndex
kNotificationOperation -> operation
kNotificationOrigin -> *notification*Origin
kNotificationId -> *notification*Id
kNotificationProfileId -> profileId
kNotificationIncognito -> isIncognito

[Miguel Garcia, 2016/06/17 17:53:08]

so I am using notificationId because id is a semi reserved keyword in
objective-C
for notificationOrigin I just did it so that I can call the final variable
origin (the GURL in 216)


On 2016/06/16 17:02:43, Peter Beverloo - OOO June 8-16 wrote:

+  NSString* notificationId = [notification.userInfo
+      objectForKey:notification_constants::kNotificationId];
+  std::string notification_id = base::SysNSStringToUTF8(notificationId);
+  int64_t persistent_notification_id;
+  if (!base::StringToInt64(notification_id, &persistent_notification_id)) {
+    LOG(ERROR) << "Unable to convert notification ID: " << notification_id
+               << " to integer.";
+    return;
+  }

[Peter Beverloo, 2016/06/16 17:02:42]

This method has a mixture of macStyle and unix_style variables (notification_id,
notificationId, persistent_notification_id). Since this is pure Mac code,
shouldn't it always be macStyle?

[Miguel Garcia, 2016/06/17 17:53:08]

Yeah good point.

+  std::string profileId = base::SysNSStringToUTF8(
+      [response objectForKey:notification_constants::kNotificationProfileId]);
+  NSNumber* isIncognito =
+      [response objectForKey:notification_constants::kNotificationIncognito];
 
   GURL origin(notificationOrigin);
 
-  // Initialize operation and button index for the case where the
-  // notification itself was clicked.
-  PlatformNotificationServiceImpl::NotificationOperation operation =
-      PlatformNotificationServiceImpl::NOTIFICATION_CLICK;
-  int buttonIndex = -1;
-
-  // Determine whether the user clicked on a button, and if they did, whether it
-  // was a developer-provided button or the mandatory Settings button.
-  if (notification.activationType ==
-      NSUserNotificationActivationTypeActionButtonClicked) {
-    NSArray* alternateButtons = @[];
-    if ([notification
-            respondsToSelector:@selector(_alternateActionButtonTitles)]) {
-      alternateButtons =
-          [notification valueForKey:@"_alternateActionButtonTitles"];
-    }
-
-    bool multipleButtons = (alternateButtons.count > 0);
-
-    // No developer actions, just the settings button.
-    if (!multipleButtons) {
-      operation = PlatformNotificationServiceImpl::NOTIFICATION_SETTINGS;
-      buttonIndex = -1;
-    } else {
-      // 0 based array containing.
-      // Button 1
-      // Button 2 (optional)
-      // Settings
-      NSNumber* actionIndex =
-          [notification valueForKey:@"_alternateActionIndex"];
-      operation = (actionIndex.unsignedLongValue == alternateButtons.count - 1)
-                      ? PlatformNotificationServiceImpl::NOTIFICATION_SETTINGS
-                      : PlatformNotificationServiceImpl::NOTIFICATION_CLICK;
-      buttonIndex =
-          (actionIndex.unsignedLongValue == alternateButtons.count - 1)
-              ? -1
-              : actionIndex.intValue;
-    }
-  }
-
   PlatformNotificationServiceImpl::GetInstance()
       ->ProcessPersistentNotificationOperation(
-          operation, base::SysNSStringToUTF8(profileId),
-          [isIncognito boolValue], origin, notificationId.longLongValue,
-          buttonIndex);
+          static_cast<PlatformNotificationServiceImpl::NotificationOperation>(
+              operation.intValue),
+          profileId, [isIncognito boolValue], origin,
+          persistent_notification_id, buttonIndex.intValue);
 }
 
 - (BOOL)userNotificationCenter:(NSUserNotificationCenter*)center