Move StoragePartition into content/public and remove BrowserContext::GetDOMStorageContext().

Move StoragePartition into content/public and remove BrowserContext::GetDOMStorageContext().

Eventually all the storage context accessors will be removed from BrowserContext. Instead, users should retrieve the storage context from the StoragePartition.

This also changes RenderProcessHost to take in a StoragePartition removing the need for a re-lookup its storage contexts.

BUG=85121, 143486
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152251

[awong, 2012-08-14 17:12:08]

Primary review is for Charlie, my favorite reviewer :)

marja, michaeln, ericu, alecfett CCed as an FYI

[Charlie Reis, 2012-08-14 18:50:12]

Nice progress towards where we want to be!

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/bro...
File content/browser/browser_context.cc (right):

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/bro...
content/browser/browser_context.cc:154: if (!partition_map) {
nit: No braces.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/bro...
content/browser/browser_context.cc:169: //
ResourceContext::InitializeResourceContext() directly here is that if
nit: Drop the "if"

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/bro...
content/browser/browser_context.cc:171: // and when that call returns it'll end
rewriting its UserData map. It will
nit: end up rewriting

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/bro...
content/browser/browser_context.cc:188: // TODO(ajwong): This is the only usage
of GetDefaultDOMStorageContextImpl().
nit: This comment is now stale.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/ren...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.h:45: class
CONTENT_EXPORT RenderProcessHostImpl
Can you add something about the RenderProcessHost being dedicated to a given
StoragePartition here in the class-level comment?  That will make it clear why
it takes one in the constructor.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.h:50:
StoragePartitionImpl* storage_partition_impl,
Why is this a StoragePartitionImpl rather than a StoragePartition?  Looks like
we only use the public API.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/web...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1305: // re-lookup
via |instance|?
Do you think there will be any other places that we'll have a partition_id on
hand?  If this is the only one, then it's not clear it's worth exposing
GetStoragePartitionByPartitionId in the public API.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1307:
BrowserContext::GetStoragePartition(browser_context_, instance);
nit: Wrong indent.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/web...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:1238:
BrowserContext::GetStoragePartition(GetBrowserContext(),
I guess this is another case where it would be useful to expose
GetStoragePartitionById.  Maybe it's worth it.  (Note my attempt at a shorter
name, which you're free to shoot down.)  :)

nit: Wrong indent here.

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

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/public/brow...
content/public/browser/browser_context.h:67: static void
ForEachStoragePartition(
Maybe add a comment here or on the callback typedef saying what the string in
the callback is for?

[awong, 2012-08-14 19:12:46]

Comments addressed.

There's some suspicious try failures though. Debugging.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/bro...
File content/browser/browser_context.cc (right):

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/bro...
content/browser/browser_context.cc:154: if (!partition_map) {
On 2012/08/14 18:50:12, creis wrote:
> nit: No braces.

Gha...I will never get use to that. :(

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/bro...
content/browser/browser_context.cc:169: //
ResourceContext::InitializeResourceContext() directly here is that if
On 2012/08/14 18:50:12, creis wrote:
> nit: Drop the "if"

Done.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/bro...
content/browser/browser_context.cc:171: // and when that call returns it'll end
rewriting its UserData map. It will
On 2012/08/14 18:50:12, creis wrote:
> nit: end up rewriting

Done.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/bro...
content/browser/browser_context.cc:188: // TODO(ajwong): This is the only usage
of GetDefaultDOMStorageContextImpl().
On 2012/08/14 18:50:12, creis wrote:
> nit: This comment is now stale.

Done.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/ren...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.h:45: class
CONTENT_EXPORT RenderProcessHostImpl
On 2012/08/14 18:50:12, creis wrote:
> Can you add something about the RenderProcessHost being dedicated to a given
> StoragePartition here in the class-level comment?  That will make it clear why
> it takes one in the constructor.

Done.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.h:50:
StoragePartitionImpl* storage_partition_impl,
On 2012/08/14 18:50:12, creis wrote:
> Why is this a StoragePartitionImpl rather than a StoragePartition?  Looks like
> we only use the public API.

We get covariant return which removes the need to downcast
DOMStorageContextImpl, IndexDBContextImp, and ChromeAppCacheService in the
implementation.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/web...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1305: // re-lookup
via |instance|?
On 2012/08/14 18:50:12, creis wrote:
> Do you think there will be any other places that we'll have a partition_id on
> hand?  If this is the only one, then it's not clear it's worth exposing
> GetStoragePartitionByPartitionId in the public API.

I think it's mostly a SessionStorage thing.  For right now, I'd rather leave it
as a double-lookup because it keeps SiteInstance as the only easy way to use the
API.  File a bug though.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1307:
BrowserContext::GetStoragePartition(browser_context_, instance);
On 2012/08/14 18:50:12, creis wrote:
> nit: Wrong indent.

Done.

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/web...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:1238:
BrowserContext::GetStoragePartition(GetBrowserContext(),
On 2012/08/14 18:50:12, creis wrote:
> I guess this is another case where it would be useful to expose
> GetStoragePartitionById.  Maybe it's worth it.  (Note my attempt at a shorter
> name, which you're free to shoot down.)  :)
> 
> nit: Wrong indent here.

Shorter name is better.  :) But decided to punt on this.  Fixed indent.

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

https://chromiumcodereview.appspot.com/10837230/diff/5032/content/public/brow...
content/public/browser/browser_context.h:67: static void
ForEachStoragePartition(
On 2012/08/14 18:50:12, creis wrote:
> Maybe add a comment here or on the callback typedef saying what the string in
> the callback is for?

Right. Good idea.

[michaeln, 2012-08-14 22:12:50]

jam should probably take a look at this too since it's adding public content api

http://codereview.chromium.org/10837230/diff/13002/content/browser/site_insta...
File content/browser/site_instance_impl.cc (right):

http://codereview.chromium.org/10837230/diff/13002/content/browser/site_insta...
content/browser/site_instance_impl.cc:119: if (render_process_host_factory_) {
How is render_process_host_factory_ used? Is there a use case for this in chrome
and do we need to widen it's CreateRenderProcessHost method to take a
'partition' and is_guest param?

Ah... looks like it's used for tests.

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

http://codereview.chromium.org/10837230/diff/13002/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:54: partitions_[partition_id] =
storage_partition;
Nothing is ever removed from this collection. Worth a TODO?

http://codereview.chromium.org/10837230/diff/13002/content/public/browser/sto...
File content/public/browser/storage_partition.h (right):

http://codereview.chromium.org/10837230/diff/13002/content/public/browser/sto...
content/public/browser/storage_partition.h:41: virtual ~StoragePartition() {}
Should this dtor be protected, is there any use case for the content layer
consumer to delete an instance?

[awong, 2012-08-14 23:05:49]

Stupid me accidentally deleted the wrong patch set, but I responded to your
comments below.

On 2012/08/14 22:12:50, michaeln wrote:
> jam should probably take a look at this too since it's adding public content
api
> 
>
http://codereview.chromium.org/10837230/diff/13002/content/browser/site_insta...
> File content/browser/site_instance_impl.cc (right):
> 
>
http://codereview.chromium.org/10837230/diff/13002/content/browser/site_insta...
> content/browser/site_instance_impl.cc:119: if (render_process_host_factory_) {
> How is render_process_host_factory_ used? Is there a use case for this in
chrome
> and do we need to widen it's CreateRenderProcessHost method to take a
> 'partition' and is_guest param?
> 
> Ah... looks like it's used for tests.

Yeah, I think we can leave that alone for now.

>
http://codereview.chromium.org/10837230/diff/13002/content/browser/storage_pa...
> File content/browser/storage_partition_impl_map.cc (right):
> 
>
http://codereview.chromium.org/10837230/diff/13002/content/browser/storage_pa...
> content/browser/storage_partition_impl_map.cc:54: partitions_[partition_id] =
> storage_partition;
> Nothing is ever removed from this collection. Worth a TODO?

This is intentional for now.  Currently, there's no way to delete the partition
yet because we haven't fixed what happens on uninstall.

http://codereview.chromium.org/10837230/diff/13002/content/public/browser/sto...
> File content/public/browser/storage_partition.h (right):
> 
>
http://codereview.chromium.org/10837230/diff/13002/content/public/browser/sto...
> content/public/browser/storage_partition.h:41: virtual ~StoragePartition() {}
> Should this dtor be protected, is there any use case for the content layer
> consumer to delete an instance?

Good idea. Made protected.

[Charlie Reis, 2012-08-14 23:48:48]

Ok, LGTM.

[awong, 2012-08-14 23:58:28]

jam@ did you want to look over adding StoragePartition into the public API for
content?

The idea is to eventually remove all the individual static GetXXXContext()
functions from BrowserContext.

[jam, 2012-08-15 15:16:21]

public lgtm, just some nits

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

http://codereview.chromium.org/10837230/diff/4039/content/browser/storage_par...
content/browser/storage_partition_impl.h:11: #include
"content/browser/appcache/chrome_appcache_service.h"
nit: i think the next 3 headers don't need to be included here, forward
declarations (from the parent interface) should be enough

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/reso...
File content/public/browser/resource_context.h (right):

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/reso...
content/public/browser/resource_context.h:37:
DISALLOW_COPY_AND_ASSIGN(ResourceContext);
ditto

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/stor...
File content/public/browser/storage_partition.h (right):

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/stor...
content/public/browser/storage_partition.h:40: StoragePartition() {}
nit: don't need, so get rid of (convention we follow with interfaces in public)

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/stor...
content/public/browser/storage_partition.h:46: virtual
content::DOMStorageContext* GetDOMStorageContext() = 0;
nit: drop the "content::"

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/stor...
content/public/browser/storage_partition.h:53:
DISALLOW_COPY_AND_ASSIGN(StoragePartition);
nit: not needed since this interface is non-copyable

[awong, 2012-08-17 00:52:38]

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

http://codereview.chromium.org/10837230/diff/4039/content/browser/storage_par...
content/browser/storage_partition_impl.h:11: #include
"content/browser/appcache/chrome_appcache_service.h"
On 2012/08/15 15:16:22, John Abd-El-Malek wrote:
> nit: i think the next 3 headers don't need to be included here, forward
> declarations (from the parent interface) should be enough

They need to be here because the covariant overrides below need to know that
IndexDBContextImpl is indeed a child of IndexDBContext, etc.

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/reso...
File content/public/browser/resource_context.h (right):

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/reso...
content/public/browser/resource_context.h:37:
DISALLOW_COPY_AND_ASSIGN(ResourceContext);
On 2012/08/15 15:16:22, John Abd-El-Malek wrote:
> ditto

I had taken the convention to redeclare these things similar to how we redeclare
"virtual."  Is this something you'd rather I remove?

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/stor...
File content/public/browser/storage_partition.h (right):

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/stor...
content/public/browser/storage_partition.h:40: StoragePartition() {}
On 2012/08/15 15:16:22, John Abd-El-Malek wrote:
> nit: don't need, so get rid of (convention we follow with interfaces in
public)

See comment for DISALLOW_COPY_AND_ASSIGN().

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/stor...
content/public/browser/storage_partition.h:46: virtual
content::DOMStorageContext* GetDOMStorageContext() = 0;
On 2012/08/15 15:16:22, John Abd-El-Malek wrote:
> nit: drop the "content::"

Done.

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/stor...
content/public/browser/storage_partition.h:53:
DISALLOW_COPY_AND_ASSIGN(StoragePartition);
On 2012/08/15 15:16:22, John Abd-El-Malek wrote:
> nit: not needed since this interface is non-copyable

I'm not sure I understand.  If we leave both the default constructor and this
out, then I think it *is* copyable.

StoragePartition *a1 = new StoragePartitionImpl();
StoragePartition *a2 = new StoragePartitionImpl();
*a1 = *a2;

This will compile even if StoragePartitionImpl has
DISALLOW_COPY_AND_ASSIGN...it'll just do the wrong thing.  

If we have DISALLOW_COPY_AND_ASSIGN() her eto force this type to be
non-copyable, then we also need the default constructor to be public.

[jam, 2012-08-17 16:42:23]

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/reso...
File content/public/browser/resource_context.h (right):

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/reso...
content/public/browser/resource_context.h:37:
DISALLOW_COPY_AND_ASSIGN(ResourceContext);
On 2012/08/17 00:52:39, awong wrote:
> On 2012/08/15 15:16:22, John Abd-El-Malek wrote:
> > ditto
> 
> I had taken the convention to redeclare these things similar to how we
redeclare
> "virtual."  Is this something you'd rather I remove?

I think this is a little different from putting "virtual" in derived classes. in
this case, since a class with pure virtual functions isn't copyable,
DISALLOW_COPY_AND_ASSIGN is unnecessary.

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/stor...
File content/public/browser/storage_partition.h (right):

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/stor...
content/public/browser/storage_partition.h:40: StoragePartition() {}
On 2012/08/17 00:52:39, awong wrote:
> On 2012/08/15 15:16:22, John Abd-El-Malek wrote:
> > nit: don't need, so get rid of (convention we follow with interfaces in
> public)
> 
> See comment for DISALLOW_COPY_AND_ASSIGN().

same, only put stuff that's needed. the convention is just to put destructors
(since that's needed). see the rest of the interfaces in content/public :)

http://codereview.chromium.org/10837230/diff/4039/content/public/browser/stor...
content/public/browser/storage_partition.h:53:
DISALLOW_COPY_AND_ASSIGN(StoragePartition);
On 2012/08/17 00:52:39, awong wrote:
> On 2012/08/15 15:16:22, John Abd-El-Malek wrote:
> > nit: not needed since this interface is non-copyable
> 
> I'm not sure I understand.  If we leave both the default constructor and this
> out, then I think it *is* copyable.
> 
> StoragePartition *a1 = new StoragePartitionImpl();
> StoragePartition *a2 = new StoragePartitionImpl();
> *a1 = *a2;
> 
> This will compile even if StoragePartitionImpl has
> DISALLOW_COPY_AND_ASSIGN...it'll just do the wrong thing.  
> 
> If we have DISALLOW_COPY_AND_ASSIGN() her eto force this type to be
> non-copyable, then we also need the default constructor to be public.

hmm, I didn't even think that *a1= *a2 would compile. but now that I tried it
out, I see that it does. is this even defined C++ behavior? I tried this with a
simple class and I do see that it's not doing the right thing as you say.

btw it's my understanding that in google3 DISALLOW_COPY_AND_ASSIGN isn't put on
interfaces.

[awong, 2012-08-18 01:35:21]

Removed all the DISALLOW_COPY_AND_ASSIGNs for now as I track down some other C++
readability guys to see if there's a style guide consensus.  Also, leaving it
out maintains consistency. If we revisit it, we can do it across all interfaces
at once.

Lastly, I changed RPHI::CreateMessageFilter() to not use the storage partition
for all the contexts.  This causes a number of unittests to fail (surprise
surprise) because they are depending on the lack of isolation in some cases. 
Doing this lets us fix each of the contexts individually.

[commit-bot: I haz the power, 2012-08-18 06:34:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/10837230/12064

[commit-bot: I haz the power, 2012-08-18 08:54:36]

Change committed as 152251