Nuke NotificationUIManager from PlatformNotificationServiceImpl

Nuke NotificationUIManager from PlatformNotificationServiceImpl

A few highlights
- Always use one kind of profile id (the stable one)
- Always use a single notification id (the persistent one when possible)
- The mac implementation injects the NSUSerNotificationService to allow unittests

Note that the mac and android files are not renamed. Ideally I would land this and then land a straight forward rename cl.

BUG=571056, 596161
Committed: https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418
Cr-Commit-Position: refs/heads/master@{#388776}

[Miguel Garcia, 2016-03-18 09:45:30]

Description was changed from

==========
WIP: Nuke NotificationUIManager from PlatformNotificationServiceImpl

BUG=571056
==========

to

==========
WIP: Nuke NotificationUIManager from PlatformNotificationServiceImpl

A few highlights
- Always use one kind of profile id (the stable one)
- Always use a single notification id (the persistent one when possible)
- The mac implementation injects the NSUSerNotificationService to allow
unittests

-Things that don't work (yet)

- Tests
- Android
- The close callback for non persistent notifications

BUG=571056
==========

[Miguel Garcia, 2016-03-18 09:53:55]

Description was changed from

==========
WIP: Nuke NotificationUIManager from PlatformNotificationServiceImpl

A few highlights
- Always use one kind of profile id (the stable one)
- Always use a single notification id (the persistent one when possible)
- The mac implementation injects the NSUSerNotificationService to allow
unittests

-Things that don't work (yet)

- Tests
- Android
- The close callback for non persistent notifications

BUG=571056
==========

to

==========
WIP: Nuke NotificationUIManager from PlatformNotificationServiceImpl

A few highlights
- Always use one kind of profile id (the stable one)
- Always use a single notification id (the persistent one when possible)
- The mac implementation injects the NSUSerNotificationService to allow
unittests

Things that don't work (yet)

- Tests
- Android
- The close callback for non persistent notifications
- need to rename notification_ui_manager_mac (and android)

BUG=571056
==========

[Miguel Garcia, 2016-03-18 20:21:36]

Description was changed from

==========
WIP: Nuke NotificationUIManager from PlatformNotificationServiceImpl

A few highlights
- Always use one kind of profile id (the stable one)
- Always use a single notification id (the persistent one when possible)
- The mac implementation injects the NSUSerNotificationService to allow
unittests

Things that don't work (yet)

- Tests
- Android
- The close callback for non persistent notifications
- need to rename notification_ui_manager_mac (and android)

BUG=571056
==========

to

==========
WIP: Nuke NotificationUIManager from PlatformNotificationServiceImpl

A few highlights
- Always use one kind of profile id (the stable one)
- Always use a single notification id (the persistent one when possible)
- The mac implementation injects the NSUSerNotificationService to allow
unittests

Things that don't work (yet)

- Tests
- Android
- The close callback for non persistent notifications
- need to rename notification_ui_manager_mac (and android)

BUG=571056,596161
==========

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

peter@chromium.org changed reviewers:
+ peter@chromium.org

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

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_display_service.h (right):

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:13: class
Notification;
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.)

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:16: class
NotificationDisplayService : public KeyedService {
++docs

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:18: static
NotificationDisplayService* Create(Profile* profile);
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.

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:21: virtual void
Display(const Notification& notification) = 0;
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.

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:24: virtual bool
Close(const std::string& notification_id) = 0;
What are we going to use the returned boolean for?

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:29: virtual
std::set<std::string> GetDisplayed() const = 0;
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; ?

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:34: protected:
What interfaces do have are virtual destructors :-).

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:34: protected:
This is an interface, and interfaces don't have members such as the following.

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_display_service_factory.h
(right):

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service_factory.h:16: //
Probably move to cc file
Yes.

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_ui_manager_mac.h (right):

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_ui_manager_mac.h:28:
NSUserNotificationCenter* notification_center);
What about having a SetUserNotificationCenterForTests instead? Reduces logic for
whatever creates this object.

