ServiceWorkerWriteToCacheJob: refactor

ServiceWorkerWriteToCacheJob: refactor

This CL is a re-land of a previously reviewed and approved CL that was reverted: https://codereview.chromium.org/1356153002/

ServiceWorkerWriteToCacheJob is a subclass of URLRequestJob that is responsible for filling and updating a write-through cache for ServiceWorker data. Specifically, ServiceWorkerWriteToCacheJob creates an underlying URLRequestJob which it uses to do the actual network request for the script, then returns the script body to the creator of the ServiceWorkerWriteToCacheJob while also storing it in the ServiceWorker cache if it has been updated.

In order to avoid spurious cache rewrites, ServiceWorkerWriteToCacheJob defers writing an entry for as long as possible: if there is an existing entry, it compares incoming network data to the cache entry's data, and only starts writing once there is a mismatch. Since the mismatch may occur after much data has already been handled, once this happens ServiceWorkerWriteToCacheJob must transparently copy all the existing data up to that point from the cache, then resume streaming network data into the cache and back to its consumer. This is the "compare-and-copy" process. If there is no existing entry, network data is streamed directly to the cache; this is the "passthrough" process.

This CL removes the code implementing the compare-and-copy and passthrough processes from ServiceWorkerWriteToCacheJob and moves it into a separate class, ServiceWorkerCacheWriter. ServiceWorkerCacheWriter exposes an asynchronous API for overwriting a single cache entry that is now used by ServiceWorkerWriteToCacheJob.

BUG=474859
TBR=jochen,falken

Committed: https://crrev.com/7320022d75696c0f0b4ea02ed801fbbec4783658
Cr-Commit-Position: refs/heads/master@{#352593}

[Elly Fong-Jones, 2015-10-01 18:04:26]

ellyjones@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2015-10-01 21:17:31]

On 2015/10/01 18:04:26, Elly Jones wrote:
> mailto:ellyjones@chromium.org changed reviewers:
> + mailto:mmenke@chromium.org

Sorry I didn't get to this today - I will tomorrow.  Don't think we want to land
this right before branch, anyways.

[mmenke, 2015-10-01 22:01:45]

