Support partitioning of storage contexts based on render_id.

Support partitioning of storage contexts based on render_id.

This modifies BrowserContext to support having multiple storage partitions based on the child process id. The embedder is given a function that allows it to map a child process id into different storage buckets.

R=creis,nasko,jam
TBR=marja
BUG=85121
TEST=add an isolated app that covers a path like http://www.example.com/isolated/.  On that page, add an entry to local storage. Access another page on www.example.com that is not isolated. Check that it cannot see the previously set entry.  Repeat test in the reverse direction.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=146443

[awong, 2012-06-29 22:07:36]

This is finally ready for a first pass review.

Note that the IsolateAppTest is still not quite done. It's hanging for some
reason so I need to debug that.

[nasko, 2012-06-29 23:08:56]

Overall LGTM with some notes.

http://codereview.chromium.org/10600009/diff/8001/content/browser/browser_con...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/10600009/diff/8001/content/browser/browser_con...
content/browser/browser_context.cc:271: void DOMStorageContextForEach(
nit: In ForEachStoragePartition the "ForEach" part is at the beginning, here it
is at the end. Any reason? It is nice to be consistent.

http://codereview.chromium.org/10600009/diff/8001/content/browser/web_content...
File content/browser/web_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/10600009/diff/8001/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl.cc:197:
BrowserContext::GetDefaultDOMStorageContext(browser_context)));
Any particular reason you moved from browser_context_ to browser_context?

http://codereview.chromium.org/10600009/diff/8001/content/browser/worker_host...
File content/browser/worker_host/worker_process_host.cc (right):

http://codereview.chromium.org/10600009/diff/8001/content/browser/worker_host...
content/browser/worker_host/worker_process_host.cc:625: // ResourceContext
equivalence is being used as a proxy for ensure we only
nit: s/for/to/?

[Charlie Reis, 2012-07-02 23:21:23]

Great.  Looks promising, and I like the test.

Please change the TEST= line to something about localStorage of isolated apps
being partitioned.  (I learned recently that it's intended for brief steps for
manual verification, not a description of which test cases we added or
modified.)

