Initial refactor to get profiles to propagate storage partition details.

For storage partitions to work properly, we need to know whether a storage partition is in-memory only and what directory it is supposed to live under. This information is currently not available on the IO thread, where we create the cookie store and HTTP cache.

This CL is an the first step, aiming to to get profiles to propagate storage partition details from GetResourceContext* methods on the UI thread to the IO thread.


BUG=145500, 138296
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165057

[nasko, 2012-10-18 18:33:45]

Hey Albert,
Can you review this change? It is the Profile side refactoring to get
StoragePartition to work.
Thanks,
Nasko

[awong, 2012-10-18 22:36:15]

Overall looks on the right track.

My main thought is that we can use partition->GetRequestContext() in a lot of
places and then possibly even avoid exposing the InMemory() accessor.

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/extensions/...
File chrome/browser/extensions/data_deleter.cc (right):

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/extensions/...
chrome/browser/extensions/data_deleter.cc:105:
profile->GetRequestContextForStoragePartition(
Why isn't this just the RequestContext() out of storage_partiton?

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/pr...
chrome/browser/profiles/profile.h:262: const bool& in_memory) = 0;
don't pass primitives types by const-ref. That's just wasteful :)

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:768: content::StoragePartition*
storage_partition = rph->GetStoragePartition();
If we're already reading out of the StoragePartition, can this just become
return storage_partition->GetRequestContext()?

I think this is safe because the true entry-point where StoragePartition calls
back into profile is actually GetRequestContextForStoragePartition(), no?

