AppCache and StoragePartition'ing

AppCache and StoragePartition'ing

* Get rid of BrowserContext::GetAppCacheService and ResourceContext::GetAppCacheService as they've been replace by accessors on the StoragePartition and WorkerStoragePartition classes.

* Added a BrowsingContext::GetRequestContextForStoragePartition(id) accessor so the constellation of storage context + main request context can initialized properly. Implemented that method in chrome's Profile class in terms of the existing GetRequestContextForIsolatedApp(id) accessor.

* Hold references to the ChromeAppCacheService and ChromeBlobStorageContext inside of ResourceMessageFilter and provide accessors to them. These are for use by the ResourceDispatcherHost singleton which would otherwise not have enough context to get needed references to partitioned things.

* Widen the ResourceDispatcherHostDelegate::RequestBeginning method to also take an AppCacheService* parameter since that value can no longer be retrieved via the ResourceContext. Chrome's impl of this delegate interface needs that value to construct OfflineResourceThrottles.

* Poke at WorkerProcessHost to create ResourceMessageFilters and others to utlize the correct URLRequestContext so the right set of cookies are used in shared workers.

TBR=mihaip,sail,thakis
BUG=85121
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156991

[awong, 2012-09-06 22:29:27]

I agree that it's definitely cleaner in the Worker* classes to have some single
object to pass around with all this state.  But it feels really unforutnate to
extend the StoragePartition object contract to become usable by the IO thread in
general and changing it to a refcounted lifetime.

I had been shying away from creating some sort of StoragePartitionForIO object,
but maybe that's not so bad -- especially if it's just a nested class inside
WorkerHost or WorkerMessageFilter like instance.

We could implement an Equals which compared each of the context pointers which
would be very direct.  The the only place where we have "API explosion" is in
WorkerMessageFilter.

https://chromiumcodereview.appspot.com/10916132/diff/4018/content/browser/sto...
File content/browser/storage_partition_impl.h (right):

https://chromiumcodereview.appspot.com/10916132/diff/4018/content/browser/sto...
content/browser/storage_partition_impl.h:29: virtual appcache::AppCacheService*
GetAppCacheService() OVERRIDE;
Why'd we remove the covariant return type here?

