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

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

The resource data is put into an in-memory instance of a net::DiskCache for now. That cache will be used for (some) subsequent starts of the worker in the current browsing session, but will not be used across browser restarts. The next step is to have the registration job store the resource ids with the registration data on disk and to also put the resource data to disk.

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

[michaeln, 2014-05-07 05:46:04]

notes to self

https://codereview.chromium.org/269373002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_context_request_handler.cc
(right):

https://codereview.chromium.org/269373002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_context_request_handler.cc:34: 
todo: bail out if method != GET

https://codereview.chromium.org/269373002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/269373002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:148:
version_->NotifyMainScriptCacheResult(true);  // todo: all cases
Do this only for the main script, not imports.

[michaeln, 2014-05-08 01:20:25]

ptal, i think this is ready for a look. resources are cached then later
retrieved.

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

https://codereview.chromium.org/269373002/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:89: virtual void
OnMainScriptCacheResult(ServiceWorkerVersion* version,
I'll change the name of this to OnMainScriptCached. Also i think we need another
for OnAllImportsCached and the transition to INSTALLED should wait for that to
occur.

[nhiroki, 2014-05-08 01:58:56]

Looks good overall. nits only.

https://codereview.chromium.org/269373002/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_context_request_handler.h
(right):

https://codereview.chromium.org/269373002/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler.h:9:
#include "content/browser/service_worker/service_worker_version.h"
nit: Maybe you don't have to include this here (you have a forward decl for
SWVersion in *.h, and *.cc includes this)

https://codereview.chromium.org/269373002/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_disk_cache.h (right):

https://codereview.chromium.org/269373002/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_disk_cache.h:48: :
appcache::HttpResponseInfoIOBuffer(info) {}
nit: This should be 4-space indent.

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

https://codereview.chromium.org/269373002/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:220: // Propagates to to
Listeners.
nit: s/to to/to/

https://codereview.chromium.org/269373002/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/269373002/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
nit: can you remove "(c)"?

https://codereview.chromium.org/269373002/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:29:
response_id_(response_id),
"response_id_" need to be initialized before "version_" (decl order).

https://codereview.chromium.org/269373002/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:193:
DCHECK_NE(0, amount_to_write);
"DCHECK_LE" would be better.

https://codereview.chromium.org/269373002/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/269373002/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.h:66: int
*bytes_read);
nit: indent looks broken.

[michaeln, 2014-05-08 02:12:57]

thnx for looking

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

