Introduce a new API to handle native notification clicks

chrome/browser/notifications/notification_display_service.h

 // 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.
 
 #ifndef CHROME_BROWSER_NOTIFICATIONS_NOTIFICATION_DISPLAY_SERVICE_H_
 #define CHROME_BROWSER_NOTIFICATIONS_NOTIFICATION_DISPLAY_SERVICE_H_
 
 #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;
 class NotificationPlatformBridge;
 class Profile;
 
 // Profile-bound service that enables notifications to be displayed and
 // interacted with on the user's screen, orthogonal of whether this
 // functionality is provided by the browser or by the operating system. An
 // instance can be retrieved through the NotificationDisplayServiceFactory.
 //
 // TODO(peter): Add a NotificationHandler mechanism for registering listeners.
 // TODO(miguelg): Remove the SupportsNotificationCenter method.
 class NotificationDisplayService : public KeyedService {
  public:
   NotificationDisplayService() {}
   ~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;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(NotificationDisplayService);
 };
 
 #endif  // CHROME_BROWSER_NOTIFICATIONS_NOTIFICATION_DISPLAY_SERVICE_H_