https://chromiumcodereview.appspot.com/10916132/diff/4018/content/browser/sto...
File content/browser/storage_partition_impl_map.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/4018/content/browser/sto...
content/browser/storage_partition_impl_map.cc:77:
static_cast<ChromeAppCacheService*>(
If we keep the covariant return, we don't need this cast.

https://chromiumcodereview.appspot.com/10916132/diff/4018/content/browser/wor...
File content/browser/worker_host/worker_process_host.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/4018/content/browser/wor...
content/browser/worker_host/worker_process_host.cc:270:
static_cast<ChromeAppCacheService*>(
Also unnecessary with covariant return...

[awong, 2012-09-11 20:31:41]

I've stuffed http://codereview.chromium.org/10885044/ onto the CQ.  After that's
submitting am I right in thinking this will reduce down to just the appcache
changes?

[michaeln, 2012-09-11 20:42:21]

yup :)

[michaeln, 2012-09-13 00:21:20]

ptal

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/st...
File content/browser/storage_partition_impl_map.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/st...
content/browser/storage_partition_impl_map.cc:59: // state by providing the NULL
input.  http://crbug.com/85121
NOTE!!!

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/wo...
File content/browser/worker_host/worker_process_host.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/wo...
content/browser/worker_host/worker_process_host.cc:261: // cache associated with
normal web browsing.
The wrong RequestContext here is a blocker i think.

[awong, 2012-09-13 00:45:50]

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/ap...
File content/browser/appcache/chrome_appcache_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/ap...
content/browser/appcache/chrome_appcache_service_unittest.cc:36: 
spurious new line.

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/re...
File content/browser/renderer_host/resource_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/re...
content/browser/renderer_host/resource_message_filter.cc:30: // appcache_service
and blob_storage_context may be NULL in unittests.
|appcache_service| and |blob_storage_context|

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/st...
File content/browser/storage_partition_impl_map.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/st...
content/browser/storage_partition_impl_map.cc:59: // state by providing the NULL
input.  http://crbug.com/85121
On 2012/09/13 00:21:20, michaeln wrote:
> NOTE!!!

Ah hah...yes. This is a problem.

A solution would be add a GetRequestContextForPartitionId() into
BrowserContext(). This would move us closer to getting the the URLRequestContext
into the StoragePartition proper.

The partition_id is exactly equivalent to the installed_app->id(), so in the
ProfileImpl, you could just return
GetRequestContextForIsolatedApp(partition_id);

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/wo...
File content/browser/worker_host/worker_process_host.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/wo...
content/browser/worker_host/worker_process_host.cc:261: // cache associated with
normal web browsing.
On 2012/09/13 00:21:20, michaeln wrote:
> The wrong RequestContext here is a blocker i think.

Ick. It is a blocker to 144549, but not m23 luckily. Erik triaged out workers
from m23.

Can we cite 144549 in this TODO?

[michaeln, 2012-09-13 01:37:20]

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/st...
File content/browser/storage_partition_impl_map.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/st...
content/browser/storage_partition_impl_map.cc:59: // state by providing the NULL
input.  http://crbug.com/85121
Should i be worried about "guest-<appid>" being used as an isolated appid in
ProfileImpl::GetRequestContextForRenderProcess?

[awong, 2012-09-13 01:41:11]

On 2012/09/13 01:37:20, michaeln wrote:
>
https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/st...
> File content/browser/storage_partition_impl_map.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/st...
> content/browser/storage_partition_impl_map.cc:59: // state by providing the
NULL
> input.  http://crbug.com/85121
> Should i be worried about "guest-<appid>" being used as an isolated appid in
> ProfileImpl::GetRequestContextForRenderProcess?

No, can't happen yet.

[awong, 2012-09-13 01:43:14]

errr...actually it can, but I don't think we care right now.

On Wed, Sep 12, 2012 at 6:41 PM, <ajwong@chromium.org> wrote:

> On 2012/09/13 01:37:20, michaeln wrote:
>
> https://chromiumcodereview.**appspot.com/10916132/diff/**
>
10044/content/browser/storage_**partition_impl_map.cc<https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/storage_partition_impl_map.cc>
>
>> File content/browser/storage_**partition_impl_map.cc (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/10916132/diff/**
>
10044/content/browser/storage_**partition_impl_map.cc#**newcode59<https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/storage_partition_impl_map.cc#newcode59>
>
>> content/browser/storage_**partition_impl_map.cc:59: // state by
>> providing the
>>
> NULL
>
>> input.  http://crbug.com/85121
>> Should i be worried about "guest-<appid>" being used as an isolated appid
>> in
>> ProfileImpl::**GetRequestContextForRenderProc**ess?
>>
>
> No, can't happen yet.
>
>
>
https://chromiumcodereview.**appspot.com/10916132/<https://chromiumcodereview...
>

[michaeln, 2012-09-13 02:20:22]

On 2012/09/13 01:43:14, awong wrote:
> No, can't happen yet.
> errr...actually it can, but I don't think we care right now.

ok... i think i have my answer ;)

I'll add the GetResourceContextForPartitionId() accessor like you suggest and i
guess we'll go from there.

[michaeln, 2012-09-13 21:47:21]

ptal, i'll probably have to update some test code too... running tryjobs to find
those things

https://chromiumcodereview.appspot.com/10916132/diff/1052/content/browser/wor...
File content/browser/worker_host/worker_process_host.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/1052/content/browser/wor...
content/browser/worker_host/worker_process_host.cc:259: // TODO(michaeln): This
is hacky but correct. The request context should be
ptal at this part too

[michaeln, 2012-09-13 22:53:08]

try jobs found a couple of derived classes so far

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/ap...
File content/browser/appcache/chrome_appcache_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/ap...
content/browser/appcache/chrome_appcache_service_unittest.cc:36: 
On 2012/09/13 00:45:50, awong wrote:
> spurious new line.

Done.

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/re...
File content/browser/renderer_host/resource_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/10044/content/browser/re...
content/browser/renderer_host/resource_message_filter.cc:30: // appcache_service
and blob_storage_context may be NULL in unittests.
On 2012/09/13 00:45:50, awong wrote:
> |appcache_service| and |blob_storage_context|

Done.

[awong, 2012-09-14 00:09:57]

LGTM w/ nits

Thanks for fixing this!

Now if I can just stop writing e-mail and actually fix FSContext...

https://chromiumcodereview.appspot.com/10916132/diff/1052/content/browser/wor...
File content/browser/worker_host/worker_process_host.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/1052/content/browser/wor...
content/browser/worker_host/worker_process_host.cc:259: // TODO(michaeln): This
is hacky but correct. The request context should be
On 2012/09/13 21:47:22, michaeln wrote:
> ptal at this part too

Cleaver.  I agree this is a good enough hack until we get GetRequestContext()
into StoragePartition.

https://chromiumcodereview.appspot.com/10916132/diff/1058/chrome/browser/prof...
File chrome/browser/profiles/profile.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/1058/chrome/browser/prof...
chrome/browser/profiles/profile.cc:120: net::URLRequestContextGetter*
Profile::GetRequestContextForStoragePartition(
This this be here, or in ProfileImpl?  I'm never certain about these classes.

Also, please add a TODO with my name to collapse this function with
GetRequestContextForRenderProcess().

https://chromiumcodereview.appspot.com/10916132/diff/1058/content/public/brow...
File content/public/browser/browser_context.h (right):

https://chromiumcodereview.appspot.com/10916132/diff/1058/content/public/brow...
content/public/browser/browser_context.h:102: 
Needs a comment explaining this function and likely its relation to
GetRequestContextForRenderProcess()

[michaeln, 2012-09-14 00:24:55]

i put the new method impl in profile.cc to reduce the number of overrides needed
with the same method body, but lets see what a profile OWNER would prefer?

i'm still tracking down derived BrowserContext classes that need to override the
new pure virtual :(

https://chromiumcodereview.appspot.com/10916132/diff/1058/content/browser/wor...
File content/browser/worker_host/worker_process_host.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/1058/content/browser/wor...
content/browser/worker_host/worker_process_host.cc:254: // TODO(michaeln): Put
ChromeBlobStorageContext in StorageParition too.
i'll remove this TODO comment, it's not clear we need to or we should do what it
suggests... but it nags at me that this 'context' is not handled as the others
are (but then again, there's nothing persistent in here)... really this could be
a process wide singleton and that'd work just fine too.

[awong, 2012-09-14 00:27:21]

https://chromiumcodereview.appspot.com/10916132/diff/1058/content/browser/wor...
File content/browser/worker_host/worker_process_host.cc (right):

https://chromiumcodereview.appspot.com/10916132/diff/1058/content/browser/wor...
content/browser/worker_host/worker_process_host.cc:254: // TODO(michaeln): Put
ChromeBlobStorageContext in StorageParition too.
On 2012/09/14 00:24:56, michaeln wrote:
> i'll remove this TODO comment, it's not clear we need to or we should do what
it
> suggests... but it nags at me that this 'context' is not handled as the others
> are (but then again, there's nothing persistent in here)... really this could
be
> a process wide singleton and that'd work just fine too.

Yeah...FileSystemContext is also worrying me, but I think that's more clearly
something you want to partition.

[michaeln, 2012-09-14 02:50:18]

Made some further changes like we chatted about in test_browser_context.cc

[awong, 2012-09-14 03:04:21]

LGTM if nits on previous review addressed.

[awong, 2012-09-14 19:59:34]

http://codereview.chromium.org/10916132/diff/116/content/public/test/test_bro...
File content/public/test/test_browser_context.cc (right):

http://codereview.chromium.org/10916132/diff/116/content/public/test/test_bro...
content/public/test/test_browser_context.cc:21: virtual bool
RunsTasksOnCurrentThread() const OVERRIDE {
nit: indentation.

[michaeln, 2012-09-15 00:30:43]

Fixed the chrome unittest problem like we talked about.

http://codereview.chromium.org/10916132/diff/116/content/public/test/test_bro...
File content/public/test/test_browser_context.cc (right):

http://codereview.chromium.org/10916132/diff/116/content/public/test/test_bro...
content/public/test/test_browser_context.cc:21: virtual bool
RunsTasksOnCurrentThread() const OVERRIDE {
On 2012/09/14 19:59:34, awong wrote:
> nit: indentation.

Done.

[awong, 2012-09-15 00:33:29]

Still LGTM.

2 nits.

http://codereview.chromium.org/10916132/diff/14064/content/browser/appcache/c...
File content/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/10916132/diff/14064/content/browser/appcache/c...
content/browser/appcache/chrome_appcache_service.cc:34: // The
|request_context_getter| can be NULL in some unit tests.
Can you file a bug against me and add a TODO to remove this requirement once we
bring sanity to the malware tests?

http://codereview.chromium.org/10916132/diff/14064/content/public/test/test_b...
File content/public/test/test_browser_context.cc (right):

http://codereview.chromium.org/10916132/diff/14064/content/public/test/test_b...
content/public/test/test_browser_context.cc:38: }
IIRC, this needs a private destructor or clang will die on you.

[awong, 2012-09-15 00:54:21]

+ creis@ for OWNERS.

[Charlie Reis, 2012-09-15 01:06:24]

LGTM with one nit.  Thanks!

http://codereview.chromium.org/10916132/diff/14064/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/10916132/diff/14064/chrome/browser/profiles/pr...
chrome/browser/profiles/profile.cc:120: net::URLRequestContextGetter*
Profile::GetRequestContextForStoragePartition(
Why not just rename GetRequestContextForIsolatedApp to
GetRequestContextForStoragePartition?

[michaeln, 2012-09-15 01:50:56]

http://codereview.chromium.org/10916132/diff/14064/content/browser/appcache/c...
File content/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/10916132/diff/14064/content/browser/appcache/c...
content/browser/appcache/chrome_appcache_service.cc:34: // The
|request_context_getter| can be NULL in some unit tests.
On 2012/09/15 00:33:29, awong wrote:
> Can you file a bug against me and add a TODO to remove this requirement once
we
> bring sanity to the malware tests?

Done.

http://codereview.chromium.org/10916132/diff/14064/content/public/test/test_b...
File content/public/test/test_browser_context.cc (right):

http://codereview.chromium.org/10916132/diff/14064/content/public/test/test_b...
content/public/test/test_browser_context.cc:38: }
On 2012/09/15 00:33:29, awong wrote:
> IIRC, this needs a private destructor or clang will die on you.

Done.

[commit-bot: I haz the power, 2012-09-15 02:40:45]

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

[commit-bot: I haz the power, 2012-09-15 03:09:16]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-15 03:21:34]

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

[commit-bot: I haz the power, 2012-09-15 05:12:37]

Change committed as 156991