Add test without skipWaiting() to demonstrate extension & service worker update.

Add test without skipWaiting() to demonstrate extension & service worker update.

Since the updated extension's SW doesn't call skipWaiting(), the old
SW would control the updated extension page in the beginning.
Once the extension page is closed, the new SW starts controlling
subsequent pages. This follows the SW model: "the incumbent worker
remains active until all pages controlled by it are closed."

BUG=533065
Committed: https://crrev.com/22eddc719fba6edd5eeb77485c1e9b1ab1f8f06a
Cr-Commit-Position: refs/heads/master@{#364479}

[lazyboy, 2015-12-08 04:08:39]

Description was changed from

==========
Add test without skipWaiting() to demonstrate extension & service worker update.

BUG=
==========

to

==========
Add test without skipWaiting() to demonstrate extension & service worker update.

Since the updated extension's SW doesn't call skipWaiting(), the old
SW would control the updated extension page in the beginning.
Once the extension page is closed, the new SW starts controlling
subsequent pages. This follows the SW model: "the incumbent worker
remains active until all pages controlled by it are closed."

BUG=533065
==========

[lazyboy, 2015-12-08 18:00:46]

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

[falken, 2015-12-09 03:59:16]

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

https://chromiumcodereview.appspot.com/1510573003/diff/40001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:309: // seem to be enough
because it returns too early.
Yes the update is quite async. This looks fragile and maybe flaky.

Maybe something like this works better:
1) Install v1
2) Load page.html, wait for "Pong from version 1"
3) Update to v2
4) Load page.html. This page can call update(), see an update is happening, and
wait until registration.waiting exists before responding with "Pong from version
1".
5) Close the tabs/navigate away.
6) Load page.html. This page calls update(), which should resolve without a
.installing and .waiting since no new update() is occurring, and respond with
"Pong from version 2".

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

https://chromiumcodereview.appspot.com/1510573003/diff/40001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/update_without_skip_waiting/v2/page.js:13:
});
Remove this dead code.

https://chromiumcodereview.appspot.com/1510573003/diff/40001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/update_without_skip_waiting/v2/page.js:23:
resolve(serviceWorkerRegistration.active);
This is kind of tricky. The update() promise resolves before update actually
completes. It resolves once the new version starts being installed. So this code
that's trying to wait for update to complete actually isn't.

[lazyboy, 2015-12-09 21:47:56]

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

https://chromiumcodereview.appspot.com/1510573003/diff/40001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:309: // seem to be enough
because it returns too early.
On 2015/12/09 03:59:16, falken wrote:
> Yes the update is quite async. This looks fragile and maybe flaky.
> 
> Maybe something like this works better:
> 1) Install v1
> 2) Load page.html, wait for "Pong from version 1"
> 3) Update to v2
> 4) Load page.html. This page can call update(), see an update is happening,
and
> wait until registration.waiting exists before responding with "Pong from
version
> 1".
> 5) Close the tabs/navigate away.
> 6) Load page.html. This page calls update(), which should resolve without a
> .installing and .waiting since no new update() is occurring, and respond with
> "Pong from version 2".

Done.
Note that I'm still not getting the test to work with just closing tab in step 5
above. It sees a waiting worker.
Navigate away still works.

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

https://chromiumcodereview.appspot.com/1510573003/diff/40001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/update_without_skip_waiting/v2/page.js:13:
});
On 2015/12/09 03:59:16, falken wrote:
> Remove this dead code.

Done.

https://chromiumcodereview.appspot.com/1510573003/diff/40001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/update_without_skip_waiting/v2/page.js:23:
resolve(serviceWorkerRegistration.active);
On 2015/12/09 03:59:16, falken wrote:
> This is kind of tricky. The update() promise resolves before update actually
> completes. It resolves once the new version starts being installed. So this
code
> that's trying to wait for update to complete actually isn't.

OK, I'm waiting for registration.onupdatefound now when we expect an update. I'm
also looking for waiting worker's existence before proceeding.

[falken, 2015-12-10 03:41:41]

LGTM!

https://codereview.chromium.org/1510573003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/service_worker_apitest.cc (right):

https://codereview.chromium.org/1510573003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/service_worker_apitest.cc:277: .GetByID(kId) != NULL);
nit: nullptr or maybe you can just remove the != ... completely (and below)

[lazyboy, 2015-12-10 04:21:19]

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

[lazyboy, 2015-12-10 04:21:20]

+Devlin for service_worker_apitest.cc OWNER

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

https://chromiumcodereview.appspot.com/1510573003/diff/60001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:277: .GetByID(kId) != NULL);
On 2015/12/10 03:41:41, falken wrote:
> nit: nullptr or maybe you can just remove the != ... completely (and below)

Done.

[Devlin, 2015-12-10 18:16:57]

lgtm

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

