Nuke NotificationUIManager from PlatformNotificationServiceImpl

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 "base/macros.h"
+#include "components/keyed_service/core/keyed_service.h"
+#include "ui/message_center/notification.h"
+
+class Notification;

[Peter Beverloo, 2016/03/21 15:57:04]

You both include the message_center::Notification and forward-declare
Notification, only the latter of which is used. Could we start reducing
knowledge and use message_center::Notification right now? (The MC delegate will
have to be provided by the MessageCenterDisplayService impl anways.)

[Miguel Garcia, 2016/04/11 19:51:26]

Done.

+class Profile;
+
+class NotificationDisplayService : public KeyedService {

[Peter Beverloo, 2016/03/21 15:57:03]

++docs

[Miguel Garcia, 2016/04/11 19:51:26]

Done.

+ public:
+  static NotificationDisplayService* Create(Profile* profile);

[Peter Beverloo, 2016/03/21 15:57:04]

Can we try to have the creation logic in
NotificationDisplayServiceFactory::BuildServiceInstanceFor(). This allows the
necessary #ifdefs to stay in a single file. I hope the .mm file will allow this
somehow.

[Miguel Garcia, 2016/03/21 17:43:02]

No, this won't work for mac because we would need to include obcj++ code in a
pure C++ file. We had a similar discussion last time so let's figure out the
path forward live.

[Miguel Garcia, 2016/04/11 19:51:26]

Reworked a bit after chatting offline, *most* of the logic happens in the
factory but for mac we have an internal static method implemented only in the
.mm file

On 2016/03/21 17:43:02, Miguel Garcia wrote:

+
+  // Shows a toast on string using the data passed in |notification|.
+  virtual void Display(const Notification& notification) = 0;

[Peter Beverloo, 2016/03/21 15:57:04]

Why won't we need to pass the |notification_id| here? I'd like to be explicit
here (even if the Notification object can store it) because the same key needs
to be used when closing the notification.

[Miguel Garcia, 2016/03/21 17:43:03]

Sure, originally you mentioned that the NotificationData object would have it
but it can be an explicit parameter.

[Miguel Garcia, 2016/04/11 19:51:26]

Added now, this highlights how mac right now does not work with non-persistent
notifications and will probably need to wait until the id is passed from the
upper layers for it to actually work.


On 2016/03/21 17:43:03, Miguel Garcia wrote:

+
+  // Closes a nofication. Returns wether a notiication was actually closed.
+  virtual bool Close(const std::string& notification_id) = 0;

[Peter Beverloo, 2016/03/21 15:57:04]

What are we going to use the returned boolean for?

[Miguel Garcia, 2016/03/21 17:43:03]

I can remove it. This was something the old ui manager offered but it's
certainly the time to clean up.

+
+
+  // Return a set of notification ids currently being displayed either as popups
+  // or in the native platform's notification center.
+  virtual std::set<std::string> GetDisplayed() const = 0;

[Peter Beverloo, 2016/03/21 15:57:03]

This needs some way to indicate whether the list of displayed notifications
*can* be retrieved. For example, Android <M will always return an empty set, not
because there are no notifications, but because we don't know.

virtual bool GetDisplayed(std::set<std::string>* notifications) = 0; ?

[Miguel Garcia, 2016/03/21 17:43:02]

ok

[Miguel Garcia, 2016/04/11 19:51:26]

Done.

+
+  // Temporary method while we finish the refactor
+  virtual bool SupportsNotificationCenter() const = 0;
+
+ protected:

[Peter Beverloo, 2016/03/21 15:57:03]

What interfaces do have are virtual destructors :-).

[Peter Beverloo, 2016/03/21 15:57:04]

This is an interface, and interfaces don't have members such as the following.

[Miguel Garcia, 2016/03/21 17:43:03]

I would like to avoid having to pass and store the profile in all subclasses
though. Note that Keyed Service is an actual class so this is not going to be a
pure interface anyway.

[Miguel Garcia, 2016/03/21 17:43:03]

? do, have, are?

In general keyservice destruction is its own science so this WIP patch was not
trying to address it yet

[Miguel Garcia, 2016/04/11 19:51:26]

Reluctantly added it to each subclass :(

+  explicit NotificationDisplayService(Profile* profile);
+  Profile* profile_;  // weak
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NotificationDisplayService);
+};
+
+#endif  // CHROME_BROWSER_NOTIFICATIONS_NOTIFICATION_DISPLAY_SERVICE_H_