https://chromiumcodereview.appspot.com/10600009/diff/8001/chrome/browser/chro...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/10600009/diff/8001/chrome/browser/chro...
chrome/browser/chrome_content_browser_client.cc:409: int child_process_id) {
Let's add a comment explaining that Chrome uses the extension ID for any
isolated apps or extensions as the partition ID.

https://chromiumcodereview.appspot.com/10600009/diff/8001/chrome/browser/chro...
chrome/browser/chrome_content_browser_client.cc:416: if (installed_app != NULL
&& installed_app->is_storage_isolated()) {
Is this going to work with browser tags and other types of extensions with
isolated storage?  It should be consistent with how we handle cookies and cache,
and if those still need to be updated, that's fine.

https://chromiumcodereview.appspot.com/10600009/diff/8001/chrome/browser/exte...
File chrome/browser/extensions/isolated_app_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10600009/diff/8001/chrome/browser/exte...
chrome/browser/extensions/isolated_app_browsertest.cc:71: // sessionStorage get
wiped away during reload?
Normally session storage exists for the lifetime of the tab.  If you navigate to
a storage isolated page, though, you should get a different session storage.

I imagine the test would be something like: normal page A sets x=3, then
navigate to an app page which doesn't see it but can set y=4, then navigate to a
normal page B that sees x=3 and not y=4.

https://chromiumcodereview.appspot.com/10600009/diff/8001/chrome/browser/exte...
chrome/browser/extensions/isolated_app_browsertest.cc:144: // Check that tabs
see cannot each others localStorage even though they are
nit: other's

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

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:40: // Dirname for storing persistent data
for renderers with partitioned storage.
partitioned -> isolated

I think this would be more consistent with the other descriptions, while still
letting us talk about a "partition" as the place where isolated storage resides.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:48: class StoragePartition {
Please give this a class-level comment.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:69: // that utlizes the QuotaManager.
nits: Drop the "So", and utlizes -> utilizes

I don't understand if "clients," "consumers," and "contexts" are all the same
thing or not.  What are examples?

(I realize these are from pre-existing code, but it would be nice to clean up if
possible.)

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:161: class StoragePartitionMap : public
base::SupportsUserData::Data {
Class level comment.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:176: if (it != partitions_.end()) {
nit: No braces.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:183: CHECK(IsStringASCII(partition_id));
Are we ever taking this in from an untrusted context?  I wonder if a DCHECK is
sufficient.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:195: // InitializeResourceContext()
understand having different parition data
nit: partition

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:216: void
PostAddInitialization(StoragePartition* partition,
PostCreateInitialization?  Seems more consistent than Add.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:265: void DomStorageAdapter(
These three names aren't making sense to me.  If they're functions, can we make
them verbs?  For example, DOMStorageAdapter sounds like a class.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:328: QuotaManager*
BrowserContext::GetQuotaManager(BrowserContext* browser_context) {
Should this be GetDefaultQuotaManager?  Or do we not need a process ID specific
one?

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:331:
ChildProcessHostImpl::kInvalidChildProcessId);
This looks odd to me, but I suppose it makes sense.  Can we document this
behavior (i.e., that an invalid ID returns the default context)?

Should we be special casing this in GetStoragePartition so that we don't have to
rely on the ContentBrowserClient to map the invalid ID to an empty string?

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

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:194: if
(!session_storage_namespace_) {
Did you decide that having this on the NavigationController is ok, even though
we can navigate from one namespace to another?

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/public/brow...
File content/public/browser/content_browser_client.h (right):

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/public/brow...
content/public/browser/content_browser_client.h:250: // Allow the embedder to
specify which set of persistent state a child
I feel like "which set of persistent state" is leaving out some of our
expectations.  We're returning an ASCII ID that gets used for the directory name
on disk, right?  Maybe we need to be clearer about what's expected from the
ContentBrowserClient.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/public/brow...
content/public/browser/content_browser_client.h:253: // browser processes.
Returns the empty string for the regular storage
browser processes -> web browsers

[awong, 2012-07-09 20:37:42]

ready for another pass.

https://chromiumcodereview.appspot.com/10600009/diff/8001/chrome/browser/chro...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/10600009/diff/8001/chrome/browser/chro...
chrome/browser/chrome_content_browser_client.cc:409: int child_process_id) {
On 2012/07/02 23:21:23, creis wrote:
> Let's add a comment explaining that Chrome uses the extension ID for any
> isolated apps or extensions as the partition ID.

Added...do we need to do any sort of prefixing?  like ia-EXTENSION_ID to avoid
future namespace collisions?  (eg., gt-STORAGE_PARTITION for guest tag).

https://chromiumcodereview.appspot.com/10600009/diff/8001/chrome/browser/chro...
chrome/browser/chrome_content_browser_client.cc:416: if (installed_app != NULL
&& installed_app->is_storage_isolated()) {
On 2012/07/02 23:21:23, creis wrote:
> Is this going to work with browser tags and other types of extensions with
> isolated storage?  It should be consistent with how we handle cookies and
cache,
> and if those still need to be updated, that's fine.

Cookies and cache should be migrated into the storage partition to make this all
consistent.  But I have to sort out the lifetime issues first.

As for browser tag, I hadn't realized there are CLs in yet to setup isolated
storage.  If so, I expect we will unify them when we migrate the
URLRequestContext into the StoragePartition.

https://chromiumcodereview.appspot.com/10600009/diff/8001/chrome/browser/exte...
File chrome/browser/extensions/isolated_app_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10600009/diff/8001/chrome/browser/exte...
chrome/browser/extensions/isolated_app_browsertest.cc:71: // sessionStorage get
wiped away during reload?
On 2012/07/02 23:21:23, creis wrote:
> Normally session storage exists for the lifetime of the tab.  If you navigate
to
> a storage isolated page, though, you should get a different session storage.
> 
> I imagine the test would be something like: normal page A sets x=3, then
> navigate to an app page which doesn't see it but can set y=4, then navigate to
a
> normal page B that sees x=3 and not y=4.

I've amended the comment.  FYI, session storage isn't isolated yet, so I can't
implement the test quite yet.

https://chromiumcodereview.appspot.com/10600009/diff/8001/chrome/browser/exte...
chrome/browser/extensions/isolated_app_browsertest.cc:144: // Check that tabs
see cannot each others localStorage even though they are
On 2012/07/02 23:21:23, creis wrote:
> nit: other's

Done.

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

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:40: // Dirname for storing persistent data
for renderers with partitioned storage.
On 2012/07/02 23:21:23, creis wrote:
> partitioned -> isolated
> 
> I think this would be more consistent with the other descriptions, while still
> letting us talk about a "partition" as the place where isolated storage
resides.

sgtm.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:48: class StoragePartition {
On 2012/07/02 23:21:23, creis wrote:
> Please give this a class-level comment.

Done.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:69: // that utlizes the QuotaManager.
On 2012/07/02 23:21:23, creis wrote:
> nits: Drop the "So", and utlizes -> utilizes
> 
> I don't understand if "clients," "consumers," and "contexts" are all the same
> thing or not.  What are examples?
> 
> (I realize these are from pre-existing code, but it would be nice to clean up
if
> possible.)

Done.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:161: class StoragePartitionMap : public
base::SupportsUserData::Data {
On 2012/07/02 23:21:23, creis wrote:
> Class level comment.

Done.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:176: if (it != partitions_.end()) {
On 2012/07/02 23:21:23, creis wrote:
> nit: No braces.

Done.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:183: CHECK(IsStringASCII(partition_id));
On 2012/07/02 23:21:23, creis wrote:
> Are we ever taking this in from an untrusted context?  I wonder if a DCHECK is
> sufficient.

We aren't taking it from an untrusted context now, but the consequence of future
modifications feeding this bad data scares me.  

If this invaraint is broken, AppendASCII will just produce a corrupt path
otherwise which may do all sorts of unpredictable things. This seems like a
place where always dying hard is a good thing.

Biasing towards CHECK is always safer anyways :)

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:195: // InitializeResourceContext()
understand having different parition data
On 2012/07/02 23:21:23, creis wrote:
> nit: partition

Done.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:216: void
PostAddInitialization(StoragePartition* partition,
On 2012/07/02 23:21:23, creis wrote:
> PostCreateInitialization?  Seems more consistent than Add.

Good point. Done.

I hate this function BTW. It's logic is really confusing, but I haven't figured
out how to clean it up yet. :(

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:265: void DomStorageAdapter(
On 2012/07/02 23:21:23, creis wrote:
> These three names aren't making sense to me.  If they're functions, can we
make
> them verbs?  For example, DOMStorageAdapter sounds like a class.

Called this one ProcessDomStorageContext().

As for replacing the ForEach...I don't know what else I would call it.
RunOnStoragePartition? ExecuteOnEachPartition? ProcessEachPartition?  It's
really running the callback on each partition like a for loop...ForEeach

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:271: void DOMStorageContextForEach(
On 2012/06/29 23:08:56, nasko wrote:
> nit: In ForEachStoragePartition the "ForEach" part is at the beginning, here
it
> is at the end. Any reason? It is nice to be consistent.

Done.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:328: QuotaManager*
BrowserContext::GetQuotaManager(BrowserContext* browser_context) {
On 2012/07/02 23:21:23, creis wrote:
> Should this be GetDefaultQuotaManager?  Or do we not need a process ID
specific
> one?

There should be no "Default" anythings, but the transition has to be staged
cause it's just too complicated to remove the default accessor at once.

Right now all the "unmigrated" storage APIs just return a "default" under the
covers.  Later CLs are going to migrate each of these APIs one by one so they
always require a process ID.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/bro...
content/browser/browser_context.cc:331:
ChildProcessHostImpl::kInvalidChildProcessId);
On 2012/07/02 23:21:23, creis wrote:
> This looks odd to me, but I suppose it makes sense.  Can we document this
> behavior (i.e., that an invalid ID returns the default context)?
> 
> Should we be special casing this in GetStoragePartition so that we don't have
to
> rely on the ContentBrowserClient to map the invalid ID to an empty string?

I think we actually want ContentBrowserClient to do the mapping.  This code as
is is a temporary thing. I added a TODO to each place that uses
kInvalidChildProcess.

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

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:194: if
(!session_storage_namespace_) {
On 2012/07/02 23:21:23, creis wrote:
> Did you decide that having this on the NavigationController is ok, even though
> we can navigate from one namespace to another?

No...this isn't okay.  But I'm going to fix it in another CL. This one has been
pending too long.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:197:
BrowserContext::GetDefaultDOMStorageContext(browser_context)));
On 2012/06/29 23:08:56, nasko wrote:
> Any particular reason you moved from browser_context_ to browser_context?

nope. Moved back.

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

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/browser/wor...
content/browser/worker_host/worker_process_host.cc:625: // ResourceContext
equivalence is being used as a proxy for ensure we only
On 2012/06/29 23:08:56, nasko wrote:
> nit: s/for/to/?

Done.

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/public/brow...
File content/public/browser/content_browser_client.h (right):

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/public/brow...
content/public/browser/content_browser_client.h:250: // Allow the embedder to
specify which set of persistent state a child
On 2012/07/02 23:21:23, creis wrote:
> I feel like "which set of persistent state" is leaving out some of our
> expectations.  We're returning an ASCII ID that gets used for the directory
name
> on disk, right?  Maybe we need to be clearer about what's expected from the
> ContentBrowserClient.

I don't like adding information about how we implement storage on disk into this
API.  This seems to break the abstraction boundary.  Is the concern that we are
missing validation criteria for the id?  What if I say it should be ascii or
maybe even force it to be [a-z0-9]* (lowercase plus digits)?

https://chromiumcodereview.appspot.com/10600009/diff/8001/content/public/brow...
content/public/browser/content_browser_client.h:253: // browser processes.
Returns the empty string for the regular storage
On 2012/07/02 23:21:23, creis wrote:
> browser processes -> web browsers

Done.

[awong, 2012-07-10 19:47:37]

Yay! Looks like the try runs are going green!

Review ready!

[Charlie Reis, 2012-07-10 21:37:47]

LGTM

http://codereview.chromium.org/10600009/diff/8001/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10600009/diff/8001/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:409: int child_process_id) {
On 2012/07/09 20:37:43, awong wrote:
> On 2012/07/02 23:21:23, creis wrote:
> > Let's add a comment explaining that Chrome uses the extension ID for any
> > isolated apps or extensions as the partition ID.
> 
> Added...do we need to do any sort of prefixing?  like ia-EXTENSION_ID to avoid
> future namespace collisions?  (eg., gt-STORAGE_PARTITION for guest tag).

I don't think prefixing is needed.

http://codereview.chromium.org/10600009/diff/8001/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:416: if (installed_app != NULL
&& installed_app->is_storage_isolated()) {
On 2012/07/09 20:37:43, awong wrote:
> On 2012/07/02 23:21:23, creis wrote:
> > Is this going to work with browser tags and other types of extensions with
> > isolated storage?  It should be consistent with how we handle cookies and
> cache,
> > and if those still need to be updated, that's fine.
> 
> Cookies and cache should be migrated into the storage partition to make this
all
> consistent.  But I have to sort out the lifetime issues first.
> 
> As for browser tag, I hadn't realized there are CLs in yet to setup isolated
> storage.  If so, I expect we will unify them when we migrate the
> URLRequestContext into the StoragePartition.

No, Nasko's CL hasn't landed yet.  We can unify all those things as we go.

http://codereview.chromium.org/10600009/diff/8001/content/browser/browser_con...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/10600009/diff/8001/content/browser/browser_con...
content/browser/browser_context.cc:265: void DomStorageAdapter(
On 2012/07/09 20:37:43, awong wrote:
> On 2012/07/02 23:21:23, creis wrote:
> > These three names aren't making sense to me.  If they're functions, can we
> make
> > them verbs?  For example, DOMStorageAdapter sounds like a class.
> 
> Called this one ProcessDomStorageContext().
> 
> As for replacing the ForEach...I don't know what else I would call it.
> RunOnStoragePartition? ExecuteOnEachPartition? ProcessEachPartition?  It's
> really running the callback on each partition like a for loop...ForEeach

Thanks.  ForEach* is ok.

http://codereview.chromium.org/10600009/diff/8001/content/public/browser/cont...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/10600009/diff/8001/content/public/browser/cont...
content/public/browser/content_browser_client.h:250: // Allow the embedder to
specify which set of persistent state a child
On 2012/07/09 20:37:43, awong wrote:
> On 2012/07/02 23:21:23, creis wrote:
> > I feel like "which set of persistent state" is leaving out some of our
> > expectations.  We're returning an ASCII ID that gets used for the directory
> name
> > on disk, right?  Maybe we need to be clearer about what's expected from the
> > ContentBrowserClient.
> 
> I don't like adding information about how we implement storage on disk into
this
> API.  This seems to break the abstraction boundary.  Is the concern that we
are
> missing validation criteria for the id?  What if I say it should be ascii or
> maybe even force it to be [a-z0-9]* (lowercase plus digits)?

That's sufficient.  Thanks.

http://codereview.chromium.org/10600009/diff/14002/chrome/browser/extensions/...
File chrome/browser/extensions/isolated_app_browsertest.cc (right):

http://codereview.chromium.org/10600009/diff/14002/chrome/browser/extensions/...
chrome/browser/extensions/isolated_app_browsertest.cc:142:
chrome::SelectNumberedTab(browser(), 0);
Oh, good catch.  Unfortunate naming (tab at index 0 is tab1).

http://codereview.chromium.org/10600009/diff/14002/content/browser/browser_co...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/10600009/diff/14002/content/browser/browser_co...
content/browser/browser_context.cc:346: // TODO(ajwong): Force all users to know
which process id they performing
nit: they are
nit: behavior -> behalf

[awong, 2012-07-10 23:13:01]

On 2012/07/10 21:37:47, creis wrote:
> LGTM
> 
>
http://codereview.chromium.org/10600009/diff/8001/chrome/browser/chrome_conte...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
http://codereview.chromium.org/10600009/diff/8001/chrome/browser/chrome_conte...
> chrome/browser/chrome_content_browser_client.cc:409: int child_process_id) {
> On 2012/07/09 20:37:43, awong wrote:
> > On 2012/07/02 23:21:23, creis wrote:
> > > Let's add a comment explaining that Chrome uses the extension ID for any
> > > isolated apps or extensions as the partition ID.
> > 
> > Added...do we need to do any sort of prefixing?  like ia-EXTENSION_ID to
avoid
> > future namespace collisions?  (eg., gt-STORAGE_PARTITION for guest tag).
> 
> I don't think prefixing is needed.
> 
>
http://codereview.chromium.org/10600009/diff/8001/chrome/browser/chrome_conte...
> chrome/browser/chrome_content_browser_client.cc:416: if (installed_app != NULL
> && installed_app->is_storage_isolated()) {
> On 2012/07/09 20:37:43, awong wrote:
> > On 2012/07/02 23:21:23, creis wrote:
> > > Is this going to work with browser tags and other types of extensions with
> > > isolated storage?  It should be consistent with how we handle cookies and
> > cache,
> > > and if those still need to be updated, that's fine.
> > 
> > Cookies and cache should be migrated into the storage partition to make this
> all
> > consistent.  But I have to sort out the lifetime issues first.
> > 
> > As for browser tag, I hadn't realized there are CLs in yet to setup isolated
> > storage.  If so, I expect we will unify them when we migrate the
> > URLRequestContext into the StoragePartition.
> 
> No, Nasko's CL hasn't landed yet.  We can unify all those things as we go.
> 
>
http://codereview.chromium.org/10600009/diff/8001/content/browser/browser_con...
> File content/browser/browser_context.cc (right):
> 
>
http://codereview.chromium.org/10600009/diff/8001/content/browser/browser_con...
> content/browser/browser_context.cc:265: void DomStorageAdapter(
> On 2012/07/09 20:37:43, awong wrote:
> > On 2012/07/02 23:21:23, creis wrote:
> > > These three names aren't making sense to me.  If they're functions, can we
> > make
> > > them verbs?  For example, DOMStorageAdapter sounds like a class.
> > 
> > Called this one ProcessDomStorageContext().
> > 
> > As for replacing the ForEach...I don't know what else I would call it.
> > RunOnStoragePartition? ExecuteOnEachPartition? ProcessEachPartition?  It's
> > really running the callback on each partition like a for loop...ForEeach
> 
> Thanks.  ForEach* is ok.
> 
>
http://codereview.chromium.org/10600009/diff/8001/content/public/browser/cont...
> File content/public/browser/content_browser_client.h (right):
> 
>
http://codereview.chromium.org/10600009/diff/8001/content/public/browser/cont...
> content/public/browser/content_browser_client.h:250: // Allow the embedder to
> specify which set of persistent state a child
> On 2012/07/09 20:37:43, awong wrote:
> > On 2012/07/02 23:21:23, creis wrote:
> > > I feel like "which set of persistent state" is leaving out some of our
> > > expectations.  We're returning an ASCII ID that gets used for the
directory
> > name
> > > on disk, right?  Maybe we need to be clearer about what's expected from
the
> > > ContentBrowserClient.
> > 
> > I don't like adding information about how we implement storage on disk into
> this
> > API.  This seems to break the abstraction boundary.  Is the concern that we
> are
> > missing validation criteria for the id?  What if I say it should be ascii or
> > maybe even force it to be [a-z0-9]* (lowercase plus digits)?
> 
> That's sufficient.  Thanks.
> 
>
http://codereview.chromium.org/10600009/diff/14002/chrome/browser/extensions/...
> File chrome/browser/extensions/isolated_app_browsertest.cc (right):
> 
>
http://codereview.chromium.org/10600009/diff/14002/chrome/browser/extensions/...
> chrome/browser/extensions/isolated_app_browsertest.cc:142:
> chrome::SelectNumberedTab(browser(), 0);
> Oh, good catch.  Unfortunate naming (tab at index 0 is tab1).
> 
>
http://codereview.chromium.org/10600009/diff/14002/content/browser/browser_co...
> File content/browser/browser_context.cc (right):
> 
>
http://codereview.chromium.org/10600009/diff/14002/content/browser/browser_co...
> content/browser/browser_context.cc:346: // TODO(ajwong): Force all users to
know
> which process id they performing
> nit: they are
> nit: behavior -> behalf

Done.

[awong, 2012-07-10 23:14:04]

john, mind doing an owners review for this?

[jam, 2012-07-11 02:09:25]

lgtm

I'd much rather prefer that StoragePartition is landed in a separate file.
smaller files are much easier to read, and this seems sufficiently different
from BrowserContext. splitting it now isn't much of a trouble, and keeps less
churn in the code than if you move it later.

question: will we have the same problem of GetDefaultProfile with the new
default getters? i.e. if someone is sloppy and calls it instead of passing the
pid, then the wrong context will be used?

http://codereview.chromium.org/10600009/diff/13003/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10600009/diff/13003/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:419: if (installed_app != NULL
&& installed_app->is_storage_isolated()) {
nit: i thought the convention was just
if (installed_app &&...

http://codereview.chromium.org/10600009/diff/13003/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.h (right):

http://codereview.chromium.org/10600009/diff/13003/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.h:41: OVERRIDE;
nit: override on previous line

http://codereview.chromium.org/10600009/diff/13003/content/browser/browser_co...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/10600009/diff/13003/content/browser/browser_co...
content/browser/browser_context.cc:124: return quota_manager_;
nit: fit on one line where you can for readability?

[awong, 2012-07-11 21:17:16]

StoragePartition and StoragePartitionMap moved into their own files.

As to your question about GetDefaultDOMStorageContext, yes, we will have the
same problem as GetDefaultProfile.  My goal though is to completely remove
GetDefaultDOMStorageContext so I don't think it will be a long term problem.

The larger issue is people keeping ahold of pointers to these objects beyond
when they should.  I really want some sort of thread-safe weakpointer that you
can detach when we switch contexts or similar.

https://chromiumcodereview.appspot.com/10600009/diff/13003/chrome/browser/chr...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/10600009/diff/13003/chrome/browser/chr...
chrome/browser/chrome_content_browser_client.cc:419: if (installed_app != NULL
&& installed_app->is_storage_isolated()) {
On 2012/07/11 02:09:25, John Abd-El-Malek wrote:
> nit: i thought the convention was just
> if (installed_app &&...

I wasn't aware of a convention, but happy to do it that way.

Done.

https://chromiumcodereview.appspot.com/10600009/diff/13003/chrome/browser/chr...
File chrome/browser/chrome_content_browser_client.h (right):

https://chromiumcodereview.appspot.com/10600009/diff/13003/chrome/browser/chr...
chrome/browser/chrome_content_browser_client.h:41: OVERRIDE;
On 2012/07/11 02:09:25, John Abd-El-Malek wrote:
> nit: override on previous line

Done.

https://chromiumcodereview.appspot.com/10600009/diff/13003/content/browser/br...
File content/browser/browser_context.cc (right):

https://chromiumcodereview.appspot.com/10600009/diff/13003/content/browser/br...
content/browser/browser_context.cc:124: return quota_manager_;
On 2012/07/11 02:09:25, John Abd-El-Malek wrote:
> nit: fit on one line where you can for readability?

Done.

[awong, 2012-07-11 22:59:25]

Seems like it's passing most of try servers.  I'm thinking mac bot is flake...at
least some previous runs are having RlZ failures. I'll rerun mac_rel to be sure.

Will commit tomorrow if I don't hear any objections.

[jam, 2012-07-11 23:36:31]

slgtm with nits

https://chromiumcodereview.appspot.com/10600009/diff/6011/content/browser/sto...
File content/browser/storage_partition.h (right):

https://chromiumcodereview.appspot.com/10600009/diff/6011/content/browser/sto...
content/browser/storage_partition.h:37: class StoragePartition {
nit: new code should be in the content namespace

https://chromiumcodereview.appspot.com/10600009/diff/6011/content/browser/sto...
File content/browser/storage_partition_map.h (right):

https://chromiumcodereview.appspot.com/10600009/diff/6011/content/browser/sto...
content/browser/storage_partition_map.h:22: class StoragePartitionMap : public
base::SupportsUserData::Data {
ditto

https://chromiumcodereview.appspot.com/10600009/diff/6011/content/browser/sto...
content/browser/storage_partition_map.h:35: // partition map.  This feels
dangerous.  Should this not be in this class?
i'm not sure i understand this comment. I would find it dangerous if it has to
be called, but the embedder is the one responsible. since you control this
class, why is it dangerous?

[awong, 2012-07-12 20:28:40]

https://chromiumcodereview.appspot.com/10600009/diff/6011/content/browser/sto...
File content/browser/storage_partition_map.h (right):

https://chromiumcodereview.appspot.com/10600009/diff/6011/content/browser/sto...
content/browser/storage_partition_map.h:22: class StoragePartitionMap : public
base::SupportsUserData::Data {
On 2012/07/11 23:36:31, John Abd-El-Malek wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10600009/diff/6011/content/browser/sto...
content/browser/storage_partition_map.h:35: // partition map.  This feels
dangerous.  Should this not be in this class?
On 2012/07/11 23:36:31, John Abd-El-Malek wrote:
> i'm not sure i understand this comment. I would find it dangerous if it has to
> be called, but the embedder is the one responsible. since you control this
> class, why is it dangerous?

It's not dangerous for users of the class, but for future maintainers.  When I
was refactoring this, I kept having code paths where this wasn't called at the
right time which took me time to track down.

I think we can actually remove the need for this function, so this comment is to
remind myself about the issue when I get to seeing how to make AppCache and
ResourceContext reflect the storage isolation.

Reworded it slightly to be more clear.

[commit-bot: I haz the power, 2012-07-12 21:23:35]

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

[commit-bot: I haz the power, 2012-07-12 21:23:42]

Presubmit check for 10600009-11011 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/sessions

Presubmit checks took 2.5s to calculate.

[michaeln, 2012-08-10 00:42:11]

https://chromiumcodereview.appspot.com/10600009/diff/11011/content/browser/st...
File content/browser/storage_partition_map.cc (right):

https://chromiumcodereview.appspot.com/10600009/diff/11011/content/browser/st...
content/browser/storage_partition_map.cc:88:
browser_context_->GetResourceContext(),
Passing in browser_context_->GetResourceContext() doesn't look completely
correct for the isolated app case.

* The URLRequestContext of this ResourceContext will be used when populating
appcaches under this service, it will be using the 'default' cookies and http
cache when doing so... that's not incorrect.

* The ResourceContext is also used as inputs to the
ContentBrowserClient::AllowAppCache() method. The chrome level impl of that
interface uses the ResourceContext to navigate to the content settings
structures associated with the user profile... which does seem correct (assuming
isolated apps dont have different settings).

[awong, 2012-08-10 13:59:36]

https://chromiumcodereview.appspot.com/10600009/diff/11011/content/browser/st...
File content/browser/storage_partition_map.cc (right):

https://chromiumcodereview.appspot.com/10600009/diff/11011/content/browser/st...
content/browser/storage_partition_map.cc:88:
browser_context_->GetResourceContext(),
On 2012/08/10 00:42:11, michaeln wrote:
> Passing in browser_context_->GetResourceContext() doesn't look completely
> correct for the isolated app case.

Yes...nothing that uses GetResourceContext() has been fixed for isolated apps.
At the time of this CL, it was just too large of a chunk to bite off at once,
but maybe now it's time to start looking at it.

> * The URLRequestContext of this ResourceContext will be used when populating
> appcaches under this service, it will be using the 'default' cookies and http
> cache when doing so... that's not incorrect.

If I'm understanding correctly, can this be solved by passing in the appropriate
URLRequestContext instead of the whole ResourceContext?
 
> * The ResourceContext is also used as inputs to the
> ContentBrowserClient::AllowAppCache() method. The chrome level impl of that
> interface uses the ResourceContext to navigate to the content settings
> structures associated with the user profile... which does seem correct
(assuming
> isolated apps dont have different settings).

Interesting.  ResourceContext is a hard chunk to bite off since it is
conceptually the IO thread projection of BrowserContext.  However, when I looked
at ResourceContext, I recall thinking that, excluding URLRequestContext, all the
storage related state was only used by 1-2 spots in the Workers implementation. 
I was hoping that instead of introducing something like a StoragePartition
concept onto the IO thread, that we would be able to look at the uses of
URLRequestContext, AppCache, etc., individually on the IO thread and make them
not use ResourceContext.

Is this doable in the AppcCache case?