Implement notificationclose event

Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

Layout tests to come in a follow-up patch:
https://codereview.chromium.org/1656823002/

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fePanzKy6ec

BUG=579150
TEST=PlatformNotificationServiceBrowserTest.UserClosesPersistentNotification

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

[Nina, 2016-01-21 13:55:36]

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

[Nina, 2016-01-21 13:55:37]

Peter,

Please give this patch an initial review. It requires polishing, especially
NotificationEventDispatcherImpl. I left some comments on the code as well.

Thanks,
Nico.

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

https://codereview.chromium.org/1619703002/diff/20001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:100: if
(action_index == -2) {
This is the thing I like the least about my CL. This class seems to expect its
functions will only be called for NotificationClick events and binds the
callback in a way it's difficult to reuse its code.

Right now DispatchNotification*Event binds ReadNotificationDatabaseData, which
binds FindServiceWorkerRegistration, which binds
DispatchNotificationClickEventOnRegistration. Maybe we should add a parameter to
the callback stack to let FindServiceWorkerRegistration call either
DispatchNotificationClickOnRegistration or
DispatchNotificationCloseEventOnRegistration?

https://codereview.chromium.org/1619703002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1619703002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:659: void
ServiceWorkerVersion::DispatchNotificationCloseEvent(
Maybe we should refactor a little to avoid code repetition with
DispatchNotificationClickEvent.

https://codereview.chromium.org/1619703002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp
(right):

https://codereview.chromium.org/1619703002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:174:
m_consumeWindowInteractionTimer.stop();
I can't figure out what this does. Mind giving me a hint?

[Peter Beverloo, 2016-01-21 14:17:37]

Cool, this is exciting! We need to talk process (spec & intent to implement) :-)

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

https://codereview.chromium.org/1619703002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:72: void
OnPersistentNotificationDataDeleted(bool success) {
nit: This is unused now.

If we will deprecate the Notifications.PersistentNotificationDataDeleted, you
can use the <obsolete> annotation in histograms.xml.

https://codereview.chromium.org/1619703002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:91:
BrowserThread::PostTask(
Since we're already calling into content per DispatchNotificationCloseEvent, we
could remove DeleteNotificationData from the public //content API and instead
just call it directly. (See e.g. the ReadNotificationDatabaseData helper part of
the NotificationEventDispatcherImpl.)

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

https://codereview.chromium.org/1619703002/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:100: if
(action_index == -2) {
Yes, this will need some work :-). Adding yet another magic value to
|action_index| isn't great. Perhaps we can share the loading code and use a
closure for doing the specific action, whether it be a click or close event?

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

https://codereview.chromium.org/1619703002/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:31: void
DispatchNotificationCloseEvent(
micro nit: no blank line above here, since the comment on line 22 applies to the
block.

https://codereview.chromium.org/1619703002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1619703002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.h:273: // Sends
notificationclose event to the associated embedded worker and
You might want to take a look at the following CL, which is going to create a
conflict:

https://codereview.chromium.org/1575283003/

https://codereview.chromium.org/1619703002/diff/40001/content/public/browser/...
File content/public/browser/notification_event_dispatcher.h (right):

https://codereview.chromium.org/1619703002/diff/40001/content/public/browser/...
content/public/browser/notification_event_dispatcher.h:53: 
micro nit: blank line too much.

https://codereview.chromium.org/1619703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1619703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:161:
RefPtrWillBeRawPtr<Event>
event(NotificationEvent::create(EventTypeNames::notificationclose, eventInit,
observer));
Mm. We'll create the notification with |m_state == NotificationStateShowing|,
which of course is not correct. Perhaps we should have a method or a flag to
advance the state to closed?

(This being the case when the developer calls |notification.close();| in
JavaScript.)

https://codereview.chromium.org/1619703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:165: 
micro nit: blank line too much

[Nina, 2016-01-25 14:54:54]

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

[Nina, 2016-01-25 14:54:55]

Hi Peter,

I believe I've addressed the comments. Please give the updated patch another
look (especially that NotificationEventDispatcher :))

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

https://codereview.chromium.org/1619703002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:72: void
OnPersistentNotificationDataDeleted(bool success) {
On 2016/01/21 14:17:37, Peter Beverloo wrote:
> nit: This is unused now.
> 
> If we will deprecate the Notifications.PersistentNotificationDataDeleted, you
> can use the <obsolete> annotation in histograms.xml.

Removed and deprecated histogram.

https://codereview.chromium.org/1619703002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:91:
BrowserThread::PostTask(
On 2016/01/21 14:17:37, Peter Beverloo wrote:
> Since we're already calling into content per DispatchNotificationCloseEvent,
we
> could remove DeleteNotificationData from the public //content API and instead
> just call it directly. (See e.g. the ReadNotificationDatabaseData helper part
of
> the NotificationEventDispatcherImpl.)

Refactored code to avoid this ping - pong between content and browser.

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

https://codereview.chromium.org/1619703002/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:100: if
(action_index == -2) {
On 2016/01/21 14:17:37, Peter Beverloo wrote:
> Yes, this will need some work :-). Adding yet another magic value to
> |action_index| isn't great. Perhaps we can share the loading code and use a
> closure for doing the specific action, whether it be a click or close event?

I used a closure... the result is not super elegant but works and is DRY. Please
give it a look.

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

https://codereview.chromium.org/1619703002/diff/40001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:31: void
DispatchNotificationCloseEvent(
On 2016/01/21 14:17:37, Peter Beverloo wrote:
> micro nit: no blank line above here, since the comment on line 22 applies to
the
> block.

Done.

https://codereview.chromium.org/1619703002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1619703002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.h:273: // Sends
notificationclose event to the associated embedded worker and
On 2016/01/21 14:17:37, Peter Beverloo wrote:
> You might want to take a look at the following CL, which is going to create a
> conflict:
> 
> https://codereview.chromium.org/1575283003/

Thanks for the heads up!

https://codereview.chromium.org/1619703002/diff/40001/content/public/browser/...
File content/public/browser/notification_event_dispatcher.h (right):

https://codereview.chromium.org/1619703002/diff/40001/content/public/browser/...
content/public/browser/notification_event_dispatcher.h:53: 
On 2016/01/21 14:17:37, Peter Beverloo wrote:
> micro nit: blank line too much.

Done.

[Nina, 2016-01-26 10:45:10]

Description was changed from

==========
[WIP] Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

WIP: this is an initial implementation and needs polishing. Uploading to
kick off the discussion.

BUG=579150
==========

to

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

BUG=579150
==========

[Nina, 2016-01-26 10:46:22]

Description was changed from

==========
[WIP] Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

WIP: this is an initial implementation and needs polishing. Uploading to
kick off the discussion.

BUG=579150
==========

to

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.


BUG=579150
==========

[Nina, 2016-01-26 10:46:33]

Description was changed from

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.


BUG=579150
==========

to

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

BUG=579150
==========

[Nina, 2016-01-26 10:48:05]

Description was changed from

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.


BUG=579150
==========

to

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

BUG=579150
==========

[Peter Beverloo, 2016-01-26 16:13:45]

Cool, I like the clean-up! I think we can go a little bit further in that area,
and we need to figure out where to make the decision to fire the event or not,
see my comment in the PlatformNotificationServiceImpl.

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

https://codereview.chromium.org/1619703002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:91:
BrowserThread::PostTask(
On 2016/01/25 14:54:55, Nicolás wrote:
> On 2016/01/21 14:17:37, Peter Beverloo wrote:
> > Since we're already calling into content per DispatchNotificationCloseEvent,
> we
> > could remove DeleteNotificationData from the public //content API and
instead
> > just call it directly. (See e.g. the ReadNotificationDatabaseData helper
part
> of
> > the NotificationEventDispatcherImpl.)
> 
> Refactored code to avoid this ping - pong between content and browser.

Note that it's between //chrome and //content - both in the browser process.
It's just layering :-)

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

https://codereview.chromium.org/1619703002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:225:
content::RecordAction(base::UserMetricsAction(
Even when a notification was clicked by the user, we'd still want it to be
removed from the NotificationDatabase. In other words, //content always has to
be told about the closure.

I think what we'll want here is something like...

if (by_user) { uma_one }
else { uma_two }

content::NED->DispatchNotificationCloseEvent(..., by_user);

and then make the decision on whether or not to fire the event in //content.

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

https://codereview.chromium.org/1619703002/diff/60001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:40: void
SWNotificationEventFinished(
nit: s/SW/ServiceWorker/ (we don't usually use acronyms like this)

https://codereview.chromium.org/1619703002/diff/60001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:214: if
(success) {
You could reduce duplication by branching for the arguments only -

NotificationEventFinished(
    dispatch_complete_callback,
    success ? PERSISTENT_NOTIFICATION_STATUS_SUCCESS
            : PERSISTENT_NOTIFICATION_STATUS_DATABASE_ERROR);

https://codereview.chromium.org/1619703002/diff/60001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:230:
ServiceWorkerStatusCode status_code) {
We'll want to return the result status of dispatching the close event in this
case as well, e.g. consider |status_code| like SWNotificationEventFinished()
does. That would allow for more comprehensive UMA in the //chrome layer. WDYT?

https://codereview.chromium.org/1619703002/diff/60001/content/browser/notific...
File content/browser/notifications/notification_event_dispatcher_impl.h (right):

https://codereview.chromium.org/1619703002/diff/60001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:56: void
DispatchNotificationEvent(
Is there a reason that we'd like to expose this in the header? It could be an
anonymous function in the implementation (and with it, the typedefs + additional
forward declarations).

https://codereview.chromium.org/1619703002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1619703002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:684: void
ServiceWorkerVersion::DispatchNotificationCloseEvent(
(I'm not really looking at this bit because Marijn's CL will land before yours,
which will require some further refactoring --
https://codereview.chromium.org/1575283003/)

https://codereview.chromium.org/1619703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/1619703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.h:69: //
Notification's state as showing, or as closing otherwise.
closing? I think we should initialize it as NotificationStateClosed (the event
will fire *after* the notification has closed)

https://codereview.chromium.org/1619703002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1619703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:34:
notifications.append(Notification::create(resolver->executionContext(),
notificationInfo.persistentId, notificationInfo.data, true));
, true /* showing */)

[Nina, 2016-01-27 18:31:29]

Description was changed from

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

BUG=579150
==========

to

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

More tests and UMA metrics to come in a follow-up patch.

BUG=579150
TEST=PlatformNotificationServiceBrowserTest.UserClosesPersistentNotification
==========

[Nina, 2016-01-27 18:48:58]

I like that the notification click dispatch has been moved out of
ServiceWorkerVersion [1], a bunch of plumbing has been removed, but I did have
to update NotificationEventDispatcher. Please take a look at the new patch. 

[1] https://codereview.chromium.org/1575283003/

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

https://codereview.chromium.org/1619703002/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:91:
BrowserThread::PostTask(
On 2016/01/26 16:13:45, Peter Beverloo wrote:
> On 2016/01/25 14:54:55, Nicolás wrote:
> > On 2016/01/21 14:17:37, Peter Beverloo wrote:
> > > Since we're already calling into content per
DispatchNotificationCloseEvent,
> > we
> > > could remove DeleteNotificationData from the public //content API and
> instead
> > > just call it directly. (See e.g. the ReadNotificationDatabaseData helper
> part
> > of
> > > the NotificationEventDispatcherImpl.)
> > 
> > Refactored code to avoid this ping - pong between content and browser.
> 
> Note that it's between //chrome and //content - both in the browser process.
> It's just layering :-)

Acknowledged.

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

https://codereview.chromium.org/1619703002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/platform_notification_service_impl.cc:225:
content::RecordAction(base::UserMetricsAction(
On 2016/01/26 16:13:45, Peter Beverloo wrote:
> Even when a notification was clicked by the user, we'd still want it to be
> removed from the NotificationDatabase. In other words, //content always has to
> be told about the closure.
> 
> I think what we'll want here is something like...
> 
> if (by_user) { uma_one }
> else { uma_two }
> 
> content::NED->DispatchNotificationCloseEvent(..., by_user);
> 
> and then make the decision on whether or not to fire the event in //content.

Good catch - I think this became an issue when I moved the deletion code to
//content.

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

https://codereview.chromium.org/1619703002/diff/60001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:40: void
SWNotificationEventFinished(
On 2016/01/26 16:13:45, Peter Beverloo wrote:
> nit: s/SW/ServiceWorker/ (we don't usually use acronyms like this)

Done.

https://codereview.chromium.org/1619703002/diff/60001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:214: if
(success) {
On 2016/01/26 16:13:45, Peter Beverloo wrote:
> You could reduce duplication by branching for the arguments only -
> 
> NotificationEventFinished(
>     dispatch_complete_callback,
>     success ? PERSISTENT_NOTIFICATION_STATUS_SUCCESS
>             : PERSISTENT_NOTIFICATION_STATUS_DATABASE_ERROR);

Done.

https://codereview.chromium.org/1619703002/diff/60001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:230:
ServiceWorkerStatusCode status_code) {
On 2016/01/26 16:13:45, Peter Beverloo wrote:
> We'll want to return the result status of dispatching the close event in this
> case as well, e.g. consider |status_code| like SWNotificationEventFinished()
> does. That would allow for more comprehensive UMA in the //chrome layer. WDYT?

Sounds good. Since we want to call the callback only once per event dispatch, I
changed the code to run the callback with the error code if we get one here and
ignore any DB error later.

https://codereview.chromium.org/1619703002/diff/60001/content/browser/notific...
File content/browser/notifications/notification_event_dispatcher_impl.h (right):

https://codereview.chromium.org/1619703002/diff/60001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.h:56: void
DispatchNotificationEvent(
On 2016/01/26 16:13:45, Peter Beverloo wrote:
> Is there a reason that we'd like to expose this in the header? It could be an
> anonymous function in the implementation (and with it, the typedefs +
additional
> forward declarations).

No reason - fixed.

https://codereview.chromium.org/1619703002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1619703002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:684: void
ServiceWorkerVersion::DispatchNotificationCloseEvent(
On 2016/01/26 16:13:45, Peter Beverloo wrote:
> (I'm not really looking at this bit because Marijn's CL will land before
yours,
> which will require some further refactoring --
> https://codereview.chromium.org/1575283003/)

Acknowledged.

https://codereview.chromium.org/1619703002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/1619703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/Notification.h:69: //
Notification's state as showing, or as closing otherwise.
On 2016/01/26 16:13:45, Peter Beverloo wrote:
> closing? I think we should initialize it as NotificationStateClosed (the event
> will fire *after* the notification has closed)

Makes sense. Fixed.

https://codereview.chromium.org/1619703002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1619703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:34:
notifications.append(Notification::create(resolver->executionContext(),
notificationInfo.persistentId, notificationInfo.data, true));
On 2016/01/26 16:13:45, Peter Beverloo wrote:
> , true /* showing */)

Added here and in two more analogous places.

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

https://codereview.chromium.org/1619703002/diff/80001/content/browser/notific...
content/browser/notifications/notification_event_dispatcher_impl.cc:271: void
OnDidDispatchNotificationCloseEvent(
Should we rename this to OnMaybeDidDispatchNotificationCloseEvent and update the
comment?

[Peter Beverloo, 2016-01-27 19:03:58]

Yay! Very close :) Thank you

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

https://codereview.chromium.org/1619703002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_browsertest.cc:337:
notification.delegate()->Close(true);
nit: "true /* by_user */"

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

https://codereview.chromium.org/1619703002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:85: //
metric.
We'll probably want these to be included in the first patch? You're modifying
the histograms.xml file anyway, so we'll need their owner review.

https://codereview.chromium.org/1619703002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/1619703002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:22: using
NotificationDispatchCompleteCallback =
Why are you forward-declaring this? To avoid wrapping lines?

https://codereview.chromium.org/1619703002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:27: using
CurriedNotificationActionCallback =
NotificationActionCallbackWithContext?

"Curried" doesn't really matter, that's inherent to the abilities of
base::Bind()

https://codereview.chromium.org/1619703002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:236: void
DoDispatchNotificationClickEvent(
One thing you could do here to improve readability is to create logical sections
with horizontal lines.

...

void ReadNotificationDatabaseData()

// -----------------------------------------------------------------------

void DispatchNotificationClickEventOnWorker()

void DoDispatchNotificationClickEvent()

// -----------------------------------------------------------------------

void DeleteNotificationDataFromDatabase()

void DispatchNotificationCloseEventOnWorker()

void DoDispatchNotificationCloseEvent()

https://codereview.chromium.org/1619703002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:271: void
OnDidDispatchNotificationCloseEvent(
What do you think of naming this method like what it's doing?

DeleteNotificationDataFromDatabase

https://codereview.chromium.org/1619703002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:323: void
DispatchNotificationEvent(
This can be in the anonymous namespace above.

https://codereview.chromium.org/1619703002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/1619703002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_metrics.cc:239: //
TODO(nsatragno): create UMA metric.
dito re: UMA

https://codereview.chromium.org/1619703002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1619703002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:160:
eventInit.setAction(WTF::String());
Why is this line necessary?

[Nina, 2016-01-28 11:59:01]

Hello Peter,

Please review my new patch.

Thanks!

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

https://codereview.chromium.org/1619703002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_browsertest.cc:337:
notification.delegate()->Close(true);
On 2016/01/27 19:03:57, Peter Beverloo wrote:
> nit: "true /* by_user */"

Done.

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

https://codereview.chromium.org/1619703002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/platform_notification_service_impl.cc:85: //
metric.
On 2016/01/27 19:03:57, Peter Beverloo wrote:
> We'll probably want these to be included in the first patch? You're modifying
> the histograms.xml file anyway, so we'll need their owner review.

Done.

https://codereview.chromium.org/1619703002/diff/100001/content/browser/notifi...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/1619703002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:22: using
NotificationDispatchCompleteCallback =
On 2016/01/27 19:03:58, Peter Beverloo wrote:
> Why are you forward-declaring this? To avoid wrapping lines?

Yes, it's being referenced in 12 other places so avoiding the namespace is worth
the additional declaration.

https://codereview.chromium.org/1619703002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:27: using
CurriedNotificationActionCallback =
On 2016/01/27 19:03:57, Peter Beverloo wrote:
> NotificationActionCallbackWithContext?
> 
> "Curried" doesn't really matter, that's inherent to the abilities of
> base::Bind()

Done.

https://codereview.chromium.org/1619703002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:236: void
DoDispatchNotificationClickEvent(
On 2016/01/27 19:03:57, Peter Beverloo wrote:
> One thing you could do here to improve readability is to create logical
sections
> with horizontal lines.
> 
> ...
> 
> void ReadNotificationDatabaseData()
> 
> // -----------------------------------------------------------------------
> 
> void DispatchNotificationClickEventOnWorker()
> 
> void DoDispatchNotificationClickEvent()
> 
> // -----------------------------------------------------------------------
> 
> void DeleteNotificationDataFromDatabase()
> 
> void DispatchNotificationCloseEventOnWorker()
> 
> void DoDispatchNotificationCloseEvent()

Done.

https://codereview.chromium.org/1619703002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:271: void
OnDidDispatchNotificationCloseEvent(
On 2016/01/27 19:03:57, Peter Beverloo wrote:
> What do you think of naming this method like what it's doing?
> 
> DeleteNotificationDataFromDatabase

Done.

https://codereview.chromium.org/1619703002/diff/100001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:323: void
DispatchNotificationEvent(
On 2016/01/27 19:03:57, Peter Beverloo wrote:
> This can be in the anonymous namespace above.

Done.

https://codereview.chromium.org/1619703002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/1619703002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_metrics.cc:239: //
TODO(nsatragno): create UMA metric.
On 2016/01/27 19:03:58, Peter Beverloo wrote:
> dito re: UMA

Done.

https://codereview.chromium.org/1619703002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1619703002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:160:
eventInit.setAction(WTF::String());
On 2016/01/27 19:03:58, Peter Beverloo wrote:
> Why is this line necessary?

We want the action to be null when closing, and it's empty ("") by default.
Should I add a comment?

[Peter Beverloo, 2016-01-28 16:14:23]

sgtm! A few more minor nits and then I'll happily stamp :)

Please update your description with the following points:

* Link to the Intent to Implement and Ship on blink-dev

* Update "More tests and UMA metrics to come in a follow-up patch." -
specifically, call out that more thorough layout tests will follow this CL. It
may be useful to have the layout test CL in-progress before sending this to the
API owners for a review.

You will also need to modify the following file:

third_party/WebKit/Source/modules/notifications/ServiceWorkerGlobalScopeNotifications.idl

(which will, in turn, require you to update a few test expectation files)

https://codereview.chromium.org/1619703002/diff/140001/content/browser/notifi...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/1619703002/diff/140001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:24: using
NotificationActionCallback =
One thing to consider - "notification action" could currently be read as the
action button feature, e.g. the |action_index| parameter elsewhere in this file.

"notification operation" as a concept might be better, and matches the name we
use in the PlatformNotificationServiceImpl.

https://codereview.chromium.org/1619703002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1619703002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:160:
eventInit.setAction(WTF::String());
Yes, a comment would be useful.  In-line would be fine ("...());  // initialize
as NULL.")

https://codereview.chromium.org/1619703002/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1619703002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:30917: + 
<owner>deepak.m1@samsumg.com</owner>
s/deepak/you/

https://codereview.chromium.org/1619703002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:30919: +    Recorded delivery status for
persistent notification close events to a
s/Recorded/Records/

https://codereview.chromium.org/1619703002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:30920: +    Service Worker when handling
a click on a persistent WebNotification has
WebNotification -> Web Notification

https://codereview.chromium.org/1619703002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44230: +    The time taken between
dispatching a NotificationCloseEvent to a Service
notificationcloseevent (no capitals)

[Nina, 2016-02-01 16:59:28]

Description was changed from

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

More tests and UMA metrics to come in a follow-up patch.

BUG=579150
TEST=PlatformNotificationServiceBrowserTest.UserClosesPersistentNotification
==========

to

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

Layout tests to come in a follow-up patch:
https://codereview.chromium.org/1656823002/

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fePanzKy6ec

BUG=579150
TEST=PlatformNotificationServiceBrowserTest.UserClosesPersistentNotification
==========

[Nina, 2016-02-01 17:59:06]

Hi Peter,

I looked at your comments and fixed them. I haven't updated the test
expectations file for the exposed service worker API yet because the test seems
to be crashing on my PC. Could you give it a look?

https://codereview.chromium.org/1619703002/diff/140001/content/browser/notifi...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/1619703002/diff/140001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:24: using
NotificationActionCallback =
On 2016/01/28 16:14:23, Peter Beverloo wrote:
> One thing to consider - "notification action" could currently be read as the
> action button feature, e.g. the |action_index| parameter elsewhere in this
file.
> 
> "notification operation" as a concept might be better, and matches the name we
> use in the PlatformNotificationServiceImpl.

Done.

https://codereview.chromium.org/1619703002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1619703002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:160:
eventInit.setAction(WTF::String());
On 2016/01/28 16:14:23, Peter Beverloo wrote:
> Yes, a comment would be useful.  In-line would be fine ("...());  //
initialize
> as NULL.")

Done.

https://codereview.chromium.org/1619703002/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1619703002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:30917: + 
<owner>deepak.m1@samsumg.com</owner>
On 2016/01/28 16:14:23, Peter Beverloo wrote:
> s/deepak/you/

Done.

https://codereview.chromium.org/1619703002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:30919: +    Recorded delivery status for
persistent notification close events to a
On 2016/01/28 16:14:23, Peter Beverloo wrote:
> s/Recorded/Records/

I rewrote the summary as it didn't sound right even after the modifications.

https://codereview.chromium.org/1619703002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:30920: +    Service Worker when handling
a click on a persistent WebNotification has
On 2016/01/28 16:14:23, Peter Beverloo wrote:
> WebNotification -> Web Notification

ditto

https://codereview.chromium.org/1619703002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44230: +    The time taken between
dispatching a NotificationCloseEvent to a Service
On 2016/01/28 16:14:23, Peter Beverloo wrote:
> notificationcloseevent (no capitals)

Done.

[Nina, 2016-02-01 18:22:02]

Okay, looks like I had forgotten to recompile after rebasing. Fixed the
expectations.

[Peter Beverloo, 2016-02-03 00:00:23]

lgtm!

https://codereview.chromium.org/1619703002/diff/180001/content/browser/notifi...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/1619703002/diff/180001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:94: const
scoped_refptr<PlatformNotificationContext> notification_context,
const& (elsewhere too)

https://codereview.chromium.org/1619703002/diff/180001/content/public/browser...
File content/public/browser/notification_event_dispatcher.h (right):

https://codereview.chromium.org/1619703002/diff/180001/content/public/browser...
content/public/browser/notification_event_dispatcher.h:45: // with
|persistent_notification_id| belonging to |origin|. The |callback|
s/callback/dispatch_complete_callback/

https://codereview.chromium.org/1619703002/diff/180001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1619703002/diff/180001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44336: +    The time taken between
dispatching a notificationcloseevent to a Service
Please capitalize similar to the other histogram descriptions (e.g.
ServiceWorker.NotificationClickEvent.Time)

[Nina, 2016-02-03 12:15:12]

nsatragno@chromium.org changed reviewers:
+ avi@chromium.org, falken@chromium.org, isherman@chromium.org,
mkwst@chromium.org, rbyers@chromium.org

[Nina, 2016-02-03 12:15:13]

avi@chromium.org: //content/public

rbyers@chromium.org: stable web exposed tests.

falken@chromium.org: service worker changes.

mkwst@chromium.org: IPC.

isherman@chromium.org: histograms.

https://codereview.chromium.org/1619703002/diff/180001/content/browser/notifi...
File content/browser/notifications/notification_event_dispatcher_impl.cc
(right):

https://codereview.chromium.org/1619703002/diff/180001/content/browser/notifi...
content/browser/notifications/notification_event_dispatcher_impl.cc:94: const
scoped_refptr<PlatformNotificationContext> notification_context,
On 2016/02/03 00:00:21, Peter Beverloo wrote:
> const& (elsewhere too)

Made scoped_refptrs const and passed by ref.

https://codereview.chromium.org/1619703002/diff/180001/content/public/browser...
File content/public/browser/notification_event_dispatcher.h (right):

https://codereview.chromium.org/1619703002/diff/180001/content/public/browser...
content/public/browser/notification_event_dispatcher.h:45: // with
|persistent_notification_id| belonging to |origin|. The |callback|
On 2016/02/03 00:00:21, Peter Beverloo wrote:
> s/callback/dispatch_complete_callback/

Done.

[Rick Byers, 2016-02-03 14:33:54]

On 2016/02/03 12:15:13, Nicolás wrote:
> mailto:avi@chromium.org: //content/public
> 
> mailto:rbyers@chromium.org: stable web exposed tests.

third_party/WebKit/LayoutTests/ LGTM

> 
> mailto:falken@chromium.org: service worker changes.
> 
> mailto:mkwst@chromium.org: IPC.
> 
> mailto:isherman@chromium.org: histograms.
> 
>
https://codereview.chromium.org/1619703002/diff/180001/content/browser/notifi...
> File content/browser/notifications/notification_event_dispatcher_impl.cc
> (right):
> 
>
https://codereview.chromium.org/1619703002/diff/180001/content/browser/notifi...
> content/browser/notifications/notification_event_dispatcher_impl.cc:94: const
> scoped_refptr<PlatformNotificationContext> notification_context,
> On 2016/02/03 00:00:21, Peter Beverloo wrote:
> > const& (elsewhere too)
> 
> Made scoped_refptrs const and passed by ref.
> 
>
https://codereview.chromium.org/1619703002/diff/180001/content/public/browser...
> File content/public/browser/notification_event_dispatcher.h (right):
> 
>
https://codereview.chromium.org/1619703002/diff/180001/content/public/browser...
> content/public/browser/notification_event_dispatcher.h:45: // with
> |persistent_notification_id| belonging to |origin|. The |callback|
> On 2016/02/03 00:00:21, Peter Beverloo wrote:
> > s/callback/dispatch_complete_callback/
> 
> Done.

[Avi (use Gerrit), 2016-02-03 16:20:43]

On 2016/02/03 12:15:13, Nicolás wrote:
> mailto:avi@chromium.org: //content/public
> 
> mailto:rbyers@chromium.org: stable web exposed tests.
> 
> mailto:falken@chromium.org: service worker changes.
> 
> mailto:mkwst@chromium.org: IPC.
> 
> mailto:isherman@chromium.org: histograms.
> 
>
https://codereview.chromium.org/1619703002/diff/180001/content/browser/notifi...
> File content/browser/notifications/notification_event_dispatcher_impl.cc
> (right):
> 
>
https://codereview.chromium.org/1619703002/diff/180001/content/browser/notifi...
> content/browser/notifications/notification_event_dispatcher_impl.cc:94: const
> scoped_refptr<PlatformNotificationContext> notification_context,
> On 2016/02/03 00:00:21, Peter Beverloo wrote:
> > const& (elsewhere too)
> 
> Made scoped_refptrs const and passed by ref.
> 
>
https://codereview.chromium.org/1619703002/diff/180001/content/public/browser...
> File content/public/browser/notification_event_dispatcher.h (right):
> 
>
https://codereview.chromium.org/1619703002/diff/180001/content/public/browser...
> content/public/browser/notification_event_dispatcher.h:45: // with
> |persistent_notification_id| belonging to |origin|. The |callback|
> On 2016/02/03 00:00:21, Peter Beverloo wrote:
> > s/callback/dispatch_complete_callback/
> 
> Done.

content/public lgtm

[Ilya Sherman, 2016-02-03 19:43:38]

histograms lgtm

[Mike West, 2016-02-04 18:04:06]

IPC LGTM.

[falken, 2016-02-05 01:53:15]

service worker lgtm

[Nina, 2016-02-05 11:12:29]

The CQ bit was checked by nsatragno@chromium.org

[Nina, 2016-02-05 11:12:30]

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/1619703002/#ps200001
(title: "Fixed nits")

[commit-bot: I haz the power, 2016-02-05 11:13:05]

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

[commit-bot: I haz the power, 2016-02-05 12:55:16]

Description was changed from

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

Layout tests to come in a follow-up patch:
https://codereview.chromium.org/1656823002/

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fePanzKy6ec

BUG=579150
TEST=PlatformNotificationServiceBrowserTest.UserClosesPersistentNotification
==========

to

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

Layout tests to come in a follow-up patch:
https://codereview.chromium.org/1656823002/

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fePanzKy6ec

BUG=579150
TEST=PlatformNotificationServiceBrowserTest.UserClosesPersistentNotification
==========

[commit-bot: I haz the power, 2016-02-05 12:55:18]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-02-05 12:56:57]

Description was changed from

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

Layout tests to come in a follow-up patch:
https://codereview.chromium.org/1656823002/

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fePanzKy6ec

BUG=579150
TEST=PlatformNotificationServiceBrowserTest.UserClosesPersistentNotification
==========

to

==========
Implement notificationclose event

This patch introduces a notificationclose event fired in the service
worker when a notification has been closed by the user.

Layout tests to come in a follow-up patch:
https://codereview.chromium.org/1656823002/

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/fePanzKy6ec

BUG=579150
TEST=PlatformNotificationServiceBrowserTest.UserClosesPersistentNotification

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

[commit-bot: I haz the power, 2016-02-05 12:56:59]

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