https://codereview.chromium.org/269373002/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_version.h:89: virtual void
OnMainScriptCacheResult(ServiceWorkerVersion* version,
On 2014/05/08 01:20:25, michaeln wrote:
> I'll change the name of this to OnMainScriptCached. Also i think we need
another
> for OnAllImportsCached and the transition to INSTALLED should wait for that to
> occur.

I might back this part out of this cl and do it when i hook the registration job
up.

[michaeln, 2014-05-08 03:35:47]

> I might back this part out of this cl and do it when i hook the registration
job
> up.

I did back that out. There are plenty of TODO cases in the job class that won't
be taken care of till later CLs.

There are a few items remaining to make this functional in m36:
- notify registration job when diskcaching
- regjob defers storing till ^^^
- (maybe) get an 'EvalIsDone' message from the renderer to help with ^^^
- another taskrunner for the diskcache to IO on and put it on disk

[kinuko, 2014-05-08 08:00:23]

Looks good.  Is it still in wip status?  Can we have some tests now or do we
want to do so later?

https://codereview.chromium.org/269373002/diff/70001/content/browser/service_...
File content/browser/service_worker/service_worker_context_request_handler.cc
(right):

https://codereview.chromium.org/269373002/diff/70001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler.cc:36:
return NULL;
Is this to abort (fall back to net) redirected requests?

https://codereview.chromium.org/269373002/diff/70001/content/browser/service_...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/269373002/diff/70001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:144: //
todo: NotifyDoneHelper?
Yes, calling NotifyDoneHelper here looks right?

https://codereview.chromium.org/269373002/diff/70001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:150:
SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
nit: we seem to call this in WriteDataToCache

https://codereview.chromium.org/269373002/diff/70001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:168: writer_
= context_->storage()->CreateResponseWriter(response_id_);
nit: DCHECK(!writer_) before creating to make sure?

https://codereview.chromium.org/269373002/diff/70001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:292: return;
nit: not necessary

[michaeln, 2014-05-08 22:27:23]

> Looks good.  Is it still in wip status?  Can we
> have some tests now or do we want to do so later?

I think i should bail on completing the scriptcaching for m36. Having
registration data stored but not the scripts is a useful stepping stone that
enables demos provided you're running on the network. I looks like regdata
persistence will get in for m36.

About tests, yes we should have tests for this class and the other one that was
already submitted w/o tests.

I'll prep a cl to do some of the other integration'y things listed a couple
comments up and leave this core write to disk one alone for now. This code
functions in the limited cases i've tried it but thats not saying much.

https://codereview.chromium.org/269373002/diff/70001/content/browser/service_...
File content/browser/service_worker/service_worker_context_request_handler.cc
(right):

https://codereview.chromium.org/269373002/diff/70001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler.cc:36:
return NULL;
On 2014/05/08 08:00:24, kinuko wrote:
> Is this to abort (fall back to net) redirected requests?

This is to always send redirected requests thru the network. At least for now,
there's no case where this class needs to create a custom job when redirection
is involved.

https://codereview.chromium.org/269373002/diff/70001/content/browser/service_...
File content/browser/service_worker/service_worker_read_from_cache_job.cc
(right):

https://codereview.chromium.org/269373002/diff/70001/content/browser/service_...
content/browser/service_worker/service_worker_read_from_cache_job.cc:137:
NotifyDone(net::URLRequestStatus(net::URLRequestStatus::FAILED, result));
NotifyRestartRequired to read it from the network if possible?

https://codereview.chromium.org/269373002/diff/70001/content/browser/service_...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/269373002/diff/70001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:181:
NotifyDoneHelper(net::URLRequestStatus(
NotifyRestartRequired or return but dont write the response

[michaeln, 2014-05-09 04:41:21]

> I think i should bail on completing the scriptcaching for m36. Having
> registration data stored but not the scripts is a useful stepping stone that
> enables demos provided you're running on the network. I looks like regdata
> persistence will get in for m36.
> 
> About tests, yes we should have tests for this class and the other one that
was
> already submitted w/o tests.
> 
> I'll prep a cl to do some of the other integration'y things listed a couple
> comments up and leave this core write to disk one alone for now. This code
> functions in the limited cases i've tried it but thats not saying much.

here's part of the "other integration'y things"
https://codereview.chromium.org/279683002/

[michaeln, 2014-05-13 05:45:40]

> here's part of the "other integration'y things"
> https://codereview.chromium.org/279683002/

and another https://codereview.chromium.org/280423005/

[michaeln, 2014-05-14 23:25:26]

I've been looking more closely at the sequence of events when a new version is
first brought up. I'll be changing some things in light of that and focusing
only on the initial install case (nothing for the update case). Here's the
sequence of events:

* version is created with a status of NEW
* job class start
* main script is downloaded
* background thread started
* EmbeddedWorkerHostMsg_WorkerScriptLoaded is sent
* main script evaluated
   - import scripts complete syncly
   - todo: we need to bust the memcache for these
* EmbeddedWorkerHostMsg_WorkerScriptStarted is  sent
* version status changes to INSTALLING

[michaeln, 2014-05-15 03:18:42]

I think this is ready for another closer look.

[kinuko, 2014-05-15 16:54:59]

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_database.h (left):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_database.h:60: 
Why do we remove it? ...oh ok to make it POD type?

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_disk_cache.h (right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_disk_cache.h:48: :
appcache::HttpResponseInfoIOBuffer(info) {}
nit: indent is off?

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_request_handler.cc:45: bool
IsSchemeAndMethodSupported(const net::URLRequest* request) {
nit: can we restore the TODO comment that we may eventually need to make this
one HTTPS only?

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_script_cache_map.cc (right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_script_cache_map.cc:54: {
it->second, it->first  };
nit: extra space after first

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_script_cache_map.h (right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_script_cache_map.h:31: // Used to
retrieve the results of the intiial run of a new version.
nit: intiial -> initial

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_script_cache_map.h:38: const
std::vector<ServiceWorkerDatabase::ResourceRecord>& resources);
When will these be called? I guess the the get one's called from RegisterJob
when it commits the resource IDs to the storage.

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:113:
request()->url(), status.is_success());
Ok... so we only reach here when all main script is loaded and also written to
the disk cache, and then EmbeddedWorkerHostMsg_WorkerScriptStarted is sent back
only after it's evaluated and all (sync) importScripts are done.

In the current code we always write disk before returning back the data, would
it have performance issue? (This one's only for SW scripts so should be ok?)

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:298: void
ServiceWorkerWriteToCacheJob::AsyncNotifyDoneHelper(
Why it's called Async?

[kinuko, 2014-05-15 16:56:06]

(I think the new flow's looking nicer/simpler, btw)

[nhiroki, 2014-05-15 23:30:08]

Looks good overall. Nitpicks only.

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_context_request_handler.h
(right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_context_request_handler.h:9:
#include "content/browser/service_worker/service_worker_version.h"
Can you remove this? The header is included in *.cc

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
nit: can you remove "(c)" ?

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:66: 
nit: an empty line.

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.h:107: bool
is_main_script_;
This does not seem to be used in this CL. Can you remove this?

[michaeln, 2014-05-15 23:54:30]

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_context_request_handler.h
(right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_context_request_handler.h:9:
#include "content/browser/service_worker/service_worker_version.h"
On 2014/05/15 23:30:08, nhiroki wrote:
> Can you remove this? The header is included in *.cc

Done.

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_database.h (left):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_database.h:60: 
On 2014/05/15 16:55:00, kinuko wrote:
> Why do we remove it? ...oh ok to make it POD type?

yup, its not really needed, i removed the dependency on version.h while tracking
down a compile error i had introduced at some point... and removing that
dependency seems like a good thing

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_disk_cache.h (right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_disk_cache.h:48: :
appcache::HttpResponseInfoIOBuffer(info) {}
On 2014/05/15 16:55:00, kinuko wrote:
> nit: indent is off?

Done.

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_request_handler.cc:45: bool
IsSchemeAndMethodSupported(const net::URLRequest* request) {
On 2014/05/15 16:55:00, kinuko wrote:
> nit: can we restore the TODO comment that we may eventually need to make this
> one HTTPS only?

I removed it after realizing that todo is in the wrong place. We only support
registrations on secure origins, but secure origins can request resources from
non-secure sources and the sw gets a crack at those too.

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_script_cache_map.cc (right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_script_cache_map.cc:54: {
it->second, it->first  };
On 2014/05/15 16:55:00, kinuko wrote:
> nit: extra space after first

Done.

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_script_cache_map.h (right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_script_cache_map.h:31: // Used to
retrieve the results of the intiial run of a new version.
On 2014/05/15 16:55:00, kinuko wrote:
> nit: intiial -> initial

Done (that may be my most frequently misspelled word)

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_script_cache_map.h:38: const
std::vector<ServiceWorkerDatabase::ResourceRecord>& resources);
On 2014/05/15 16:55:00, kinuko wrote:
> When will these be called? I guess the the get one's called from RegisterJob
> when it commits the resource IDs to the storage.

GetResources is for use at StoreRegistration time. SetResources is for use at
FindRegistrationFor<<>> time when creating a new live version object.

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
On 2014/05/15 23:30:08, nhiroki wrote:
> nit: can you remove "(c)" ? 

i don't know why i keep ending up with this copy/paste typo... i assure you i'm
not typing (c)'s anywhere.... i copied once from the wrong file and gaahhhhh :)

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:66: 
On 2014/05/15 23:30:08, nhiroki wrote:
> nit: an empty line.

Done.

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:113:
request()->url(), status.is_success());
On 2014/05/15 16:55:00, kinuko wrote:
> Ok... so we only reach here when all main script is loaded and also written to
> the disk cache, and then EmbeddedWorkerHostMsg_WorkerScriptStarted is sent
back
> only after it's evaluated and all (sync) importScripts are done.

yes, that's worth a comment somewhere, i'll drop something in script_cache_map.h
about that.

> In the current code we always write disk before returning back the data, would
> it have performance issue? (This one's only for SW scripts so should be ok?)

I doubt it, this and other stuff that happens later has to complete prior to the
SW becoming installed and functioning. Having one io_buffer and doing
read/write/return serially is greatly simplifying and that's really my
motivation for how it's structured right now. We could make a copy and
write/return in parallel but its more complicated that way.

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.h (right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.h:107: bool
is_main_script_;
On 2014/05/15 23:30:08, nhiroki wrote:
> This does not seem to be used in this CL. Can you remove this?

Done, thnx

[michaeln, 2014-05-16 00:06:35]

> (I think the new flow's looking nicer/simpler, btw)

thnx

[michaeln, 2014-05-16 00:12:09]

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/269373002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:298: void
ServiceWorkerWriteToCacheJob::AsyncNotifyDoneHelper(
> Why it's called Async?

Oh well... this was a poor attempt to try to clarify something. Earlier, we were
wondering whether ReadNetData should call NotifyDone or not when there is no
more to read. There answer is "no", notify done is for use when done'ness is
determined asyncly wrt ReadRawData. The base class internally calls NotifyDone
upon returning 0 from readraw and that DCHECKS() have not been called before.

[michaeln, 2014-05-16 01:44:26]

I've started working on the next CL that builds on this one, lmk if there's
anything else i should do for this cl.

My plan is for the next one to actually put stuff to disk.

[kinuko, 2014-05-16 03:57:06]

I might be missing some details, but lgtm, let's move this forward

https://codereview.chromium.org/269373002/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/269373002/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:126: NULL); 
// TODO(michaeln): request()->cookie_store()
nit: could you give a few more words about why we don't use it now?

[nhiroki, 2014-05-16 05:15:26]

LGTM

On 2014/05/16 01:44:26, michaeln wrote:
> I've started working on the next CL that builds on this one, lmk if there's
> anything else i should do for this cl.
> 
> My plan is for the next one to actually put stuff to disk.

Cool! I'm looking forward to playing with script persistence :)

[michaeln, 2014-05-16 21:27:02]

https://codereview.chromium.org/269373002/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/269373002/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:126: NULL); 
// TODO(michaeln): request()->cookie_store()
On 2014/05/16 03:57:06, kinuko wrote:
> nit: could you give a few more words about why we don't use it now?

Done

Its a protected method of URLRequest, but I found how 'jobs' access it and
switched to use that for this param.

[michaeln, 2014-05-16 23:40:28]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-05-16 23:43:11]

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

[commit-bot: I haz the power, 2014-05-17 11:47:12]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-17 18:22:16]

Change committed as 271253