Merge the code paths for closing different kinds of notifications.

Merge the code paths for closing different kinds of notifications.

BUG=

Committed: https://crrev.com/2d0f25366dfd49e463110059e5b5675a05ffb645
Cr-Commit-Position: refs/heads/master@{#425682}

[Peter Beverloo, 2016-09-16 16:51:20]

peter@chromium.org changed reviewers:
+ harkness@chromium.org, johnme@chromium.org

[Peter Beverloo, 2016-09-16 16:51:21]

+harkness for initial review

https://codereview.chromium.org/2344983003/diff/1/content/child/notifications...
File content/child/notifications/notification_manager.cc (right):

https://codereview.chromium.org/2344983003/diff/1/content/child/notifications...
content/child/notifications/notification_manager.cc:203:
blink::WebNotificationDelegate* delegate) {
I'm not super happy about this function, but since it's still O([total
non-closed notifications for the page]) I guess we can live with it.

[Peter Beverloo, 2016-09-16 16:51:32]

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

[commit-bot: I haz the power, 2016-09-16 16:51:59]

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, 2016-09-16 18:08:02]

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

[commit-bot: I haz the power, 2016-09-16 18:08:03]

Dry run: This issue passed the CQ dry run.

[harkness, 2016-09-23 15:23:25]

lgtm

https://codereview.chromium.org/2344983003/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_message_filter.cc (right):

https://codereview.chromium.org/2344983003/diff/1/content/browser/notificatio...
content/browser/notifications/notification_message_filter.cc:320: if
(NotificationIdGenerator::IsPersistentNotification(notification_id)) {
Is this one of the code paths that you will be able to merge more in the future?
Right now this structure makes it almost harder to read.

https://codereview.chromium.org/2344983003/diff/1/content/child/notifications...
File content/child/notifications/notification_manager.cc (right):

https://codereview.chromium.org/2344983003/diff/1/content/child/notifications...
content/child/notifications/notification_manager.cc:203:
blink::WebNotificationDelegate* delegate) {
On 2016/09/16 16:51:20, Peter Beverloo wrote:
> I'm not super happy about this function, but since it's still O([total
> non-closed notifications for the page]) I guess we can live with it.

Acknowledged.

[Peter Beverloo, 2016-09-23 15:25:33]

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

[commit-bot: I haz the power, 2016-09-23 15:25:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Beverloo, 2016-09-23 15:28:34]

Thanks Jen! John, WDYT?

https://codereview.chromium.org/2344983003/diff/1/content/browser/notificatio...
File content/browser/notifications/notification_message_filter.cc (right):

https://codereview.chromium.org/2344983003/diff/1/content/browser/notificatio...
content/browser/notifications/notification_message_filter.cc:320: if
(NotificationIdGenerator::IsPersistentNotification(notification_id)) {
On 2016/09/23 15:23:25, harkness wrote:
> Is this one of the code paths that you will be able to merge more in the
future?
> Right now this structure makes it almost harder to read. 

I hope so, as that will allow us to kill the PageNotificationDelegate too.

[johnme, 2016-09-23 16:14:59]

lgtm with nits

https://codereview.chromium.org/2344983003/diff/20001/content/child/notificat...
File content/child/notifications/notification_manager.cc (right):

https://codereview.chromium.org/2344983003/diff/20001/content/child/notificat...
content/child/notifications/notification_manager.cc:232:
IPC_MESSAGE_HANDLER(PlatformNotificationMsg_DidClick, OnDidClick)
Why did you reorder this, but not the corresponding method definitions?

https://codereview.chromium.org/2344983003/diff/20001/content/child/notificat...
content/child/notifications/notification_manager.cc:248: return;  // the
notification has been destroyed by Blink since
Nit: capital T, full stop

https://codereview.chromium.org/2344983003/diff/20001/content/child/notificat...
content/child/notifications/notification_manager.cc:253:
non_persistent_notification_ids_[notification_id] =
Move this before the didShowNotification, so that it's called with valid state
(avoids future surprises)

https://codereview.chromium.org/2344983003/diff/20001/content/child/notificat...
content/child/notifications/notification_manager.cc:283:
non_persistent_notification_ids_.erase(notification_id);
Nit: is it worth being defensive and erasing this even if iter == end()?

https://codereview.chromium.org/2344983003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/2344983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.cpp:170: if
(m_state != State::Showing || m_notificationId.isEmpty())
Is this possible? Or can it just be a DCHECK? If it is possible, should you set
state to Closing here and have didShowNotification perform the close if state is
Closing?

https://codereview.chromium.org/2344983003/diff/20001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/notifications/WebNotificationDelegate.h
(right):

https://codereview.chromium.org/2344983003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/notifications/WebNotificationDelegate.h:21:
virtual void dispatchErrorEvent() = 0;
Remove this from WebNotificationDelegate. It's only called from
Notification.cpp, so it can become a private method of Notification.

[commit-bot: I haz the power, 2016-09-23 16:44:21]

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

[commit-bot: I haz the power, 2016-09-23 16:44:22]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2016-10-04 10:25:39]

Patchset #3 (id:40001) has been deleted

[Peter Beverloo, 2016-10-04 10:27:22]

peter@chromium.org changed reviewers:
+ isherman@chromium.org, nasko@chromium.org

[Peter Beverloo, 2016-10-04 10:27:23]

+nasko for //content and IPC
+isherman for histograms

https://codereview.chromium.org/2344983003/diff/20001/content/child/notificat...
File content/child/notifications/notification_manager.cc (right):

https://codereview.chromium.org/2344983003/diff/20001/content/child/notificat...
content/child/notifications/notification_manager.cc:232:
IPC_MESSAGE_HANDLER(PlatformNotificationMsg_DidClick, OnDidClick)
On 2016/09/23 16:14:58, johnme wrote:
> Why did you reorder this, but not the corresponding method definitions?

Done.

https://codereview.chromium.org/2344983003/diff/20001/content/child/notificat...
content/child/notifications/notification_manager.cc:248: return;  // the
notification has been destroyed by Blink since
On 2016/09/23 16:14:58, johnme wrote:
> Nit: capital T, full stop

Done.

https://codereview.chromium.org/2344983003/diff/20001/content/child/notificat...
content/child/notifications/notification_manager.cc:253:
non_persistent_notification_ids_[notification_id] =
On 2016/09/23 16:14:58, johnme wrote:
> Move this before the didShowNotification, so that it's called with valid state
> (avoids future surprises)

Done.

https://codereview.chromium.org/2344983003/diff/20001/content/child/notificat...
content/child/notifications/notification_manager.cc:283:
non_persistent_notification_ids_.erase(notification_id);
On 2016/09/23 16:14:58, johnme wrote:
> Nit: is it worth being defensive and erasing this even if iter == end()?

Acknowledged.

https://codereview.chromium.org/2344983003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/2344983003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.cpp:170: if
(m_state != State::Showing || m_notificationId.isEmpty())
On 2016/09/23 16:14:58, johnme wrote:
> Is this possible? Or can it just be a DCHECK? If it is possible, should you
set
> state to Closing here and have didShowNotification perform the close if state
is
> Closing?

This would happen in time it takes for the Show IPC to round-trip for
non-persistent notifications.

I've added an m_requestedClose to do this in a delayed manner, and added a TODO
for canceling the load instead of waiting for it to finish. Will follow up after
the reformat.

https://codereview.chromium.org/2344983003/diff/20001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/notifications/WebNotificationDelegate.h
(right):

https://codereview.chromium.org/2344983003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/notifications/WebNotificationDelegate.h:21:
virtual void dispatchErrorEvent() = 0;
On 2016/09/23 16:14:58, johnme wrote:
> Remove this from WebNotificationDelegate. It's only called from
> Notification.cpp, so it can become a private method of Notification.

Done.

[Peter Beverloo, 2016-10-04 10:27:35]

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

[commit-bot: I haz the power, 2016-10-04 10:27:45]

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, 2016-10-04 12:03:08]

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

