Redo the Storage Partition directory layout to support guest tags and origin based partitions.

Redo the Storage Partition directory layout to support guest tags and origin based partitions.

The new layout is
    (1) ProfileDir/Default/Storage Partitions/extensions/{ extension_id }/default/
    (2) ProfileDir/Default/Storage Partitions/extensions/{ extension_id }/{ hash(BrowserTag.partition) }/

In the future, after we add support for browser tags in webui or add support for server
administrators of websites to opt into storage isolation, we will also need:

    (3) ProfileDir/Default/Storage Partitions/origins/{ hash(origin) }/default/
    (4) ProfileDir/Default/Storage Partitions/origins/{ hash(origin) }/{ hash(BrowserTag.partition) }/

TBR=willchan
BUG=85121
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157039

[awong, 2012-09-14 07:22:15]

Here's the new format.

FYI, this doesn't lock us into using any particular hash function until we start
supporting persistent partitions for browser tags.

However, when that happens, we will need to cross the bridge of how long our
pathnames can get.  A particular point of concern is Local Storage which
flattens the whole URL into the filename for the sqlite databases.  I worry
about exceeding 260 chars for the full path on windows.

One possibility for keeping the path lengths short is to use a system like
FileSystemContext where they create a little database file that maps random
string (in their case an origin) to a short number.  I think this directory
structure could live in parallel to the "default" subdirectory for extensions.

[awong, 2012-09-14 17:42:27]

Hold off for a bit on this review. I missed a spot.

On 2012/09/14 07:22:15, awong wrote:
> Here's the new format.
> 
> FYI, this doesn't lock us into using any particular hash function until we
start
> supporting persistent partitions for browser tags.
> 
> However, when that happens, we will need to cross the bridge of how long our
> pathnames can get.  A particular point of concern is Local Storage which
> flattens the whole URL into the filename for the sqlite databases.  I worry
> about exceeding 260 chars for the full path on windows.
> 
> One possibility for keeping the path lengths short is to use a system like
> FileSystemContext where they create a little database file that maps random
> string (in their case an origin) to a short number.  I think this directory
> structure could live in parallel to the "default" subdirectory for extensions.

[awong, 2012-09-14 18:32:50]

Charlie, after this set of changes, I think you're the more appropriate
reviewer.  Can you take a look?

[Charlie Reis, 2012-09-14 21:03:13]

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

https://codereview.chromium.org/10913265/diff/10002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:125: // Keep track of isolated
app path and cache sizes separately so we can use
Maybe update this comment as well?

https://codereview.chromium.org/10913265/diff/10002/content/browser/storage_p...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/10913265/diff/10002/content/browser/storage_p...
content/browser/storage_partition_impl.cc:20: // The kStoragePartitionDirname is
subdivided into an extensions
nit: is subdivided into -> contains
(Not much of a subdivision until we later add "origins".)

https://codereview.chromium.org/10913265/diff/10002/content/browser/storage_p...
content/browser/storage_partition_impl.cc:23: // and one directory for each
browser tag. Example:
nit: browser tag -> persistent browser tag partition.

By default, each browser tag will end up in the same partition.

https://codereview.chromium.org/10913265/diff/10002/content/browser/storage_p...
content/browser/storage_partition_impl.cc:25: //  
{kStoragePartitionDirname}/extensions/ABCEDF/default
nit: ABCDEF?  :)

https://codereview.chromium.org/10913265/diff/10002/content/browser/storage_p...
content/browser/storage_partition_impl.cc:32: const char kDefaultDirname[] =
"default";
kDefaultDirname is pretty vague for something in the content namespace.  Should
we have it in a separate namespace for this file, or use something more
descriptive like kDefaultPartitionDirname?

https://codereview.chromium.org/10913265/diff/10002/content/browser/storage_p...
content/browser/storage_partition_impl.cc:37: // The default profile just sits
inside the top-level directory.
nit: top-level profile directory.

https://codereview.chromium.org/10913265/diff/10002/content/browser/storage_p...
content/browser/storage_partition_impl.cc:41: // TODO(ajwong): This should check
the path is valid?
Valid in what sense?  We don't want to go to disk here, presumably.