Think I'm going to have to defer to the service worker people on this one -
figuring out its correctness seems like I'd have to read a huge chunk of service
worker code, unless this behavior is documented somewhere.

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

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:426: //
script because the new script was identical, are considered to have failed.
method level docs should generally go above the declaration, or just above
they're describing, rather than above the method definition.

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:431:
net::URLRequestStatus real_status = status;
Maybe real_status -> reported_status?  I don't think the ERR_FAILED is a very
real status.  :(

Same for real_status_message, just to be consistent.

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
So in the no-replace case, we pass the entire response body to the calling
URLRequest, and then tell it the request failed?  Does the service worker
restart the request at that point, or claim it succeeded, or what?

[michaeln, 2015-10-02 00:22:44]

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

[michaeln, 2015-10-02 00:22:45]

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

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
On 2015/10/01 22:01:45, mmenke wrote:
> So in the no-replace case, we pass the entire response body to the calling
> URLRequest, and then tell it the request failed?  Does the service worker
> restart the request at that point, or claim it succeeded, or what?

The worker script doesn't load or execute in that case. Everything is told to
"nevermind" and c++ objs (ServiceWorkerVersion + EmbeddedWorkerInstance) are
deleted. The new instance only needs to run if the script has changed and the
installation sequence has to happen.

[mmenke, 2015-10-02 00:30:05]

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

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
On 2015/10/02 00:22:45, michaeln wrote:
> On 2015/10/01 22:01:45, mmenke wrote:
> > So in the no-replace case, we pass the entire response body to the calling
> > URLRequest, and then tell it the request failed?  Does the service worker
> > restart the request at that point, or claim it succeeded, or what?
> 
> The worker script doesn't load or execute in that case. Everything is told to
> "nevermind" and c++ objs (ServiceWorkerVersion + EmbeddedWorkerInstance) are
> deleted. The new instance only needs to run if the script has changed and the
> installation sequence has to happen.

Ohh...right... The name of this class was confusing me.  This isn't writing to a
service worker's cache, but writing a service worker script to the special cache
for service worker script.

The fact that this is structured as a URLRequestJob seems unintuitive to me,
though I suppose that lets you reuse the infrastructure to read from URLRequest
and stream the result to another process.

Looks like the ServiceWorkerWriteToCacheJob mentions this behavior (I was
looking at script_cache_map instead).

[michaeln, 2015-10-02 01:41:39]

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

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
On 2015/10/02 00:30:04, mmenke wrote:
> On 2015/10/02 00:22:45, michaeln wrote:
> > On 2015/10/01 22:01:45, mmenke wrote:
> > > So in the no-replace case, we pass the entire response body to the calling
> > > URLRequest, and then tell it the request failed?  Does the service worker
> > > restart the request at that point, or claim it succeeded, or what?
> > 
> > The worker script doesn't load or execute in that case. Everything is told
to
> > "nevermind" and c++ objs (ServiceWorkerVersion + EmbeddedWorkerInstance) are
> > deleted. The new instance only needs to run if the script has changed and
the
> > installation sequence has to happen.
> 
> Ohh...right... The name of this class was confusing me.  This isn't writing to
a
> service worker's cache, but writing a service worker script to the special
cache
> for service worker script.

The class name is odd, it says 'write' but the renderer also 'reads' the
response involved. A better name might be
ServiceWorkerInstallationURLRequestJob?

> The fact that this is structured as a URLRequestJob seems unintuitive to me,
> though I suppose that lets you reuse the infrastructure to read from
URLRequest
> and stream the result to another process.

Maybe the name makes it unintuitive? This is how the serviceworker scripts are
delivered to renderers on initial install, so of course it uses jobs, just like
all other cases of loading a worker in a renderer.

That blinkloader + weburlloader + resource dispatcher host + url request job
pipeline works well for streaming data from here to there incrementally and
asyncly with flow control stuff built in. And that loading pipeline is used for
all other cases of starting a worker (shared, dedicated, currently installed sw,
retrieval from the net, retreival from the httpcache, retrieval from the
appcache, retrieval from a cachestorage cache, etc).

There's a lot of not-so-easy-to-replicate-code reuse by virtue of leveraging the
urlrequestjob hook. All manner of data is loaded via urlrequest jobs:
chrome:settings, chrome-extensions, blobs, filesystem-files, net-internals,
devtools, etc.

[mmenke, 2015-10-02 03:12:08]

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

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
On 2015/10/02 01:41:39, michaeln wrote:
> On 2015/10/02 00:30:04, mmenke wrote:
> > On 2015/10/02 00:22:45, michaeln wrote:
> > > On 2015/10/01 22:01:45, mmenke wrote:
> > > > So in the no-replace case, we pass the entire response body to the
calling
> > > > URLRequest, and then tell it the request failed?  Does the service
worker
> > > > restart the request at that point, or claim it succeeded, or what?
> > > 
> > > The worker script doesn't load or execute in that case. Everything is told
> to
> > > "nevermind" and c++ objs (ServiceWorkerVersion + EmbeddedWorkerInstance)
are
> > > deleted. The new instance only needs to run if the script has changed and
> the
> > > installation sequence has to happen.
> > 
> > Ohh...right... The name of this class was confusing me.  This isn't writing
to
> a
> > service worker's cache, but writing a service worker script to the special
> cache
> > for service worker script.
> 
> The class name is odd, it says 'write' but the renderer also 'reads' the
> response involved. A better name might be
> ServiceWorkerInstallationURLRequestJob?

I think that's much clearer.

> > The fact that this is structured as a URLRequestJob seems unintuitive to me,
> > though I suppose that lets you reuse the infrastructure to read from
> URLRequest
> > and stream the result to another process.
> 
> Maybe the name makes it unintuitive? This is how the serviceworker scripts are
> delivered to renderers on initial install, so of course it uses jobs, just
like
> all other cases of loading a worker in a renderer.
>
> That blinkloader + weburlloader + resource dispatcher host + url request job
> pipeline works well for streaming data from here to there incrementally and
> asyncly with flow control stuff built in. And that loading pipeline is used
for
> all other cases of starting a worker (shared, dedicated, currently installed
sw,
> retrieval from the net, retreival from the httpcache, retrieval from the
> appcache, retrieval from a cachestorage cache, etc).
> 
> There's a lot of not-so-easy-to-replicate-code reuse by virtue of leveraging
the
> urlrequestjob hook. All manner of data is loaded via urlrequest jobs:
> chrome:settings, chrome-extensions, blobs, filesystem-files, net-internals,
> devtools, etc.


The thing that's weird here is a URLRequestJob normally handles a request for a
URL.  The renderer says "Give me foo://yada/yada/yada", and then the RDH
requests the resource at that location, which net/ then provides to it.  Here,
the other process is saying "I want this resource, and I want you to inject a
URLRequestJob into the network stack of this special type to provide the
response for specific request."  So you really aren't using any of net's
infrastructure here.  Even the assumption that all requests of the same scheme
are handled in the same way, with the same ProtocolHandler, is violated.

I'm sure there are many reasons we can't just use standard HTTP caching
semantics here, unfortunately, though in an ideal world, that's how I feel this
sort of thing would work (With an additional small table of the service workers
we know about, of course).

[mmenke, 2015-10-02 03:14:14]

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

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
On 2015/10/02 03:12:08, mmenke wrote:
> On 2015/10/02 01:41:39, michaeln wrote:
> > On 2015/10/02 00:30:04, mmenke wrote:
> > > On 2015/10/02 00:22:45, michaeln wrote:
> > > > On 2015/10/01 22:01:45, mmenke wrote:
> > > > > So in the no-replace case, we pass the entire response body to the
> calling
> > > > > URLRequest, and then tell it the request failed?  Does the service
> worker
> > > > > restart the request at that point, or claim it succeeded, or what?
> > > > 
> > > > The worker script doesn't load or execute in that case. Everything is
told
> > to
> > > > "nevermind" and c++ objs (ServiceWorkerVersion + EmbeddedWorkerInstance)
> are
> > > > deleted. The new instance only needs to run if the script has changed
and
> > the
> > > > installation sequence has to happen.
> > > 
> > > Ohh...right... The name of this class was confusing me.  This isn't
writing
> to
> > a
> > > service worker's cache, but writing a service worker script to the special
> > cache
> > > for service worker script.
> > 
> > The class name is odd, it says 'write' but the renderer also 'reads' the
> > response involved. A better name might be
> > ServiceWorkerInstallationURLRequestJob?
> 
> I think that's much clearer.
> 
> > > The fact that this is structured as a URLRequestJob seems unintuitive to
me,
> > > though I suppose that lets you reuse the infrastructure to read from
> > URLRequest
> > > and stream the result to another process.
> > 
> > Maybe the name makes it unintuitive? This is how the serviceworker scripts
are
> > delivered to renderers on initial install, so of course it uses jobs, just
> like
> > all other cases of loading a worker in a renderer.
> >
> > That blinkloader + weburlloader + resource dispatcher host + url request job
> > pipeline works well for streaming data from here to there incrementally and
> > asyncly with flow control stuff built in. And that loading pipeline is used
> for
> > all other cases of starting a worker (shared, dedicated, currently installed
> sw,
> > retrieval from the net, retreival from the httpcache, retrieval from the
> > appcache, retrieval from a cachestorage cache, etc).
> > 
> > There's a lot of not-so-easy-to-replicate-code reuse by virtue of leveraging
> the
> > urlrequestjob hook. All manner of data is loaded via urlrequest jobs:
> > chrome:settings, chrome-extensions, blobs, filesystem-files, net-internals,
> > devtools, etc.
> 
> 
> The thing that's weird here is a URLRequestJob normally handles a request for
a
> URL.  The renderer says "Give me foo://yada/yada/yada", and then the RDH
> requests the resource at that location, which net/ then provides to it.  Here,
> the other process is saying "I want this resource, and I want you to inject a
> URLRequestJob into the network stack of this special type to provide the
> response for specific request."  So you really aren't using any of net's
> infrastructure here.  Even the assumption that all requests of the same scheme
> are handled in the same way, with the same ProtocolHandler, is violated.
> 
> I'm sure there are many reasons we can't just use standard HTTP caching
> semantics here, unfortunately, though in an ideal world, that's how I feel
this
> sort of thing would work (With an additional small table of the service
workers
> we know about, of course).

I don't mean to say that this is the wrong design, just that it's something that
makes ServiceWorker (And AppCache's) use of the network stack a bit unusual. 
There are also magic blob URLs that work the same way, I believe (Not sure if
all blob URLs work similarly, but some do).

[mmenke, 2015-10-02 14:10:44]

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

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
On 2015/10/02 03:14:14, mmenke wrote:
> On 2015/10/02 03:12:08, mmenke wrote:
> > On 2015/10/02 01:41:39, michaeln wrote:
> > > On 2015/10/02 00:30:04, mmenke wrote:
> > > > On 2015/10/02 00:22:45, michaeln wrote:
> > > > > On 2015/10/01 22:01:45, mmenke wrote:
> > > > > > So in the no-replace case, we pass the entire response body to the
> > calling
> > > > > > URLRequest, and then tell it the request failed?  Does the service
> > worker
> > > > > > restart the request at that point, or claim it succeeded, or what?
> > > > > 
> > > > > The worker script doesn't load or execute in that case. Everything is
> told
> > > to
> > > > > "nevermind" and c++ objs (ServiceWorkerVersion +
EmbeddedWorkerInstance)
> > are
> > > > > deleted. The new instance only needs to run if the script has changed
> and
> > > the
> > > > > installation sequence has to happen.
> > > > 
> > > > Ohh...right... The name of this class was confusing me.  This isn't
> writing
> > to
> > > a
> > > > service worker's cache, but writing a service worker script to the
special
> > > cache
> > > > for service worker script.
> > > 
> > > The class name is odd, it says 'write' but the renderer also 'reads' the
> > > response involved. A better name might be
> > > ServiceWorkerInstallationURLRequestJob?
> > 
> > I think that's much clearer.
> > 
> > > > The fact that this is structured as a URLRequestJob seems unintuitive to
> me,
> > > > though I suppose that lets you reuse the infrastructure to read from
> > > URLRequest
> > > > and stream the result to another process.
> > > 
> > > Maybe the name makes it unintuitive? This is how the serviceworker scripts
> are
> > > delivered to renderers on initial install, so of course it uses jobs, just
> > like
> > > all other cases of loading a worker in a renderer.
> > >
> > > That blinkloader + weburlloader + resource dispatcher host + url request
job
> > > pipeline works well for streaming data from here to there incrementally
and
> > > asyncly with flow control stuff built in. And that loading pipeline is
used
> > for
> > > all other cases of starting a worker (shared, dedicated, currently
installed
> > sw,
> > > retrieval from the net, retreival from the httpcache, retrieval from the
> > > appcache, retrieval from a cachestorage cache, etc).
> > > 
> > > There's a lot of not-so-easy-to-replicate-code reuse by virtue of
leveraging
> > the
> > > urlrequestjob hook. All manner of data is loaded via urlrequest jobs:
> > > chrome:settings, chrome-extensions, blobs, filesystem-files,
net-internals,
> > > devtools, etc.
> > 
> > 
> > The thing that's weird here is a URLRequestJob normally handles a request
for
> a
> > URL.  The renderer says "Give me foo://yada/yada/yada", and then the RDH
> > requests the resource at that location, which net/ then provides to it. 
Here,
> > the other process is saying "I want this resource, and I want you to inject
a
> > URLRequestJob into the network stack of this special type to provide the
> > response for specific request."  So you really aren't using any of net's
> > infrastructure here.  Even the assumption that all requests of the same
scheme
> > are handled in the same way, with the same ProtocolHandler, is violated.
> > 
> > I'm sure there are many reasons we can't just use standard HTTP caching
> > semantics here, unfortunately, though in an ideal world, that's how I feel
> this
> > sort of thing would work (With an additional small table of the service
> workers
> > we know about, of course).
> 
> I don't mean to say that this is the wrong design, just that it's something
that
> makes ServiceWorker (And AppCache's) use of the network stack a bit unusual. 
> There are also magic blob URLs that work the same way, I believe (Not sure if
> all blob URLs work similarly, but some do).

It's generally the goal of a URLRequestJob to fetch a resource using the
underlying protocol.

The purpose of this job is to fetch a resource (Wrapping the real URLRequest),
and then provide bonus semantics, because the consumer injected them into the
network stack.  The fact that this is a URLRequest is just because you want a
way to send the result of the bonus logic to the process running a service
worker.

If instead, you used a normal URLRequest, with the normal cache, and compared it
against the previously loaded script in the SW process, everything would still
work, and make a lot more sense, from a layering standpoint.  I think the only
problem with doing that is you want extra caching rules.

[Elly Fong-Jones, 2015-10-02 14:24:46]

done, ptal? :)

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

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:426: //
script because the new script was identical, are considered to have failed.
On 2015/10/01 22:01:45, mmenke wrote:
> method level docs should generally go above the declaration, or just above
> they're describing, rather than above the method definition.

Done.

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:431:
net::URLRequestStatus real_status = status;
On 2015/10/01 22:01:45, mmenke wrote:
> Maybe real_status -> reported_status?  I don't think the ERR_FAILED is a very
> real status.  :(
> 
> Same for real_status_message, just to be consistent.

Done.

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
On 2015/10/01 22:01:45, mmenke wrote:
> So in the no-replace case, we pass the entire response body to the calling
> URLRequest, and then tell it the request failed?  Does the service worker
> restart the request at that point, or claim it succeeded, or what?

Acknowledged.

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
On 2015/10/02 00:22:45, michaeln wrote:
> On 2015/10/01 22:01:45, mmenke wrote:
> > So in the no-replace case, we pass the entire response body to the calling
> > URLRequest, and then tell it the request failed?  Does the service worker
> > restart the request at that point, or claim it succeeded, or what?
> 
> The worker script doesn't load or execute in that case. Everything is told to
> "nevermind" and c++ objs (ServiceWorkerVersion + EmbeddedWorkerInstance) are
> deleted. The new instance only needs to run if the script has changed and the
> installation sequence has to happen.

Acknowledged.

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
On 2015/10/02 00:30:04, mmenke wrote:
> On 2015/10/02 00:22:45, michaeln wrote:
> > On 2015/10/01 22:01:45, mmenke wrote:
> > > So in the no-replace case, we pass the entire response body to the calling
> > > URLRequest, and then tell it the request failed?  Does the service worker
> > > restart the request at that point, or claim it succeeded, or what?
> > 
> > The worker script doesn't load or execute in that case. Everything is told
to
> > "nevermind" and c++ objs (ServiceWorkerVersion + EmbeddedWorkerInstance) are
> > deleted. The new instance only needs to run if the script has changed and
the
> > installation sequence has to happen.
> 
> Ohh...right... The name of this class was confusing me.  This isn't writing to
a
> service worker's cache, but writing a service worker script to the special
cache
> for service worker script.
> 
> The fact that this is structured as a URLRequestJob seems unintuitive to me,
> though I suppose that lets you reuse the infrastructure to read from
URLRequest
> and stream the result to another process.
> 
> Looks like the ServiceWorkerWriteToCacheJob mentions this behavior (I was
> looking at script_cache_map instead).

Acknowledged.

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
On 2015/10/02 01:41:39, michaeln wrote:
> On 2015/10/02 00:30:04, mmenke wrote:
> > On 2015/10/02 00:22:45, michaeln wrote:
> > > On 2015/10/01 22:01:45, mmenke wrote:
> > > > So in the no-replace case, we pass the entire response body to the
calling
> > > > URLRequest, and then tell it the request failed?  Does the service
worker
> > > > restart the request at that point, or claim it succeeded, or what?
> > > 
> > > The worker script doesn't load or execute in that case. Everything is told
> to
> > > "nevermind" and c++ objs (ServiceWorkerVersion + EmbeddedWorkerInstance)
are
> > > deleted. The new instance only needs to run if the script has changed and
> the
> > > installation sequence has to happen.
> > 
> > Ohh...right... The name of this class was confusing me.  This isn't writing
to
> a
> > service worker's cache, but writing a service worker script to the special
> cache
> > for service worker script.
> 
> The class name is odd, it says 'write' but the renderer also 'reads' the
> response involved. A better name might be
> ServiceWorkerInstallationURLRequestJob?
> 
> > The fact that this is structured as a URLRequestJob seems unintuitive to me,
> > though I suppose that lets you reuse the infrastructure to read from
> URLRequest
> > and stream the result to another process.
> 
> Maybe the name makes it unintuitive? This is how the serviceworker scripts are
> delivered to renderers on initial install, so of course it uses jobs, just
like
> all other cases of loading a worker in a renderer.
> 
> That blinkloader + weburlloader + resource dispatcher host + url request job
> pipeline works well for streaming data from here to there incrementally and
> asyncly with flow control stuff built in. And that loading pipeline is used
for
> all other cases of starting a worker (shared, dedicated, currently installed
sw,
> retrieval from the net, retreival from the httpcache, retrieval from the
> appcache, retrieval from a cachestorage cache, etc).
> 
> There's a lot of not-so-easy-to-replicate-code reuse by virtue of leveraging
the
> urlrequestjob hook. All manner of data is loaded via urlrequest jobs:
> chrome:settings, chrome-extensions, blobs, filesystem-files, net-internals,
> devtools, etc.

Acknowledged.

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
On 2015/10/02 03:12:08, mmenke wrote:
> On 2015/10/02 01:41:39, michaeln wrote:
> > On 2015/10/02 00:30:04, mmenke wrote:
> > > On 2015/10/02 00:22:45, michaeln wrote:
> > > > On 2015/10/01 22:01:45, mmenke wrote:
> > > > > So in the no-replace case, we pass the entire response body to the
> calling
> > > > > URLRequest, and then tell it the request failed?  Does the service
> worker
> > > > > restart the request at that point, or claim it succeeded, or what?
> > > > 
> > > > The worker script doesn't load or execute in that case. Everything is
told
> > to
> > > > "nevermind" and c++ objs (ServiceWorkerVersion + EmbeddedWorkerInstance)
> are
> > > > deleted. The new instance only needs to run if the script has changed
and
> > the
> > > > installation sequence has to happen.
> > > 
> > > Ohh...right... The name of this class was confusing me.  This isn't
writing
> to
> > a
> > > service worker's cache, but writing a service worker script to the special
> > cache
> > > for service worker script.
> > 
> > The class name is odd, it says 'write' but the renderer also 'reads' the
> > response involved. A better name might be
> > ServiceWorkerInstallationURLRequestJob?
> 
> I think that's much clearer.
> 
> > > The fact that this is structured as a URLRequestJob seems unintuitive to
me,
> > > though I suppose that lets you reuse the infrastructure to read from
> > URLRequest
> > > and stream the result to another process.
> > 
> > Maybe the name makes it unintuitive? This is how the serviceworker scripts
are
> > delivered to renderers on initial install, so of course it uses jobs, just
> like
> > all other cases of loading a worker in a renderer.
> >
> > That blinkloader + weburlloader + resource dispatcher host + url request job
> > pipeline works well for streaming data from here to there incrementally and
> > asyncly with flow control stuff built in. And that loading pipeline is used
> for
> > all other cases of starting a worker (shared, dedicated, currently installed
> sw,
> > retrieval from the net, retreival from the httpcache, retrieval from the
> > appcache, retrieval from a cachestorage cache, etc).
> > 
> > There's a lot of not-so-easy-to-replicate-code reuse by virtue of leveraging
> the
> > urlrequestjob hook. All manner of data is loaded via urlrequest jobs:
> > chrome:settings, chrome-extensions, blobs, filesystem-files, net-internals,
> > devtools, etc.
> 
> 
> The thing that's weird here is a URLRequestJob normally handles a request for
a
> URL.  The renderer says "Give me foo://yada/yada/yada", and then the RDH
> requests the resource at that location, which net/ then provides to it.  Here,
> the other process is saying "I want this resource, and I want you to inject a
> URLRequestJob into the network stack of this special type to provide the
> response for specific request."  So you really aren't using any of net's
> infrastructure here.  Even the assumption that all requests of the same scheme
> are handled in the same way, with the same ProtocolHandler, is violated.
> 
> I'm sure there are many reasons we can't just use standard HTTP caching
> semantics here, unfortunately, though in an ideal world, that's how I feel
this
> sort of thing would work (With an additional small table of the service
workers
> we know about, of course).

Acknowledged.

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
On 2015/10/02 03:14:14, mmenke wrote:
> On 2015/10/02 03:12:08, mmenke wrote:
> > On 2015/10/02 01:41:39, michaeln wrote:
> > > On 2015/10/02 00:30:04, mmenke wrote:
> > > > On 2015/10/02 00:22:45, michaeln wrote:
> > > > > On 2015/10/01 22:01:45, mmenke wrote:
> > > > > > So in the no-replace case, we pass the entire response body to the
> > calling
> > > > > > URLRequest, and then tell it the request failed?  Does the service
> > worker
> > > > > > restart the request at that point, or claim it succeeded, or what?
> > > > > 
> > > > > The worker script doesn't load or execute in that case. Everything is
> told
> > > to
> > > > > "nevermind" and c++ objs (ServiceWorkerVersion +
EmbeddedWorkerInstance)
> > are
> > > > > deleted. The new instance only needs to run if the script has changed
> and
> > > the
> > > > > installation sequence has to happen.
> > > > 
> > > > Ohh...right... The name of this class was confusing me.  This isn't
> writing
> > to
> > > a
> > > > service worker's cache, but writing a service worker script to the
special
> > > cache
> > > > for service worker script.
> > > 
> > > The class name is odd, it says 'write' but the renderer also 'reads' the
> > > response involved. A better name might be
> > > ServiceWorkerInstallationURLRequestJob?
> > 
> > I think that's much clearer.
> > 
> > > > The fact that this is structured as a URLRequestJob seems unintuitive to
> me,
> > > > though I suppose that lets you reuse the infrastructure to read from
> > > URLRequest
> > > > and stream the result to another process.
> > > 
> > > Maybe the name makes it unintuitive? This is how the serviceworker scripts
> are
> > > delivered to renderers on initial install, so of course it uses jobs, just
> > like
> > > all other cases of loading a worker in a renderer.
> > >
> > > That blinkloader + weburlloader + resource dispatcher host + url request
job
> > > pipeline works well for streaming data from here to there incrementally
and
> > > asyncly with flow control stuff built in. And that loading pipeline is
used
> > for
> > > all other cases of starting a worker (shared, dedicated, currently
installed
> > sw,
> > > retrieval from the net, retreival from the httpcache, retrieval from the
> > > appcache, retrieval from a cachestorage cache, etc).
> > > 
> > > There's a lot of not-so-easy-to-replicate-code reuse by virtue of
leveraging
> > the
> > > urlrequestjob hook. All manner of data is loaded via urlrequest jobs:
> > > chrome:settings, chrome-extensions, blobs, filesystem-files,
net-internals,
> > > devtools, etc.
> > 
> > 
> > The thing that's weird here is a URLRequestJob normally handles a request
for
> a
> > URL.  The renderer says "Give me foo://yada/yada/yada", and then the RDH
> > requests the resource at that location, which net/ then provides to it. 
Here,
> > the other process is saying "I want this resource, and I want you to inject
a
> > URLRequestJob into the network stack of this special type to provide the
> > response for specific request."  So you really aren't using any of net's
> > infrastructure here.  Even the assumption that all requests of the same
scheme
> > are handled in the same way, with the same ProtocolHandler, is violated.
> > 
> > I'm sure there are many reasons we can't just use standard HTTP caching
> > semantics here, unfortunately, though in an ideal world, that's how I feel
> this
> > sort of thing would work (With an additional small table of the service
> workers
> > we know about, of course).
> 
> I don't mean to say that this is the wrong design, just that it's something
that
> makes ServiceWorker (And AppCache's) use of the network stack a bit unusual. 
> There are also magic blob URLs that work the same way, I believe (Not sure if
> all blob URLs work similarly, but some do).

Acknowledged.

https://codereview.chromium.org/1375293004/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:441:
NotifyDone(real_status);
On 2015/10/02 14:10:44, mmenke wrote:
> On 2015/10/02 03:14:14, mmenke wrote:
> > On 2015/10/02 03:12:08, mmenke wrote:
> > > On 2015/10/02 01:41:39, michaeln wrote:
> > > > On 2015/10/02 00:30:04, mmenke wrote:
> > > > > On 2015/10/02 00:22:45, michaeln wrote:
> > > > > > On 2015/10/01 22:01:45, mmenke wrote:
> > > > > > > So in the no-replace case, we pass the entire response body to the
> > > calling
> > > > > > > URLRequest, and then tell it the request failed?  Does the service
> > > worker
> > > > > > > restart the request at that point, or claim it succeeded, or what?
> > > > > > 
> > > > > > The worker script doesn't load or execute in that case. Everything
is
> > told
> > > > to
> > > > > > "nevermind" and c++ objs (ServiceWorkerVersion +
> EmbeddedWorkerInstance)
> > > are
> > > > > > deleted. The new instance only needs to run if the script has
changed
> > and
> > > > the
> > > > > > installation sequence has to happen.
> > > > > 
> > > > > Ohh...right... The name of this class was confusing me.  This isn't
> > writing
> > > to
> > > > a
> > > > > service worker's cache, but writing a service worker script to the
> special
> > > > cache
> > > > > for service worker script.
> > > > 
> > > > The class name is odd, it says 'write' but the renderer also 'reads' the
> > > > response involved. A better name might be
> > > > ServiceWorkerInstallationURLRequestJob?
> > > 
> > > I think that's much clearer.
> > > 
> > > > > The fact that this is structured as a URLRequestJob seems unintuitive
to
> > me,
> > > > > though I suppose that lets you reuse the infrastructure to read from
> > > > URLRequest
> > > > > and stream the result to another process.
> > > > 
> > > > Maybe the name makes it unintuitive? This is how the serviceworker
scripts
> > are
> > > > delivered to renderers on initial install, so of course it uses jobs,
just
> > > like
> > > > all other cases of loading a worker in a renderer.
> > > >
> > > > That blinkloader + weburlloader + resource dispatcher host + url request
> job
> > > > pipeline works well for streaming data from here to there incrementally
> and
> > > > asyncly with flow control stuff built in. And that loading pipeline is
> used
> > > for
> > > > all other cases of starting a worker (shared, dedicated, currently
> installed
> > > sw,
> > > > retrieval from the net, retreival from the httpcache, retrieval from the
> > > > appcache, retrieval from a cachestorage cache, etc).
> > > > 
> > > > There's a lot of not-so-easy-to-replicate-code reuse by virtue of
> leveraging
> > > the
> > > > urlrequestjob hook. All manner of data is loaded via urlrequest jobs:
> > > > chrome:settings, chrome-extensions, blobs, filesystem-files,
> net-internals,
> > > > devtools, etc.
> > > 
> > > 
> > > The thing that's weird here is a URLRequestJob normally handles a request
> for
> > a
> > > URL.  The renderer says "Give me foo://yada/yada/yada", and then the RDH
> > > requests the resource at that location, which net/ then provides to it. 
> Here,
> > > the other process is saying "I want this resource, and I want you to
inject
> a
> > > URLRequestJob into the network stack of this special type to provide the
> > > response for specific request."  So you really aren't using any of net's
> > > infrastructure here.  Even the assumption that all requests of the same
> scheme
> > > are handled in the same way, with the same ProtocolHandler, is violated.
> > > 
> > > I'm sure there are many reasons we can't just use standard HTTP caching
> > > semantics here, unfortunately, though in an ideal world, that's how I feel
> > this
> > > sort of thing would work (With an additional small table of the service
> > workers
> > > we know about, of course).
> > 
> > I don't mean to say that this is the wrong design, just that it's something
> that
> > makes ServiceWorker (And AppCache's) use of the network stack a bit unusual.

> > There are also magic blob URLs that work the same way, I believe (Not sure
if
> > all blob URLs work similarly, but some do).
> 
> It's generally the goal of a URLRequestJob to fetch a resource using the
> underlying protocol.
> 
> The purpose of this job is to fetch a resource (Wrapping the real URLRequest),
> and then provide bonus semantics, because the consumer injected them into the
> network stack.  The fact that this is a URLRequest is just because you want a
> way to send the result of the bonus logic to the process running a service
> worker.
> 
> If instead, you used a normal URLRequest, with the normal cache, and compared
it
> against the previously loaded script in the SW process, everything would still
> work, and make a lot more sense, from a layering standpoint.  I think the only
> problem with doing that is you want extra caching rules.

Acknowledged.

[mmenke, 2015-10-02 16:10:59]

LGTM, but do wait for michaeln to sign off as well...This is sufficiently weird
that someone more familiar with the code should sign off on it.

[michaeln, 2015-10-02 23:53:50]

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

[michaeln, 2015-10-02 23:53:50]

michaeln@chromium.org changed required reviewers:
+ falken@chromium.org

[michaeln, 2015-10-02 23:54:19]

falken really would be the best reviewer for this, he was the primary reviewer
for the original cl (and the original implementer of the incremental compare and
write behavior)

i just noticed the question about what happens when the scripts are found to be
identical and provided an answer to that. a code comment at that point were
noerr is transformed into error would probably be a good idea.

[falken, 2015-10-05 07:03:30]

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

https://codereview.chromium.org/1375293004/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:438:
NotifyFinishedCaching(reported_status, reported_status_message);
This doesn't look right... in the compare equals case |reported_status| is
FAILED here so NotifyFinishedCaching wouldn't set status to
SERVICE_WORKER_ERROR_EXISTS as expected.

[Elly Fong-Jones, 2015-10-05 13:50:28]

ptal?

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

https://codereview.chromium.org/1375293004/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:438:
NotifyFinishedCaching(reported_status, reported_status_message);
On 2015/10/05 07:03:30, falken wrote:
> This doesn't look right... in the compare equals case |reported_status| is
> FAILED here so NotifyFinishedCaching wouldn't set status to
> SERVICE_WORKER_ERROR_EXISTS as expected.

Done.

[falken, 2015-10-05 13:55:46]

LGTM

[Elly Fong-Jones, 2015-10-05 13:58:27]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2015-10-05 13:58:28]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1375293004/#ps60001
(title: "NotifyFinishedCaching with the real status")

[commit-bot: I haz the power, 2015-10-05 13:58:35]

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

[commit-bot: I haz the power, 2015-10-05 14:07:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-05 14:07:03]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elly Fong-Jones, 2015-10-06 13:31:13]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2015-10-06 13:31:53]

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

[commit-bot: I haz the power, 2015-10-06 13:50:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-06 13:51:41]

Patchset 4 (id:??) landed as
https://crrev.com/7320022d75696c0f0b4ea02ed801fbbec4783658
Cr-Commit-Position: refs/heads/master@{#352593}

[Elly Fong-Jones, 2015-10-06 16:37:52]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1388123002/ by ellyjones@chromium.org.

The reason for reverting is: Broke this blink bot:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/55132.