PlatformNotificationService layering cleanup.

PlatformNotificationService layering cleanup.

Make the display service execute the delegate->Display() call instead of having every bridge do it.

Access NotificationDisplayService from the profile directly
Simplify how PlatformNotificationServiceImpl unittest access the display
service to make it more similar to production.
Make GetNotificationUIManager private.

BUG=596161
Committed: https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff
Cr-Commit-Position: refs/heads/master@{#389736}

[Miguel Garcia, 2016-04-22 15:28:27]

Description was changed from

==========
PlatformNotificationService layering cleanup.

Access NotificationDisplayService from the profile directly
Simplify how PlatformNotificationServiceImpl unittest access the display
service to make it more similar to production.
Make GetNotificationUIManager private.

BUG=596161
==========

to

==========
PlatformNotificationService layering cleanup.

Make the display service execute the delegate->Display() call instead of having
every bridge do it.

Access NotificationDisplayService from the profile directly
Simplify how PlatformNotificationServiceImpl unittest access the display
service to make it more similar to production.
Make GetNotificationUIManager private.

BUG=596161
==========

[Miguel Garcia, 2016-04-25 10:57:14]

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

[Miguel Garcia, 2016-04-25 10:57:15]

PTAL

[Peter Beverloo, 2016-04-25 13:37:29]

Thanks!

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_unittest.cc
(right):

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_unittest.cc:159:
#endif
This is why I'd like this test to use a mocked NotificationDisplayService
instead, it shouldn't have to care about these sorts of differences. (That's an
implementation detail of the NotificationDisplayService.)

That said, we're still in the same boat for the PNSI as well, so we should
figure out a plan and address all cases at once.

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_unittest.cc:207:
const Notification& notification = GetDisplayedNotification();
nit: no const& (GetDisplayedNotification returns by value)

elsewhere too

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/stub_notification_platform_bridge.cc (right):

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/stub_notification_platform_bridge.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
2014??

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/stub_notification_platform_bridge.cc:32: }
This should probably care about the |profile_id|? Should we have a map of
vectors?

Also why doesn't this have the incognito boolean?

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

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/stub_notification_platform_bridge.h:4: #ifndef
CHROME_BROWSER_NOTIFICATIONS_STUB_NOTIFICATION_PLATFORM_BRIDGE_H_
nit: blank line above here.

[Miguel Garcia, 2016-04-25 17:19:59]

PTAL

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_unittest.cc
(right):

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_unittest.cc:159:
#endif
Yeah I knew you would not like this one. I was hoping I would be able to change
the test just from the public interface (like I did with GetNotificationCount)
but all in all this seems like a net improvement.

On 2016/04/25 13:37:29, Peter Beverloo wrote:
> This is why I'd like this test to use a mocked NotificationDisplayService
> instead, it shouldn't have to care about these sorts of differences. (That's
an
> implementation detail of the NotificationDisplayService.)
> 
> That said, we're still in the same boat for the PNSI as well, so we should
> figure out a plan and address all cases at once.

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_unittest.cc:207:
const Notification& notification = GetDisplayedNotification();
On 2016/04/25 13:37:29, Peter Beverloo wrote:
> nit: no const& (GetDisplayedNotification returns by value)
> 
> elsewhere too

Done.

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
File chrome/browser/notifications/stub_notification_platform_bridge.cc (right):

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/stub_notification_platform_bridge.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2016/04/25 13:37:29, Peter Beverloo wrote:
> 2014??

Done.

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/stub_notification_platform_bridge.cc:32: }
On 2016/04/25 13:37:29, Peter Beverloo wrote:
> This should probably care about the |profile_id|? Should we have a map of
> vectors?
> 
> Also why doesn't this have the incognito boolean?

Moved to a map of vectors

Happy to discuss if the incognito boolean is needed in the API and add it (in a
separate cl I assume).I actually think we might want to consider removing the
boolean everywhere since notifications is not supported in incognito these days.

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

https://codereview.chromium.org/1895473002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/stub_notification_platform_bridge.h:4: #ifndef
CHROME_BROWSER_NOTIFICATIONS_STUB_NOTIFICATION_PLATFORM_BRIDGE_H_
On 2016/04/25 13:37:29, Peter Beverloo wrote:
> nit: blank line above here.

Done.

[Peter Beverloo, 2016-04-25 17:40:31]

lgtm

