Introduce a new API to handle native notification clicks

chrome/browser/notifications/notification_display_service.h

Index: chrome/browser/notifications/notification_display_service.h
diff --git a/chrome/browser/notifications/notification_display_service.h b/chrome/browser/notifications/notification_display_service.h
index 6bbd645747f3eecca310333ac705dab6008ad777..8e82b04b437ad1411f859fad2ec287038cbb67e9 100644
--- a/chrome/browser/notifications/notification_display_service.h
+++ b/chrome/browser/notifications/notification_display_service.h
@@ -8,6 +8,7 @@
 #include <set>
 #include <string>
 #include "base/macros.h"
+#include "chrome/browser/notifications/notification_operation_common.h"
 #include "components/keyed_service/core/keyed_service.h"
 
 class Notification;
@@ -27,16 +28,30 @@ class NotificationDisplayService : public KeyedService {
   ~NotificationDisplayService() override {}
 
   // Displays the |notification| identified by |notification_id|.
-  virtual void Display(const std::string& notification_id,
-                       const Notification& notification) = 0;
+  virtual void Display(
+      notification_operation_common::NotificationHandlerType notification_type,

[Peter Beverloo, 2016/06/29 00:23:15]

In the longer run, taking the services model, this knowledge needs to be removed
from the notification "service" entirely.

Let's not ignore that— could we consider a GCMAppHandler::CanHandler-kind of
construct where identification of owned notifications is the handler's
responsibility? (Is that even sufficient?) It would completely remove the need
for these arguments at least.

Would a service own a token-based registry of sorts?

[Peter Beverloo, 2016/06/29 00:23:15]

Needing to use the `notification_operation_common` prefix every time a
notification handler type or operation is mentioned is unfortunately long. We
can't use enum classes since it needs to be casted to an int, either...

Could we instead put the enumeration in the global scope w/ values such as
NOTIFICATION_TYPE_NON_PERSISTENT?

[Miguel Garcia, 2016/07/05 13:54:50]

As discussed offline I am maintaining the type enum for now given that we have a
finite and known set of notification types.

On 2016/06/29 00:23:15, Peter Beverloo wrote:

[Miguel Garcia, 2016/07/05 13:54:50]

I went with a class and enum classes.

+      const std::string& notification_id,
+      const Notification& notification) = 0;
 
   // Closes the notification identified by |notification_id|.
-  virtual void Close(const std::string& notification_id) = 0;
+  // This only handles programatic close calls, notifications closed
+  // by users get handled by |ProcessNotificationOperation|.
+  virtual void Close(
+      notification_operation_common::NotificationHandlerType notification_type,
+      const std::string& notification_id) = 0;
 
   // Returns whether the implementation can retrieve a list of currently visible
   // notifications and stores them in |*notification_ids| when possible.
   virtual bool GetDisplayed(std::set<std::string>* notifications) const = 0;
 
+  // Used by the notification bridge to propagate back events (click, close...)
+  virtual void ProcessNotificationOperation(
+      notification_operation_common::NotificationOperation operation,
+      notification_operation_common::NotificationHandlerType notification_type,
+      const std::string& origin,
+      const std::string& notification_id,
+      int action_index) = 0;

[Peter Beverloo, 2016/06/29 00:23:15]

NotificationDisplayService defines the [feature] -> [notification system]
interface. This method is an implementation detail of the [notification system]
that [feature] does not have to know about, so should not be part of the public
API.

[Miguel Garcia, 2016/07/05 13:54:50]

Indeed, made it a method of the native display service and removed it from the
overall interface.

+
   // Temporary method while we finish the refactor. Returns whether there is
   // a native notification center backing up notifications.
   virtual bool SupportsNotificationCenter() const = 0;