Add a 'Service-Worker: script' header to SW script downloads

Add and test the 'Service-Worker: script' header to script downloads.

Also fix a bug in which the cache job wasn't being used in tests because the state was set to "INSTALL" before starting the SW.

BUG=389124, 389561
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280553

[jkarlin, 2014-06-26 18:04:30]

michaeln: Thanks!

[jkarlin, 2014-06-26 18:13:00]

Whoops, forgot to actually add you.

michaeln: Please review all, thanks!

[gavinp, 2014-06-26 18:15:24]

https://codereview.chromium.org/332913003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/332913003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:164:
request.headers.find("Service-Worker");
Don't we normally write ServiceWorker as one word?

[gavinp, 2014-06-26 18:16:59]

https://codereview.chromium.org/332913003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/332913003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:164:
request.headers.find("Service-Worker");
On 2014/06/26 18:15:24, gavinp wrote:
> Don't we normally write ServiceWorker as one word?

Looking over the standard, I think I'm wrong; it's two words, so service-worker
is the right name for the header.

[michaeln, 2014-06-26 19:57:56]

lgtm

To ensure we never pick the resource up from the browser cache, in addition to
setting the header, i think we may need to set the BYPASS_CACHE flag (or some
set of flags) in the URLRequest actually given to the http stack. If we want to
apply the custom-header and load flags on that URLRequest in the same vicinity
of one another, ServiceWorkerWriteToCacheJob::InitNetRequest might be a good
place for it.

[jkarlin, 2014-06-26 23:48:03]

On 2014/06/26 19:57:56, michaeln wrote:
> lgtm
> 
> To ensure we never pick the resource up from the browser cache, in addition to
> setting the header, i think we may need to set the BYPASS_CACHE flag (or some
> set of flags) in the URLRequest actually given to the http stack. If we want
to
> apply the custom-header and load flags on that URLRequest in the same vicinity
> of one another, ServiceWorkerWriteToCacheJob::InitNetRequest might be a good
> place for it.

Good point, we should set that flag too.  I'll make a separate CL for it. That
(like the header) can be set in Blink by setting the ResourceRequest's cache
policy to ReloadIgnoringCacheData.

Somehow it seems cleaner to me for Blink to set its own headers and flags than
for the browser to manipulate the request on the way out the door.

[michaeln1, 2014-06-27 00:13:59]

> Good point, we should set that flag too.  I'll make a separate CL for it. That
> (like the header) can be set in Blink by setting the ResourceRequest's cache
> policy to ReloadIgnoringCacheData.
> 
> Somehow it seems cleaner to me for Blink to set its own headers and flags than
> for the browser to manipulate the request on the way out the door.

The browser makes a new url request, you'll have to modify InitNetRequest to
propagate the flags. Many of the requests constructed by blink here, will never
actually
be handled by the network stack.

To me, blink is very far removed from the network layer so it seems natural to
manipulate these things in the browser process closer to the where the network
layer is actually invoked. The sw specific code in blink is largely about
mechanical plumbing: v8bindings, webkitapi, promisecallback'ification. The bulk
of the logic is in chrome and running in the browser process.

[dominicc (has gone to gerrit), 2014-06-27 07:12:09]

On 2014/06/27 00:13:59, michaeln1 wrote:
> > Good point, we should set that flag too.  I'll make a separate CL for it.
That
> > (like the header) can be set in Blink by setting the ResourceRequest's cache
> > policy to ReloadIgnoringCacheData.

What are the implications of that? I think we want to use the browser cache but
vary by this header.

> To me, blink is very far removed from the network layer so it seems natural to
> manipulate these things in the browser process closer to the where the network
> layer is actually invoked.

This may be true.

> The sw specific code in blink is largely about
> mechanical plumbing: v8bindings, webkitapi, promisecallback'ification. The
bulk
> of the logic is in chrome and running in the browser process.

This is wrong; Blink is not just plumbing to V8. There's a set of things it is
responsible for and treating it as a thunking layer to V8 will slow things (the
software, our implementation effort) down.

[jkarlin, 2014-06-27 15:05:35]

PTAL michaeln.  I've moved the header logic into browser instead of Blink. 
Fixed a bug along the way.

