Minimize the delegate dependencies for non persistent notifications.

content/browser/notifications/notification_event_dispatcher_impl.h

 // Copyright 2014 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 CONTENT_BROWSER_NOTIFICATIONS_NOTIFICATION_EVENT_DISPATCHER_IMPL_H_
 #define CONTENT_BROWSER_NOTIFICATIONS_NOTIFICATION_EVENT_DISPATCHER_IMPL_H_
 
+#include <map>
+
 #include "base/macros.h"
 #include "base/memory/singleton.h"
 #include "content/public/browser/notification_database_data.h"
 #include "content/public/browser/notification_event_dispatcher.h"
 
 namespace content {
 
 class NotificationEventDispatcherImpl : public NotificationEventDispatcher {
  public:
   // Returns the instance of the NotificationEventDispatcherImpl. Must be called
   // on the UI thread.
   static NotificationEventDispatcherImpl* GetInstance();
 
   // NotificationEventDispatcher implementation.
+
+  // TODO(miguelg) rename so it says Persistent.

[Peter Beverloo, 2017/05/31 17:52:43]

Out of interest, why? Wouldn't it make the API simpler if we'd just branch on
NotificationIdGenerator::IsPersistentNotification()?

In either case, this should (a) be in the interface definition, not in the
implementation, and (b) not have a blank line on l24.

[Miguel Garcia, 2017/06/01 17:00:53]

You mean having a single Dispatch method with completely different
implementations depending on whether it's persistent or not? That could work but
it seems better to be explicit in a public API? I am happy to revisit this
anyway.
For now I will move the comment to the api and add "consider renaming" instead.

[Peter Beverloo, 2017/06/01 17:56:27]

Ideally the embedder (i.e. //chrome) wouldn't have to care about whether it's a
persistent or a non-persistent notification, it'd just be "a notification".
Long-term.

[Miguel Garcia, 2017/06/02 12:37:12]

Acknowledged.

   void DispatchNotificationClickEvent(
       BrowserContext* browser_context,
       const std::string& notification_id,
       const GURL& origin,
       int action_index,
       const base::NullableString16& reply,
       const NotificationDispatchCompleteCallback& dispatch_complete_callback)
       override;
   void DispatchNotificationCloseEvent(
       BrowserContext* browser_context,
       const std::string& notification_id,
       const GURL& origin,
       bool by_user,
       const NotificationDispatchCompleteCallback& dispatch_complete_callback)
       override;
 

[Peter Beverloo, 2017/05/31 17:52:43]

nit: no line breaks in override blocks like this.

[Miguel Garcia, 2017/06/01 17:00:53]

Done.

+  void RegisterNonPersistentNotification(const std::string& notification_id,
+                                         int renderer_id,
+                                         int non_persistent_id) override;
+  void DispatchNonPersistentShowEvent(
+      const std::string& notification_id) override;
+  void DispatchNonPersistentClickEvent(
+      const std::string& notification_id) override;
+  void DispatchNonPersistentCloseEvent(
+      const std::string& notification_id) override;
+  void RendererGone(int renderer_id) override;
+
  private:
   NotificationEventDispatcherImpl();
   ~NotificationEventDispatcherImpl() override;
 
+  // Notification Id -> renderer id
+  std::map<std::string, int> notification_renderers_;
+
+  // Notification real id -> non persistent id.

[Peter Beverloo, 2017/05/31 17:52:43]

What is a "real" notification Id?

Since everything is keyed by the notification id, can we just have the following
struct?

  std::map<std::string, std::pair<int /* renderer_id */, int /*
non_persistent_notification_id */>>

[Miguel Garcia, 2017/06/01 17:00:53]

It just feels harder to read when both members of the pair are ints (also more
error prone). Would it be fine to keep both maps?

I changed the comment.

On 2017/05/31 17:52:43, Peter Beverlif you oo wrote:

[Peter Beverloo, 2017/06/01 17:56:27]

But much simpler code since we'd only have to care about a single map and can
drop the manual iteration. I have a reasonable preference but won't block you on
it :).

[Peter Beverloo, 2017/06/01 17:56:27]

"Peter Beverlif you oo wrote:"

lol how did you do that?

[Miguel Garcia, 2017/06/02 12:37:13]

I have no idea...

+  std::map<std::string, int> notification_ids_;
+
   friend struct base::DefaultSingletonTraits<NotificationEventDispatcherImpl>;
 
   DISALLOW_COPY_AND_ASSIGN(NotificationEventDispatcherImpl);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_NOTIFICATIONS_NOTIFICATION_EVENT_DISPATCHER_IMPL_H_