Browser side of ServiceWorker requestSyncEvents() function

DO NOT LAND BEFORE https://codereview.chromium.org/224333003

Adding registration function for ServiceWorker 'sync' event.

The event has an optional Dictionary argument that is unused for now
but we may add meaning to it later.

Blink side: https://codereview.chromium.org/224333003

BUG=354112

[jkarlin, 2014-04-03 19:08:09]

Kinuko + Michael: Please review everything

Brett: Please review sync.js

tsepez: Please review service_worker_messages.h

[brettw, 2014-04-03 20:03:31]

I'm a terrible choice for a JS reviewer. Is there somebody else that's actually
a good JS programmer?

[jsbell, 2014-04-03 21:47:33]

https://codereview.chromium.org/214383005/diff/20001/content/test/data/servic...
File content/test/data/service_worker/sync.js (right):

https://codereview.chromium.org/214383005/diff/20001/content/test/data/servic...
content/test/data/service_worker/sync.js:5: var code = 404;
The test would be clearer if the global state was `var sawSyncEvent` to avoid
conflating the test's state (was sync fired?) with the signaling mechanism (200
vs 404)

https://codereview.chromium.org/214383005/diff/20001/content/test/data/servic...
content/test/data/service_worker/sync.js:18: if (fetchCount == 0) {
Since the sync event won't fire until after the event.respondWith() is handled
anyway, IMHO it would be clearer to move this all to the end of the function.
Then the function reads in order with (1) respond with previous state, (2) set
up test for the next state.

(Assuming I'm reading it correctly.)

https://codereview.chromium.org/214383005/diff/20001/content/test/data/servic...
content/test/data/service_worker/sync.js:29: statusCode: code,
And this would be sawSyncEvent ? 200 : 404;

https://codereview.chromium.org/214383005/diff/20001/content/test/data/servic...
content/test/data/service_worker/sync.js:33: 'Content-Language': 'fi',
Just for kicks? :)

[jsbell, 2014-04-03 21:48:09]

On 2014/04/03 20:03:31, brettw wrote:
> Is there somebody else that's actually a good JS programmer?

I think you win a cookie for asking that question. :)

[michaeln, 2014-04-03 22:52:22]

https://codereview.chromium.org/214383005/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/214383005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:167: void
RequestSyncEvents(bool requested);
naming nit: Can we rename this to use the term Enable sync events instead of
request in both blink and chrome? I think thats a more clear verb for this
function. wdyt?

[jkarlin, 2014-04-04 00:32:19]

https://codereview.chromium.org/214383005/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/214383005/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:167: void
RequestSyncEvents(bool requested);
On 2014/04/03 22:52:22, michaeln wrote:
> naming nit: Can we rename this to use the term Enable sync events instead of
> request in both blink and chrome? I think thats a more clear verb for this
> function. wdyt?

I like request because there is no guarantee as to when (or even if) the sync
event will occur and there will soon be parameters hinting when it should be
called.

[jkarlin, 2014-04-04 11:59:09]

https://codereview.chromium.org/214383005/diff/20001/content/test/data/servic...
File content/test/data/service_worker/sync.js (right):

https://codereview.chromium.org/214383005/diff/20001/content/test/data/servic...
content/test/data/service_worker/sync.js:5: var code = 404;
On 2014/04/03 21:47:34, jsbell wrote:
> The test would be clearer if the global state was `var sawSyncEvent` to avoid
> conflating the test's state (was sync fired?) with the signaling mechanism
(200
> vs 404)

Done.

https://codereview.chromium.org/214383005/diff/20001/content/test/data/servic...
content/test/data/service_worker/sync.js:18: if (fetchCount == 0) {
On 2014/04/03 21:47:34, jsbell wrote:
> Since the sync event won't fire until after the event.respondWith() is handled
> anyway, IMHO it would be clearer to move this all to the end of the function.
> Then the function reads in order with (1) respond with previous state, (2) set
> up test for the next state.
> 
> (Assuming I'm reading it correctly.)

Done.  Great suggestions, thanks.

https://codereview.chromium.org/214383005/diff/20001/content/test/data/servic...
content/test/data/service_worker/sync.js:29: statusCode: code,
On 2014/04/03 21:47:34, jsbell wrote:
> And this would be sawSyncEvent ? 200 : 404;

Done.

https://codereview.chromium.org/214383005/diff/20001/content/test/data/servic...
content/test/data/service_worker/sync.js:33: 'Content-Language': 'fi',
On 2014/04/03 21:47:34, jsbell wrote:
> Just for kicks? :)

Why not :)