Cross-platform tests could be written by using the factory's
SetTestingFactory().

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_ui_manager_mac.mm (right):

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_ui_manager_mac.mm:57: #if
defined(OS_MACOSX)
When is this ever not true?

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

Thanks! I think we have enough here to nail the design of this part. Let's chat
live about the different options and also how to land this in different pieces.

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_display_service.h (right):

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:18: static
NotificationDisplayService* Create(Profile* profile);
On 2016/03/21 15:57:04, Peter Beverloo wrote:
> 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.

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.

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:21: virtual void
Display(const Notification& notification) = 0;
On 2016/03/21 15:57:04, Peter Beverloo wrote:
> 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.

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

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:24: virtual bool
Close(const std::string& notification_id) = 0;
On 2016/03/21 15:57:04, Peter Beverloo wrote:
> What are we going to use the returned boolean for?

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

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:29: virtual
std::set<std::string> GetDisplayed() const = 0;
On 2016/03/21 15:57:03, Peter Beverloo wrote:
> 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; ?

ok

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:34: protected:
On 2016/03/21 15:57:03, Peter Beverloo wrote:
> What interfaces do have are virtual destructors :-).

? do, have, are?

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

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_display_service.h:34: protected:
On 2016/03/21 15:57:04, Peter Beverloo wrote:
> This is an interface, and interfaces don't have members such as the following.

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.

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/notification_ui_manager_mac.h (right):

https://codereview.chromium.org/1814923002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_ui_manager_mac.h:28:
NSUserNotificationCenter* notification_center);
On 2016/03/21 15:57:04, Peter Beverloo wrote:
> What about having a SetUserNotificationCenterForTests instead? Reduces logic
for
> whatever creates this object.
> 
> Cross-platform tests could be written by using the factory's
> SetTestingFactory().

The usual pattern for testing has usually been to inject the objects you are
planning to mock since that helps wi. This would make writing a unit test for
NotificationUIManagerMac very straight forward and would not require to add
special code to handle the test pieces in the actual file.

I don't feel to strongly about it though. 

Ok, I think injecting is nicer but this will in fact reduce the friction for
cross platform tests. I will resurrect the one I've removed from
platformnotificationserviceimpl as well then.

[Miguel Garcia, 2016-04-11 19:44:35]

Patchset #4 (id:60001) has been deleted

[Miguel Garcia, 2016-04-11 19:44:43]

Patchset #4 (id:80001) has been deleted

[Miguel Garcia, 2016-04-11 19:47:08]

Description was changed from

==========
WIP: Nuke NotificationUIManager from PlatformNotificationServiceImpl

A few highlights
- Always use one kind of profile id (the stable one)
- Always use a single notification id (the persistent one when possible)
- The mac implementation injects the NSUSerNotificationService to allow
unittests

Things that don't work (yet)

- Tests
- Android
- The close callback for non persistent notifications
- need to rename notification_ui_manager_mac (and android)

BUG=571056,596161
==========

to

==========
Nuke NotificationUIManager from PlatformNotificationServiceImpl

A few highlights
- Always use one kind of profile id (the stable one)
- Always use a single notification id (the persistent one when possible)
- The mac implementation injects the NSUSerNotificationService to allow
unittests

Note that the mac and android files are not renamed. Ideally I would land this
and then land a straight forward rename cl.

BUG=571056,596161
==========

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

PTAL

I have gone ahead with a single patch since it's ~700 lines which seems border
line but still reviewable and since so many follow ups need to happen in future
CLs I thought at least we can land the basics in one go. Happy to split it into
smaller patches if you prefer.

Upcoming follow ups once this lands

- Rename mac and android files and classes
- Mock notification display service directly and use that in the platform
notification service tests instead of (ab)using the mock notification ui manager
- Implement support for click and close events

https://chromiumcodereview.appspot.com/1814923002/diff/40001/chrome/browser/n...
File chrome/browser/notifications/notification_display_service.h (right):

