Introduce a new API to handle native notification clicks

chrome/browser/notifications/native_notification_display_service.cc

 // Copyright 2016 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/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 {
 
 std::string GetProfileId(Profile* profile) {
 #if defined(OS_WIN)
   std::string profile_id =
       base::WideToUTF8(profile->GetPath().BaseName().value());
 #elif defined(OS_POSIX)
   std::string profile_id = profile->GetPath().BaseName().value();
 #endif
   return profile_id;
 }
 
 }  // namespace
 
 NativeNotificationDisplayService::NativeNotificationDisplayService(
     Profile* profile,
     NotificationPlatformBridge* notification_bridge)
     : 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(
     std::set<std::string>* notifications) const {
   return notification_bridge_->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();
 }