Deprecate per notification type delegates.

Deprecate per notification type delegates.

Both persistent and non persistent notifications can now be served from
the same delegate which in turn proxies all requests to the handlers.

BUG=720345
Review-Url: https://codereview.chromium.org/2906883003
Cr-Commit-Position: refs/heads/master@{#478263}
Committed: https://chromium.googlesource.com/chromium/src/+/40b10fd5dcb862ce08df81ef5c55c629f64c6394

[Miguel Garcia, 2017-05-26 15:32:01]

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 15:32:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 15:32:17]

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

[commit-bot: I haz the power, 2017-05-26 15:32:18]

Dry run: CLs for remote refs other than refs/heads/master must contain
NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them

[Miguel Garcia, 2017-06-06 12:53:11]

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 12:53:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Miguel Garcia, 2017-06-06 12:59:29]

Note to self: The following tests pass now (they use to time out which I believe
meant they were broken)
Expected to timeout, but passed: (3)
  http/tests/notifications/click-shared-worker.html
  http/tests/notifications/close-shared-worker.html
  http/tests/notifications/update-shared-worker.html

Consider re-enabling them on a separate CL once this lands.

[Miguel Garcia, 2017-06-06 13:01:34]

The CQ bit was checked by miguelg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 13:01:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Miguel Garcia, 2017-06-06 14:01:58]

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

[commit-bot: I haz the power, 2017-06-06 14:36:49]

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

[commit-bot: I haz the power, 2017-06-06 14:36:51]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-06-08 06:48:11]

Cool!! lgtm

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

https://codereview.chromium.org/2906883003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.cc:61: bool
WebNotificationDelegate::ShouldDisplayOverFullscreen() const {
I guess these buttons should also migrate to the NotificationHandler eventually?

https://codereview.chromium.org/2906883003/diff/40001/content/browser/notific...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/2906883003/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:403:
DCHECK(renderer_ids_[notification_id] == renderer_id);
I don't think this DCHECK is valid. What if an origin runs in multiple renderer
processes?

https://codereview.chromium.org/2906883003/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:404:
DispatchNonPersistentCloseEvent(notification_id);
This feels like the wrong layer. Right now the Message Center UI Manager will
fire this event on the delegate. I guess doing that would require the NDS to get
the list of shown notifications, check if the notification ID is in it, and then
manually fire the close event?

This should be a TODO.

https://codereview.chromium.org/2906883003/diff/40001/content/test/mock_platf...
File content/test/mock_platform_notification_service.cc (right):

https://codereview.chromium.org/2906883003/diff/40001/content/test/mock_platf...
content/test/mock_platform_notification_service.cc:46:
content::NotificationEventDispatcher::GetInstance()
nit: drop content::, elsewhere too (I see that there is some precedent, sorry)

https://codereview.chromium.org/2906883003/diff/40001/content/test/mock_platf...
content/test/mock_platform_notification_service.cc:172:
->DispatchNonPersistentCloseEvent(notification_id);
Should we check for having a valid |non_persistent_iter| first?

https://codereview.chromium.org/2906883003/diff/40001/content/test/mock_platf...
content/test/mock_platform_notification_service.cc:187: }
Hmm. Since calling erase() with a key that's in in the map is safe, we could now
just replace this method with one that's two lines: 184 and 186

[Miguel Garcia, 2017-06-08 06:51:56]

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

[Miguel Garcia, 2017-06-08 06:51:57]

Thanks! 

+avi for the content layer

[Avi (use Gerrit), 2017-06-08 14:49:43]

lgtm stamp

[Miguel Garcia, 2017-06-09 12:24:45]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2017-06-09 12:24:46]

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

[Miguel Garcia, 2017-06-09 12:24:52]

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

https://codereview.chromium.org/2906883003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/web_notification_delegate.cc:61: bool
WebNotificationDelegate::ShouldDisplayOverFullscreen() const {
On 2017/06/08 06:48:11, Peter Beverloo (OOO - Jun 12) wrote:
> I guess these buttons should also migrate to the NotificationHandler
eventually?

Yes! in progress CL for that
https://codereview.chromium.org/2921263002/patch/160001/170033

https://codereview.chromium.org/2906883003/diff/40001/content/browser/notific...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/2906883003/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:403:
DCHECK(renderer_ids_[notification_id] == renderer_id);
On 2017/06/08 06:48:11, Peter Beverloo (OOO - Jun 12) wrote:
> I don't think this DCHECK is valid. What if an origin runs in multiple
renderer
> processes?

Good point. Removed

https://codereview.chromium.org/2906883003/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:404:
DispatchNonPersistentCloseEvent(notification_id);
On 2017/06/08 06:48:11, Peter Beverloo (OOO - Jun 12) wrote:
> This feels like the wrong layer. Right now the Message Center UI Manager will
> fire this event on the delegate. I guess doing that would require the NDS to
get
> the list of shown notifications, check if the notification ID is in it, and
then
> manually fire the close event?
> 
> This should be a TODO.

Done.

https://codereview.chromium.org/2906883003/diff/40001/content/test/mock_platf...
File content/test/mock_platform_notification_service.cc (right):

https://codereview.chromium.org/2906883003/diff/40001/content/test/mock_platf...
content/test/mock_platform_notification_service.cc:46:
content::NotificationEventDispatcher::GetInstance()
On 2017/06/08 06:48:11, Peter Beverloo (OOO - Jun 12) wrote:
> nit: drop content::, elsewhere too (I see that there is some precedent, sorry)

Done.

https://codereview.chromium.org/2906883003/diff/40001/content/test/mock_platf...
content/test/mock_platform_notification_service.cc:172:
->DispatchNonPersistentCloseEvent(notification_id);
On 2017/06/08 06:48:11, Peter Beverloo (OOO - Jun 12) wrote:
> Should we check for having a valid |non_persistent_iter| first?

Done.

https://codereview.chromium.org/2906883003/diff/40001/content/test/mock_platf...
content/test/mock_platform_notification_service.cc:187: }
On 2017/06/08 06:48:11, Peter Beverloo (OOO - Jun 12) wrote:
> Hmm. Since calling erase() with a key that's in in the map is safe, we could
now
> just replace this method with one that's two lines: 184 and 186

Good point

[commit-bot: I haz the power, 2017-06-09 12:24:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 13:54:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497011085976750,
"parent_rev": "fc202b2b1174f5732f7b9e63a6c46fd01ac2be02", "commit_rev":
"40b10fd5dcb862ce08df81ef5c55c629f64c6394"}

[commit-bot: I haz the power, 2017-06-09 13:54:28]

Description was changed from

==========
Deprecate per notification type delegates.

Both persistent and non persistent notifications can now be served from
the same delegate which in turn proxies all requests to the handlers.

BUG=720345
==========

to

==========
Deprecate per notification type delegates.

Both persistent and non persistent notifications can now be served from
the same delegate which in turn proxies all requests to the handlers.

BUG=720345

Review-Url: https://codereview.chromium.org/2906883003
Cr-Commit-Position: refs/heads/master@{#478263}
Committed:
https://chromium.googlesource.com/chromium/src/+/40b10fd5dcb862ce08df81ef5c55...
==========

[commit-bot: I haz the power, 2017-06-09 13:54:29]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/40b10fd5dcb862ce08df81ef5c55...