https://chromiumcodereview.appspot.com/1814923002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_display_service.h:13: class
Notification;
On 2016/03/21 15:57:04, Peter Beverloo wrote:
> 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.)

Done.

https://chromiumcodereview.appspot.com/1814923002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_display_service.h:16: class
NotificationDisplayService : public KeyedService {
On 2016/03/21 15:57:03, Peter Beverloo wrote:
> ++docs

Done.

https://chromiumcodereview.appspot.com/1814923002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_display_service.h:18: static
NotificationDisplayService* Create(Profile* profile);
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:
> On 2016/03/21 15:57:04, Peter Beverloo wrote:
> > 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.
> 
> 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.

https://chromiumcodereview.appspot.com/1814923002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_display_service.h:21: virtual void
Display(const Notification& notification) = 0;
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:
> On 2016/03/21 15:57:04, Peter Beverloo wrote:
> > 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.
> 
> Sure, originally you mentioned that the NotificationData object would have it
> but it can be an explicit parameter.

https://chromiumcodereview.appspot.com/1814923002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_display_service.h:29: virtual
std::set<std::string> GetDisplayed() const = 0;
On 2016/03/21 17:43:02, Miguel Garcia wrote:
> On 2016/03/21 15:57:03, Peter Beverloo wrote:
> > 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; ?
> 
> ok

Done.

https://chromiumcodereview.appspot.com/1814923002/diff/40001/chrome/browser/n...
chrome/browser/notifications/notification_display_service.h:34: protected:
On 2016/03/21 17:43:03, Miguel Garcia wrote:
> On 2016/03/21 15:57:04, Peter Beverloo wrote:
> > This is an interface, and interfaces don't have members such as the
following.
> 
> 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.

Reluctantly added it to each subclass :(

[Miguel Garcia, 2016-04-14 16:00:17]

Introduce a notification bridge

[Miguel Garcia, 2016-04-15 15:16:25]

On 2016/04/14 16:00:17, Miguel Garcia wrote:
This change makes some interesting clean ups possible, the first of which is in
https://codereview.chromium.org/1895473002

[Peter Beverloo, 2016-04-18 14:57:10]

While rough, I do like the direction it's going in.

I think we need to talk a little bit more about the testing approaches you're
taking.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/chrome_notification_display_service.cc
(right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/chrome_notification_display_service.cc:21:
ui_manager_->Add(notification, profile_);
How do you imagine this will be updated with delegates? Right now
|notification_id| is unused.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/chrome_notification_display_service.h (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/chrome_notification_display_service.h:15: //
Implementation of display service for notifications displayed by chrome
Could we call this MessageCenter(Notification)DisplayService, consistent with
the other message_center_* files in this directory?

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/chrome_notification_display_service.h:15: //
Implementation of display service for notifications displayed by chrome
Meta question: should we have a MessageCenterNotificationBridge instead? What's
the benefit of forking on the level of the DisplayManager?

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/chrome_notification_display_service.h:23: void
Display(const std::string& notification_id,
nit:

// NotificationDisplayService implementation.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/native_notification_display_service.cc
(right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/native_notification_display_service.cc:12:
std::string GetProfileId(Profile* profile) {
micro nit: blank lines after namespace opening and before namespace closing.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/native_notification_display_service.cc:33:
DCHECK(notification_bridge_);
nit: DCHECK in the constructor instead? That way we can assume elsewhere.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/native_notification_display_service.h (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/native_notification_display_service.h:18: //
centers on platforms that support it..
micro nit: no double points...

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/native_notification_display_service.h:25: //
NotificationDisplayServiceImplementation.
// NotificationDisplayService implementation.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_bridge.h (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_bridge.h:10: #include "base/macros.h"
micro nit: don't we usually have blank lines between STL includes and
Chrome-based ones?

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_bridge.h:15: // notification.
Let's try to be a bit more inclusive. What about:

// Provides the low-level interface 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.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_bridge.h:21: NotificationBridge() {}
If we need a static Create method, let's make the constructor protected. A
hypothetical FakeNotificationBridge could increase visibility as needed.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_bridge.h:44: // Temporary method while
the refactor is finished.
Please do document what it does and why it's significant.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_display_service.h (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service.h:18: // notification
centers.
// 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.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service.h:24: // Shows a toast
on string using the data passed in |notification|.
// Displays the |notification| identified by |notification_id|.

Is there any sensible way we can provide guarantees about whether it actually
happened, i.e. have this return a bool (or a callback)?

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service.h:28: // Closes a
notification.
// Closes the notification identified by |notification_id|.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service.h:36: // can be
disambiguated.
  // Returns whether the implementation can retrieve a list of currently visible
  // notifications and stores them in |*notification_ids| when possible.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_display_service_factory.cc
(right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service_factory.cc:24:
GetInstance()->GetServiceForBrowserContext(profile, true));
GetInstance()->GetServiceForBrowserContext(profile, true /* create */));

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_display_service_factory.h
(right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service_factory.h:11: #include
"components/keyed_service/content/browser_context_keyed_service_factory.h"
nit: Forward declare the NotificationDisplayService.
nit: I don't think we have to include browser_context_dependency_manager.h here.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service_factory.h:27: };
DISALLOW_COPY_AND_ASSIGN(NotificationDisplayServiceFactory);

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_android.h (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.h:24: // the
existing notifications. Because of this, they return false.
Let's rephrase this entire paragraph, since it's not true anymore.

// Prior to Android Marshmellow, Android did not have the ability to retrieve
// the notifications currently showing for an app without a rather intrusive
// permission. The GetDisplayed() method may return false because of this.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.h:28: // persistent
nit: odd line break

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_mac.mm (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_mac.mm:80:
[notification_center_ removeAllDeliveredNotifications];
TODO for not doing this?

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_mac.mm:209: // For now,
when chrome quits we cancel all pending notifications.
Why is this relevant here?

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_browsertest.cc
(right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_browsertest.cc:134:
service()->SetNotificationDisplayServiceForTesting(display_service_.get());
Why can't we use KeyedServiceFactory::SetTestingFactory()? That way
PlatformNotificationServiceImpl wouldn't have to care at all.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:96:
PlatformNotificationServiceImpl::GetInstance()->GetNotificationDisplayService(
NotificationDisplayServiceFactory::GetForProfile(profile)->Close(notification_id);

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:138:
ProfileManager* profile_manager = g_browser_process->profile_manager();
To confirm, we imagine all of this moving to the NotificationDisplayService,
right?

https://codereview.chromium.org/1814923002/diff/160001/chrome/test/base/testi...
File chrome/test/base/testing_browser_process.cc (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/test/base/testi...
chrome/test/base/testing_browser_process.cc:217: #if defined(OS_ANDROID)
Why not modify the #ifdef on line 221? Displaying the NOTIMPLEMENTED() should be
fine because we shouldn't be calling this in the first place.

[Miguel Garcia, 2016-04-19 14:24:58]

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/chrome_notification_display_service.cc
(right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/chrome_notification_display_service.cc:21:
ui_manager_->Add(notification, profile_);
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> How do you imagine this will be updated with delegates? Right now
> |notification_id| is unused.

Once we start getting mojo objects instead of Notification objects and once the
notifications are generated at the content layer I don't think the mojo object
will necessarily have the notification id. I actually didn't have the id as a
parameter in the first iteration of the cl but you suggested to have it added to
the API in the initial code review.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/chrome_notification_display_service.h (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/chrome_notification_display_service.h:15: //
Implementation of display service for notifications displayed by chrome
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> Meta question: should we have a MessageCenterNotificationBridge instead?
What's
> the benefit of forking on the level of the DisplayManager?

Discussed offline, I found myself converting back and forth from Profile* to
profile_id. While NotificationUIManager exists we need to fork at this level.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/chrome_notification_display_service.h:15: //
Implementation of display service for notifications displayed by chrome
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> Could we call this MessageCenter(Notification)DisplayService, consistent with
> the other message_center_* files in this directory?

OK, it seemed more descriptive this way, went with MessageCenterDisplayService
sine the other one seemed too long.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/chrome_notification_display_service.h:23: void
Display(const std::string& notification_id,
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> nit:
> 
> // NotificationDisplayService implementation.

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/native_notification_display_service.cc
(right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/native_notification_display_service.cc:12:
std::string GetProfileId(Profile* profile) {
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> micro nit: blank lines after namespace opening and before namespace closing.

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/native_notification_display_service.cc:33:
DCHECK(notification_bridge_);
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> nit: DCHECK in the constructor instead? That way we can assume elsewhere.

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/native_notification_display_service.h (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/native_notification_display_service.h:18: //
centers on platforms that support it..
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> micro nit: no double points...

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/native_notification_display_service.h:25: //
NotificationDisplayServiceImplementation.
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> // NotificationDisplayService implementation.

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_bridge.h (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_bridge.h:10: #include "base/macros.h"
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> micro nit: don't we usually have blank lines between STL includes and
> Chrome-based ones?

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_bridge.h:15: // notification.
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> Let's try to be a bit more inclusive. What about:
> 
> // Provides the low-level interface 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.

Except that right now there is no bridge for chrome notifications but ok.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_bridge.h:21: NotificationBridge() {}
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> If we need a static Create method, let's make the constructor protected. A
> hypothetical FakeNotificationBridge could increase visibility as needed.

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_bridge.h:44: // Temporary method while
the refactor is finished.
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> Please do document what it does and why it's significant.

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_display_service.h (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service.h:18: // notification
centers.
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> // 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.

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service.h:24: // Shows a toast
on string using the data passed in |notification|.
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> // Displays the |notification| identified by |notification_id|.
> 
> Is there any sensible way we can provide guarantees about whether it actually
> happened, i.e. have this return a bool (or a callback)?

Not on mac (nor Android I think).If Windows offers it we could add it later.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service.h:28: // Closes a
notification.
On 2016/04/18 14:57:09, Peter Beverloo wrote:
> // Closes the notification identified by |notification_id|.

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service.h:36: // can be
disambiguated.
On 2016/04/18 14:57:09, Peter Beverloo wrote:
>   // Returns whether the implementation can retrieve a list of currently
visible
>   // notifications and stores them in |*notification_ids| when possible.

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_display_service_factory.cc
(right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service_factory.cc:24:
GetInstance()->GetServiceForBrowserContext(profile, true));
On 2016/04/18 14:57:10, Peter Beverloo wrote:
> GetInstance()->GetServiceForBrowserContext(profile, true /* create */));

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_display_service_factory.h
(right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service_factory.h:11: #include
"components/keyed_service/content/browser_context_keyed_service_factory.h"
On 2016/04/18 14:57:10, Peter Beverloo wrote:
> nit: Forward declare the NotificationDisplayService.
> nit: I don't think we have to include browser_context_dependency_manager.h
here.

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service_factory.h:27: };
On 2016/04/18 14:57:10, Peter Beverloo wrote:
> DISALLOW_COPY_AND_ASSIGN(NotificationDisplayServiceFactory);

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_android.h (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.h:24: // the
existing notifications. Because of this, they return false.
On 2016/04/18 14:57:10, Peter Beverloo wrote:
> Let's rephrase this entire paragraph, since it's not true anymore.
> 
> // Prior to Android Marshmellow, Android did not have the ability to retrieve
> // the notifications currently showing for an app without a rather intrusive
> // permission. The GetDisplayed() method may return false because of this.

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_android.h:28: // persistent
On 2016/04/18 14:57:10, Peter Beverloo wrote:
> nit: odd line break

Done.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/notification_ui_manager_mac.mm (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_mac.mm:80:
[notification_center_ removeAllDeliveredNotifications];
On 2016/04/18 14:57:10, Peter Beverloo wrote:
> TODO for not doing this?

I will add the TODO but Robert and I are discussing it and we might still need
to do it in some occasions because the way the XPC connections are established
:(

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/notification_ui_manager_mac.mm:209: // For now,
when chrome quits we cancel all pending notifications.
On 2016/04/18 14:57:10, Peter Beverloo wrote:
> Why is this relevant here?

Used to be relevant because we were indexing by void* profile id. Not anymore.

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_browsertest.cc
(right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_browsertest.cc:134:
service()->SetNotificationDisplayServiceForTesting(display_service_.get());
On 2016/04/18 14:57:10, Peter Beverloo wrote:
> Why can't we use KeyedServiceFactory::SetTestingFactory()? That way
> PlatformNotificationServiceImpl wouldn't have to care at all.

Looked into it, we can for the unit test but the browser tests still need this
method so it seems better to just use the same mechanism in all tests.

https://codereview.chromium.org/1814923002/diff/160001/chrome/test/base/testi...
File chrome/test/base/testing_browser_process.cc (right):

https://codereview.chromium.org/1814923002/diff/160001/chrome/test/base/testi...
chrome/test/base/testing_browser_process.cc:217: #if defined(OS_ANDROID)
On 2016/04/18 14:57:10, Peter Beverloo wrote:
> Why not modify the #ifdef on line 221? Displaying the NOTIMPLEMENTED() should
be
> fine because we shouldn't be calling this in the first place.

SG

[Peter Beverloo, 2016-04-20 17:34:08]

lgtm

This still is in rough shape, but I know we'll follow up so let's do this
iteratively.

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/browser...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/browser...
chrome/browser/browser_process_impl.cc:580: #endif
nit: I think we generally use #if / #else / #endif here to avoid multiple return
statements in a single block, though it doesn't look like we have bots actually
checking for that.

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/notific...
File chrome/browser/notifications/notification_bridge.h (right):

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/notific...
chrome/browser/notifications/notification_bridge.h:19: class NotificationBridge
{
Sorry for the bike-shedding, but what do you think about
NotificationPlatformBridge? Could do in a follow-up as well..

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/notific...
File chrome/browser/notifications/notification_display_service_factory.cc
(right):

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service_factory.cc:42: //
Everything else uses the chrome service
nit: I would phrase this as follows:

// Selection of the implementation works as follows:
//   - Android always uses the NativeNotificationDisplayService.
//   - Mac uses the MessageCenterDisplayService by default, but can use the
//     NativeNotificationDisplayService by using the chrome://flags or the
//     --enable-native-notifications command line flag.
//   - All other platforms always use the MessageCenterDisplayService.

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/notific...
File chrome/browser/notifications/notification_display_service_factory.h
(right):

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/notific...
chrome/browser/notifications/notification_display_service_factory.h:17:
NotificationDisplayServiceFactory();
nit: this can be private w/

#include "base/memory/singleton.h"
friend struct base::DefaultSingletonTraits<NotificationDisplayServiceFactory>;

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:93: void
OnCloseEphimeralNotificationProfileLoaded(
We call them non-persistent notifications.

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:134: //
Callback used to close an ephimeral notification from blink.
non-persistent

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/notific...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.h:54:
NotificationDisplayService* GetNotificationDisplayService(Profile* profile);
TODO on changing to be private for prosperity?

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_browsertest.cc:252: #if
defined(ENABLE_NOTIFICATIONS)
On a tangent, now that iOS doesn't use //chrome anymore I think we can kill
ENABLE_NOTIFICATIONS. Let's keep that in mind :)

[Miguel Garcia, 2016-04-20 18:19:16]

miguelg@chromium.org changed reviewers:
+ sky@chromium.org

[Miguel Garcia, 2016-04-20 18:19:17]

Thanks Peter I will fix all the comments (including the renaming) before
submitting and of course follow up on the bigger picture with further patches

+sky for OWNERS on chrome/browser/ and chrome/test/base/

[sky, 2016-04-20 19:51:59]

LGTM - please get a more local owner for the android changes.

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/browser...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/1814923002/diff/180001/chrome/browser/browser...
chrome/browser/browser_process_impl.cc:580: #endif
On 2016/04/20 17:34:08, Peter Beverloo wrote:
> nit: I think we generally use #if / #else / #endif here to avoid multiple
return
> statements in a single block, though it doesn't look like we have bots
actually
> checking for that.

+1 (same comment for notification_bridge()).

[Miguel Garcia, 2016-04-21 14:32:10]

Ok, all changes done, will land this once the tree opens again.

For the android change, Peter is also OWNERS there and he explicitly said it was
ok to land so I think we are covered.

https://chromiumcodereview.appspot.com/1814923002/diff/160001/chrome/browser/...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://chromiumcodereview.appspot.com/1814923002/diff/160001/chrome/browser/...
chrome/browser/notifications/platform_notification_service_impl.cc:96:
PlatformNotificationServiceImpl::GetInstance()->GetNotificationDisplayService(
On 2016/04/18 14:57:10, Peter Beverloo wrote:
>
NotificationDisplayServiceFactory::GetForProfile(profile)->Close(notification_id);

The follow up CL I noted does this.
https://codereview.chromium.org/1895473002

I cannot do it right now without changing the tests more which makes the CL
bigger unnecessarily

https://chromiumcodereview.appspot.com/1814923002/diff/160001/chrome/browser/...
chrome/browser/notifications/platform_notification_service_impl.cc:138:
ProfileManager* profile_manager = g_browser_process->profile_manager();
On 2016/04/18 14:57:10, Peter Beverloo wrote:
> To confirm, we imagine all of this moving to the NotificationDisplayService,
> right?

Yes, once ids are generated in content.

https://chromiumcodereview.appspot.com/1814923002/diff/180001/chrome/browser/...
File chrome/browser/browser_process_impl.cc (right):

https://chromiumcodereview.appspot.com/1814923002/diff/180001/chrome/browser/...
chrome/browser/browser_process_impl.cc:580: #endif
On 2016/04/20 19:51:59, sky wrote:
> On 2016/04/20 17:34:08, Peter Beverloo wrote:
> > nit: I think we generally use #if / #else / #endif here to avoid multiple
> return
> > statements in a single block, though it doesn't look like we have bots
> actually
> > checking for that.
> 
> +1 (same comment for notification_bridge()).

Done.

https://chromiumcodereview.appspot.com/1814923002/diff/180001/chrome/browser/...
chrome/browser/browser_process_impl.cc:580: #endif
On 2016/04/20 17:34:08, Peter Beverloo wrote:
> nit: I think we generally use #if / #else / #endif here to avoid multiple
return
> statements in a single block, though it doesn't look like we have bots
actually
> checking for that.

Acknowledged.

https://chromiumcodereview.appspot.com/1814923002/diff/180001/chrome/browser/...
File chrome/browser/notifications/notification_display_service_factory.cc
(right):

https://chromiumcodereview.appspot.com/1814923002/diff/180001/chrome/browser/...
chrome/browser/notifications/notification_display_service_factory.cc:42: //
Everything else uses the chrome service
On 2016/04/20 17:34:08, Peter Beverloo wrote:
> nit: I would phrase this as follows:
> 
> // Selection of the implementation works as follows:
> //   - Android always uses the NativeNotificationDisplayService.
> //   - Mac uses the MessageCenterDisplayService by default, but can use the
> //     NativeNotificationDisplayService by using the chrome://flags or the
> //     --enable-native-notifications command line flag.
> //   - All other platforms always use the MessageCenterDisplayService.

Done.

https://chromiumcodereview.appspot.com/1814923002/diff/180001/chrome/browser/...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://chromiumcodereview.appspot.com/1814923002/diff/180001/chrome/browser/...
chrome/browser/notifications/platform_notification_service_impl.cc:93: void
OnCloseEphimeralNotificationProfileLoaded(
On 2016/04/20 17:34:08, Peter Beverloo wrote:
> We call them non-persistent notifications.

Done.

https://chromiumcodereview.appspot.com/1814923002/diff/180001/chrome/browser/...
chrome/browser/notifications/platform_notification_service_impl.cc:134: //
Callback used to close an ephimeral notification from blink.
On 2016/04/20 17:34:08, Peter Beverloo wrote:
> non-persistent

Done.

https://chromiumcodereview.appspot.com/1814923002/diff/180001/chrome/browser/...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://chromiumcodereview.appspot.com/1814923002/diff/180001/chrome/browser/...
chrome/browser/notifications/platform_notification_service_impl.h:54:
NotificationDisplayService* GetNotificationDisplayService(Profile* profile);
On 2016/04/20 17:34:08, Peter Beverloo wrote:
> TODO on changing to be private for prosperity?

Done.

https://chromiumcodereview.appspot.com/1814923002/diff/180001/chrome/browser/...
File chrome/browser/push_messaging/push_messaging_browsertest.cc (right):

https://chromiumcodereview.appspot.com/1814923002/diff/180001/chrome/browser/...
chrome/browser/push_messaging/push_messaging_browsertest.cc:252: #if
defined(ENABLE_NOTIFICATIONS)
On 2016/04/20 17:34:08, Peter Beverloo wrote:
> On a tangent, now that iOS doesn't use //chrome anymore I think we can kill
> ENABLE_NOTIFICATIONS. Let's keep that in mind :)

Acknowledged.

[Miguel Garcia, 2016-04-21 14:37:32]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2016-04-21 14:37:33]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
sky@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1814923002/#ps220001 (title: " ")

[commit-bot: I haz the power, 2016-04-21 14:37:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1814923002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1814923002/220001

[commit-bot: I haz the power, 2016-04-21 15:24:13]

Description was changed from

==========
Nuke NotificationUIManager from PlatformNotificationServiceImpl

A few highlights
- Always use one kind of profile id (the stable one)
- Always use a single notification id (the persistent one when possible)
- The mac implementation injects the NSUSerNotificationService to allow
unittests

Note that the mac and android files are not renamed. Ideally I would land this
and then land a straight forward rename cl.

BUG=571056,596161
==========

to

==========
Nuke NotificationUIManager from PlatformNotificationServiceImpl

A few highlights
- Always use one kind of profile id (the stable one)
- Always use a single notification id (the persistent one when possible)
- The mac implementation injects the NSUSerNotificationService to allow
unittests

Note that the mac and android files are not renamed. Ideally I would land this
and then land a straight forward rename cl.

BUG=571056,596161
==========

[commit-bot: I haz the power, 2016-04-21 15:24:14]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-04-22 19:34:49]

Description was changed from

==========
Nuke NotificationUIManager from PlatformNotificationServiceImpl

A few highlights
- Always use one kind of profile id (the stable one)
- Always use a single notification id (the persistent one when possible)
- The mac implementation injects the NSUSerNotificationService to allow
unittests

Note that the mac and android files are not renamed. Ideally I would land this
and then land a straight forward rename cl.

BUG=571056,596161
==========

to

==========
Nuke NotificationUIManager from PlatformNotificationServiceImpl

A few highlights
- Always use one kind of profile id (the stable one)
- Always use a single notification id (the persistent one when possible)
- The mac implementation injects the NSUSerNotificationService to allow
unittests

Note that the mac and android files are not renamed. Ideally I would land this
and then land a straight forward rename cl.

BUG=571056,596161

Committed: https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418
Cr-Commit-Position: refs/heads/master@{#388776}
==========

[commit-bot: I haz the power, 2016-04-22 19:34:50]

Patchset 10 (id:??) landed as
https://crrev.com/23cd2dd719c8e76e17e764100a6eff131c34a418
Cr-Commit-Position: refs/heads/master@{#388776}