The Service Worker notificationclick event should carry a notification.

The Service Worker notificationclick event should carry a notification.

When the notificationclick event fires on a Service Worker (of type
NotificationEvent), it should carry the notification it was fired for as
a member. This allows the developer to identify the notification, and
close it if they so desire.

BUG=432527
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187049

[Peter Beverloo, 2014-12-09 16:31:44]

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

[Peter Beverloo, 2014-12-09 16:31:45]

+mvanouwerkerk for review.

This depends on https://codereview.chromium.org/787893002/.

[Michael van Ouwerkerk, 2014-12-09 19:49:21]

lgtm with nits

https://codereview.chromium.org/789643003/diff/20001/LayoutTests/http/tests/n...
File
LayoutTests/http/tests/notifications/resources/click-close-service-worker.js
(right):

https://codereview.chromium.org/789643003/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/resources/click-close-service-worker.js:1:
// Sends a message to all clients when the notificationclick event gets
received.
Does it? Afaict it just messages to its single message port. Maybe there's only
one client :-)

https://codereview.chromium.org/789643003/diff/20001/LayoutTests/http/tests/n...
File
LayoutTests/http/tests/notifications/service-worker-show-notification-close.html
(right):

https://codereview.chromium.org/789643003/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/service-worker-show-notification-close.html:51:
assert_equals(event.data, 'Clicked on Notificationx: ' + scope);
This is not the data sent, with "Notificationx".

https://codereview.chromium.org/789643003/diff/20001/Source/modules/notificat...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/789643003/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.h:58: static Notification*
create(ExecutionContext*, const String& persistentId, const
WebNotificationData&);
It would be nice to document the different types being created here.

[Peter Beverloo, 2014-12-10 14:11:08]

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

[Peter Beverloo, 2014-12-10 14:11:09]

+mkwst

https://codereview.chromium.org/789643003/diff/20001/LayoutTests/http/tests/n...
File
LayoutTests/http/tests/notifications/resources/click-close-service-worker.js
(right):

https://codereview.chromium.org/789643003/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/resources/click-close-service-worker.js:1:
// Sends a message to all clients when the notificationclick event gets
received.
On 2014/12/09 19:49:20, Michael van Ouwerkerk wrote:
> Does it? Afaict it just messages to its single message port. Maybe there's
only
> one client :-)

Done.

https://codereview.chromium.org/789643003/diff/20001/LayoutTests/http/tests/n...
File
LayoutTests/http/tests/notifications/service-worker-show-notification-close.html
(right):

https://codereview.chromium.org/789643003/diff/20001/LayoutTests/http/tests/n...
LayoutTests/http/tests/notifications/service-worker-show-notification-close.html:51:
assert_equals(event.data, 'Clicked on Notificationx: ' + scope);
On 2014/12/09 19:49:20, Michael van Ouwerkerk wrote:
> This is not the data sent, with "Notificationx".

Done.

https://codereview.chromium.org/789643003/diff/20001/Source/modules/notificat...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/789643003/diff/20001/Source/modules/notificat...
Source/modules/notifications/Notification.h:58: static Notification*
create(ExecutionContext*, const String& persistentId, const
WebNotificationData&);
On 2014/12/09 19:49:21, Michael van Ouwerkerk wrote:
> It would be nice to document the different types being created here.

Done.

[Peter Beverloo, 2014-12-10 19:40:18]

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

[Peter Beverloo, 2014-12-10 19:40:19]

+michaeln for very minor ServiceWorkerGlobalScopeProxy.cpp change.

[michaeln1, 2014-12-10 20:40:17]

michaeln@google.com changed reviewers:
+ michaeln@google.com

[michaeln1, 2014-12-10 20:40:17]

lgtm

[Peter Beverloo, 2014-12-10 22:39:55]

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

[Peter Beverloo, 2014-12-10 22:39:56]

+jochen for Source/web/

[Peter Beverloo, 2014-12-11 19:05:17]

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

[Peter Beverloo, 2014-12-11 19:05:17]

+haraken for Source/web/ maybe?

It's already been functionally reviewed by feature owners.

[haraken, 2014-12-11 23:38:15]

Source/web/ LGTM

[Peter Beverloo, 2014-12-12 17:38:38]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-12-12 17:39:39]

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

[commit-bot: I haz the power, 2014-12-12 19:33:38]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187049

[sof, 2014-12-15 12:41:45]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2014-12-15 12:41:46]

I notice that 

 http/tests/notifications/service-worker-show-notification-close.html

is flakily failing on some of the bots; expected?

Also, calling close() on a detached context appears to fail following this CL,


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_ASA...

Could you take a look, please?

[Peter Beverloo, 2014-12-15 12:43:08]

On 2014/12/15 12:41:46, sof wrote:
> I notice that 
> 
>  http/tests/notifications/service-worker-show-notification-close.html
> 
> is flakily failing on some of the bots; expected?
> 
> Also, calling close() on a detached context appears to fail following this CL,
> 
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_ASA...
> 
> Could you take a look, please?

Ack, I'll make sure this is fixed before EoD. Thanks for pointing it out!