Push API: define push event on Service Worker.

Push API: define push event on Service Worker.

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

[Michael van Ouwerkerk, 2014-05-16 15:26:06]

Michael: please review from Service Worker perspective
Peter: please review from Push API perspective

[Peter Beverloo, 2014-05-16 15:41:43]

Hi Michael, thanks for the patch! I think we can wire this up much closer to the
push_messaging module, because this creates a rather creative dependency model.

https://codereview.chromium.org/285403002/diff/20001/Source/core/events/Event...
File Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/285403002/diff/20001/Source/core/events/Event...
Source/core/events/EventTypeNames.in:137: push
alphabet.

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushEvent.cpp (right):

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.cpp:20: : Event(type, true, true)
Could you change this to be somewhat more readable?

: Event(type, true /* canBubble */, true /* cancelable */)

Why are both of these true? Where would it bubble to, and what happens when it
gets cancelled?

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushEvent.h (right):

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.h:9: 
IWYU please

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.h:20: static PassRefPtr<PushEvent>
create()
PassRefPtrWillBeRawPtr (elsewhere too)

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.h:22: return adoptRef(new PushEvent);
adoptRefWillBeNoop (elsewhere too)

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.h:38: 
Hmm. Most other Event implementations override the virtual trace() method, only
to call the method in the class which defines it. That's pointless, and there's
no traceable members of this class, so fine to keep it out.

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.h:42: PushEvent(const AtomicString&,
const PushEventInit&);
Please name the first argument (since AtomicString is not clear on itself).

https://codereview.chromium.org/285403002/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/285403002/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:63:
DEFINE_ATTRIBUTE_EVENT_LISTENER(push);
Please define these in a new partial interface called
ServiceWorkerGlobalScopePush (or something similar) and use the ImplementedAs
annotation to contain implementation in the push_messaging module.

I'll continue my review after this is done, because I suspect the rest will
change significantly because of it.

[michaeln, 2014-05-16 20:24:37]

Assuming the only possible recipients of push events are serviceworkers (is that
right), the webkit api layer and cpp code to traverse the blink/webkit api layer
looks reasonable.

[Peter Beverloo, 2014-05-19 13:47:55]

On 2014/05/16 20:24:37, michaeln wrote:
> Assuming the only possible recipients of push events are serviceworkers (is
that
> right), the webkit api layer and cpp code to traverse the blink/webkit api
layer
> looks reasonable.

I worry about the scalability of this solution. As more APIs start to depend on
Service Workers, we would either have to split up modules (part in their own
module, part in the serviceworkers module) or have modules depend on each other.
Right now modules/serviceworkers/ already services for Service Workers itself,
as well as the Background Sync API, and upcoming candidates are the Push API,
Geofencing and Notifications.

Specific to this CL, I do not like how part of the Push API's functionality is
implemented in modules/push_messaging/, and part in modules/serviceworkers/.
This is not an obvious design.

I would very much like to see a solution in which push can implement a partial
ServiceWorkerGlobalScope interface (using [ImplementedAs] to indicate which
class the implementation is in) to define the event itself. The SW browser side
should then (1) start up the ServiceWorker, (2) request the Push API --or
whichever API-- to fire the onpush event, passing the
WebServiceWorkerContext(proxy). Does that sound reasonable?

[michaeln, 2014-05-19 22:18:59]

> I worry about the scalability of this solution. As more APIs start to depend
on
> Service Workers, we would either have to split up modules (part in their own
> module, part in the serviceworkers module) or have modules depend on each
other.
> Right now modules/serviceworkers/ already services for Service Workers itself,
> as well as the Background Sync API, and upcoming candidates are the Push API,
> Geofencing and Notifications.
> 
> Specific to this CL, I do not like how part of the Push API's functionality is
> implemented in modules/push_messaging/, and part in modules/serviceworkers/.
> This is not an obvious design.
> 
> I would very much like to see a solution in which push can implement a partial
> ServiceWorkerGlobalScope interface (using [ImplementedAs] to indicate which
> class the implementation is in) to define the event itself. The SW browser
side
> should then (1) start up the ServiceWorker, (2) request the Push API --or
> whichever API-- to fire the onpush event, passing the
> WebServiceWorkerContext(proxy). Does that sound reasonable?

Good point about splitting the Source/modules more cleanly. I think the webkit
api layer is ok as is.

[Michael van Ouwerkerk, 2014-05-21 17:06:41]