If that is true, and we make this change, go back to the comments in profile.h
and note which of the GetRequestContextXXX APIs just dispatch back into
StoragePartition (I think it'll only be this one).

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:801: content::RenderProcessHost* rph =
content::RenderProcessHost::FromID(
Same comment as for GetRequestContextForRenderProcess()

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:813:
storage_partition->IsInMemoryOnly());
Wouldn't it be cleaner to just keep this struct inside io_data and make the
various GetXXXGetter() functions take 2 params?

That way we keep StoragePartitionDetails scoped to only the various io_data
classes.

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_par...
File chrome/browser/storage_partition_details.h (right):

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_par...
chrome/browser/storage_partition_details.h:15: struct StoragePartitionDetails {
nit: Can we call this StoragePartitionDescriptor or something that indicates it
is an identiifer more than "Details"?

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_par...
chrome/browser/storage_partition_details.h:19: StoragePartitionDetails(const
FilePath& partition_path,
The constructor should come first in declaration ordering.

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_par...
chrome/browser/storage_partition_details.h:28: bool operator()(const
StoragePartitionDetails& left,
lhs and rhs seem to be more common than "left" and "right"

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_par...
chrome/browser/storage_partition_details.h:32: if (left.in_memory !=
right.in_memory)
This isn't a valid Strict Weak Ordering which stl algorithms require.

SPD a("foo", false);
SPD b("foo", true);

here a < b and b < a.

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_par...
chrome/browser/storage_partition_details.h:42: const StoragePartitionDetails&
right) const {
Simplify to

return left.path == right.path && left.in_memory == right.in_memory.

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_par...
chrome/browser/storage_partition_details.h:69: return input.in_memory ? value :
value + 1;
My hash algorithm OCDness really wants to say this probably biases the range of
output in a way we don't expect. I betcha because our range of inputs are biased
to in_memory versus not, we might get more collisions on even numbers rather
than odd ones..but it also probably doesn't matter and I'm not sure how I'd fix
it. :-/

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/cookies_view_handler.cc (right):

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/cookies_view_handler.cc:240:
profile->GetRequestContextForStoragePartition(
I think all these cases where we call GetRCForStoragePartition from the
StoragePartition itself can be replaced jsut with
storage_partition->GetRequestContext().

Can you verify?

http://codereview.chromium.org/11147026/diff/23003/content/browser/storage_pa...
File content/browser/storage_partition_impl.h (right):

http://codereview.chromium.org/11147026/diff/23003/content/browser/storage_pa...
content/browser/storage_partition_impl.h:38: virtual bool IsInMemoryOnly()
OVERRIDE;
InMemory() is sufficient I think.

[nasko, 2012-10-19 17:29:13]

Fixes based on Albert's review.

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/extensions...
File chrome/browser/extensions/data_deleter.cc (right):

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/extensions...
chrome/browser/extensions/data_deleter.cc:105:
profile->GetRequestContextForStoragePartition(
On 2012/10/18 22:36:15, awong wrote:
> Why isn't this just the RequestContext() out of storage_partiton?

Done.

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.h (right):

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/p...
chrome/browser/profiles/profile.h:262: const bool& in_memory) = 0;
On 2012/10/18 22:36:15, awong wrote:
> don't pass primitives types by const-ref. That's just wasteful :)

Done.

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:768: content::StoragePartition*
storage_partition = rph->GetStoragePartition();
On 2012/10/18 22:36:15, awong wrote:
> If we're already reading out of the StoragePartition, can this just become
> return storage_partition->GetRequestContext()?

I'm going to keep the guest check temporarily, until I get the other piece of
this change, because browser tag doesn't create a proper storage partition yet.

> I think this is safe because the true entry-point where StoragePartition calls
> back into profile is actually GetRequestContextForStoragePartition(), no?
> 
> If that is true, and we make this change, go back to the comments in profile.h
> and note which of the GetRequestContextXXX APIs just dispatch back into
> StoragePartition (I think it'll only be this one).

Even in the case of GetRequestContextForStoragePartition, we don't call back
into StoragePartition, as we have the data passed in as parameters. Hence I
think it is safe and we don't really need any comments.

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:801: content::RenderProcessHost* rph =
content::RenderProcessHost::FromID(
On 2012/10/18 22:36:15, awong wrote:
> Same comment as for GetRequestContextForRenderProcess()

Done.

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:813:
storage_partition->IsInMemoryOnly());
On 2012/10/18 22:36:15, awong wrote:
> Wouldn't it be cleaner to just keep this struct inside io_data and make the
> various GetXXXGetter() functions take 2 params?
> 
> That way we keep StoragePartitionDetails scoped to only the various io_data
> classes.

Done.

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_pa...
File chrome/browser/storage_partition_details.h (right):

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_pa...
chrome/browser/storage_partition_details.h:15: struct StoragePartitionDetails {
On 2012/10/18 22:36:15, awong wrote:
> nit: Can we call this StoragePartitionDescriptor or something that indicates
it
> is an identiifer more than "Details"?

Done.

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_pa...
chrome/browser/storage_partition_details.h:19: StoragePartitionDetails(const
FilePath& partition_path,
On 2012/10/18 22:36:15, awong wrote:
> The constructor should come first in declaration ordering.

Done.

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_pa...
chrome/browser/storage_partition_details.h:28: bool operator()(const
StoragePartitionDetails& left,
On 2012/10/18 22:36:15, awong wrote:
> lhs and rhs seem to be more common than "left" and "right"

Done.

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_pa...
chrome/browser/storage_partition_details.h:32: if (left.in_memory !=
right.in_memory)
On 2012/10/18 22:36:15, awong wrote:
> This isn't a valid Strict Weak Ordering which stl algorithms require.
> 
> SPD a("foo", false);
> SPD b("foo", true);
> 
> here a < b and b < a.

Done.

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_pa...
chrome/browser/storage_partition_details.h:42: const StoragePartitionDetails&
right) const {
On 2012/10/18 22:36:15, awong wrote:
> Simplify to
> 
> return left.path == right.path && left.in_memory == right.in_memory.

Done.

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/storage_pa...
chrome/browser/storage_partition_details.h:69: return input.in_memory ? value :
value + 1;
On 2012/10/18 22:36:15, awong wrote:
> My hash algorithm OCDness really wants to say this probably biases the range
of
> output in a way we don't expect. I betcha because our range of inputs are
biased
> to in_memory versus not, we might get more collisions on even numbers rather
> than odd ones..but it also probably doesn't matter and I'm not sure how I'd
fix
> it. :-/

Done.

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/cookies_view_handler.cc (right):

https://codereview.chromium.org/11147026/diff/23003/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/cookies_view_handler.cc:240:
profile->GetRequestContextForStoragePartition(
On 2012/10/18 22:36:15, awong wrote:
> I think all these cases where we call GetRCForStoragePartition from the
> StoragePartition itself can be replaced jsut with
> storage_partition->GetRequestContext().
> 
> Can you verify?

Done.

https://codereview.chromium.org/11147026/diff/23003/content/browser/storage_p...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/11147026/diff/23003/content/browser/storage_p...
content/browser/storage_partition_impl.h:38: virtual bool IsInMemoryOnly()
OVERRIDE;
On 2012/10/18 22:36:15, awong wrote:
> InMemory() is sufficient I think.

Done.

[awong, 2012-10-19 21:20:24]

couple more comments, but looking pretty close.

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11147026/diff/23003/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:768: content::StoragePartition*
storage_partition = rph->GetStoragePartition();
On 2012/10/19 17:29:14, nasko wrote:
> On 2012/10/18 22:36:15, awong wrote:
> > If we're already reading out of the StoragePartition, can this just become
> > return storage_partition->GetRequestContext()?
> 
> I'm going to keep the guest check temporarily, until I get the other piece of
> this change, because browser tag doesn't create a proper storage partition
yet.
> 
> > I think this is safe because the true entry-point where StoragePartition
calls
> > back into profile is actually GetRequestContextForStoragePartition(), no?
> > 
> > If that is true, and we make this change, go back to the comments in
profile.h
> > and note which of the GetRequestContextXXX APIs just dispatch back into
> > StoragePartition (I think it'll only be this one).
> 
> Even in the case of GetRequestContextForStoragePartition, we don't call back
> into StoragePartition, as we have the data passed in as parameters. Hence I
> think it is safe and we don't really need any comments.

It's more to undersatnd when a function is just delegating back to the
StoragePartition so someone (eg., me) doesn't have to retrace which of the
various GetRequestContext functions are dispatching into StoragePartition.

http://codereview.chromium.org/11147026/diff/38013/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

http://codereview.chromium.org/11147026/diff/38013/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:285: return
storage_partition->GetURLRequestContext();
return rpn->GetStoragePartition()->GetURLRequestContext()?

http://codereview.chromium.org/11147026/diff/38013/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

http://codereview.chromium.org/11147026/diff/38013/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.h:59:
DefineStoragePartitionDescriptorMap(
What is DefineStroagePartitionDescriptorMap?

http://codereview.chromium.org/11147026/diff/38013/chrome/browser/storage_par...
File chrome/browser/storage_partition_descriptor.h (right):

http://codereview.chromium.org/11147026/diff/38013/chrome/browser/storage_par...
chrome/browser/storage_partition_descriptor.h:74: #define
DefineStoragePartitionDescriptorMap(_X_) \
We almost never allow macros, and if we do, they are done in UPPER_CASE. Is
there precedent for this kind of function?  It was really confusing to see it
used.

http://codereview.chromium.org/11147026/diff/38013/content/browser/storage_pa...
File content/browser/storage_partition_impl_map.cc (right):

http://codereview.chromium.org/11147026/diff/38013/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:215: partition->GetPath(),
partition->InMemory()));
This is the only usage of InMemory() I think.  Can't we discern the "in memory"
in this function?

Then I don't think StoragePartition needs to keep this information and we don't
need to add it to the public API.

[nasko, 2012-10-19 22:09:00]

Cleaned up the things mentioned in the last round.

https://codereview.chromium.org/11147026/diff/38013/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11147026/diff/38013/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:285: return
storage_partition->GetURLRequestContext();
On 2012/10/19 21:20:24, awong wrote:
> return rpn->GetStoragePartition()->GetURLRequestContext()?

Done.

https://codereview.chromium.org/11147026/diff/38013/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

https://codereview.chromium.org/11147026/diff/38013/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.h:59:
DefineStoragePartitionDescriptorMap(
On 2012/10/19 21:20:24, awong wrote:
> What is DefineStroagePartitionDescriptorMap?

Done.

https://codereview.chromium.org/11147026/diff/38013/chrome/browser/storage_pa...
File chrome/browser/storage_partition_descriptor.h (right):

https://codereview.chromium.org/11147026/diff/38013/chrome/browser/storage_pa...
chrome/browser/storage_partition_descriptor.h:74: #define
DefineStoragePartitionDescriptorMap(_X_) \
On 2012/10/19 21:20:24, awong wrote:
> We almost never allow macros, and if we do, they are done in UPPER_CASE. Is
> there precedent for this kind of function?  It was really confusing to see it
> used.

Done.

https://codereview.chromium.org/11147026/diff/38013/content/browser/storage_p...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11147026/diff/38013/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:215: partition->GetPath(),
partition->InMemory()));
On 2012/10/19 21:20:24, awong wrote:
> This is the only usage of InMemory() I think.  Can't we discern the "in
memory"
> in this function?
> 
> Then I don't think StoragePartition needs to keep this information and we
don't
> need to add it to the public API.

Done.

[awong, 2012-10-19 23:26:50]

http://codereview.chromium.org/11147026/diff/36005/chrome/browser/extensions/...
File chrome/browser/extensions/data_deleter.cc (right):

http://codereview.chromium.org/11147026/diff/36005/chrome/browser/extensions/...
chrome/browser/extensions/data_deleter.cc:101: content::SiteInstance* site =
process_manager->GetSiteInstanceForURL(url);
variable should be site_instance.

Or if BaseURL is guaranteed to be a correctly formed "Site" URL, we could also
do BrowserContext::GetStoragePartitionForSite(site)

and avoid ExtensionProcessManager

http://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

http://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:283: CHECK(rph);
unnecessary CHECK().  The next line will crash due to null-pointer with just as
much information.

http://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:789: CHECK(rph);
remove superfluous CHECK()

http://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:216: //
CHECK_NE(partition_path.value(), profile_->GetPath().value());
General policy is to not check in commented out code. Just describe it, or if
the follow up CL is coming pretty soon, just do it in there.

http://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:248: //
CHECK_NE(partition_path.value(), profile_->GetPath().value());
same here.

http://codereview.chromium.org/11147026/diff/36005/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/cookies_view_handler.cc (right):

http://codereview.chromium.org/11147026/diff/36005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/cookies_view_handler.cc:235:
content::SiteInstance* site = process_manager->GetSiteInstanceForURL(
Similar question...are we guaranteed that the (*it)->url() is the Site Url?  If
so, then we can just do BC::GetSPForSite()

http://codereview.chromium.org/11147026/diff/36005/chrome/test/base/testing_p...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/11147026/diff/36005/chrome/test/base/testing_p...
chrome/test/base/testing_profile.cc:627: CHECK(rph);
remove CHECK()

[nasko, 2012-10-19 23:55:16]

https://codereview.chromium.org/11147026/diff/36005/chrome/browser/extensions...
File chrome/browser/extensions/data_deleter.cc (right):

https://codereview.chromium.org/11147026/diff/36005/chrome/browser/extensions...
chrome/browser/extensions/data_deleter.cc:101: content::SiteInstance* site =
process_manager->GetSiteInstanceForURL(url);
On 2012/10/19 23:26:50, awong wrote:
> variable should be site_instance.
> 
> Or if BaseURL is guaranteed to be a correctly formed "Site" URL, we could also
> do BrowserContext::GetStoragePartitionForSite(site)
> 
> and avoid ExtensionProcessManager

Done.

https://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:283: CHECK(rph);
On 2012/10/19 23:26:50, awong wrote:
> unnecessary CHECK().  The next line will crash due to null-pointer with just
as
> much information.

Done.

https://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:789: CHECK(rph);
On 2012/10/19 23:26:50, awong wrote:
> remove superfluous CHECK()

Done.

https://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:216: //
CHECK_NE(partition_path.value(), profile_->GetPath().value());
On 2012/10/19 23:26:50, awong wrote:
> General policy is to not check in commented out code. Just describe it, or if
> the follow up CL is coming pretty soon, just do it in there.

Done.

https://codereview.chromium.org/11147026/diff/36005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:248: //
CHECK_NE(partition_path.value(), profile_->GetPath().value());
On 2012/10/19 23:26:50, awong wrote:
> same here.

Done.

https://codereview.chromium.org/11147026/diff/36005/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/cookies_view_handler.cc (right):

https://codereview.chromium.org/11147026/diff/36005/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/cookies_view_handler.cc:235:
content::SiteInstance* site = process_manager->GetSiteInstanceForURL(
On 2012/10/19 23:26:50, awong wrote:
> Similar question...are we guaranteed that the (*it)->url() is the Site Url? 
If
> so, then we can just do BC::GetSPForSite()

Done.

https://codereview.chromium.org/11147026/diff/36005/chrome/test/base/testing_...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/11147026/diff/36005/chrome/test/base/testing_...
chrome/test/base/testing_profile.cc:627: CHECK(rph);
On 2012/10/19 23:26:50, awong wrote:
> remove CHECK()

Done.

[awong, 2012-10-29 23:37:40]

Mostly nits.  Getting close.

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/extensions/...
File chrome/browser/extensions/data_deleter.cc (right):

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/extensions/...
chrome/browser/extensions/data_deleter.cc:97: GURL url =
Extension::GetBaseURLFromExtensionId(extension_id);
const GURL&

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/extensions/...
chrome/browser/extensions/data_deleter.cc:100: extension_request_context_ =
storage_partition->GetURLRequestContext();
Add a TODO for me here will ya? I think this code is wrong and that even with
isolated sites, we might need to look at the
profile->GetRequestContextForExtensions().

Please cite http://crbug.com/158386

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:766: CHECK(rph);
remove CHECK() since you immediately use it below.

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:773: storage_partition->GetPath(),
true);
Add a comment explaining the current behavior.  I think you are basically just
forcing in_memory to "true".

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:794: return
GetRequestContextForStoragePartition(
Should this somehow get the MediaRequestContext?  If not, please explain why
this is okay.

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:245: // context.
add a extra line before the start of the TODO()

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/st...
File chrome/browser/profiles/storage_partition_descriptor.h (right):

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/st...
chrome/browser/profiles/storage_partition_descriptor.h:9: #include "base/hash.h"
you don't need these hash includes anymore.

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/st...
chrome/browser/profiles/storage_partition_descriptor.h:17: const bool
in_memory_only)
spacing

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/cookies_view_handler.cc (right):

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/cookies_view_handler.cc:229:
profile->GetExtensionProcessManager();
2 more spaces

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/cookies_view_handler.cc:239:
storage_partition->GetURLRequestContext();
Why is this changing? Is it just cleanup, or is there some subtle bug I'm
missing?

[nasko, 2012-10-30 00:32:58]

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/extensions/...
File chrome/browser/extensions/data_deleter.cc (right):

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/extensions/...
chrome/browser/extensions/data_deleter.cc:97: GURL url =
Extension::GetBaseURLFromExtensionId(extension_id);
On 2012/10/29 23:37:40, awong wrote:
> const GURL&

Done.

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/extensions/...
chrome/browser/extensions/data_deleter.cc:100: extension_request_context_ =
storage_partition->GetURLRequestContext();
On 2012/10/29 23:37:40, awong wrote:
> Add a TODO for me here will ya? I think this code is wrong and that even with
> isolated sites, we might need to look at the
> profile->GetRequestContextForExtensions().
> 
> Please cite http://crbug.com/158386

Done.

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:766: CHECK(rph);
On 2012/10/29 23:37:40, awong wrote:
> remove CHECK() since you immediately use it below.

Done.

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:773: storage_partition->GetPath(),
true);
On 2012/10/29 23:37:40, awong wrote:
> Add a comment explaining the current behavior.  I think you are basically just
> forcing in_memory to "true".

Done.

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:794: return
GetRequestContextForStoragePartition(
On 2012/10/29 23:37:40, awong wrote:
> Should this somehow get the MediaRequestContext?  If not, please explain why
> this is okay.

Oh, good catch! It should be getting the media context.

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:245: // context.
On 2012/10/29 23:37:40, awong wrote:
> add a extra line before the start of the TODO()

Done.

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/st...
File chrome/browser/profiles/storage_partition_descriptor.h (right):

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/st...
chrome/browser/profiles/storage_partition_descriptor.h:9: #include "base/hash.h"
On 2012/10/29 23:37:40, awong wrote:
> you don't need these hash includes anymore.

Done.

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/profiles/st...
chrome/browser/profiles/storage_partition_descriptor.h:17: const bool
in_memory_only)
On 2012/10/29 23:37:40, awong wrote:
> spacing

Done.

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/cookies_view_handler.cc (right):

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/cookies_view_handler.cc:229:
profile->GetExtensionProcessManager();
On 2012/10/29 23:37:40, awong wrote:
> 2 more spaces

Done.

http://codereview.chromium.org/11147026/diff/39054/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/cookies_view_handler.cc:239:
storage_partition->GetURLRequestContext();
On 2012/10/29 23:37:40, awong wrote:
> Why is this changing? Is it just cleanup, or is there some subtle bug I'm
> missing?

Because GetRequestContextForStoragePartition is changing from taking a
partition_id string to taking FilePath and bool. We don't have those here, so it
needs to go through the BrowserContext to get the partition.

That said, I think we need to remove his code based on our discussion of
isolated sites today. This code was added for supporting the "Apps cookies"
dialog, which will not be shipping if we don't do isolated sites.

[awong, 2012-10-30 00:41:13]

LGTM w/nit

nit: Beef up the changelist description please :)

[nasko, 2012-10-30 15:16:01]

Adding OWNERS reviewers.

jhawkins@ - chrome/browser/ui/webui/options
mihaip@ - chrome/browser/extensions
mmenke@ - chrome/browser/{net|extensions}
phajdan.jr@ - chrome/test
ben@ - chrome/
joth@ - android_webview

[joth, 2012-10-30 16:32:58]

android_webview - lgtm

[James Hawkins, 2012-10-30 17:40:52]

LGTM with nit.

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/cookies_view_handler.cc (right):

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/cookies_view_handler.cc:231: if (service &&
process_manager) {
Save indentation and processing by returning early if !service:

if (!service)
  return;

ExtensionProcessManager* process_manager = ...
if (!process_manager)
  return;

// Do such and such, but with must less indentation.

[Charlie Reis, 2012-10-30 17:47:47]

Awesome.  A few comments below.

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/extensions/...
File chrome/browser/extensions/web_view_browsertest.cc (right):

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:103: // The default behavior
is to combine browser tags with no explicit partition
nit: webview tags  :)

Can you also add bug 138296 to the CL description, since this appears to fix
that?

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:742: // TODO(nasko): Remove this
conditional, once browser tag creates a proper
nit: webview tag

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:766: // TODO(nasko): Remove this
conditional, once browser tag creates a proper
ditto

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:215: // base profile path. We
expect isolated partition, which will never go
nit: expect an

Can we just put the check in rather than a TODO, comparing against
profile_->GetPath()?  (From the comment, I'm assuming partition_path already
contains the base profile path, and isn't just empty vs a relative path.)

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/st...
File chrome/browser/profiles/storage_partition_descriptor.h (right):

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/st...
chrome/browser/profiles/storage_partition_descriptor.h:11: // This structure
combines the on-disk path and a boolean whether the partition
nit: combines a StoragePartition's on-disk path and a boolean for whether...

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/st...
chrome/browser/profiles/storage_partition_descriptor.h:12: // should be
persisted on disk. It's purpose is to serve as a unique key to
nit: Its

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/cookies_view_handler.cc (right):

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/cookies_view_handler.cc:240:
storage_partition->GetURLRequestContext();
nit: 2 more spaces

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/cookies_view_handler.cc:243: // For now, just
cookies are isolated, so other parameters are NULL.
Nasko, is this still true?  We do isolate other types of storage now.  I suppose
that's for another CL, but should this be updated?

http://codereview.chromium.org/11147026/diff/52031/chrome/test/base/testing_p...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/11147026/diff/52031/chrome/test/base/testing_p...
chrome/test/base/testing_profile.cc:618: // TODO(nasko): Remove this
conditional, once browser tag creates a proper
nit: webview tag

http://codereview.chromium.org/11147026/diff/52031/content/public/browser/ren...
File content/public/browser/render_process_host.h (right):

http://codereview.chromium.org/11147026/diff/52031/content/public/browser/ren...
content/public/browser/render_process_host.h:100: // URLRequestContextGetter's
creation is moved into StoragePartition.
Do we have a bug on file for this that we could cite?

http://codereview.chromium.org/11147026/diff/52031/content/public/test/mock_r...
File content/public/test/mock_render_process_host.h (right):

http://codereview.chromium.org/11147026/diff/52031/content/public/test/mock_r...
content/public/test/mock_render_process_host.h:15: class StoragePartition;
nit: Alphabetize

[Paweł Hajdan Jr., 2012-10-30 17:48:44]

chrome/test LGTM

[mmenke, 2012-10-30 18:27:15]

LGTM.

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/net/chrome_...
chrome/browser/net/chrome_url_request_context.h:12: #include
"chrome/browser/profiles/storage_partition_descriptor.h"
This shouldn't be needed, with the forward declaration.

If it is needed for some reason I'm missing, both the forward declaration and
the include in the cc file should be removed.

[mmenke, 2012-10-30 18:27:37]

Erm, that should be chrome/browser/net LGTM.

On 2012/10/30 18:27:15, Matt Menke wrote:
> LGTM.
> 
>
http://codereview.chromium.org/11147026/diff/52031/chrome/browser/net/chrome_...
> File chrome/browser/net/chrome_url_request_context.h (right):
> 
>
http://codereview.chromium.org/11147026/diff/52031/chrome/browser/net/chrome_...
> chrome/browser/net/chrome_url_request_context.h:12: #include
> "chrome/browser/profiles/storage_partition_descriptor.h"
> This shouldn't be needed, with the forward declaration.
> 
> If it is needed for some reason I'm missing, both the forward declaration and
> the include in the cc file should be removed.

[nasko, 2012-10-30 19:55:25]

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/extensions...
File chrome/browser/extensions/web_view_browsertest.cc (right):

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:103: // The default behavior
is to combine browser tags with no explicit partition
On 2012/10/30 17:47:48, creis wrote:
> nit: webview tags  :)
> 
> Can you also add bug 138296 to the CL description, since this appears to fix
> that?

Done.

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/net/chrome...
File chrome/browser/net/chrome_url_request_context.h (right):

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/net/chrome...
chrome/browser/net/chrome_url_request_context.h:12: #include
"chrome/browser/profiles/storage_partition_descriptor.h"
On 2012/10/30 18:27:15, Matt Menke wrote:
> This shouldn't be needed, with the forward declaration.
> 
> If it is needed for some reason I'm missing, both the forward declaration and
> the include in the cc file should be removed.

Done.

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:742: // TODO(nasko): Remove this
conditional, once browser tag creates a proper
On 2012/10/30 17:47:48, creis wrote:
> nit: webview tag

Done.

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl.cc:766: // TODO(nasko): Remove this
conditional, once browser tag creates a proper
On 2012/10/30 17:47:48, creis wrote:
> ditto

Done.

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:215: // base profile path. We
expect isolated partition, which will never go
On 2012/10/30 17:47:48, creis wrote:
> nit: expect an
> 
> Can we just put the check in rather than a TODO, comparing against
> profile_->GetPath()?  (From the comment, I'm assuming partition_path already
> contains the base profile path, and isn't just empty vs a relative path.)

Your assumption is correct and that will be the check. The issue is that the
webview tag is not wired yet to create the proper path and it is still in
memory, so we can't yet enforce this as CHECK. This is why I have a TODO to
enable it once I've done the rest of the work.

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/s...
File chrome/browser/profiles/storage_partition_descriptor.h (right):

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/s...
chrome/browser/profiles/storage_partition_descriptor.h:11: // This structure
combines the on-disk path and a boolean whether the partition
On 2012/10/30 17:47:48, creis wrote:
> nit: combines a StoragePartition's on-disk path and a boolean for whether...

Done.

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/s...
chrome/browser/profiles/storage_partition_descriptor.h:12: // should be
persisted on disk. It's purpose is to serve as a unique key to
On 2012/10/30 17:47:48, creis wrote:
> nit: Its

Done.

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/cookies_view_handler.cc (right):

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/cookies_view_handler.cc:231: if (service &&
process_manager) {
On 2012/10/30 17:40:53, James Hawkins wrote:
> Save indentation and processing by returning early if !service:
> 
> if (!service)
>   return;
> 
> ExtensionProcessManager* process_manager = ...
> if (!process_manager)
>   return;
> 
> // Do such and such, but with must less indentation.

Done.

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/cookies_view_handler.cc:240:
storage_partition->GetURLRequestContext();
On 2012/10/30 17:47:48, creis wrote:
> nit: 2 more spaces

Done.

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/cookies_view_handler.cc:243: // For now, just
cookies are isolated, so other parameters are NULL.
On 2012/10/30 17:47:48, creis wrote:
> Nasko, is this still true?  We do isolate other types of storage now.  I
suppose
> that's for another CL, but should this be updated?

Based on our discussion from yesterday, since isolated sites will not be
shipping, this code should be removed. I'll be filing a bug to undo the entire
App Data dialog work I did.

https://codereview.chromium.org/11147026/diff/52031/chrome/test/base/testing_...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/11147026/diff/52031/chrome/test/base/testing_...
chrome/test/base/testing_profile.cc:618: // TODO(nasko): Remove this
conditional, once browser tag creates a proper
On 2012/10/30 17:47:48, creis wrote:
> nit: webview tag

Done.

https://codereview.chromium.org/11147026/diff/52031/content/public/browser/re...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/11147026/diff/52031/content/public/browser/re...
content/public/browser/render_process_host.h:100: // URLRequestContextGetter's
creation is moved into StoragePartition.
On 2012/10/30 17:47:48, creis wrote:
> Do we have a bug on file for this that we could cite?

Done.

https://codereview.chromium.org/11147026/diff/52031/content/public/test/mock_...
File content/public/test/mock_render_process_host.h (right):

https://codereview.chromium.org/11147026/diff/52031/content/public/test/mock_...
content/public/test/mock_render_process_host.h:15: class StoragePartition;
On 2012/10/30 17:47:48, creis wrote:
> nit: Alphabetize

Done.

[Charlie Reis, 2012-10-30 20:13:07]

One nit below, and one question: Is the in_memory flag supposed to be used for
incognito?  If not, what makes an incognito process's HTML5 storage stay in
memory?  (Sorry, just trying to wrap my head around it.)

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:215: // base profile path. We
expect isolated partition, which will never go
On 2012/10/30 19:55:25, nasko wrote:
> On 2012/10/30 17:47:48, creis wrote:
> > nit: expect an
> > 
> > Can we just put the check in rather than a TODO, comparing against
> > profile_->GetPath()?  (From the comment, I'm assuming partition_path already
> > contains the base profile path, and isn't just empty vs a relative path.)
> 
> Your assumption is correct and that will be the check. The issue is that the
> webview tag is not wired yet to create the proper path and it is still in
> memory, so we can't yet enforce this as CHECK. This is why I have a TODO to
> enable it once I've done the rest of the work.

Ah, ok.

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/s...
File chrome/browser/profiles/storage_partition_descriptor.h (right):

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/s...
chrome/browser/profiles/storage_partition_descriptor.h:11: // This structure
combines the on-disk path and a boolean whether the partition
On 2012/10/30 19:55:25, nasko wrote:
> On 2012/10/30 17:47:48, creis wrote:
> > nit: combines a StoragePartition's on-disk path and a boolean for whether...
> 
> Done.

Can you also add the "combines a StoragePartition's" part?

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/cookies_view_handler.cc (right):

https://codereview.chromium.org/11147026/diff/52031/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/cookies_view_handler.cc:243: // For now, just
cookies are isolated, so other parameters are NULL.
On 2012/10/30 19:55:25, nasko wrote:
> On 2012/10/30 17:47:48, creis wrote:
> > Nasko, is this still true?  We do isolate other types of storage now.  I
> suppose
> > that's for another CL, but should this be updated?
> 
> Based on our discussion from yesterday, since isolated sites will not be
> shipping, this code should be removed. I'll be filing a bug to undo the entire
> App Data dialog work I did.

Ok.

[nasko, 2012-10-30 20:27:42]

Fixed the comment omission. 

All of the HTML5 storage is created when creating the StoragePartition class,
which uses the profile's IsOffTheRecord() method to determine whether to be
in-memory or not. 
The cookie storage and HTTP cache are the only pieces created by this code path.

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/st...
File chrome/browser/profiles/storage_partition_descriptor.h (right):

http://codereview.chromium.org/11147026/diff/52031/chrome/browser/profiles/st...
chrome/browser/profiles/storage_partition_descriptor.h:11: // This structure
combines the on-disk path and a boolean whether the partition
On 2012/10/30 20:13:07, creis wrote:
> On 2012/10/30 19:55:25, nasko wrote:
> > On 2012/10/30 17:47:48, creis wrote:
> > > nit: combines a StoragePartition's on-disk path and a boolean for
whether...
> > 
> > Done.
> 
> Can you also add the "combines a StoragePartition's" part?

Done.

[Charlie Reis, 2012-10-30 20:30:20]

On 2012/10/30 20:27:42, nasko wrote:
> All of the HTML5 storage is created when creating the StoragePartition class,
> which uses the profile's IsOffTheRecord() method to determine whether to be
> in-memory or not. 
> The cookie storage and HTTP cache are the only pieces created by this code
path.

Thanks for explaining.  LGTM!

[commit-bot: I haz the power, 2012-10-30 21:17:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/11147026/50004

[commit-bot: I haz the power, 2012-10-30 23:22:51]

Change committed as 165057