[commit-bot: I haz the power, 2016-10-04 12:03:09]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2016-10-05 00:32:24]

histograms.xml lgtm

[nasko, 2016-10-05 20:14:04]

LGTM

https://codereview.chromium.org/2344983003/diff/60001/content/browser/bad_mes...
File content/browser/bad_message.h (right):

https://codereview.chromium.org/2344983003/diff/60001/content/browser/bad_mes...
content/browser/bad_message.h:164: NMF_INVALID_ID_CLOSE = 140,
You are racing another CL that uses the same number. Just be aware it might land
first.

[Peter Beverloo, 2016-10-17 13:25:01]

Thanks! We're plenty past branch point; landing this now.

[Peter Beverloo, 2016-10-17 13:26:14]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-10-17 13:26:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
harkness@chromium.org, johnme@chromium.org, isherman@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2344983003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-17 13:26:28]

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, 2016-10-17 15:19:16]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-10-17 15:21:40]

Description was changed from

==========
Merge the code paths for closing different kinds of notifications.

BUG=
==========

to

==========
Merge the code paths for closing different kinds of notifications.

BUG=

Committed: https://crrev.com/2d0f25366dfd49e463110059e5b5675a05ffb645
Cr-Commit-Position: refs/heads/master@{#425682}
==========

[commit-bot: I haz the power, 2016-10-17 15:21:42]

Patchset 5 (id:??) landed as
https://crrev.com/2d0f25366dfd49e463110059e5b5675a05ffb645
Cr-Commit-Position: refs/heads/master@{#425682}