Thanks for your comments so far. This is a WIP upload. Changes:
* Partial interface to define onpush on ServiceWorkerGlobalScope
* A callback to avoid modifying ServiceWorkerGlobalScopeClient

A chromium repo WIP patch to demonstrate the callpoint:
https://codereview.chromium.org/296463005/

What do you think of this approach?

https://codereview.chromium.org/285403002/diff/20001/Source/core/events/Event...
File Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/285403002/diff/20001/Source/core/events/Event...
Source/core/events/EventTypeNames.in:137: push
On 2014/05/16 15:41:43, Peter Beverloo wrote:
> alphabet.

Done.

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushEvent.cpp (right):

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.cpp:20: : Event(type, true, true)
On 2014/05/16 15:41:43, Peter Beverloo wrote:
> Could you change this to be somewhat more readable?
> 
> : Event(type, true /* canBubble */, true /* cancelable */)
> 
> Why are both of these true? Where would it bubble to, and what happens when it
> gets cancelled?

You're right, these should probably not bubble or be cancelable.

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
File Source/modules/push_messaging/PushEvent.h (right):

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.h:9: 
On 2014/05/16 15:41:43, Peter Beverloo wrote:
> IWYU please

Done.

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.h:20: static PassRefPtr<PushEvent>
create()
On 2014/05/16 15:41:43, Peter Beverloo wrote:
> PassRefPtrWillBeRawPtr (elsewhere too)

Done.

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.h:22: return adoptRef(new PushEvent);
On 2014/05/16 15:41:43, Peter Beverloo wrote:
> adoptRefWillBeNoop (elsewhere too)

Done.

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.h:38: 
On 2014/05/16 15:41:43, Peter Beverloo wrote:
> Hmm. Most other Event implementations override the virtual trace() method,
only
> to call the method in the class which defines it. That's pointless, and
there's
> no traceable members of this class, so fine to keep it out.

Ok.

https://codereview.chromium.org/285403002/diff/20001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.h:42: PushEvent(const AtomicString&,
const PushEventInit&);
On 2014/05/16 15:41:43, Peter Beverloo wrote:
> Please name the first argument (since AtomicString is not clear on itself).

Done.

https://codereview.chromium.org/285403002/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/285403002/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:63:
DEFINE_ATTRIBUTE_EVENT_LISTENER(push);
On 2014/05/16 15:41:43, Peter Beverloo wrote:
> Please define these in a new partial interface called
> ServiceWorkerGlobalScopePush (or something similar) and use the ImplementedAs
> annotation to contain implementation in the push_messaging module.
> 
> I'll continue my review after this is done, because I suspect the rest will
> change significantly because of it.

Done.

[kinuko, 2014-05-22 04:33:19]

DBC (no need to be blocked by my comments)

https://codereview.chromium.org/285403002/diff/40001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextProxy.h (right):

https://codereview.chromium.org/285403002/diff/40001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextProxy.h:61: virtual void dispatchPushEvent(int
eventID, const WebString& data, const WebServiceWorkerPushEventCallback&) = 0;
nit: this callback interface doesn't look particularly useful to me at least in
the current code, especially if it's always called at the end of
dispatchPushEvent method.  Will there be a case where we call the given callback
asyncly, with additional status info like success or failure?  (I guess the same
comment applies to SyncEvent one)

[dominicc (has gone to gerrit), 2014-05-22 06:52:28]

Nits inline.

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
File Source/modules/push_messaging/PushEvent.cpp (right):

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.cpp:20: : Event(type, false /* canBubble
*/, false /* cancelable */)
Would this be clearer if the comments were prefixes like /* canBubble? */ false
? As a suffix it is slightly unclear whether that's the parameter (which is
false) or the policy (the event CAN bubble.)

(Admittedly these parameters are infamous.)

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
File Source/modules/push_messaging/ServiceWorkerGlobalScopePush.idl (right):

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
Source/modules/push_messaging/ServiceWorkerGlobalScopePush.idl:6:
RuntimeEnabled=ServiceWorker,
Does this work? I thought you separated multiple values with | but I can't find
examples of either.

https://codereview.chromium.org/285403002/diff/40001/Source/web/ServiceWorker...
File Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/285403002/diff/40001/Source/web/ServiceWorker...
Source/web/ServiceWorkerGlobalScopeProxy.h:76: virtual void
dispatchPushEvent(int, const WebString& data, const
WebServiceWorkerPushEventCallback&) OVERRIDE;
Maybe alphabetize these?

[Peter Beverloo, 2014-05-22 16:09:49]

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
File Source/modules/push_messaging/PushEvent.idl (right):

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.idl:6: EventConstructor,
Please guard this behind the runtime flag. It would otherwise be accessible via:

document.createEvent('PushEvent')

https://codereview.chromium.org/285403002/diff/40001/public/web/WebServiceWor...
File public/web/WebServiceWorkerPushEventCallback.h (right):

https://codereview.chromium.org/285403002/diff/40001/public/web/WebServiceWor...
public/web/WebServiceWorkerPushEventCallback.h:19: virtual
~WebServiceWorkerPushEventCallback() { }
We don't need this destructor. You could emphasize on its lifetime with a
class-level comment.

[Michael van Ouwerkerk, 2014-05-23 18:32:03]

Thanks! Please take another look.

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
File Source/modules/push_messaging/PushEvent.cpp (right):

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.cpp:20: : Event(type, false /* canBubble
*/, false /* cancelable */)
On 2014/05/22 06:52:28, dominicc wrote:
> Would this be clearer if the comments were prefixes like /* canBubble? */
false
> ? As a suffix it is slightly unclear whether that's the parameter (which is
> false) or the policy (the event CAN bubble.)
> 
> (Admittedly these parameters are infamous.)

Ah ok, I'll format it like FetchEvent then.

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
File Source/modules/push_messaging/PushEvent.idl (right):

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
Source/modules/push_messaging/PushEvent.idl:6: EventConstructor,
On 2014/05/22 16:09:49, Peter Beverloo wrote:
> Please guard this behind the runtime flag. It would otherwise be accessible
via:
> 
> document.createEvent('PushEvent')

Done.

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
File Source/modules/push_messaging/ServiceWorkerGlobalScopePush.idl (right):

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
Source/modules/push_messaging/ServiceWorkerGlobalScopePush.idl:6:
RuntimeEnabled=ServiceWorker,
On 2014/05/22 06:52:28, dominicc wrote:
> Does this work? I thought you separated multiple values with | but I can't
find
> examples of either.

Gating it on PushMessaging seems to be sufficient.

https://codereview.chromium.org/285403002/diff/40001/Source/web/ServiceWorker...
File Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/285403002/diff/40001/Source/web/ServiceWorker...
Source/web/ServiceWorkerGlobalScopeProxy.h:76: virtual void
dispatchPushEvent(int, const WebString& data, const
WebServiceWorkerPushEventCallback&) OVERRIDE;
On 2014/05/22 06:52:28, dominicc wrote:
> Maybe alphabetize these?

Done.

https://codereview.chromium.org/285403002/diff/40001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextProxy.h (right):

https://codereview.chromium.org/285403002/diff/40001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextProxy.h:61: virtual void dispatchPushEvent(int
eventID, const WebString& data, const WebServiceWorkerPushEventCallback&) = 0;
On 2014/05/22 04:33:19, kinuko wrote:
> nit: this callback interface doesn't look particularly useful to me at least
in
> the current code, especially if it's always called at the end of
> dispatchPushEvent method.  Will there be a case where we call the given
callback
> asyncly, with additional status info like success or failure?  (I guess the
same
> comment applies to SyncEvent one)

I hoped you would know what this is for, this is the existing pattern. It seems
ServiceWorkerVersion likes to track these callbacks in the browser process.

In future, this may be relevant in context of limiting execution time. The
callback value would be different if the script is killed:
https://code.google.com/p/chromium/issues/detail?id=372436

For now, the callback is an interesting problem to be solved for modular
addition of API's that live in Service Worker.

https://codereview.chromium.org/285403002/diff/40001/public/web/WebServiceWor...
File public/web/WebServiceWorkerPushEventCallback.h (right):

https://codereview.chromium.org/285403002/diff/40001/public/web/WebServiceWor...
public/web/WebServiceWorkerPushEventCallback.h:19: virtual
~WebServiceWorkerPushEventCallback() { }
On 2014/05/22 16:09:49, Peter Beverloo wrote:
> We don't need this destructor. You could emphasize on its lifetime with a
> class-level comment.

Done.

[dominicc (has gone to gerrit), 2014-05-26 02:31:43]

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
File Source/modules/push_messaging/ServiceWorkerGlobalScopePush.idl (right):

https://codereview.chromium.org/285403002/diff/40001/Source/modules/push_mess...
Source/modules/push_messaging/ServiceWorkerGlobalScopePush.idl:6:
RuntimeEnabled=ServiceWorker,
On 2014/05/23 18:32:03, Michael van Ouwerkerk wrote:
> On 2014/05/22 06:52:28, dominicc wrote:
> > Does this work? I thought you separated multiple values with | but I can't
> find
> > examples of either.
> 
> Gating it on PushMessaging seems to be sufficient.