https://codereview.chromium.org/10913265/diff/10002/content/browser/storage_p...
content/browser/storage_partition_impl.cc:46: .AppendASCII(kDefaultDirname);
Do we expect to have something like a separate GetBrowserTagPartitionPath()
later on, when we don't want to use .../default?

https://codereview.chromium.org/10913265/diff/10002/content/public/browser/st...
File content/public/browser/storage_partition.h (right):

https://codereview.chromium.org/10913265/diff/10002/content/public/browser/st...
content/public/browser/storage_partition.h:51: // Returns the base directory
under which all the storage context hold their
nit: contexts

https://codereview.chromium.org/10913265/diff/10002/content/public/browser/st...
content/public/browser/storage_partition.h:58: const std::string& partition_id);
nit: wrong indent

[awong, 2012-09-14 22:26:06]

ptal

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

http://codereview.chromium.org/10913265/diff/10002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:125: // Keep track of isolated
app path and cache sizes separately so we can use
On 2012/09/14 21:03:13, creis wrote:
> Maybe update this comment as well?

Done.

http://codereview.chromium.org/10913265/diff/10002/content/browser/storage_pa...
File content/browser/storage_partition_impl.cc (right):

http://codereview.chromium.org/10913265/diff/10002/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:20: // The kStoragePartitionDirname is
subdivided into an extensions
On 2012/09/14 21:03:13, creis wrote:
> nit: is subdivided into -> contains
> (Not much of a subdivision until we later add "origins".)

Done.

http://codereview.chromium.org/10913265/diff/10002/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:23: // and one directory for each
browser tag. Example:
On 2012/09/14 21:03:13, creis wrote:
> nit: browser tag -> persistent browser tag partition.
> 
> By default, each browser tag will end up in the same partition.

Done.

http://codereview.chromium.org/10913265/diff/10002/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:25: //  
{kStoragePartitionDirname}/extensions/ABCEDF/default
On 2012/09/14 21:03:13, creis wrote:
> nit: ABCDEF?  :)

Done.

http://codereview.chromium.org/10913265/diff/10002/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:32: const char kDefaultDirname[] =
"default";
On 2012/09/14 21:03:13, creis wrote:
> kDefaultDirname is pretty vague for something in the content namespace. 
Should
> we have it in a separate namespace for this file, or use something more
> descriptive like kDefaultPartitionDirname?

moved all these into an anonymous namespace.   Technically overkill since const
symbols are default internal linkage, but might as well be explicit.

Also called it kDefaultPartitionDirname.  The name still isn't super useful, but
it's better than before.

http://codereview.chromium.org/10913265/diff/10002/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:37: // The default profile just sits
inside the top-level directory.
On 2012/09/14 21:03:13, creis wrote:
> nit: top-level profile directory.

Done.

http://codereview.chromium.org/10913265/diff/10002/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:41: // TODO(ajwong): This should check
the path is valid?
On 2012/09/14 21:03:13, creis wrote:
> Valid in what sense?  We don't want to go to disk here, presumably.

Done.

http://codereview.chromium.org/10913265/diff/10002/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:46: .AppendASCII(kDefaultDirname);
On 2012/09/14 21:03:13, creis wrote:
> Do we expect to have something like a separate GetBrowserTagPartitionPath()
> later on, when we don't want to use .../default?

Nope. I expect this function to get more complicated.

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

http://codereview.chromium.org/10913265/diff/10002/content/public/browser/sto...
content/public/browser/storage_partition.h:51: // Returns the base directory
under which all the storage context hold their
On 2012/09/14 21:03:13, creis wrote:
> nit: contexts

Done.

http://codereview.chromium.org/10913265/diff/10002/content/public/browser/sto...
content/public/browser/storage_partition.h:58: const std::string& partition_id);
On 2012/09/14 21:03:13, creis wrote:
> nit: wrong indent

Done.

[Charlie Reis, 2012-09-14 22:37:31]

LGTM with nits.

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

http://codereview.chromium.org/10913265/diff/2002/content/browser/storage_par...
content/browser/storage_partition_impl.cc:25: // browser tag with persistent
storage. Example:
nit: This still isn't quite right, since you can have multiple browser tags with
the same persistent storage partition.  (It's not per-tag, it's per-partition.)