[jsbell, 2014-04-04 19:14:57]

sync.js changes lgtm! 

On 2014/04/03 21:48:09, jsbell wrote:
> On 2014/04/03 20:03:31, brettw wrote:
> > Is there somebody else that's actually a good JS programmer?
> 
> I think you win a cookie for asking that question. :)

BTW, I was attempting to be funny and, with the zeitgeist of "JS is a terrible
language", interpret brettw's question as philosophical. I hope the attempt at
humor came through.

[michaeln, 2014-04-05 01:54:37]

This looks ok to me too (modulo i think the name is funky).

I'm partly being slow to response because most things that are accessible to
content go thru a pretty serious vetting process, and this feature seems to be
riding quietly on the coat tails of the sw effort w/o going thru that serious
vetting process. Before we commit a new webapi, let's run it by
chrome-serviceworker@google.com and see what the team thinks of the proposed
api? I know I think 'enable' is clearly a better verb for this(and since we're
toying with a public api it matters). Do you have any concrete thoughts about
the 'options' property bag?

We should probably bring this up in or F2F with moz next week too.

[michaeln, 2014-04-05 01:54:46]

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

https://codereview.chromium.org/214383005/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:167: void
RequestSyncEvents(bool requested);
maybe a getter too

[jkarlin, 2014-04-05 16:09:18]

On 2014/04/05 01:54:37, michaeln wrote:
> This looks ok to me too (modulo i think the name is funky).
> 
> I'm partly being slow to response because most things that are accessible to
> content go thru a pretty serious vetting process, and this feature seems to be
> riding quietly on the coat tails of the sw effort w/o going thru that serious
> vetting process. Before we commit a new webapi, let's run it by
> mailto:chrome-serviceworker@google.com and see what the team thinks of the
proposed
> api? I know I think 'enable' is clearly a better verb for this(and since we're
> toying with a public api it matters). Do you have any concrete thoughts about
> the 'options' property bag?
> 
> We should probably bring this up in or F2F with moz next week too.

That makes sense.  I'm working on a design doc for the sync API and will get
that out to everybody next week.  Hopefully then things will be more clear.  For
instance, my next CL will be to put the sync API behind a separate flag from
SWs.  Then we can enable SW without also enabling sync, which I agree needs to
go through its own vetting process and SW should not have its fate tied to sync.
 How does that sound?

[michaeln, 2014-04-07 03:38:48]

On 2014/04/05 16:09:18, jkarlin wrote:
> On 2014/04/05 01:54:37, michaeln wrote:
> > This looks ok to me too (modulo i think the name is funky).
> > 
> > I'm partly being slow to response because most things that are accessible to
> > content go thru a pretty serious vetting process, and this feature seems to
be
> > riding quietly on the coat tails of the sw effort w/o going thru that
serious
> > vetting process. Before we commit a new webapi, let's run it by
> > mailto:chrome-serviceworker@google.com and see what the team thinks of the
> proposed
> > api? I know I think 'enable' is clearly a better verb for this(and since
we're
> > toying with a public api it matters). Do you have any concrete thoughts
about
> > the 'options' property bag?
> > 
> > We should probably bring this up in or F2F with moz next week too.
> 
> That makes sense.  I'm working on a design doc for the sync API and will get
> that out to everybody next week.  Hopefully then things will be more clear. 
For
> instance, my next CL will be to put the sync API behind a separate flag from
> SWs.  Then we can enable SW without also enabling sync, which I agree needs to
> go through its own vetting process and SW should not have its fate tied to
sync.
>  How does that sound?

that svgtm, thank you!

[jkarlin, 2014-04-08 21:16:05]

On 2014/04/07 03:38:48, michaeln wrote:
> On 2014/04/05 16:09:18, jkarlin wrote:
> > On 2014/04/05 01:54:37, michaeln wrote:
> > > This looks ok to me too (modulo i think the name is funky).
> > > 
> > > I'm partly being slow to response because most things that are accessible
to
> > > content go thru a pretty serious vetting process, and this feature seems
to
> be
> > > riding quietly on the coat tails of the sw effort w/o going thru that
> serious
> > > vetting process. Before we commit a new webapi, let's run it by
> > > mailto:chrome-serviceworker@google.com and see what the team thinks of the
> > proposed
> > > api? I know I think 'enable' is clearly a better verb for this(and since
> we're
> > > toying with a public api it matters). Do you have any concrete thoughts
> about
> > > the 'options' property bag?
> > > 
> > > We should probably bring this up in or F2F with moz next week too.
> > 
> > That makes sense.  I'm working on a design doc for the sync API and will get
> > that out to everybody next week.  Hopefully then things will be more clear. 
> For
> > instance, my next CL will be to put the sync API behind a separate flag from
> > SWs.  Then we can enable SW without also enabling sync, which I agree needs
to
> > go through its own vetting process and SW should not have its fate tied to
> sync.
> >  How does that sound?
> 
> that svgtm, thank you!