I agree that gating it on PushMessaging is fine; FWIW I chatted with nbarth and
the synax is X=A|B

You should add a virtual/webexposed test for this if you didn't already.

[kinuko, 2014-05-26 04:03:00]

https://codereview.chromium.org/285403002/diff/40001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextProxy.h (right):

https://codereview.chromium.org/285403002/diff/40001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextProxy.h:61: virtual void dispatchPushEvent(int
eventID, const WebString& data, const WebServiceWorkerPushEventCallback&) = 0;
On 2014/05/23 18:32:03, Michael van Ouwerkerk wrote:
> On 2014/05/22 04:33:19, kinuko wrote:
> > nit: this callback interface doesn't look particularly useful to me at least
> in
> > the current code, especially if it's always called at the end of
> > dispatchPushEvent method.  Will there be a case where we call the given
> callback
> > asyncly, with additional status info like success or failure?  (I guess the
> same
> > comment applies to SyncEvent one)
> 
> I hoped you would know what this is for, this is the existing pattern. It
seems
> ServiceWorkerVersion likes to track these callbacks in the browser process.
> 
> In future, this may be relevant in context of limiting execution time. The
> callback value would be different if the script is killed:
> https://code.google.com/p/chromium/issues/detail?id=372436
> 
> For now, the callback is an interesting problem to be solved for modular
> addition of API's that live in Service Worker.

Two questions:

* I know this is from the existing pattern, I just wonder if we could just do
something like below in the chromium side (in service_worker_script_context.cc)
if it doesn't need to be called back asyncly:

dispatchPushEvent();
DidHandlePushEvent(request_id);

* If this follows the existing pattern should we really want to apply a callback
pattern only for this method now?  I can see some benefits for using the pattern
here (it removes some additional plumbing for calling back to chromium side),
but I also feel it'd make these interfaces a bit  inconsistent.  In other words,
say, if we want to convert all the dispatch*Event() stuff into this pattern do
we need to define new callback types for each of them?

[Michael van Ouwerkerk, 2014-06-05 14:24:48]

ptal

[dominicc (has gone to gerrit), 2014-06-06 01:44:52]

On 2014/06/05 14:24:48, Michael van Ouwerkerk wrote:
> ptal

Informal LGTM; you will still need OWNERS for everything.

[Michael van Ouwerkerk, 2014-06-06 09:38:11]

Jochen, could you take a look for the missing owner bits for the */web/* files?

[jochen (gone - plz use gerrit), 2014-06-06 09:44:21]

lgtm

[Michael van Ouwerkerk, 2014-06-06 09:45:32]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-06-06 09:46:23]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/285403002/1...

[Peter Beverloo, 2014-06-06 09:47:48]

lgtm % question

https://codereview.chromium.org/285403002/diff/140001/Source/modules/push_mes...
File Source/modules/push_messaging/PushEvent.idl (right):

https://codereview.chromium.org/285403002/diff/140001/Source/modules/push_mes...
Source/modules/push_messaging/PushEvent.idl:8: ] interface PushEvent : Event {
The spec defines this as PushMessage. Are you deliberately deriving from that
name?

[commit-bot: I haz the power, 2014-06-06 09:51:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7125)

[commit-bot: I haz the power, 2014-06-06 09:55:15]

Change committed as 175652

[Michael van Ouwerkerk, 2014-06-06 09:59:48]

https://codereview.chromium.org/285403002/diff/140001/Source/modules/push_mes...
File Source/modules/push_messaging/PushEvent.idl (right):

https://codereview.chromium.org/285403002/diff/140001/Source/modules/push_mes...
Source/modules/push_messaging/PushEvent.idl:8: ] interface PushEvent : Event {
On 2014/06/06 09:47:47, Peter Beverloo wrote:
> The spec defines this as PushMessage. Are you deliberately deriving from that
> name?

The spec seems to define no name for the event interface. Maybe we should add a
WebIDL snippet to the spec like for GamepadEvent:
http://www.w3.org/TR/2014/WD-gamepad-20140225/#gamepadevent-interface

[Michael van Ouwerkerk, 2014-06-06 10:10:19]

A revert of this CL has been created in
https://codereview.chromium.org/319973002/ by mvanouwerkerk@chromium.org.

The reason for reverting is: Broke all builds:
http://build.chromium.org/p/chromium.webkit/console

e.g.
http://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Bui....