Store the service worker script and its imports on first load... really

Store the service worker script and its imports on first load, read them on subsequent loads. The list of resource ids is stored with registration data. As registrations are deleted the old resources are also deleted.

BUG=364247, 364318
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272418

[michaeln, 2014-05-21 01:39:59]

ptal

[michaeln, 2014-05-21 02:13:45]

it might help to look at the progression from patch set 1 thru 4 to get a handle
on this change

[nhiroki, 2014-05-21 04:01:01]

LGTM!

https://codereview.chromium.org/293483002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/293483002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:512:
scoped_refptr<ServiceWorkerRegistration>());
nit: indent

https://codereview.chromium.org/293483002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:609: registration = 
new ServiceWorkerRegistration(
nit: There is an extra whitespace before "new".

[michaeln, 2014-05-21 23:42:19]

i've been working on some tests

[michaeln, 2014-05-22 00:32:02]

can you also take a look at the tests?

[nhiroki, 2014-05-22 01:20:12]

LGTM2 with some nits.

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:728:
OnResourcePurged(id, rv);
I see... DoomEntry() will run the callback only when rv==ERR_IO_PENDING.

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
File content/browser/service_worker/service_worker_storage_unittest.cc (right):

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:75: void
NoopCallback() {
You may want to use DoNothing() in bind_helpers.h.

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:106:
base::Bind(&OnIOComplete, &rv));
indent-nit.

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:123: }
nit: you can remove "{}"

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:181:
database->GetPurgeableResourceIds(purgeable_ids);
Can you check a status code of GetPurgeableResourceIds()?

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:541: const
GURL kScope("http://www.test.com/scope/*");
"www.test.com" seems to be a domain for an actual service. How about using
"www.example.com" instead?

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:593: //
purgable list and then doomed in the disk cache and removed from
nit: s/purgable/purgeable/

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:602:
&was_called, &result));
indent-nit.

[michaeln, 2014-05-22 02:02:37]

thnx, i'll do some more local testing and then click the cq unless something
looks bad

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:728:
OnResourcePurged(id, rv);
On 2014/05/22 01:20:13, nhiroki wrote:
> I see... DoomEntry() will run the callback only when rv==ERR_IO_PENDING.

yes, that flavor of async or not completion is common in the network stack

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
File content/browser/service_worker/service_worker_storage_unittest.cc (right):

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:75: void
NoopCallback() {
On 2014/05/22 01:20:13, nhiroki wrote:
> You may want to use DoNothing() in bind_helpers.h.

Done, ty!

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:106:
base::Bind(&OnIOComplete, &rv));
On 2014/05/22 01:20:13, nhiroki wrote:
> indent-nit.

Done.

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:123: }
On 2014/05/22 01:20:13, nhiroki wrote:
> nit: you can remove "{}"

Done. (mysterious macro w/o brackets spooks me though)

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:181:
database->GetPurgeableResourceIds(purgeable_ids);
On 2014/05/22 01:20:13, nhiroki wrote:
> Can you check a status code of GetPurgeableResourceIds()?

Done.

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:541: const
GURL kScope("http://www.test.com/scope/*");
On 2014/05/22 01:20:13, nhiroki wrote:
> "www.test.com" seems to be a domain for an actual service. How about using
> "www.example.com" instead?

does it matter? no network traffic is generated by any of this

i changed it to test.not to avoid line length changes

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:593: //
purgable list and then doomed in the disk cache and removed from
On 2014/05/22 01:20:13, nhiroki wrote:
> nit: s/purgable/purgeable/

Done.

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:602:
&was_called, &result));
On 2014/05/22 01:20:13, nhiroki wrote:
> indent-nit.

Done.

[nhiroki, 2014-05-22 03:15:33]

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
File content/browser/service_worker/service_worker_storage_unittest.cc (right):

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:123: }
On 2014/05/22 02:02:37, michaeln wrote:
> On 2014/05/22 01:20:13, nhiroki wrote:
> > nit: you can remove "{}"
> 
> Done. (mysterious macro w/o brackets spooks me though)

Ah... sorry, I unthoughtfully commented this after seeing line 124-125. You're
right. It'd be better to have brackets for the macro.

https://codereview.chromium.org/293483002/diff/40002/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:541: const
GURL kScope("http://www.test.com/scope/*");
On 2014/05/22 02:02:37, michaeln wrote:
> On 2014/05/22 01:20:13, nhiroki wrote:
> > "www.test.com" seems to be a domain for an actual service. How about using
> > "www.example.com" instead?
> 
> does it matter? no network traffic is generated by any of this
> 
> i changed it to test.not to avoid line length changes

Yes, this doesn't generate any traffic.

Probably I think too much... just in my personal opinion I'd prefer to avoid
such real domains from tests because I'm not sure what domain owner thinks about
it (I sometimes use them though.).

Anyway, we have already used such urls everywhere in the chromium project and
it's okay to leave as it is. I'm sorry for the bother.

[michaeln, 2014-05-23 02:16:58]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-05-23 02:18:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaeln@chromium.org/293483002/130001

[commit-bot: I haz the power, 2014-05-23 06:59:06]

Change committed as 272418