Okay, command line flag CL is in the works, will upload that tomorrow.  Added a
design doc to the bug.

[michaeln, 2014-04-09 17:44:00]

Thanks for putting that the design doc.

We talked about the 'sync event' at the serviceworker issue-a-thon we just had
with mozilla, Some notes about that are kept here:
https://etherpad.mozilla.org/serviceworker07apr. I'll make some comments on the
design doc. Here's the part relevant to onsync...

// snip ------------

<script>
  navigator.serviceWorker.whenReady().then(function(sw) {
    // sw.state is "installed" or "activating" or "activated"

    navigator.requestSync(strID, {
      description: '', // default: ua-chosen
      data: '', // default: undefined? empty string?
      interval: 1000 * 60 * 60 * 24, // default: heuristic
      once: false,
      lang: '', // default: page lang
      dir: '' // default: page dir
    }).then(function() {
      // no resolved value
    }, function() {
      // If no SW registration
      // User/UA denied permission
      // If once is true and interval is set
    });
  }, function() {
    // THIS NEVER HAPPENS
  });
</script>

self.onsync = function(event) {
  var data = JSON.parse(event.data);

  if (event.id === strID) {
    if (data.whatever === 'foo') {
      // rejection is indication that the UA should try
      // later (especially when network is ok)
      event.waitUntil(doAsyncStuff());
    }
  }
  navigator.unregisterSync(event.id);
};

Should a SW opt into onsync/fetch?
Or can it opt out of onsync/fetch?
Math.random() / Date.now() is the broken case
Should addEventListener console.warn outside the initial execution?
Can we log on inconsistent registrations?
Action: Run script, gather listeners (as the first microtask), run microtasks,
fire oninstall, those are the events we fire

[jkarlin, 2014-04-10 18:16:10]

On 2014/04/09 17:44:00, michaeln wrote:
> Thanks for putting that the design doc.
> 
> We talked about the 'sync event' at the serviceworker issue-a-thon we just had
> with mozilla, Some notes about that are kept here:
> https://etherpad.mozilla.org/serviceworker07apr. I'll make some comments on
the
> design doc. Here's the part relevant to onsync...
> 
> // snip ------------
> 
> <script>
>   navigator.serviceWorker.whenReady().then(function(sw) {
>     // sw.state is "installed" or "activating" or "activated"
> 
>     navigator.requestSync(strID, {
>       description: '', // default: ua-chosen
>       data: '', // default: undefined? empty string?
>       interval: 1000 * 60 * 60 * 24, // default: heuristic
>       once: false,
>       lang: '', // default: page lang
>       dir: '' // default: page dir
>     }).then(function() {
>       // no resolved value
>     }, function() {
>       // If no SW registration
>       // User/UA denied permission
>       // If once is true and interval is set
>     });
>   }, function() {
>     // THIS NEVER HAPPENS
>   });
> </script>
> 
> self.onsync = function(event) {
>   var data = JSON.parse(event.data);
> 
>   if (event.id === strID) {
>     if (data.whatever === 'foo') {
>       // rejection is indication that the UA should try
>       // later (especially when network is ok)
>       event.waitUntil(doAsyncStuff());
>     }
>   }
>   navigator.unregisterSync(event.id);
> };
> 
> Should a SW opt into onsync/fetch?
> Or can it opt out of onsync/fetch?
> Math.random() / Date.now() is the broken case
> Should addEventListener console.warn outside the initial execution?
> Can we log on inconsistent registrations?
> Action: Run script, gather listeners (as the first microtask), run microtasks,
> fire oninstall, those are the events we fire

Addressing this API in the doc now (right now it's in alternative designs but
will soon move it to the proposed design with some modifications).

[jkarlin, 2014-04-10 18:17:19]

Removing reviewers as this CL will change dramatically as per the design doc.