http://codereview.chromium.org/10913265/diff/2002/content/browser/storage_par...
content/browser/storage_partition_impl.cc:38: FilePath
StoragePartition::GetPartitionPath(const FilePath& profile_path,
nit: // static

[awong, 2012-09-14 23:01:22]

Nits fixed.

mihaip: OWNERS for extensions please? (Also you probably care about the layout)

willchan: ProfileIOData stuff.

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

http://codereview.chromium.org/10913265/diff/2002/content/browser/storage_par...
content/browser/storage_partition_impl.cc:25: // browser tag with persistent
storage. Example:
On 2012/09/14 22:37:31, creis wrote:
> nit: This still isn't quite right, since you can have multiple browser tags
with
> the same persistent storage partition.  (It's not per-tag, it's
per-partition.)

reworded.

http://codereview.chromium.org/10913265/diff/2002/content/browser/storage_par...
content/browser/storage_partition_impl.cc:38: FilePath
StoragePartition::GetPartitionPath(const FilePath& profile_path,
On 2012/09/14 22:37:31, creis wrote:
> nit: // static

Done.

[willchan no longer on Chromium, 2012-09-14 23:52:45]

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

http://codereview.chromium.org/10913265/diff/13002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:518:
content::StoragePartition::GetPartitionPath(profile_path_, app_id);
One thing that was sort of nice about the previous app_path thing is it was very
clearly a subdirectory. Here, this could return anything. Is it possible to get
this to append a relative path, rather than return an absolute path?

[awong, 2012-09-14 23:56:26]

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

http://codereview.chromium.org/10913265/diff/13002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:518:
content::StoragePartition::GetPartitionPath(profile_path_, app_id);
On 2012/09/14 23:52:45, willchan wrote:
> One thing that was sort of nice about the previous app_path thing is it was
very
> clearly a subdirectory. Here, this could return anything. Is it possible to
get
> this to append a relative path, rather than return an absolute path?

Not sure how I'd do that without shifting some of the path creation logic out of
GetPartitionPath().  How is this not a subdirectory path?  I can't see why this
is any different from before...

Note that long term (probably second week of october), I'm going to start
working on removing this set of stuff from ProfileImplIOData.

[willchan no longer on Chromium, 2012-09-15 00:02:54]

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

http://codereview.chromium.org/10913265/diff/13002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:518:
content::StoragePartition::GetPartitionPath(profile_path_, app_id);
On 2012/09/14 23:56:26, awong wrote:
> On 2012/09/14 23:52:45, willchan wrote:
> > One thing that was sort of nice about the previous app_path thing is it was
> very
> > clearly a subdirectory. Here, this could return anything. Is it possible to
> get
> > this to append a relative path, rather than return an absolute path?
> 
> Not sure how I'd do that without shifting some of the path creation logic out
of
> GetPartitionPath().  How is this not a subdirectory path?  I can't see why
this
> is any different from before...

Perhaps I'm confused? In the previous code, we have X = Y + Z;
In the new code, we have X = f(Y,Z);

In the former, X is clearly a subdirectory of Y. In the latter, that's not true.

> 
> Note that long term (probably second week of october), I'm going to start
> working on removing this set of stuff from ProfileImplIOData.

Can you clarify what that means?

[willchan no longer on Chromium, 2012-09-15 00:03:44]

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

http://codereview.chromium.org/10913265/diff/13002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:518:
content::StoragePartition::GetPartitionPath(profile_path_, app_id);
On 2012/09/15 00:02:55, willchan wrote:
> On 2012/09/14 23:56:26, awong wrote:
> > On 2012/09/14 23:52:45, willchan wrote:
> > > One thing that was sort of nice about the previous app_path thing is it
was
> > very
> > > clearly a subdirectory. Here, this could return anything. Is it possible
to
> > get
> > > this to append a relative path, rather than return an absolute path?
> > 
> > Not sure how I'd do that without shifting some of the path creation logic
out
> of
> > GetPartitionPath().  How is this not a subdirectory path?  I can't see why
> this
> > is any different from before...
> 
> Perhaps I'm confused? In the previous code, we have X = Y + Z;
> In the new code, we have X = f(Y,Z);
> 
> In the former, X is clearly a subdirectory of Y. In the latter, that's not
true.

Fyi, I didn't read content::StoragePartition::GetPartitionPath(). So I have no
idea what it does. I looks like some function f() which could return anything.

> 
> > 
> > Note that long term (probably second week of october), I'm going to start
> > working on removing this set of stuff from ProfileImplIOData.
> 
> Can you clarify what that means?

[awong, 2012-09-15 00:08:00]

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

http://codereview.chromium.org/10913265/diff/13002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:518:
content::StoragePartition::GetPartitionPath(profile_path_, app_id);
On 2012/09/15 00:02:55, willchan wrote:
> On 2012/09/14 23:56:26, awong wrote:
> > On 2012/09/14 23:52:45, willchan wrote:
> > > One thing that was sort of nice about the previous app_path thing is it
was
> > very
> > > clearly a subdirectory. Here, this could return anything. Is it possible
to
> > get
> > > this to append a relative path, rather than return an absolute path?
> > 
> > Not sure how I'd do that without shifting some of the path creation logic
out
> of
> > GetPartitionPath().  How is this not a subdirectory path?  I can't see why
> this
> > is any different from before...
> 
> Perhaps I'm confused? In the previous code, we have X = Y + Z;
> In the new code, we have X = f(Y,Z);

Ah, I see what you mean.  I want StoragePartition to own all the logic of how to
figure out where on disk this stuff sits.  Letting ProfileIODataImpl make
statements like the first one, that X is a subdirectory of Y, is an abstraction
break.  Hiding that detail is actually good here IMO.

> In the former, X is clearly a subdirectory of Y. In the latter, that's not
true.
> 
> > 
> > Note that long term (probably second week of october), I'm going to start
> > working on removing this set of stuff from ProfileImplIOData.
> 
> Can you clarify what that means?

As part of StorageIsolation, I want all code to retrieve the
URLRequestContextGetter() via the StoragePartition object.  I also want
StoragePartition to control the lifetime of the various
URLRequestContextGetters.  There's a lot of tangled lifetimes going on here , so
I don't have a completely clear roadmap, but at the end of this all the
following APIs should disappear:
  BrowserContext:GetRequestContext()
  BrowserContext:GetMediaRequestContext()
  BrowserContext:RequestContextForRenderProcess()

Consequently all the supporting logic in ProfileImplIOData (this included)
should go away.

[awong, 2012-09-15 00:11:53]

On 2012/09/15 00:08:00, awong wrote:
>
http://codereview.chromium.org/10913265/diff/13002/chrome/browser/profiles/pr...
> File chrome/browser/profiles/profile_impl_io_data.cc (right):
> 
>
http://codereview.chromium.org/10913265/diff/13002/chrome/browser/profiles/pr...
> chrome/browser/profiles/profile_impl_io_data.cc:518:
> content::StoragePartition::GetPartitionPath(profile_path_, app_id);
> On 2012/09/15 00:02:55, willchan wrote:
> > On 2012/09/14 23:56:26, awong wrote:
> > > On 2012/09/14 23:52:45, willchan wrote:
> > > > One thing that was sort of nice about the previous app_path thing is it
> was
> > > very
> > > > clearly a subdirectory. Here, this could return anything. Is it possible
> to
> > > get
> > > > this to append a relative path, rather than return an absolute path?
> > > 
> > > Not sure how I'd do that without shifting some of the path creation logic
> out
> > of
> > > GetPartitionPath().  How is this not a subdirectory path?  I can't see why
> > this
> > > is any different from before...
> > 
> > Perhaps I'm confused? In the previous code, we have X = Y + Z;
> > In the new code, we have X = f(Y,Z);
> 
> Ah, I see what you mean.  I want StoragePartition to own all the logic of how
to
> figure out where on disk this stuff sits.  Letting ProfileIODataImpl make
> statements like the first one, that X is a subdirectory of Y, is an
abstraction
> break.  Hiding that detail is actually good here IMO.
> 
> > In the former, X is clearly a subdirectory of Y. In the latter, that's not
> true.
> > 
> > > 
> > > Note that long term (probably second week of october), I'm going to start
> > > working on removing this set of stuff from ProfileImplIOData.
> > 
> > Can you clarify what that means?
> 
> As part of StorageIsolation, I want all code to retrieve the
> URLRequestContextGetter() via the StoragePartition object.  I also want
> StoragePartition to control the lifetime of the various
> URLRequestContextGetters.  There's a lot of tangled lifetimes going on here ,
so
> I don't have a completely clear roadmap, but at the end of this all the
> following APIs should disappear:
>   BrowserContext:GetRequestContext()
>   BrowserContext:GetMediaRequestContext()
>   BrowserContext:RequestContextForRenderProcess()
> 
> Consequently all the supporting logic in ProfileImplIOData (this included)
> should go away.

Go away is a strong term.  It should move into the StoragePartition::Create()
method.

[willchan no longer on Chromium, 2012-09-15 00:13:26]

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

http://codereview.chromium.org/10913265/diff/13002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:518:
content::StoragePartition::GetPartitionPath(profile_path_, app_id);
On 2012/09/15 00:08:00, awong wrote:
> On 2012/09/15 00:02:55, willchan wrote:
> > On 2012/09/14 23:56:26, awong wrote:
> > > On 2012/09/14 23:52:45, willchan wrote:
> > > > One thing that was sort of nice about the previous app_path thing is it
> was
> > > very
> > > > clearly a subdirectory. Here, this could return anything. Is it possible
> to
> > > get
> > > > this to append a relative path, rather than return an absolute path?
> > > 
> > > Not sure how I'd do that without shifting some of the path creation logic
> out
> > of
> > > GetPartitionPath().  How is this not a subdirectory path?  I can't see why
> > this
> > > is any different from before...
> > 
> > Perhaps I'm confused? In the previous code, we have X = Y + Z;
> > In the new code, we have X = f(Y,Z);
> 
> Ah, I see what you mean.  I want StoragePartition to own all the logic of how
to
> figure out where on disk this stuff sits.  Letting ProfileIODataImpl make
> statements like the first one, that X is a subdirectory of Y, is an
abstraction
> break.  Hiding that detail is actually good here IMO.

Can you clarify this? Don't all storage partitions belong to some profile? If
that's the case, shouldn't all storage partitions (X) be in a subdirectory under
the profile path (Y)?

I guess I'm surprised it's not something like X = Y + f(Y,Z).

> 
> > In the former, X is clearly a subdirectory of Y. In the latter, that's not
> true.
> > 
> > > 
> > > Note that long term (probably second week of october), I'm going to start
> > > working on removing this set of stuff from ProfileImplIOData.
> > 
> > Can you clarify what that means?
> 
> As part of StorageIsolation, I want all code to retrieve the
> URLRequestContextGetter() via the StoragePartition object.  I also want
> StoragePartition to control the lifetime of the various
> URLRequestContextGetters.  There's a lot of tangled lifetimes going on here ,
so
> I don't have a completely clear roadmap, but at the end of this all the
> following APIs should disappear:
>   BrowserContext:GetRequestContext()
>   BrowserContext:GetMediaRequestContext()
>   BrowserContext:RequestContextForRenderProcess()
> 
> Consequently all the supporting logic in ProfileImplIOData (this included)
> should go away.

[awong, 2012-09-15 00:43:07]

PTAL

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

http://codereview.chromium.org/10913265/diff/13002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl_io_data.cc:518:
content::StoragePartition::GetPartitionPath(profile_path_, app_id);
On 2012/09/15 00:13:26, willchan wrote:
> On 2012/09/15 00:08:00, awong wrote:
> > On 2012/09/15 00:02:55, willchan wrote:
> > > On 2012/09/14 23:56:26, awong wrote:
> > > > On 2012/09/14 23:52:45, willchan wrote:
> > > > > One thing that was sort of nice about the previous app_path thing is
it
> > was
> > > > very
> > > > > clearly a subdirectory. Here, this could return anything. Is it
possible
> > to
> > > > get
> > > > > this to append a relative path, rather than return an absolute path?
> > > > 
> > > > Not sure how I'd do that without shifting some of the path creation
logic
> > out
> > > of
> > > > GetPartitionPath().  How is this not a subdirectory path?  I can't see
why
> > > this
> > > > is any different from before...
> > > 
> > > Perhaps I'm confused? In the previous code, we have X = Y + Z;
> > > In the new code, we have X = f(Y,Z);
> > 
> > Ah, I see what you mean.  I want StoragePartition to own all the logic of
how
> to
> > figure out where on disk this stuff sits.  Letting ProfileIODataImpl make
> > statements like the first one, that X is a subdirectory of Y, is an
> abstraction
> > break.  Hiding that detail is actually good here IMO.
> 
> Can you clarify this? Don't all storage partitions belong to some profile? If
> that's the case, shouldn't all storage partitions (X) be in a subdirectory
under
> the profile path (Y)?
> 
> I guess I'm surprised it's not something like X = Y + f(Y,Z).

Updated to be a relative path.


> > > In the former, X is clearly a subdirectory of Y. In the latter, that's not
> > true.
> > > 
> > > > 
> > > > Note that long term (probably second week of october), I'm going to
start
> > > > working on removing this set of stuff from ProfileImplIOData.
> > > 
> > > Can you clarify what that means?
> > 
> > As part of StorageIsolation, I want all code to retrieve the
> > URLRequestContextGetter() via the StoragePartition object.  I also want
> > StoragePartition to control the lifetime of the various
> > URLRequestContextGetters.  There's a lot of tangled lifetimes going on here
,
> so
> > I don't have a completely clear roadmap, but at the end of this all the
> > following APIs should disappear:
> >   BrowserContext:GetRequestContext()
> >   BrowserContext:GetMediaRequestContext()
> >   BrowserContext:RequestContextForRenderProcess()
> > 
> > Consequently all the supporting logic in ProfileImplIOData (this included)
> > should go away.
>

[Mihai Parparita -not on Chrome, 2012-09-15 01:26:25]

LGTM

[awong, 2012-09-15 01:28:57]

Will seems to have gone home for the weekend, but I'm pretty confident I
addressed his concern so I'm going to move him to TBR.

Will, if on monday you see a want for more changes, let me know and I'll get
them addressed ASAP.

[commit-bot: I haz the power, 2012-09-15 01:29:08]

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

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

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

[commit-bot: I haz the power, 2012-09-15 02:31:25]

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

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

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

[commit-bot: I haz the power, 2012-09-15 05:18:51]

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

[commit-bot: I haz the power, 2012-09-15 05:18:54]

Failed to apply patch for content/browser/storage_partition_impl_map.cc:
While running patch -p1 --forward --force;
patching file content/browser/storage_partition_impl_map.cc
Hunk #1 FAILED at 37.
Hunk #2 FAILED at 72.
Hunk #3 FAILED at 81.
3 out of 3 hunks FAILED -- saving rejects to file
content/browser/storage_partition_impl_map.cc.rej

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

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

[commit-bot: I haz the power, 2012-09-15 06:20:31]

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

[commit-bot: I haz the power, 2012-09-15 06:29:00]

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

[commit-bot: I haz the power, 2012-09-15 07:48:30]

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

[commit-bot: I haz the power, 2012-09-15 14:42:46]

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

[commit-bot: I haz the power, 2012-09-15 15:18:43]

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

[willchan no longer on Chromium, 2012-09-15 19:47:42]

On 2012/09/15 01:28:57, awong wrote:
> Will seems to have gone home for the weekend, but I'm pretty confident I
> addressed his concern so I'm going to move him to TBR.
> 
> Will, if on monday you see a want for more changes, let me know and I'll get
> them addressed ASAP.

No, LGTM. Btw, I hope that my comments were reasonable and you weren't just too
tired to discuss :) If I misunderstood stuff, I'd like to understand later on.

[awong, 2012-09-16 01:04:44]

On Sat, Sep 15, 2012 at 12:47 PM, <willchan@chromium.org> wrote:

> On 2012/09/15 01:28:57, awong wrote:
>
>> Will seems to have gone home for the weekend, but I'm pretty confident I
>> addressed his concern so I'm going to move him to TBR.
>>
>
>  Will, if on monday you see a want for more changes, let me know and I'll
>> get
>> them addressed ASAP.
>>
>
> No, LGTM. Btw, I hope that my comments were reasonable and you weren't
> just too
> tired to discuss :) If I misunderstood stuff, I'd like to understand later
> on.
>

Once I understood what you were saying, it made much more sense.  Also, I
really hope to remove this stuff from ProfileIOData soon so it hopefully
won't matter anyways :)


-Albert


>
>
https://chromiumcodereview.**appspot.com/10913265/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2012-09-16 01:05:01]

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

[commit-bot: I haz the power, 2012-09-16 04:01:10]

Change committed as 157039