Make registration.update() no longer force bypassing the HTTP cache (1/2)

Make registration.update() no longer force bypassing the HTTP cache (1/2)

As per the last f2f and the follow-up discussion,
ServiceWorkerRegistration.update() should not always bypass the HTTP cache but
bypass it only when 24 hours have passed since the last update check. So, this
patch involves a change in the behavior of ServiceWorkerRegistration.update().

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#service-worker-registration-update
Spec discussion: https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-130664373

Companion CL (Chromium): https://codereview.chromium.org/1339623002/

BUG=530540

[jungkees, 2015-09-11 11:34:47]

jungkee.song@samsung.com changed reviewers:
+ jinho.bang@samsung.com, l.gombos@samsung.com

[jungkees, 2015-09-11 11:44:24]

This patch is split from https://codereview.chromium.org/1283273002/, and the
part that addresses registration.update()'s behavior change. (See #msg31). PTAL

[zino, 2015-09-11 11:49:35]

On 2015/09/11 11:44:24, jungkees wrote:
> This patch is split from https://codereview.chromium.org/1283273002/, and the
> part that addresses registration.update()'s behavior change. (See #msg31).
PTAL

lgtm

[zino, 2015-09-11 11:50:40]

Sorry, non-owner l-g-t-m. Please wait for owner's review.

[jungkees, 2015-09-11 11:54:34]

jungkee.song@samsung.com changed reviewers:
+ falken@chromium.org, kinuko@chromium.org, michaeln@chromium.org,
nhiroki@chromium.org, tkent@chromium.org

[jungkees, 2015-09-11 11:56:45]

Thanks for the peer review, zino@.

OWERs, PTAL

[falken, 2015-09-11 12:35:26]

Thanks for splitting these up into focused CLs.

https://codereview.chromium.org/1332303002/diff/1/LayoutTests/http/tests/serv...
File
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/update-worker.php
(right):

https://codereview.chromium.org/1332303002/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/update-worker.php:3:
header('Cache-Control: max-age=0');
Can you also test the converse: if the browser caches the script, update() does
not find a new version?

[jungkees, 2015-09-15 12:09:17]

I uploaded a new snapshot having addressed the comment. PTAL

https://codereview.chromium.org/1332303002/diff/1/LayoutTests/http/tests/serv...
File
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/update-worker.php
(right):

https://codereview.chromium.org/1332303002/diff/1/LayoutTests/http/tests/serv...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/update-worker.php:3:
header('Cache-Control: max-age=0');
Yes, when the script is served from cache, update() does not find a new version
nor updatefound is fired. I added a test for it.

[jungkees, 2015-10-01 05:19:36]

falken@, nhiroki@, could you take a look?

[falken, 2015-10-01 14:58:04]

I think I'd prefer this test be broken up into more targeted ones, rather than
be this monolithic thing with lots of asserts. The original test just tested
that ServiceWorkerRegistration.update() works when called from
ServiceWorkerGlobalScope. Now it's doing that plus testing the  non-cache
busting behavior of update().... it seems kind of arbitrary that it's tested
here and not in the non-SWGlobalScope version of update.html.

Is it possible to have something like:
promise_test(..., 'update() does not retrieve max-age 0 service worker from the
browser cache)  <-- is this even that helpful of a test?
promise_test(..., 'update() retrieves the service worker from the browser cache)

perhaps in the non-SWGlobalScope version of update.html?

https://codereview.chromium.org/1332303002/diff/40001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/update-worker.php
(right):

https://codereview.chromium.org/1332303002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/update-worker.php:27:
var events_seen = [];
BTW a lot of our tests do this but ideally we shouldn't rely on global state in
the sw.

https://codereview.chromium.org/1332303002/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/update.html
(right):

https://codereview.chromium.org/1332303002/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/update.html:39:
'events seen by the worker for register()');
This assert looks unneeded for this test, which is just testing update(). I'd
like to make the test as simple and targeted as possible.

[jungkees, 2015-10-02 02:08:25]

Alright. Does the following plan make sense?

On 2015/10/01 14:58:04, falken wrote:
> 
> Is it possible to have something like:
> promise_test(..., 'update() does not retrieve max-age 0 service worker from
the
> browser cache)  <-- is this even that helpful of a test?

This behavior is naturally being tested in the current serviceworker/update.html
as-is I guess.

> promise_test(..., 'update() retrieves the service worker from the browser
cache)
> 

Add serviceworker/update-served-from-cache.html to cover this. (Any better name
for it?)

> perhaps in the non-SWGlobalScope version of update.html?
> 

Yes, it sounds better to do it in the non-SWGlobalScope version as described
above and let SWGlobalScope/update.html be as-is.

[falken, 2015-10-02 02:09:54]

On 2015/10/02 02:08:25, jungkees wrote:
> Alright. Does the following plan make sense?
> 
> On 2015/10/01 14:58:04, falken wrote:
> > 
> > Is it possible to have something like:
> > promise_test(..., 'update() does not retrieve max-age 0 service worker from
> the
> > browser cache)  <-- is this even that helpful of a test?
> 
> This behavior is naturally being tested in the current
serviceworker/update.html
> as-is I guess.
> 
> > promise_test(..., 'update() retrieves the service worker from the browser
> cache)
> > 
> 
> Add serviceworker/update-served-from-cache.html to cover this. (Any better
name
> for it?)
> 
> > perhaps in the non-SWGlobalScope version of update.html?
> > 
> 
> Yes, it sounds better to do it in the non-SWGlobalScope version as described
> above and let SWGlobalScope/update.html be as-is.

Yep SGTM!

[jungkees, 2015-10-02 06:11:25]

I just uploaded a new snapshot having addressed the discussion we had. PTAL

[falken, 2015-10-02 07:25:55]

lgtm, thanks!

As for landing, Blink and Chromium repos are no longer separate. So you could
make a new patch with both 1/2 and 2/2.

[jungkees, 2015-10-03 03:39:19]

This CL has been merged to and landed as part of
https://codereview.chromium.org/1380323002/.