Make FileSystemContext respect StoragePartitions. filesystem:// urls will be properly isolated

continuing from http://http://codereview.chromium.org/10823241

This intentionally doesn't change the ChromeOS behavior at all.  They all still use the default FileSystemContext.

This code also exposes the normal and media URLRequestGetters via the StoragePartition, and cleans up a bit of code that was accessing the URLRequestGetter in odd ways.  Also, it makes Workers correctly use the Media Cache for Media fetches.

TBR=benjhyden,sky,davemoore,piman,mkwst,kalman
BUG=85121
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157284

[awong, 2012-09-14 06:37:06]

Hi Michael,

This builds on your appcache CL (it's rebased on top of it).

Hopefully the unittests pass.

-Albert

[michaeln, 2012-09-15 01:48:32]

only one real question about default vs via siteIntance in the chromeos stuff

http://codereview.chromium.org/10909182/diff/6005/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/10909182/diff/6005/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2499:
render_view_host()->GetSiteInstance())->
In some places in this module, we're looking up the partition based on the site
instance and in others referring to the default partition w/o question. Will the
indirection thru the SiteInstance ever result in a partition (and FSContext)
that is not the default?

http://codereview.chromium.org/10909182/diff/6005/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/10909182/diff/6005/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:565:
storage_partition_impl_->GetPath()) :
line length

http://codereview.chromium.org/10909182/diff/6005/content/browser/storage_par...
File content/browser/storage_partition_impl.cc (right):

http://codereview.chromium.org/10909182/diff/6005/content/browser/storage_par...
content/browser/storage_partition_impl.cc:52: 
stray newline

http://codereview.chromium.org/10909182/diff/6005/content/browser/storage_par...
File content/browser/storage_partition_impl_map.cc (right):

http://codereview.chromium.org/10909182/diff/6005/content/browser/storage_par...
content/browser/storage_partition_impl_map.cc:140:
scoped_refptr<net::URLRequestContextGetter> context_getter,
this first argument could be a raw ptr like the others

http://codereview.chromium.org/10909182/diff/6005/content/browser/storage_par...
content/browser/storage_partition_impl_map.cc:210: partition_id.empty() ? 
browser_context_->GetMediaRequestContext() :
nit: using a more consistent wrapping style for the calls to
SetURLRequestContext and SetMediaURLRequestContext might be nice

http://codereview.chromium.org/10909182/diff/6005/content/browser/worker_host...
File content/browser/worker_host/worker_storage_partition.cc (right):

http://codereview.chromium.org/10909182/diff/6005/content/browser/worker_host...
content/browser/worker_host/worker_storage_partition.cc:15: // TODO(ajwong):
Handle MediaUrlRequestContext?
Is this TODO intentional, seems like its handled in this struct?

[awong, 2012-09-15 02:05:01]

Thanks for the review.  Comments addressed.

FYI, working on renaming the Profile accessor for storage partition.

https://chromiumcodereview.appspot.com/10909182/diff/6005/chrome/browser/chro...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://chromiumcodereview.appspot.com/10909182/diff/6005/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:2499:
render_view_host()->GetSiteInstance())->
On 2012/09/15 01:48:33, michaeln wrote:
> In some places in this module, we're looking up the partition based on the
site
> instance and in others referring to the default partition w/o question. Will
the
> indirection thru the SiteInstance ever result in a partition (and FSContext)
> that is not the default?

Good catch. I'm not sure. I'm reverting them all to GetDefaultStoragePartition()
for now.

https://chromiumcodereview.appspot.com/10909182/diff/6005/content/browser/ren...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10909182/diff/6005/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:565:
storage_partition_impl_->GetPath()) :
On 2012/09/15 01:48:33, michaeln wrote:
> line length

Done.

https://chromiumcodereview.appspot.com/10909182/diff/6005/content/browser/sto...
File content/browser/storage_partition_impl.cc (right):

https://chromiumcodereview.appspot.com/10909182/diff/6005/content/browser/sto...
content/browser/storage_partition_impl.cc:52: 
On 2012/09/15 01:48:33, michaeln wrote:
> stray newline

Done.

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

https://chromiumcodereview.appspot.com/10909182/diff/6005/content/browser/sto...
content/browser/storage_partition_impl_map.cc:140:
scoped_refptr<net::URLRequestContextGetter> context_getter,
On 2012/09/15 01:48:33, michaeln wrote:
> this first argument could be a raw ptr like the others

Done.

https://chromiumcodereview.appspot.com/10909182/diff/6005/content/browser/sto...
content/browser/storage_partition_impl_map.cc:210: partition_id.empty() ? 
browser_context_->GetMediaRequestContext() :
On 2012/09/15 01:48:33, michaeln wrote:
> nit: using a more consistent wrapping style for the calls to
> SetURLRequestContext and SetMediaURLRequestContext might be nice

Done.

https://chromiumcodereview.appspot.com/10909182/diff/6005/content/browser/wor...
File content/browser/worker_host/worker_storage_partition.cc (right):

