Extension SW - add test for ServiceWorkerRegistration.sync.

Extension SW - add test for ServiceWorkerRegistration.sync.

The test puts the sync manager to offline and mode and then requests
  a sync through ServiceWorker. The ServiceWorker's onsync method
  fires when we put the sync manager back to online mode.

BUG=545535
Committed: https://crrev.com/bd325ae36bddc7d929a3562a44c5061d7015841b
Cr-Commit-Position: refs/heads/master@{#360417}

[lazyboy, 2015-11-14 00:08:22]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2015-11-14 00:08:23]

https://chromiumcodereview.appspot.com/1445603003/diff/20001/chrome/test/data...
File chrome/test/data/extensions/api_test/service_worker/sync/page.js (right):

https://chromiumcodereview.appspot.com/1445603003/diff/20001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/page.js:18: if
(syncPermissionState != 'granted') {
I'm not sure why I'm getting "granted" here. ?

[Devlin, 2015-11-16 18:12:41]

https://chromiumcodereview.appspot.com/1445603003/diff/20001/chrome/test/data...
File chrome/test/data/extensions/api_test/service_worker/sync/page.js (right):

https://chromiumcodereview.appspot.com/1445603003/diff/20001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/page.js:18: if
(syncPermissionState != 'granted') {
On 2015/11/14 00:08:23, lazyboy wrote:
> I'm not sure why I'm getting "granted" here. ?

I'm guessing:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ba...

"
  // TODO(iclelland): Implement a real policy. This is a stub implementation.
  // OneShot: crbug.com/482091
  // Periodic: crbug.com/482093
  callback.Run(BACKGROUND_SYNC_ERROR_NONE, PERMISSION_STATUS_GRANTED);
"

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/browser/e...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:374: GURL extension_url =
extension->url();
nit: Is this used?

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:375: GURL url =
extension->GetResourceURL("page.html");
I'd probably just inline this if it's only used on 376, but up to you.

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/test/data...
File chrome/test/data/extensions/api_test/service_worker/sync/page.js (right):

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/page.js:16: return
registration.sync.permissionState();
Could we just return the registration here, and avoid caching it in
serviceWorkerRegistration?

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/page.js:39:
chrome.test.sendMessage('' + e.data);
Does just sendMessage(e.data) not work (i.e., do you need the implicit cast to
string)?

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/test/data...
File chrome/test/data/extensions/api_test/service_worker/sync/sw.js (right):

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/sw.js:8:
e.waitUntil(new Promise(function(resolve) {
Maybe I'm missing something - when does this resolve?

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/sw.js:10: //
offline->online transition.
"small delays" usually correspond to flakiness.  Is there a reason we need it,
since we're not (in theory) executing any of this code until after the web
contents is offline?

https://chromiumcodereview.appspot.com/1445603003/diff/40001/content/public/t...
File content/public/test/background_sync_test_util.cc (right):

https://chromiumcodereview.appspot.com/1445603003/diff/40001/content/public/t...
content/public/test/background_sync_test_util.cc:19: using
content::BackgroundSyncManager;
Let's just put the anonymous namespace at line 26 within namespace content.

https://chromiumcodereview.appspot.com/1445603003/diff/40001/content/public/t...
File content/public/test/background_sync_test_util.h (right):

https://chromiumcodereview.appspot.com/1445603003/diff/40001/content/public/t...
content/public/test/background_sync_test_util.h:13: class BackgroundSyncTestUtil
{
nit: I prefer namespaces for collections of static methods (so this would be
namespace background_sync_test_util), but whatever's more common in content is
fine.

[jkarlin, 2015-11-16 18:33:26]

jkarlin@chromium.org changed reviewers:
+ jkarlin@chromium.org

[jkarlin, 2015-11-16 18:33:27]

Note that a much simplified API is about to land for background sync and will
conflict with this. See https://codereview.chromium.org/1437883002/.

[lazyboy, 2015-11-16 19:30:16]

@jkarlin, thanks for the heads up. I'll wait until
https://codereview.chromium.org/1437883002/ lands and update
the CL. Are there more CLs that will land as part of simplifying the API?

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/browser/e...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:374: GURL extension_url =
extension->url();
On 2015/11/16 18:12:41, Devlin wrote:
> nit: Is this used?

Fixed.

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:375: GURL url =
extension->GetResourceURL("page.html");
On 2015/11/16 18:12:41, Devlin wrote:
> I'd probably just inline this if it's only used on 376, but up to you.

Done.

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/test/data...
File chrome/test/data/extensions/api_test/service_worker/sync/page.js (right):

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/page.js:16: return
registration.sync.permissionState();
On 2015/11/16 18:12:41, Devlin wrote:
> Could we just return the registration here, and avoid caching it in
> serviceWorkerRegistration?

I want to check permissionState()'s return value and
then call registration.sync.register, so I need to cache it.

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/page.js:39:
chrome.test.sendMessage('' + e.data);
On 2015/11/16 18:12:41, Devlin wrote:
> Does just sendMessage(e.data) not work (i.e., do you need the implicit cast to
> string)?

Don't need it, just being safe to force cast, in case e.data is
not string for some reason.
Removed.

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/test/data...
File chrome/test/data/extensions/api_test/service_worker/sync/sw.js (right):

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/sw.js:8:
e.waitUntil(new Promise(function(resolve) {
On 2015/11/16 18:12:41, Devlin wrote:
> Maybe I'm missing something - when does this resolve?

It never did. Fixed.

https://chromiumcodereview.appspot.com/1445603003/diff/40001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/sw.js:10: //
offline->online transition.
On 2015/11/16 18:12:41, Devlin wrote:
> "small delays" usually correspond to flakiness.  Is there a reason we need it,
> since we're not (in theory) executing any of this code until after the web
> contents is offline?

We don't really need it, but this might catch some corner case
later on.
I've changed the comment a bit.

https://chromiumcodereview.appspot.com/1445603003/diff/40001/content/public/t...
File content/public/test/background_sync_test_util.cc (right):

https://chromiumcodereview.appspot.com/1445603003/diff/40001/content/public/t...
content/public/test/background_sync_test_util.cc:19: using
content::BackgroundSyncManager;
On 2015/11/16 18:12:41, Devlin wrote:
> Let's just put the anonymous namespace at line 26 within namespace content.

Done.

https://chromiumcodereview.appspot.com/1445603003/diff/40001/content/public/t...
File content/public/test/background_sync_test_util.h (right):

https://chromiumcodereview.appspot.com/1445603003/diff/40001/content/public/t...
content/public/test/background_sync_test_util.h:13: class BackgroundSyncTestUtil
{
On 2015/11/16 18:12:41, Devlin wrote:
> nit: I prefer namespaces for collections of static methods (so this would be
> namespace background_sync_test_util), but whatever's more common in content is
> fine.

Good point.
Done.

Should this namespace be nested inside content namespace though?

[jkarlin, 2015-11-16 19:37:26]

On 2015/11/16 19:30:16, lazyboy wrote:
> @jkarlin, thanks for the heads up. I'll wait until
> https://codereview.chromium.org/1437883002/ lands and update
> the CL. Are there more CLs that will land as part of simplifying the API?

That CL should be the last one to change the JavaScript API. Internally we'll
remove some dead code related to periodic sync, unregister, and permissionState.

[jkarlin, 2015-11-17 14:02:07]

On 2015/11/16 19:37:26, jkarlin wrote:
> On 2015/11/16 19:30:16, lazyboy wrote:
> > @jkarlin, thanks for the heads up. I'll wait until
> > https://codereview.chromium.org/1437883002/ lands and update
> > the CL. Are there more CLs that will land as part of simplifying the API?
> 
> That CL should be the last one to change the JavaScript API. Internally we'll
> remove some dead code related to periodic sync, unregister, and
permissionState.

Note that https://codereview.chromium.org/1437883002/ has landed.

[lazyboy, 2015-11-17 18:55:12]

On 2015/11/17 14:02:07, jkarlin wrote:
> On 2015/11/16 19:37:26, jkarlin wrote:
> > On 2015/11/16 19:30:16, lazyboy wrote:
> > > @jkarlin, thanks for the heads up. I'll wait until
> > > https://codereview.chromium.org/1437883002/ lands and update
> > > the CL. Are there more CLs that will land as part of simplifying the API?
> > 
> > That CL should be the last one to change the JavaScript API. Internally
we'll
> > remove some dead code related to periodic sync, unregister, and
> permissionState.
> 
> Note that https://codereview.chromium.org/1437883002/ has landed.

I've updated the CL at patch set #5.
Also @jkarlin, can you review the *background_sync* files? Thanks.

[Devlin, 2015-11-17 19:21:17]

lgtm

https://chromiumcodereview.appspot.com/1445603003/diff/40001/content/public/t...
File content/public/test/background_sync_test_util.h (right):

https://chromiumcodereview.appspot.com/1445603003/diff/40001/content/public/t...
content/public/test/background_sync_test_util.h:13: class BackgroundSyncTestUtil
{
On 2015/11/16 19:30:16, lazyboy wrote:
> On 2015/11/16 18:12:41, Devlin wrote:
> > nit: I prefer namespaces for collections of static methods (so this would be
> > namespace background_sync_test_util), but whatever's more common in content
is
> > fine.
> 
> Good point.
> Done.
> 
> Should this namespace be nested inside content namespace though?

I'll leave that up to a content/ OWNER. :)  (FWIW, my inclination is "yes")

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/browser/e...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:384:
ExtensionTestMessageListener sync_listener("SYNC: send-chats", false);
We could make this a little more strict by instantiating the listener before we
start the service worker, and then verify that it *isn't* satisfied until we go
back online.

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/test/data...
File chrome/test/data/extensions/api_test/service_worker/sync/page.js (right):

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/page.js:5: var SYNC_TAG
= 'send-chats';
I would either use this variable at both line 16 and 29, or would just inline
'send-chats' at line 16 (either is fine, and the latter might seem a little less
awkward, since it's a test).

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/test/data...
File chrome/test/data/extensions/api_test/service_worker/sync/sw.js (right):

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/sw.js:11: // NOTE: the
following 5ms delay is not a requirement for this test. It is
I still don't really like the 5ms delay here.  If you're just trying to make it
asynchronous, let's just do setTimeout with 0.

[jkarlin, 2015-11-17 20:36:15]

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/test/data...
File chrome/test/data/extensions/api_test/service_worker/sync/page.js (right):

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/page.js:7: var
startServiceWorker = new Promise(function(resolve, reject) {
Perhaps rename to registerSyncOnServiceWorker since that's what it's ultimately
doing.

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/page.js:15:
}).then(function(serviceWorkerRegistration) {
You don't need the above two lines. You can:

     // Wait until the service worker is active.
     return navigator.serviceWorker.ready;
   }).then(function(registration) {
     serviceWorker = registration.active;
     return registration.sync.register(SYNC_TAG);
   })...

https://chromiumcodereview.appspot.com/1445603003/diff/80001/content/public/t...
File content/public/test/background_sync_test_util.cc (right):

https://chromiumcodereview.appspot.com/1445603003/diff/80001/content/public/t...
content/public/test/background_sync_test_util.cc:54: // static.
no period as it's not a sentence

https://chromiumcodereview.appspot.com/1445603003/diff/80001/content/public/t...
content/public/test/background_sync_test_util.cc:56:
BackgroundSyncNetworkObserver::SetIgnoreNetworkChangeNotifierForTests(ignore);
Please update the other tests to use this function as well:
background_sync_manager_unittest.cc and
background_sync_service_impl_unittest.cc.

https://chromiumcodereview.appspot.com/1445603003/diff/80001/content/public/t...
content/public/test/background_sync_test_util.cc:59: // static.
no period as it's not a sentence

https://chromiumcodereview.appspot.com/1445603003/diff/80001/content/public/t...
File content/public/test/background_sync_test_util.h (right):

https://chromiumcodereview.appspot.com/1445603003/diff/80001/content/public/t...
content/public/test/background_sync_test_util.h:22: // start performing
background sync operation without waiting for the IO
s/operation/operations/

[lazyboy, 2015-11-17 21:41:28]

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/browser/e...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:384:
ExtensionTestMessageListener sync_listener("SYNC: send-chats", false);
On 2015/11/17 19:21:17, Devlin wrote:
> We could make this a little more strict by instantiating the listener before
we
> start the service worker, and then verify that it *isn't* satisfied until we
go
> back online.

I've checked for !was_satisfied() before going online.
Were you meaning this?

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/test/data...
File chrome/test/data/extensions/api_test/service_worker/sync/page.js (right):

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/page.js:5: var SYNC_TAG
= 'send-chats';
On 2015/11/17 19:21:17, Devlin wrote:
> I would either use this variable at both line 16 and 29, or would just inline
> 'send-chats' at line 16 (either is fine, and the latter might seem a little
less
> awkward, since it's a test).

Done.

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/page.js:7: var
startServiceWorker = new Promise(function(resolve, reject) {
On 2015/11/17 20:36:15, jkarlin wrote:
> Perhaps rename to registerSyncOnServiceWorker since that's what it's
ultimately
> doing.

Done.

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/page.js:15:
}).then(function(serviceWorkerRegistration) {
On 2015/11/17 20:36:15, jkarlin wrote:
> You don't need the above two lines. You can:
> 
>      // Wait until the service worker is active.
>      return navigator.serviceWorker.ready;
>    }).then(function(registration) {
>      serviceWorker = registration.active;
>      return registration.sync.register(SYNC_TAG);
>    })...

Done.

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/test/data...
File chrome/test/data/extensions/api_test/service_worker/sync/sw.js (right):

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/sync/sw.js:11: // NOTE: the
following 5ms delay is not a requirement for this test. It is
On 2015/11/17 19:21:17, Devlin wrote:
> I still don't really like the 5ms delay here.  If you're just trying to make
it
> asynchronous, let's just do setTimeout with 0.
Changed to 0ms.
Done.

https://chromiumcodereview.appspot.com/1445603003/diff/80001/content/public/t...
File content/public/test/background_sync_test_util.cc (right):

https://chromiumcodereview.appspot.com/1445603003/diff/80001/content/public/t...
content/public/test/background_sync_test_util.cc:54: // static.
On 2015/11/17 20:36:15, jkarlin wrote:
> no period as it's not a sentence

Done.

https://chromiumcodereview.appspot.com/1445603003/diff/80001/content/public/t...
content/public/test/background_sync_test_util.cc:56:
BackgroundSyncNetworkObserver::SetIgnoreNetworkChangeNotifierForTests(ignore);
On 2015/11/17 20:36:15, jkarlin wrote:
> Please update the other tests to use this function as well:
> background_sync_manager_unittest.cc and
> background_sync_service_impl_unittest.cc.

Done.

https://chromiumcodereview.appspot.com/1445603003/diff/80001/content/public/t...
content/public/test/background_sync_test_util.cc:59: // static.
On 2015/11/17 20:36:15, jkarlin wrote:
> no period as it's not a sentence

Done.

https://chromiumcodereview.appspot.com/1445603003/diff/80001/content/public/t...
File content/public/test/background_sync_test_util.h (right):

https://chromiumcodereview.appspot.com/1445603003/diff/80001/content/public/t...
content/public/test/background_sync_test_util.h:22: // start performing
background sync operation without waiting for the IO
On 2015/11/17 20:36:15, jkarlin wrote:
> s/operation/operations/

Done.

[Devlin, 2015-11-17 21:43:06]

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/browser/e...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://chromiumcodereview.appspot.com/1445603003/diff/80001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:384:
ExtensionTestMessageListener sync_listener("SYNC: send-chats", false);
On 2015/11/17 21:41:28, lazyboy wrote:
> On 2015/11/17 19:21:17, Devlin wrote:
> > We could make this a little more strict by instantiating the listener before
> we
> > start the service worker, and then verify that it *isn't* satisfied until we
> go
> > back online.
> 
> I've checked for !was_satisfied() before going online.
> Were you meaning this?

Yep.

[jkarlin, 2015-11-18 15:07:43]

lgtm

[lazyboy, 2015-11-18 18:25:46]

lazyboy@chromium.org changed reviewers:
+ avi@chromium.org

[lazyboy, 2015-11-18 18:25:46]

+Avi for content/public/test/*

[Avi (use Gerrit), 2015-11-18 18:57:26]

https://codereview.chromium.org/1445603003/diff/100001/content/public/test/ba...
File content/public/test/background_sync_test_util.h (right):

https://codereview.chromium.org/1445603003/diff/100001/content/public/test/ba...
content/public/test/background_sync_test_util.h:10: }  // namespace content
Why is this whole file not in namespace content?

[lazyboy, 2015-11-18 19:34:27]

Patchset #7 (id:120001) has been deleted

[lazyboy, 2015-11-18 19:35:44]

https://codereview.chromium.org/1445603003/diff/100001/content/public/test/ba...
File content/public/test/background_sync_test_util.h (right):

https://codereview.chromium.org/1445603003/diff/100001/content/public/test/ba...
content/public/test/background_sync_test_util.h:10: }  // namespace content
On 2015/11/18 18:57:26, Avi wrote:
> Why is this whole file not in namespace content?

Moved under content namespace.
Done.

[Avi (use Gerrit), 2015-11-18 19:37:47]

lgtm

[lazyboy, 2015-11-18 21:09:41]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2015-11-18 21:09:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, jkarlin@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1445603003/#ps140001 (title: "move
background_sync_test_util to content namespace")

[commit-bot: I haz the power, 2015-11-18 21:10:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1445603003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1445603003/140001

[commit-bot: I haz the power, 2015-11-18 21:35:37]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-11-18 21:36:40]

Patchset 7 (id:??) landed as
https://crrev.com/bd325ae36bddc7d929a3562a44c5061d7015841b
Cr-Commit-Position: refs/heads/master@{#360417}