Introduce a new API to handle native notification clicks

chrome/browser/notifications/native_notification_display_service.cc

Index: chrome/browser/notifications/native_notification_display_service.cc
diff --git a/chrome/browser/notifications/native_notification_display_service.cc b/chrome/browser/notifications/native_notification_display_service.cc
index 64f71b00238cacad1d30630ad94ff8594e719547..76a0d2159597182322c02151b225536f81e027c5 100644
--- a/chrome/browser/notifications/native_notification_display_service.cc
+++ b/chrome/browser/notifications/native_notification_display_service.cc
@@ -4,9 +4,14 @@
 
 #include "chrome/browser/notifications/native_notification_display_service.h"
 
+#include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/notifications/nonpersistent_notification_handler.h"
 #include "chrome/browser/notifications/notification.h"
+#include "chrome/browser/notifications/notification_delegate.h"
+#include "chrome/browser/notifications/notification_handler.h"
 #include "chrome/browser/notifications/notification_platform_bridge.h"
+#include "chrome/browser/notifications/persistent_notification_handler.h"
 #include "chrome/browser/profiles/profile.h"
 
 namespace {
@@ -29,21 +34,44 @@ NativeNotificationDisplayService::NativeNotificationDisplayService(
     : profile_(profile), notification_bridge_(notification_bridge) {
   DCHECK(profile_);
   DCHECK(notification_bridge_);
+
+  std::unique_ptr<NonPersistentNotificationHandler> non_persistent(
+      new NonPersistentNotificationHandler());
+  std::unique_ptr<PersistentNotificationHandler> persistent(
+      new PersistentNotificationHandler());
+
+  AddNotificationHandler(non_persistent->NotificationType(),
+                         std::move(non_persistent));
+  AddNotificationHandler(persistent->NotificationType(), std::move(persistent));

[Peter Beverloo, 2016/07/05 14:25:33]

Brainfart:

What if we add a GetNotificationHandler() method, and use that in
PlatformNotificationServiceImpl::Display() to register non-persistent
notifications? That allows us to kill RegisterNotification() from the
NotificationHandler altogether.

[Peter Beverloo, 2016/07/05 14:25:34]

These are the only two calls to the NotificationType() method. May be better to
remove it entirely from the NotificationHandler class, and instead register this
as:

  AddNotificationHandler(NotificationCommon::PERSISTENT,
                         base::MakeUnique<PersistentNotificationHandler>());
  AddNotificationHandler(NotificationCommon::NON_PERSISTENT,
                         base::MakeUnique<NonPersistentNotificationHandler>());

[Peter Beverloo, 2016/07/05 14:25:34]

Do you think it'd make sense to change PlatformNotificationServiceImpl to be
per-profile as well, in which case it can register these handlers rather than
having the knowledge here?

[Miguel Garcia, 2016/07/05 17:12:51]

Well extensions will need something similar too.. In general it feels like the
map of delegates kind of belongs to the display machinery, (like in the case of
message center) not the platform notification service.

[Miguel Garcia, 2016/07/05 17:12:52]

Maybe eventually? I would like to continue removing trimming the functionality
of that class (for example by moving the actual implementation of
On{Click/Close} to the handlers) before we consider that.

[Miguel Garcia, 2016/07/05 17:12:52]

I was struggling with this, I thought the knowledge of what kind of type a
handler is for should remain in the handler itself but if you feel it's ok to
have this class the one that links what a given delegate implementation is for
let's do it.

On 2016/07/05 14:25:34, Peter Beverloo wrote:

 }
 
-NativeNotificationDisplayService::~NativeNotificationDisplayService() {}
+NativeNotificationDisplayService::~NativeNotificationDisplayService() {
+  notification_handlers_.clear();

[Peter Beverloo, 2016/07/05 14:25:33]

Why is this necessary? The map's destructor will do this for us, right?

[Miguel Garcia, 2016/07/05 17:12:52]

indeed

+}
 
 void NativeNotificationDisplayService::Display(
+    NotificationCommon::Type notification_type,
     const std::string& notification_id,
     const Notification& notification) {
-  notification_bridge_->Display(notification_id, GetProfileId(profile_),
+  notification_bridge_->Display(notification_type, notification_id,
+                                GetProfileId(profile_),
                                 profile_->IsOffTheRecord(), notification);
   notification.delegate()->Display();
+  NotificationHandler* handler = GetNotificationHandler(notification_type);
+  handler->RegisterNotification(notification_id, notification.delegate());
 }
 
 void NativeNotificationDisplayService::Close(
+    NotificationCommon::Type notification_type,
     const std::string& notification_id) {
+  NotificationHandler* handler = GetNotificationHandler(notification_type);
   notification_bridge_->Close(GetProfileId(profile_), notification_id);
+
+  // TODO(miguelg): Figure out something better here, passing an empty
+  // origin works because only non persistent notifications care about
+  // this method for JS generated close calls and they don't require
+  // the origin.

[Peter Beverloo, 2016/07/05 14:25:34]

It seems to me like the GetNotificationHandler() idea could address this too?

+  handler->Close(profile_, "", notification_id, false /* by user */);
 }
 
 bool NativeNotificationDisplayService::GetDisplayed(
@@ -52,6 +80,47 @@ bool NativeNotificationDisplayService::GetDisplayed(
       GetProfileId(profile_), profile_->IsOffTheRecord(), notifications);
 }
 
+void NativeNotificationDisplayService::ProcessNotificationOperation(
+    NotificationCommon::Operation operation,
+    NotificationCommon::Type notification_type,
+    const std::string& origin,
+    const std::string& notification_id,
+    int action_index) {
+  NotificationHandler* handler = GetNotificationHandler(notification_type);

[Peter Beverloo, 2016/07/05 14:25:34]

Handle the |!handler| case— CHECK()?

[Miguel Garcia, 2016/07/05 17:12:52]

Added a check, the GetNotificationHandler itself has a DCHECK and it's really
never meant to return an unknown value so I guess this has us covered.

+  switch (operation) {
+    case NotificationCommon::CLICK:
+      handler->Click(profile_, origin, notification_id, action_index);
+      break;
+    case NotificationCommon::CLOSE:
+      handler->Close(profile_, origin, notification_id, true /*by user*/);

[Peter Beverloo, 2016/07/05 14:25:34]

nit: /* by_user */

[Miguel Garcia, 2016/07/05 17:12:52]

Done.

+      break;
+    case NotificationCommon::SETTINGS:
+      handler->Settings(profile_);
+      break;
+  }
+}
+
+void NativeNotificationDisplayService::AddNotificationHandler(
+    NotificationCommon::Type notification_type,
+    std::unique_ptr<NotificationHandler> handler) {
+  DCHECK(handler);
+  DCHECK_EQ(notification_handlers_.count(notification_type), 0u);
+  notification_handlers_[notification_type] = std::move(handler);
+}
+
+void NativeNotificationDisplayService::RemoveNotificationHandler(
+    NotificationCommon::Type notification_type) {
+  notification_handlers_.erase(notification_type);
+}
+
+NotificationHandler* NativeNotificationDisplayService::GetNotificationHandler(
+    NotificationCommon::Type notification_type) {
+  DCHECK(notification_handlers_.find(notification_type) !=
+         notification_handlers_.end())
+      << notification_type << " NOT REGISTERED";

[Peter Beverloo, 2016/07/05 14:25:34]

NOT REGISTERED -> is not registered.

[Peter Beverloo, 2016/07/05 14:25:34]

Maybe it should just be OK to return nullptrs here?

[Miguel Garcia, 2016/07/05 17:12:51]

In what situation do you think returning a null pointer makes sense? We register
the only two possible values in the constructor.

[Miguel Garcia, 2016/07/05 17:12:52]

Done.

+  return notification_handlers_[notification_type].get();
+}
+
 bool NativeNotificationDisplayService::SupportsNotificationCenter() const {
   return notification_bridge_->SupportsNotificationCenter();
 }