Pages controlled by ServiceWorkers should not participate in AppCaching

Pages controlled by ServiceWorkers should not participate in AppCaching. The manifest attribute of pages in the scope of a ServiceWorker are ignored, and existing AppCaches are ignored for navigations into a ServiceWorker's scope.

BUG=410665
Committed: https://crrev.com/22390646e70aba2bb8e90688f9a6351f9571bca3
Cr-Commit-Position: refs/heads/master@{#300406}

[michaeln, 2014-10-03 00:12:15]

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

[michaeln, 2014-10-03 00:12:28]

ptal

[michaeln, 2014-10-03 01:09:46]

https://codereview.chromium.org/625433002/diff/40001/content/browser/appcache...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/625433002/diff/40001/content/browser/appcache...
content/browser/appcache/appcache_request_handler.cc:244: // prior to the
AppCache handler.
bah... the ordering is wrong, appcache gets called first :(

[falken, 2014-10-03 03:36:13]

looks good, I guess it needs another look when the ordering problem is fixed?
Are tests possible?

[michaeln, 2014-10-09 20:53:42]

On 2014/10/03 03:36:13, falken wrote:
> looks good, I guess it needs another look when the ordering problem is fixed?
> Are tests possible?

I think i need to do fiddle with the per-urlrequestcontext interceptors that the
content lib hands to chrome when setting up the request contexts.

Appcache uses a global kind of interceptor (which supports hooking errors and
redirects in addition to the start of a request). The global one take precedence
over the more local per-urlrquest flavor of intercepts. The content lib probably
needs to setup the per-rerequest interceptor with something that provides the
order we desire for hooking the start of a request. And the global hook should
not be used by appcache for that one.

[michaeln, 2014-10-10 21:55:40]

https://codereview.chromium.org/625433002/diff/290001/content/browser/storage...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/625433002/diff/290001/content/browser/storage...
content/browser/storage_partition_impl_map.cc:441:
AppCacheInterceptor::CreateStartInterceptor().release());
here's how we get the ordering we want

[michaeln, 2014-10-14 00:51:05]

ping!

[falken, 2014-10-14 01:24:50]

I think this looks good. Does it make sense to add a test that SW wins over
AppCache?

https://codereview.chromium.org/625433002/diff/290001/content/browser/appcach...
File content/browser/appcache/appcache_interceptor.h (right):

https://codereview.chromium.org/625433002/diff/290001/content/browser/appcach...
content/browser/appcache/appcache_interceptor.h:55: // A seperate
URLRequestInterceptor derivative is used to hijack handling
nittttty nit: separate

https://codereview.chromium.org/625433002/diff/290001/content/browser/appcach...
content/browser/appcache/appcache_interceptor.h:56: // at the very start of
request processing. The seperate handler allows the
separate

https://codereview.chromium.org/625433002/diff/290001/content/browser/service...
File content/browser/service_worker/service_worker_request_handler.h (right):

https://codereview.chromium.org/625433002/diff/290001/content/browser/service...
content/browser/service_worker/service_worker_request_handler.h:72: // method
run to completion for this request. The AppCache handler uses
nit: runs

https://codereview.chromium.org/625433002/diff/290001/content/browser/storage...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/625433002/diff/290001/content/browser/storage...
content/browser/storage_partition_impl_map.cc:441:
AppCacheInterceptor::CreateStartInterceptor().release());
On 2014/10/10 21:55:40, michaeln wrote:
> here's how we get the ordering we want

It took me a while to see how this works. Before this CL, AppCache was called
before SW, so just adding the interceptor here (and not removing something
elsewhere) wouldn't seem to fix it. Now I see... the original
AppCacheInterceptor::MaybeIntercept is still called first but now does nothing
and the StartInterceptor is the one that really chooses whether AppCache kicks
in?

[michaeln, 2014-10-14 01:52:42]

> Does it make sense to add a test that SW wins over
AppCache?

Yes, probably a layout test makes the most sense.

https://codereview.chromium.org/625433002/diff/290001/content/browser/storage...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/625433002/diff/290001/content/browser/storage...
content/browser/storage_partition_impl_map.cc:441:
AppCacheInterceptor::CreateStartInterceptor().release());
On 2014/10/14 01:24:49, falken wrote:
> On 2014/10/10 21:55:40, michaeln wrote:
> > here's how we get the ordering we want
> 
> It took me a while to see how this works. Before this CL, AppCache was called
> before SW, so just adding the interceptor here (and not removing something
> elsewhere) wouldn't seem to fix it. Now I see... the original
> AppCacheInterceptor::MaybeIntercept is still called first but now does nothing
> and the StartInterceptor is the one that really chooses whether AppCache kicks
> in?

Thats right.

[michaeln, 2014-10-16 21:39:01]

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

[michaeln, 2014-10-16 21:39:01]

@jochen for content OWNERS

[jochen (gone - plz use gerrit), 2014-10-17 12:02:26]

storage partition impl lgtm

for the rest I defer to falken

[falken, 2014-10-20 01:25:36]

On 2014/10/17 12:02:26, jochen wrote:
> storage partition impl lgtm
> 
> for the rest I defer to falken

Code looks good, I think we land to land it with a test?

[michaeln, 2014-10-20 23:00:38]

> Code looks good, I think we land to land it with a test?

A layout test can't be commited with these changes, wrong repo. Here's the cl
for the layout test.
https://codereview.chromium.org/671583002/

[falken, 2014-10-21 01:29:20]

On 2014/10/20 23:00:38, michaeln wrote:
> > Code looks good, I think we land to land it with a test?
> 
> A layout test can't be commited with these changes, wrong repo. Here's the cl
> for the layout test.
> https://codereview.chromium.org/671583002/

nice test! lgtm

[michaeln, 2014-10-21 02:14:51]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-10-21 02:17:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625433002/450001

[commit-bot: I haz the power, 2014-10-21 03:09:28]

Committed patchset #9 (id:450001)

[commit-bot: I haz the power, 2014-10-21 03:11:56]

Patchset 9 (id:??) landed as
https://crrev.com/22390646e70aba2bb8e90688f9a6351f9571bca3
Cr-Commit-Position: refs/heads/master@{#300406}