[michaeln, 2014-06-27 19:50:41]

this lgtm (even better from my pov)

https://codereview.chromium.org/332913003/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/332913003/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:242:
SetStatus(INSTALLING);
what went wrong with this line here instead of there?

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

https://codereview.chromium.org/332913003/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:110:
net_request_->SetExtraRequestHeaders(headers);
do you know why job header setting is circuitous like this?

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

https://codereview.chromium.org/332913003/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.h:40: int64
response_id);
why change the indentation style when adding a new arg?

[michaeln, 2014-06-27 20:05:59]

On 2014/06/27 07:12:09, dominicc wrote:
> On 2014/06/27 00:13:59, michaeln1 wrote:
> > > Good point, we should set that flag too.  I'll make a separate CL for it.
> That
> > > (like the header) can be set in Blink by setting the ResourceRequest's
cache
> > > policy to ReloadIgnoringCacheData.
> 
> What are the implications of that? I think we want to use the browser cache
but
> vary by this header.

Right! That's a really good point, if an originserver cares to distinguish
between requests with and without this special header, it can list it in the
Vary header of its response, so i guess no action required on our part :)

[michaeln, 2014-06-27 20:14:04]

> Also fix a bug in which the cache job wasn't being used in tests because the
state was set to "INSTALL" before starting the SW.

I see. The production code will always have started the worker once prior to
getting to the INSTALL phase. Which test was it that had trouble with this?

[jkarlin, 2014-06-28 00:26:13]

https://codereview.chromium.org/332913003/diff/90001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/332913003/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:242:
SetStatus(INSTALLING);
On 2014/06/27 19:50:41, michaeln wrote:
> what went wrong with this line here instead of there?

Looks like you saw the reason in your follow-up comment.  But it comes down to
setting 'INSTALLING' before starting the worker causes a problem because the
caching job won't run while start because it checks for the state to be 'NEW'.

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

https://codereview.chromium.org/332913003/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:110:
net_request_->SetExtraRequestHeaders(headers);
On 2014/06/27 19:50:41, michaeln wrote:
> do you know why job header setting is circuitous like this?

Looking at how other jobs use SetExtraRequestHeaders() it looks like they use it
to do some header validation.  Mostly to make sure that the range headers are
okay.  They don't actually set the request's headers here.

I've removed the net_request_->SetExtraRequestHeaders(headers) line here and put
it back in InetNetRequest.

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

https://codereview.chromium.org/332913003/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.h:40: int64
response_id);
On 2014/06/27 19:50:41, michaeln wrote:
> why change the indentation style when adding a new arg?

My bad.  Done.

[jkarlin, 2014-06-28 00:29:26]

On 2014/06/27 20:14:04, michaeln wrote:
> > Also fix a bug in which the cache job wasn't being used in tests because the
> state was set to "INSTALL" before starting the SW.
> 
> I see. The production code will always have started the worker once prior to
> getting to the INSTALL phase. Which test was it that had trouble with this?

My new test requires this change. We don't seem to have test coverage for the
cache jobs yet.

[jkarlin, 2014-06-29 14:09:37]

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

https://codereview.chromium.org/332913003/diff/90001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:110:
net_request_->SetExtraRequestHeaders(headers);
On 2014/06/28 00:26:12, jkarlin wrote:
> On 2014/06/27 19:50:41, michaeln wrote:
> > do you know why job header setting is circuitous like this?
> 
> Looking at how other jobs use SetExtraRequestHeaders() it looks like they use
it
> to do some header validation.  Mostly to make sure that the range headers are
> okay.  They don't actually set the request's headers here.
> 
> I've removed the net_request_->SetExtraRequestHeaders(headers) line here and
put
> it back in InetNetRequest.

URLRequests create jobs and inform the jobs what the final extra headers are
before starting.  So the ::SetExtraRequestHeaders function is the appropriate
place to set the net_request_ headers.  I've undone the move I described in the
previous comment.

[jkarlin, 2014-06-29 19:28:15]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-06-29 19:28:54]

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

[commit-bot: I haz the power, 2014-06-29 20:37:19]

Change committed as 280553