https://chromiumcodereview.appspot.com/1510573003/diff/80001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:51: return
browser->tab_strip_model()->GetWebContentsAt(tab_count - 1);
nit: I think GetActiveWebContents() is more clear.

https://chromiumcodereview.appspot.com/1510573003/diff/80001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:338:
ExtensionTestMessageListener listener_v2("Pong from version 1", false);
Calling this listener_v2 isn't really intuitive, since it's listening for the v1
SW.

https://chromiumcodereview.appspot.com/1510573003/diff/80001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:339:
listener_v2.set_failure_message("FAILURE_V2");
Since failure here could mean V1 failed or V2 was incorrectly controlling, maybe
we should just have a generic "failure" message.

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

https://chromiumcodereview.appspot.com/1510573003/diff/80001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/update_without_skip_waiting/v2/page.js:32:
chrome.test.assertTrue(null != serviceWorkerRegistration.waiting);
prefer chrome.test.assertTrue(!!serviceWorkerRegistration.waiting)

[lazyboy, 2015-12-10 18:59:44]

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

https://chromiumcodereview.appspot.com/1510573003/diff/80001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:51: return
browser->tab_strip_model()->GetWebContentsAt(tab_count - 1);
On 2015/12/10 18:16:57, Devlin wrote:
> nit: I think GetActiveWebContents() is more clear.

Done.

https://chromiumcodereview.appspot.com/1510573003/diff/80001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:338:
ExtensionTestMessageListener listener_v2("Pong from version 1", false);
On 2015/12/10 18:16:57, Devlin wrote:
> Calling this listener_v2 isn't really intuitive, since it's listening for the
v1
> SW.

Done.

https://chromiumcodereview.appspot.com/1510573003/diff/80001/chrome/browser/e...
chrome/browser/extensions/service_worker_apitest.cc:339:
listener_v2.set_failure_message("FAILURE_V2");
On 2015/12/10 18:16:57, Devlin wrote:
> Since failure here could mean V1 failed or V2 was incorrectly controlling,
maybe
> we should just have a generic "failure" message.

Done.

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

https://chromiumcodereview.appspot.com/1510573003/diff/80001/chrome/test/data...
chrome/test/data/extensions/api_test/service_worker/update_without_skip_waiting/v2/page.js:32:
chrome.test.assertTrue(null != serviceWorkerRegistration.waiting);
On 2015/12/10 18:16:57, Devlin wrote:
> prefer chrome.test.assertTrue(!!serviceWorkerRegistration.waiting)

Done.

[lazyboy, 2015-12-10 20:07:45]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2015-12-10 20:07:46]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1510573003/#ps100001 (title: "address
comments from Devlin")

[commit-bot: I haz the power, 2015-12-10 20:09:22]

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

[commit-bot: I haz the power, 2015-12-10 21:16:36]

Description was changed from

==========
Add test without skipWaiting() to demonstrate extension & service worker update.

Since the updated extension's SW doesn't call skipWaiting(), the old
SW would control the updated extension page in the beginning.
Once the extension page is closed, the new SW starts controlling
subsequent pages. This follows the SW model: "the incumbent worker
remains active until all pages controlled by it are closed."

BUG=533065
==========

to

==========
Add test without skipWaiting() to demonstrate extension & service worker update.

Since the updated extension's SW doesn't call skipWaiting(), the old
SW would control the updated extension page in the beginning.
Once the extension page is closed, the new SW starts controlling
subsequent pages. This follows the SW model: "the incumbent worker
remains active until all pages controlled by it are closed."

BUG=533065
==========

[commit-bot: I haz the power, 2015-12-10 21:16:37]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-10 21:18:00]

Description was changed from

==========
Add test without skipWaiting() to demonstrate extension & service worker update.

Since the updated extension's SW doesn't call skipWaiting(), the old
SW would control the updated extension page in the beginning.
Once the extension page is closed, the new SW starts controlling
subsequent pages. This follows the SW model: "the incumbent worker
remains active until all pages controlled by it are closed."

BUG=533065
==========

to

==========
Add test without skipWaiting() to demonstrate extension & service worker update.

Since the updated extension's SW doesn't call skipWaiting(), the old
SW would control the updated extension page in the beginning.
Once the extension page is closed, the new SW starts controlling
subsequent pages. This follows the SW model: "the incumbent worker
remains active until all pages controlled by it are closed."

BUG=533065

Committed: https://crrev.com/22eddc719fba6edd5eeb77485c1e9b1ab1f8f06a
Cr-Commit-Position: refs/heads/master@{#364479}
==========

[commit-bot: I haz the power, 2015-12-10 21:18:01]

Patchset 6 (id:??) landed as
https://crrev.com/22eddc719fba6edd5eeb77485c1e9b1ab1f8f06a
Cr-Commit-Position: refs/heads/master@{#364479}