Push API: fire push event from chrome://serviceworker-internals/

Push API: fire push event from chrome://serviceworker-internals/

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

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

Hi guys, this is a WIP upload to flesh out the callpoints for the interfaces
defined in this Blink patch:
https://codereview.chromium.org/285403002/

[Peter Beverloo, 2014-05-22 16:52:24]

I would really like the SW folks to comment on whether they believe this design
to remain scalable as we start adding more events (past: onsync, near-term:
pusherror, notificationclick, notificationclose, longer-term: geofencing
events).

It seems to me like we need a common mechanism for features to work on the
Service Workers scope without having to be *in* Service Workers. Perhaps
something like (in context of push):

  (1) Find and start the Service Worker, fire |onactivate| and make sure it's in
ACTIVATED state.
  (2) Pass a slightly higher-level object than ServiceWorkerGlobalScopeProxy
back to the feature, primarily holding the ExecutionContext and the
WorkerGlobalScope.
  (3) Call through to the push_messaging Blink module with this object, so that
it has all the information it needs to fire the |onpush| event.

While this has obvious difficulties in regards to deciding the lifecycle of the
ServiceWorker, it has the benefit that features don't need to touch the SW APIs
at all aside from passing the SWGS(P) object.

https://codereview.chromium.org/296463005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/296463005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:308: //~ if
(!CommandLine::ForCurrentProcess()->HasSwitch(
Guard behind experimental web platform features (assuming the rest of this code
already is guarded by the SW flag)?

https://codereview.chromium.org/296463005/diff/1/content/common/service_worke...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/296463005/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_messages.h:187: int /* request_id
*/)
std::string data?

[Peter Beverloo, 2014-05-22 16:53:50]

In (2), /s/higher level/lower level/.

[michaeln, 2014-05-22 19:06:39]

> I would really like the SW folks to comment on whether

i got your ping, i'll take a look

[Michael van Ouwerkerk, 2014-05-23 18:24:49]

Thanks Peter! Please take another look.

https://codereview.chromium.org/296463005/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/296463005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:308: //~ if
(!CommandLine::ForCurrentProcess()->HasSwitch(
On 2014/05/22 16:52:25, Peter Beverloo wrote:
> Guard behind experimental web platform features (assuming the rest of this
code
> already is guarded by the SW flag)?

Done.

https://codereview.chromium.org/296463005/diff/1/content/common/service_worke...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/296463005/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_messages.h:187: int /* request_id
*/)
On 2014/05/22 16:52:25, Peter Beverloo wrote:
> std::string data?

This file seems to like base::string16.

[Michael van Ouwerkerk, 2014-05-23 18:25:48]

On 2014/05/23 18:24:49, Michael van Ouwerkerk wrote:
> Thanks Peter! Please take another look.
> 
>
https://codereview.chromium.org/296463005/diff/1/content/browser/service_work...
> File content/browser/service_worker/service_worker_version.cc (right):
> 
>
https://codereview.chromium.org/296463005/diff/1/content/browser/service_work...
> content/browser/service_worker/service_worker_version.cc:308: //~ if
> (!CommandLine::ForCurrentProcess()->HasSwitch(
> On 2014/05/22 16:52:25, Peter Beverloo wrote:
> > Guard behind experimental web platform features (assuming the rest of this
> code
> > already is guarded by the SW flag)?
> 
> Done.
> 
>
https://codereview.chromium.org/296463005/diff/1/content/common/service_worke...
> File content/common/service_worker/service_worker_messages.h (right):
> 
>
https://codereview.chromium.org/296463005/diff/1/content/common/service_worke...
> content/common/service_worker/service_worker_messages.h:187: int /* request_id
> */)
> On 2014/05/22 16:52:25, Peter Beverloo wrote:
> > std::string data?
> 
> This file seems to like base::string16.

Ah I like std::string better, switched to it.

[michaeln, 2014-05-24 00:39:20]

> > I would really like the SW folks to comment on whether
> 
> i got your ping, i'll take a look

I agree its not so scalable.

It would be great is /chrome code could define ipc protocols and events between
sw's and browserlogic with the /content lib not knowing a thing about that. I
think the way that generally works for document contexts is via delegate
interfaces that include an OnMessage() method which the /content lib calls, and
the /content lib also providing a Send() method so a pair of delegates in the
browser and renderer can conduct that protocol.

Kinuko and John (cc'd) were discussing an arrangement like that some time ago (i
think).

I don't think we should block feature work on resolving how that's composed, but
i think it would be good form somebody on the sw team to pick up building hooks
like that for others to utilize.

[kinuko, 2014-05-28 02:16:37]

I think for a short-term we could get it in (if you're in hurry) but scalability
/ messed-up interface issue worries me a bit too.

>  (1) Find and start the Service Worker, fire |onactivate| and make sure it's
in
> ACTIVATED state.
>  (2) Pass a slightly higher-level object than ServiceWorkerGlobalScopeProxy
> back to the feature, primarily holding the ExecutionContext and the
> WorkerGlobalScope.
>  (3) Call through to the push_messaging Blink module with this object, so that
> it has all the information it needs to fire the |onpush| event.

In the browser side I think we should be able to build/expose a better interface
for these purposes, which could just call generic SendMessage() for sending
messages and implement ServiceWorkerVersion::Listener interface to listen to
messages (we don't have OnMessageReceived() method in the listener interface but
we can add it).  Then ServiceWorkerVersion itself doesn't need to know about
specific non-spec events like Sync, Push or all others you're planning to add.

In the renderer side I imagine we could build a similar interface on top of the
current SW code which will expose generic methods like: OnMessageReceived(),
Send() and GetServiceWorkerGlobalScopeProxy() or sort of them.

Would that kind of interface useful?  Assuming that we could have an interface
like that in both browser- and renderer-side, could we just tuck all these
non-spec event handling code into it, or, say, does each module want to have its
own implementation (e.g. sync will have its own message handler, push will do so
too etc etc)?

[kinuko, 2014-05-28 06:14:14]

On 2014/05/28 02:16:37, kinuko wrote:
> I think for a short-term we could get it in (if you're in hurry) but
scalability
> / messed-up interface issue worries me a bit too.
> 
> >  (1) Find and start the Service Worker, fire |onactivate| and make sure it's
> in
> > ACTIVATED state.
> >  (2) Pass a slightly higher-level object than ServiceWorkerGlobalScopeProxy
> > back to the feature, primarily holding the ExecutionContext and the
> > WorkerGlobalScope.
> >  (3) Call through to the push_messaging Blink module with this object, so
that
> > it has all the information it needs to fire the |onpush| event.
> 
> In the browser side I think we should be able to build/expose a better
interface
> for these purposes, which could just call generic SendMessage() for sending
> messages and implement ServiceWorkerVersion::Listener interface to listen to
> messages (we don't have OnMessageReceived() method in the listener interface
but
> we can add it).  Then ServiceWorkerVersion itself doesn't need to know about
> specific non-spec events like Sync, Push or all others you're planning to add.
> 
> In the renderer side I imagine we could build a similar interface on top of
the
> current SW code which will expose generic methods like: OnMessageReceived(),
> Send() and GetServiceWorkerGlobalScopeProxy() or sort of them.
> 
> Would that kind of interface useful?  Assuming that we could have an interface
> like that in both browser- and renderer-side, could we just tuck all these
> non-spec event handling code into it, or, say, does each module want to have
its
> own implementation (e.g. sync will have its own message handler, push will do
so
> too etc etc)?

To track this type of potential SW infra work I filed a new bug:
http://crbug.com/378172

[jam, 2014-05-28 17:25:23]

These push/sync events etc, are they chrome-specific feature additions? If so,
then they need to land with an extensible framework so that they're in
src\chrome and not in src\content, even temporarily. we don't allow layering
violations by putting chrome-specific stuff in content.

[Michael van Ouwerkerk, 2014-06-04 16:51:25]

On 2014/05/28 17:25:23, jam wrote:
> These push/sync events etc, are they chrome-specific feature additions? If so,
> then they need to land with an extensible framework so that they're in
> src\chrome and not in src\content, even temporarily. we don't allow layering
> violations by putting chrome-specific stuff in content.

John: push and sync are Open Web Platform features exposed in JS, the content
layer seems like the correct one.

[jam, 2014-06-04 23:06:24]

On 2014/06/04 16:51:25, Michael van Ouwerkerk wrote:
> On 2014/05/28 17:25:23, jam wrote:
> > These push/sync events etc, are they chrome-specific feature additions? If
so,
> > then they need to land with an extensible framework so that they're in
> > src\chrome and not in src\content, even temporarily. we don't allow layering
> > violations by putting chrome-specific stuff in content.
> 
> John: push and sync are Open Web Platform features exposed in JS, the content
> layer seems like the correct one.

hey, sorry for the noise, Alex explained things to me in-person earlier and I
told him that's fine. I got confused by earlier comments here that talked about
chrome.

[Michael van Ouwerkerk, 2014-06-05 14:26:29]

ptal

[Michael van Ouwerkerk, 2014-06-05 18:35:25]

Chris: please take a look at service_worker_messages.h

[michaeln, 2014-06-06 22:26:36]

lgtm

[palmer, 2014-06-06 23:13:18]

IPC security LGTM.

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

The CQ bit was checked by mvanouwerkerk@chromium.org

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

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/296463005/8...

[commit-bot: I haz the power, 2014-06-10 15:28:15]

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

[commit-bot: I haz the power, 2014-06-10 16:03:07]

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

[commit-bot: I haz the power, 2014-06-10 16:03:09]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[Michael van Ouwerkerk, 2014-06-10 21:14:31]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2014-06-10 21:17:39]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/296463005/8...

[commit-bot: I haz the power, 2014-06-10 21:47:17]

Change committed as 276166