Browser side of new ServiceWorker 'sync' event.

Browser side of new ServiceWorker 'sync' event.  The blink side is https://codereview.chromium.org/205013002/

This is just the plumbing for the event, and a
chrome://serviceworker-internals button to activate it for debugging
purposes.

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

[alecflett, 2014-03-19 19:34:38]

lgtm

This one and the other are prime examples of how to add new events to the
service worker, thanks!

[jkarlin, 2014-03-20 13:28:43]

tsepez@chromium.org: Please review changes in 
content/common/service_worker/service_worker_messages.h

[jsbell, 2014-03-20 16:19:37]

The lack of tests make me a sad panda, but I'm having a hard time coming up with
non-onerous suggestions. The least-bad I've come up with so far is a browsertest
which exposes an API (e.g. via XHR) for the SW to trigger a Sync event against
itself, and the SW could report successful receipt of the event via another XHR.

[Tom Sepez, 2014-03-20 16:47:51]

Messages LGTM.

[kinuko, 2014-03-24 05:10:08]

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

https://codereview.chromium.org/205033002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_internals_ui.cc:52: void
SyncWorkerOnIOThread(scoped_refptr<ServiceWorkerContextWrapper> context,
(ditto for all these method names)

https://codereview.chromium.org/205033002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_internals_ui.h (right):

https://codereview.chromium.org/205033002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_internals_ui.h:43: void
SyncWorker(const base::ListValue* args);
Is 'SyncWorker' really appropriate to describe what this does?

StartWorker starts a worker, and StopWorker stops a worker while SyncWorker
dispatches sync event to the worker...

DispatchSyncEventToWorker or something would be better?

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

https://codereview.chromium.org/205033002/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_messages.h:83: int /*
active_version_embedded_worker_id */)
I don't really know what information this SyncEvent will also need to convey,
but I don't think you need this parameter (I guess you copied it from
InstallEvent but we're likely going to cleanup that part too)

[jkarlin, 2014-03-25 18:28:16]

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

https://codereview.chromium.org/205033002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_internals_ui.cc:52: void
SyncWorkerOnIOThread(scoped_refptr<ServiceWorkerContextWrapper> context,
On 2014/03/24 05:10:08, kinuko wrote:
> (ditto for all these method names)

Done.

https://codereview.chromium.org/205033002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_internals_ui.h (right):

https://codereview.chromium.org/205033002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_internals_ui.h:43: void
SyncWorker(const base::ListValue* args);
On 2014/03/24 05:10:08, kinuko wrote:
> Is 'SyncWorker' really appropriate to describe what this does?
> 
> StartWorker starts a worker, and StopWorker stops a worker while SyncWorker
> dispatches sync event to the worker...
> 
> DispatchSyncEventToWorker or something would be better?

Done.

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

https://codereview.chromium.org/205033002/diff/1/content/common/service_worke...
content/common/service_worker/service_worker_messages.h:83: int /*
active_version_embedded_worker_id */)
On 2014/03/24 05:10:08, kinuko wrote:
> I don't really know what information this SyncEvent will also need to convey,
> but I don't think you need this parameter (I guess you copied it from
> InstallEvent but we're likely going to cleanup that part too)

Done.

[jkarlin, 2014-03-26 14:26:30]

On 2014/03/20 16:19:37, jsbell wrote:
> The lack of tests make me a sad panda, but I'm having a hard time coming up
with
> non-onerous suggestions. The least-bad I've come up with so far is a
browsertest
> which exposes an API (e.g. via XHR) for the SW to trigger a Sync event against
> itself, and the SW could report successful receipt of the event via another
XHR.

I'm also sad about this.  How about once the PostMessage communication works
between the SW and DOM I add a test that uses messages to show that the sync
event was fired?

[jsbell, 2014-03-26 19:57:57]

On 2014/03/26 14:26:30, jkarlin wrote:
> On 2014/03/20 16:19:37, jsbell wrote:
> > The lack of tests make me a sad panda, but I'm having a hard time coming up
> with
> > non-onerous suggestions. The least-bad I've come up with so far is a
> browsertest
> > which exposes an API (e.g. via XHR) for the SW to trigger a Sync event
against
> > itself, and the SW could report successful receipt of the event via another
> XHR.
> 
> I'm also sad about this.  How about once the PostMessage communication works
> between the SW and DOM I add a test that uses messages to show that the sync
> event was fired?

PostMessage DOM->SW is in (finally!), and you can postMessage a MessagePort
through to enable SW->DOM chatting - this layout test does it:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

But that's just the "reporting" half, you still need a way to trigger the Sync
event. There are probably other approaches and examples, but I was reviewing
this CL today which includes a browsertest that exposes a service via XHR to do
work:

https://codereview.chromium.org/197333009/diff/1/content/browser/indexed_db/i...

[jkarlin, 2014-03-31 13:44:23]

PTAL as I've added a browser test.

[jkarlin, 2014-04-01 13:51:34]

On 2014/03/31 13:44:23, jkarlin wrote:
> PTAL as I've added a browser test.

Ping! Would really like to get this in as SWs are moving so fast.

[kinuko, 2014-04-01 14:40:48]

Sorry for slow review, I think this lgtm (module some more minor comments
inline)

Also can you update the CL description before landing (the DO NOT COMMIT part
looks stale now)

https://codereview.chromium.org/205033002/diff/210001/content/browser/service...
File content/browser/service_worker/service_worker_internals_ui.cc (right):

https://codereview.chromium.org/205033002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_internals_ui.cc:470: if (status ==
SERVICE_WORKER_OK) {
Shouldn't we check if registration->active_version() is non-null and its status
is ACTIVE before actually calling DispatchSyncEvent?  And if they aren't
probably this should just finish with some error code.

https://codereview.chromium.org/205033002/diff/210001/content/test/data/servi...
File content/test/data/service_worker/sync.js (right):

https://codereview.chromium.org/205033002/diff/210001/content/test/data/servi...
content/test/data/service_worker/sync.js:24: code = 200;
It's interesting to see that you're using onfetch to test onsync :)

[jkarlin, 2014-04-01 15:06:05]

https://codereview.chromium.org/205033002/diff/210001/content/browser/service...
File content/browser/service_worker/service_worker_internals_ui.cc (right):

https://codereview.chromium.org/205033002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_internals_ui.cc:470: if (status ==
SERVICE_WORKER_OK) {
On 2014/04/01 14:40:49, kinuko wrote:
> Shouldn't we check if registration->active_version() is non-null and its
status
> is ACTIVE before actually calling DispatchSyncEvent?  And if they aren't
> probably this should just finish with some error code.

Done.

[jkarlin, 2014-04-01 15:08:07]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-04-01 15:08:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/205033002/230001

[commit-bot: I haz the power, 2014-04-01 18:17:57]

Change committed as 260907