https://chromiumcodereview.appspot.com/10909182/diff/6005/content/browser/wor...
content/browser/worker_host/worker_storage_partition.cc:15: // TODO(ajwong):
Handle MediaUrlRequestContext?
On 2012/09/15 01:48:33, michaeln wrote:
> Is this TODO intentional, seems like its handled in this struct?

It was stale. removed.

[michaeln, 2012-09-15 05:54:29]

lgtm

[awong, 2012-09-17 18:13:33]

+creis for owners.

[awong, 2012-09-17 19:24:59]

Okay, merged up and ready for review.  FYI, there actually are successful try
runs, but git-cl is borken so they are only showing up in my inbox.

[Charlie Reis, 2012-09-17 22:51:12]

Looks good.  A few minor questions, but nothing blocking.

http://codereview.chromium.org/10909182/diff/2044/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/10909182/diff/2044/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:97: virtual net::URLRequestContextGetter*
GetRequestContextForStoragePartition(
Why isn't this on BrowserContext like the others above?

http://codereview.chromium.org/10909182/diff/2044/content/browser/storage_par...
File content/browser/storage_partition_impl.h (right):

http://codereview.chromium.org/10909182/diff/2044/content/browser/storage_par...
content/browser/storage_partition_impl.h:55: // initialization, if we call try
to retrieve the URLRequestContextGetter()
typo: call try

http://codereview.chromium.org/10909182/diff/2044/content/browser/storage_par...
File content/browser/storage_partition_impl_map.cc (right):

http://codereview.chromium.org/10909182/diff/2044/content/browser/storage_par...
content/browser/storage_partition_impl_map.cc:43: class BlobProtocolHandler :
public webkit_blob::BlobProtocolHandler {
Isn't it a little odd to have this much initialization code for protocol
handlers and URLRequestContexts in a map class?  Not sure I have a better
suggestion, but I wouldn't have thought to find it here.

http://codereview.chromium.org/10909182/diff/6101/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_file_system_helper.cc (right):

http://codereview.chromium.org/10909182/diff/6101/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_file_system_helper.cc:225: filesystem
= file_system_info_.begin();
Ok, but why?  (Was this another regexp victim?  Maybe we should avoid the code
churn.)

[awong, 2012-09-17 23:10:19]

PTAL

http://codereview.chromium.org/10909182/diff/2044/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/10909182/diff/2044/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:97: virtual net::URLRequestContextGetter*
GetRequestContextForStoragePartition(
On 2012/09/17 22:51:12, creis wrote:
> Why isn't this on BrowserContext like the others above?

Done.

http://codereview.chromium.org/10909182/diff/2044/content/browser/storage_par...
File content/browser/storage_partition_impl.h (right):

http://codereview.chromium.org/10909182/diff/2044/content/browser/storage_par...
content/browser/storage_partition_impl.h:55: // initialization, if we call try
to retrieve the URLRequestContextGetter()
On 2012/09/17 22:51:12, creis wrote:
> typo: call try

Done.

http://codereview.chromium.org/10909182/diff/2044/content/browser/storage_par...
File content/browser/storage_partition_impl_map.cc (right):

http://codereview.chromium.org/10909182/diff/2044/content/browser/storage_par...
content/browser/storage_partition_impl_map.cc:43: class BlobProtocolHandler :
public webkit_blob::BlobProtocolHandler {
On 2012/09/17 22:51:12, creis wrote:
> Isn't it a little odd to have this much initialization code for protocol
> handlers and URLRequestContexts in a map class?  Not sure I have a better
> suggestion, but I wouldn't have thought to find it here.

Yes, it doesn't belong here. However, until I can clean up the URLRequestContext
mess, it needs to be here to support the tricky ordering requirements of
PostCreateInitialization.  Added a note to the larger TODO in
StoragePartitionImpl to clean this up.

http://codereview.chromium.org/10909182/diff/6101/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_file_system_helper.cc (right):

http://codereview.chromium.org/10909182/diff/6101/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_file_system_helper.cc:225: filesystem
= file_system_info_.begin();
On 2012/09/17 22:51:12, creis wrote:
> Ok, but why?  (Was this another regexp victim?  Maybe we should avoid the code
> churn.)

reverted.

[Charlie Reis, 2012-09-17 23:13:17]

LGTM!

[awong, 2012-09-18 03:03:16]

TBRing places that basically just had a simple API change.

[Mike West, 2012-09-18 05:37:25]

On 2012/09/18 03:03:16, awong wrote:
> TBRing places that basically just had a simple API change.

browsing_data LGTM.

[not at google - send to devlin, 2012-09-18 05:41:24]

Putting me both in TBR and the reviewers list is confusing, I'm not sure whether
you want me to review or not.

[benjhayden, 2012-09-18 16:11:03]

downloads_api_unittest.cc and download_manager_impl_unittest.cc LGTM

[piman, 2012-09-18 17:23:42]

LGTM for content/