https://codereview.chromium.org/1895473002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/1895473002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.h:143: // This
can be overriden in tests
  // TODO(miguelg): Remove this method in favor of providing a testing factory
  // to the NotificationDisplayServiceFactory.

https://codereview.chromium.org/1895473002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/stub_notification_platform_bridge.h (right):

https://codereview.chromium.org/1895473002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/stub_notification_platform_bridge.h:37: //
ProfileId -> Notification list
  // Map of profile Ids to list of notifications shown for said profile.

https://codereview.chromium.org/1895473002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/stub_notification_platform_bridge.h:38:
std::map<std::string, std::vector<Notification>> notifications_;
nit: std::unordered_map

[Miguel Garcia, 2016-04-25 18:45:33]

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

[Miguel Garcia, 2016-04-25 18:45:33]

+sky@ for owners in chrome/test/base/ and the change in chrome/browser/BUILD.gn

https://codereview.chromium.org/1895473002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/platform_notification_service_impl.h (right):

https://codereview.chromium.org/1895473002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.h:143: // This
can be overriden in tests
On 2016/04/25 17:40:31, Peter Beverloo wrote:
>   // TODO(miguelg): Remove this method in favor of providing a testing factory
>   // to the NotificationDisplayServiceFactory.

Done.

https://codereview.chromium.org/1895473002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/stub_notification_platform_bridge.h (right):

https://codereview.chromium.org/1895473002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/stub_notification_platform_bridge.h:38:
std::map<std::string, std::vector<Notification>> notifications_;
On 2016/04/25 17:40:31, Peter Beverloo wrote:
> nit: std::unordered_map

Done.

[Miguel Garcia, 2016-04-25 18:46:09]

https://chromiumcodereview.appspot.com/1895473002/diff/60001/chrome/browser/n...
File chrome/browser/notifications/stub_notification_platform_bridge.h (right):

https://chromiumcodereview.appspot.com/1895473002/diff/60001/chrome/browser/n...
chrome/browser/notifications/stub_notification_platform_bridge.h:37: //
ProfileId -> Notification list
On 2016/04/25 17:40:31, Peter Beverloo wrote:
>   // Map of profile Ids to list of notifications shown for said profile.

Done.

[sky, 2016-04-25 21:04:40]

LGTM

[Miguel Garcia, 2016-04-26 07:01:19]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2016-04-26 07:01:19]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1895473002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-04-26 07:01:34]

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

[commit-bot: I haz the power, 2016-04-26 08:43:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-26 08:43:11]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Miguel Garcia, 2016-04-26 09:23:44]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2016-04-26 09:23:45]

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://codereview.chromium.org/1895473002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-04-26 09:24:04]

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

[commit-bot: I haz the power, 2016-04-26 09:52:01]

Description was changed from

==========
PlatformNotificationService layering cleanup.

Make the display service execute the delegate->Display() call instead of having
every bridge do it.

Access NotificationDisplayService from the profile directly
Simplify how PlatformNotificationServiceImpl unittest access the display
service to make it more similar to production.
Make GetNotificationUIManager private.

BUG=596161
==========

to

==========
PlatformNotificationService layering cleanup.

Make the display service execute the delegate->Display() call instead of having
every bridge do it.

Access NotificationDisplayService from the profile directly
Simplify how PlatformNotificationServiceImpl unittest access the display
service to make it more similar to production.
Make GetNotificationUIManager private.

BUG=596161
==========

[commit-bot: I haz the power, 2016-04-26 09:52:02]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-26 09:52:53]

Description was changed from

==========
PlatformNotificationService layering cleanup.

Make the display service execute the delegate->Display() call instead of having
every bridge do it.

Access NotificationDisplayService from the profile directly
Simplify how PlatformNotificationServiceImpl unittest access the display
service to make it more similar to production.
Make GetNotificationUIManager private.

BUG=596161
==========

to

==========
PlatformNotificationService layering cleanup.

Make the display service execute the delegate->Display() call instead of having
every bridge do it.

Access NotificationDisplayService from the profile directly
Simplify how PlatformNotificationServiceImpl unittest access the display
service to make it more similar to production.
Make GetNotificationUIManager private.

BUG=596161

Committed: https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff
Cr-Commit-Position: refs/heads/master@{#389736}
==========

[commit-bot: I haz the power, 2016-04-26 09:52:55]

Patchset 6 (id:??) landed as
https://crrev.com/884b3ecf7101cdf41820b896f5e7ed34cd3de2ff
Cr-Commit-Position: refs/heads/master@{#389736}