Webview tag creation should be using storage partitions.

Webview tag creation should be using storage partitions.

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

[nasko, 2012-11-02 18:13:43]

Hey guys,
Let's start the review on this CL.
Thanks,
Nasko

[awong, 2012-11-02 21:56:13]

Looking pretty good.  Most (all?) of my comments are style related.

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

http://codereview.chromium.org/11234032/diff/19009/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:491: //
"app_id:in_memory:partition_name", where each of the three parts is
In partition_id, we tag "in_memory" whereas in guest site, we tag "persistent". 
Do we want to unify? Does it matter?

http://codereview.chromium.org/11234032/diff/19009/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:519: StringTokenizer
t(partition_id, ":");
What about just doing:
 vector<std::string> parts;
 StringSplit(partition_id, ":", &parts);
 if (parts.size() != 3) {
   NOTREACHED();
   return false;
 }

  if (!parts[0].empty()) {
    // Code to test for the extension name.
  }

  if (!parts[1].empty() && parts[1] != "in-memory") {
    ...
  }


That seems simpler.

BTW, are we certain that we've disallowed embedded :s in the partition_name?

http://codereview.chromium.org/11234032/diff/19009/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:564: bool* in_memory) {
We need a highlevel comment that either explains what guests are versus
extensions and why in_memory is only sane for guest, or points to overview
documentation for browser tag that can let a reader explain this.

Otherwise, random engineer is going to be confused to death by this function and
will have no way to tell if the various assignment logic is.

This also makes me wonder, is there a unittest for this class?  Should we create
one?

http://codereview.chromium.org/11234032/diff/19009/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:569: net::UnescapeRule::NORMAL);
Add comment that tells where to look for the authoritative location that defines
the format of this site.

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

http://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:32: void
NavigateAndOpenAppForIsolation(
FYI, if you do this kind of pattern in the future, remember that EXPECT_*
doesn't show a stack.

Thus if you had done something like:

TEST_F(Foo, Bar) {
  // Call the helper with EXPECT_* multiple times)
  NavigateAndOpenAppForIsolation();
   ...
  NavigateAndOpenAppForIsolation();
}

when the expect fails, you'd have no clue which invocation caused it.

In those cases, adding a SCOPED_TRACE() helps.

This particular instance is safe cause you only call this function once per
TEST_F, but I wanted to point it out.

http://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:37: content::WebContents**
storage_contents2) {
Function needs top level comment.

http://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:106: 
one newline is enough

http://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:123:
EXPECT_TRUE(content::ExecuteJavaScript(web_contents->GetRenderViewHost(),
IIRC, ExecuteJavaScript() doesn't need the domAutomationContoller.send(). 
That's only needed if you call the functions that read a value.  Try it and see
if the test hangs or not?

http://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:193: // The third tag should
not have any cookies, as it is in separate partition.
s/,//

http://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:220:
EXPECT_TRUE(content::ExecuteJavaScript(
Same comment about domAutomationController().

Someone really needs to go fix content::ExecuteJSAndExtractXXX to do that
wrapping for you.

http://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:224: // Let's test the
expected values are present.
Useless comment. Describe why not what or don't describe at all. :)

http://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:234: 
This vertical spacing makes it really hard for me to see the various test
sections.  Maybe this is a case where 2 newlines helps.  Or possible use a
little ascii art in the comment?  Or maybe remove the newline bewteen the
related chunks above?

http://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:272: EXPECT_STREQ("badval",
output.c_str());
This particular test is really hard to undersatnd for some reason. It might be
partly spacing related, but I don't know what the basic structure (which tabs
are testing what?) is and what dimensions you are playing with.  No concrete
suggestion on how to solve though...

http://codereview.chromium.org/11234032/diff/19009/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js
(right):

http://codereview.chromium.org/11234032/diff/19009/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js:4: 
Can we add a comment explaining the basic action/output structure of this test
so you don't have to read all the code?

I'm thinking something that explain that a call to initIDB() should result in
"idb created" int he title bar or similar.

http://codereview.chromium.org/11234032/diff/19009/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js:9: 
whack extra newline.

http://codereview.chromium.org/11234032/diff/19009/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js:21:
request.onsuccess = function(e) {
holy crap this is a complicated API.

http://codereview.chromium.org/11234032/diff/19009/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js:70: }
need ; to be safe

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

http://codereview.chromium.org/11234032/diff/19009/content/browser/browser_co...
content/browser/browser_context.cc:40: StoragePartition*
GetStoragePartitionByPartitionDescription(
Name is too long.  This is just a helper so it's almost better to name it
GetStoragePartitionHelper().

At least it's easier to read and you don't gain much for saying it's by
PartitionDescription.

http://codereview.chromium.org/11234032/diff/19009/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/11234032/diff/19009/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:100: // being
navigated to.
Is this logic better here, or in a SiteInstance::CreateForGuest() function?

http://codereview.chromium.org/11234032/diff/19009/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:103: host.c_str(),
persist_storage ? "persist" : "",
put persist into constant?

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

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:49: FilePath::StringType domain =
descriptor.partition_domain;
Do we know if partition_domain, etc., are UTF-8?  (Hopefully we can guarantee
this).  If so, what about just doing .Append(FilePath::FromUTF8Unsafe(domain))
below and dropping the #defines?

The #defines are kinda semantically wrong anyways since what we should be keying
off of is whether or not StringType is wchar_t.  Thus, it's nicer if we can hide
behind an abstraction layer rather than fork the type switching logic.

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:61: return
path.Append(kDefaultPartitionDirname);
Drop this out of the else {} clause and remove the NOTREACHED().

The return on condition, and fall through to default is the more common paradigm
in chrome.

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

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl.h:29: // the path produced by combining
the two pieces together.
Cite bug 159193 as the fix for this forkage.

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl.h:41: };
need newline.

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl.h:43: struct
StoragePartitionDescriptorLess {
Can this type be private?

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl.h:63: const StoragePartitionDescriptor&
partition_id,
What's your thoughts on hiding the Descriptor struct completely from the public
API?  Cleaner or not cleaner?

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

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:203: if
(!resource_context_initialized_) {
If you're planning on moving out this, add TODO?

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:208: bool in_memory_only = false;
why not:

bool in_memory_only = in_memory || browser_context->IsOffTheRecord();

?

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:215: if (!partition_domain.empty()
&& !partition_name.empty())
Maybe move this into the calling function?

That way this function doesn't need to know about parsing the various names.

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
File content/browser/storage_partition_impl_map.h (right):

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl_map.h:32: const bool& in_memory);
don't pass primitives by const&!!!!!!!

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl_map.h:51: typedef
std::map<StoragePartitionImpl::StoragePartitionDescriptor,
typedefs should go at the start of their private:/public: section.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
File content/browser/storage_partition_impl_unittest.cc (right):

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl_unittest.cc:8: using namespace content;
using namespace is disallowed.

Usually I stuff the whole test into namespace content {} and posssible

namespace content { namespace{} } if I don't need to friend it.

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl_unittest.cc:16:
StoragePartitionImpl::StoragePartitionDescriptor d1("", "", false);
Wow...this is a very complete test. :)

FYI, it's probably overkill, but since you've done it, I'm happy it's here. :)

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl_unittest.cc:28: // Let's ensure basic
comparison works
period at the end of the sentence.

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl_unittest.cc:48: // Check for
irreflexivity
.

http://codereview.chromium.org/11234032/diff/19009/content/browser/storage_pa...
content/browser/storage_partition_impl_unittest.cc:51: // Check for transitivity
.

http://codereview.chromium.org/11234032/diff/19009/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/11234032/diff/19009/content/public/browser/con...
content/public/browser/content_browser_client.h:276: // Allow the embedder to
provide storage parititon description associated with
Description makes me think it's a user-visible string.

How about another name like Properties, Attributes, Configuration, etc?

[nasko, 2012-11-03 00:36:24]

I've made changes for the issues Albert raised. I didn't address all of the ones
in the web_view_browsertest, which I'll do on Monday.

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:491: //
"app_id:in_memory:partition_name", where each of the three parts is
On 2012/11/02 21:56:13, awong wrote:
> In partition_id, we tag "in_memory" whereas in guest site, we tag
"persistent". 
> Do we want to unify? Does it matter?

There is one time bit flip at the time the embedder creates the URL where we
transition from the "persist" term, which is based on the attribute to the
"in_memory", which is used all through content/chrome.
I don't see it as a big priority to use in_memory throughout the two parts.

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

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:106: 
On 2012/11/02 21:56:13, awong wrote:
> one newline is enough

Done.

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:193: // The third tag should
not have any cookies, as it is in separate partition.
On 2012/11/02 21:56:13, awong wrote:
> s/,//

Done.

https://codereview.chromium.org/11234032/diff/19009/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js
(right):

https://codereview.chromium.org/11234032/diff/19009/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js:4: 
On 2012/11/02 21:56:13, awong wrote:
> Can we add a comment explaining the basic action/output structure of this test
> so you don't have to read all the code?
> 
> I'm thinking something that explain that a call to initIDB() should result in
> "idb created" int he title bar or similar.

Done.

https://codereview.chromium.org/11234032/diff/19009/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js:9: 
On 2012/11/02 21:56:13, awong wrote:
> whack extra newline.

Done.

https://codereview.chromium.org/11234032/diff/19009/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js:21:
request.onsuccess = function(e) {
On 2012/11/02 21:56:13, awong wrote:
> holy crap this is a complicated API.

: )

https://codereview.chromium.org/11234032/diff/19009/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js:70: }
On 2012/11/02 21:56:13, awong wrote:
> need ; to be safe

Done.

https://codereview.chromium.org/11234032/diff/19009/content/browser/browser_c...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/11234032/diff/19009/content/browser/browser_c...
content/browser/browser_context.cc:40: StoragePartition*
GetStoragePartitionByPartitionDescription(
On 2012/11/02 21:56:13, awong wrote:
> Name is too long.  This is just a helper so it's almost better to name it
> GetStoragePartitionHelper().
> 
> At least it's easier to read and you don't gain much for saying it's by
> PartitionDescription.

Done.

https://codereview.chromium.org/11234032/diff/19009/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11234032/diff/19009/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:100: // being
navigated to.
On 2012/11/02 21:56:13, awong wrote:
> Is this logic better here, or in a SiteInstance::CreateForGuest() function?

Since we use it only here, I'd rather not introduce new method with new concept
in SiteInstance. Trying to keep jam@'s spirit of simple content API :).

https://codereview.chromium.org/11234032/diff/19009/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:103: host.c_str(),
persist_storage ? "persist" : "",
On 2012/11/02 21:56:13, awong wrote:
> put persist into constant?

If I put it into a constant, I have to do a lookup what the constant resolves
to. Since it is used only here, it seems more readable to me.

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

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl.cc:49: FilePath::StringType domain =
descriptor.partition_domain;
On 2012/11/02 21:56:13, awong wrote:
> Do we know if partition_domain, etc., are UTF-8?  (Hopefully we can guarantee
> this).  If so, what about just doing .Append(FilePath::FromUTF8Unsafe(domain))
> below and dropping the #defines?
> 
> The #defines are kinda semantically wrong anyways since what we should be
keying
> off of is whether or not StringType is wchar_t.  Thus, it's nicer if we can
hide
> behind an abstraction layer rather than fork the type switching logic.

Yes, the partition_domain is even ASCII, since it is the id for the
app/extension. The partition_name is UTF-8. I've added CHECK in the embedder to
enforce this.

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl.cc:61: return
path.Append(kDefaultPartitionDirname);
On 2012/11/02 21:56:13, awong wrote:
> Drop this out of the else {} clause and remove the NOTREACHED().
> 
> The return on condition, and fall through to default is the more common
paradigm
> in chrome.

Done.

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

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl.h:29: // the path produced by combining
the two pieces together.
On 2012/11/02 21:56:13, awong wrote:
> Cite bug 159193 as the fix for this forkage.

Done.

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl.h:41: };
On 2012/11/02 21:56:13, awong wrote:
> need newline.

Done.

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl.h:43: struct
StoragePartitionDescriptorLess {
On 2012/11/02 21:56:13, awong wrote:
> Can this type be private?

Done.

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl.h:63: const StoragePartitionDescriptor&
partition_id,
On 2012/11/02 21:56:13, awong wrote:
> What's your thoughts on hiding the Descriptor struct completely from the
public
> API?  Cleaner or not cleaner?

Create is only called by SPIM::Get, so it is not really "public". Now that
you've mentioned it, since SPIM is a friend to this class, I've made this method
private.

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

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:203: if
(!resource_context_initialized_) {
On 2012/11/02 21:56:13, awong wrote:
> If you're planning on moving out this, add TODO?

Meant to keep the original TODO. Thanks for the catch!

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:208: bool in_memory_only = false;
On 2012/11/02 21:56:13, awong wrote:
> why not:
> 
> bool in_memory_only = in_memory || browser_context->IsOffTheRecord();
> 
> ?

Done.

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:215: if (!partition_domain.empty()
&& !partition_name.empty())
On 2012/11/02 21:56:13, awong wrote:
> Maybe move this into the calling function?
> 
> That way this function doesn't need to know about parsing the various names.

The caller of this is BrowserContext. Since the changes for on-disk storage are
more close to the actual storage partition than BrowserContext, I chose to keep
it here.

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
File content/browser/storage_partition_impl_map.h (right):

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:32: const bool& in_memory);
On 2012/11/02 21:56:13, awong wrote:
> don't pass primitives by const&!!!!!!!

Done.

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:51: typedef
std::map<StoragePartitionImpl::StoragePartitionDescriptor,
On 2012/11/02 21:56:13, awong wrote:
> typedefs should go at the start of their private:/public: section.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
File content/browser/storage_partition_impl_unittest.cc (right):

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl_unittest.cc:8: using namespace content;
On 2012/11/02 21:56:13, awong wrote:
> using namespace is disallowed.
> 
> Usually I stuff the whole test into namespace content {} and posssible
> 
> namespace content { namespace{} } if I don't need to friend it.

Done.

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl_unittest.cc:16:
StoragePartitionImpl::StoragePartitionDescriptor d1("", "", false);
On 2012/11/02 21:56:13, awong wrote:
> Wow...this is a very complete test. :)
> 
> FYI, it's probably overkill, but since you've done it, I'm happy it's here. :)

After chasing bugs in the previous CL due to bugs in the comparator, I don't
want to repeat the same mistake :)

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl_unittest.cc:28: // Let's ensure basic
comparison works
On 2012/11/02 21:56:13, awong wrote:
> period at the end of the sentence.

Done.

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl_unittest.cc:48: // Check for
irreflexivity
On 2012/11/02 21:56:13, awong wrote:
> .

Done.

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl_unittest.cc:51: // Check for transitivity
On 2012/11/02 21:56:13, awong wrote:
> .

Done.

https://codereview.chromium.org/11234032/diff/19009/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11234032/diff/19009/content/public/browser/co...
content/public/browser/content_browser_client.h:276: // Allow the embedder to
provide storage parititon description associated with
On 2012/11/02 21:56:13, awong wrote:
> Description makes me think it's a user-visible string.
> 
> How about another name like Properties, Attributes, Configuration, etc?

Done.

[nasko, 2012-11-05 17:37:11]

I've made the fixes for the remaining comments in the browser test. I hope this
makes it better.

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

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:32: void
NavigateAndOpenAppForIsolation(
On 2012/11/02 21:56:13, awong wrote:
> FYI, if you do this kind of pattern in the future, remember that EXPECT_*
> doesn't show a stack.
> 
> Thus if you had done something like:
> 
> TEST_F(Foo, Bar) {
>   // Call the helper with EXPECT_* multiple times)
>   NavigateAndOpenAppForIsolation();
>    ...
>   NavigateAndOpenAppForIsolation();
> }
> 
> when the expect fails, you'd have no clue which invocation caused it.
> 
> In those cases, adding a SCOPED_TRACE() helps.
> 
> This particular instance is safe cause you only call this function once per
> TEST_F, but I wanted to point it out.

Yes, you've pointed it out in the past. The main goal of this method is to do
initialization, so I don't forsee it being called multiple times.

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:37: content::WebContents**
storage_contents2) {
On 2012/11/02 21:56:13, awong wrote:
> Function needs top level comment.

Done.

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:123:
EXPECT_TRUE(content::ExecuteJavaScript(web_contents->GetRenderViewHost(),
On 2012/11/02 21:56:13, awong wrote:
> IIRC, ExecuteJavaScript() doesn't need the domAutomationContoller.send(). 
> That's only needed if you call the functions that read a value.  Try it and
see
> if the test hangs or not?

Done.

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:220:
EXPECT_TRUE(content::ExecuteJavaScript(
On 2012/11/02 21:56:13, awong wrote:
> Same comment about domAutomationController().
> 
> Someone really needs to go fix content::ExecuteJSAndExtractXXX to do that
> wrapping for you.

This one doesn't use domAutomationController, does it? It passes a literal
string "initDomStorage('page1')".

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:224: // Let's test the
expected values are present.
On 2012/11/02 21:56:13, awong wrote:
> Useless comment. Describe why not what or don't describe at all. :)

Done.

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:234: 
On 2012/11/02 21:56:13, awong wrote:
> This vertical spacing makes it really hard for me to see the various test
> sections.  Maybe this is a case where 2 newlines helps.  Or possible use a
> little ascii art in the comment?  Or maybe remove the newline bewteen the
> related chunks above?

Done.

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:272: EXPECT_STREQ("badval",
output.c_str());
On 2012/11/02 21:56:13, awong wrote:
> This particular test is really hard to undersatnd for some reason. It might be
> partly spacing related, but I don't know what the basic structure (which tabs
> are testing what?) is and what dimensions you are playing with.  No concrete
> suggestion on how to solve though...

Tried to bundle things together with comments a bit improved. Hopefully that
helps.

[awong, 2012-11-05 18:00:33]

Didn't quite finish the review, but need to run to interview.  Here's the first
1/2 of the comments

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

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:220:
EXPECT_TRUE(content::ExecuteJavaScript(
On 2012/11/05 17:37:11, nasko wrote:
> On 2012/11/02 21:56:13, awong wrote:
> > Same comment about domAutomationController().
> > 
> > Someone really needs to go fix content::ExecuteJSAndExtractXXX to do that
> > wrapping for you.
> 
> This one doesn't use domAutomationController, does it? It passes a literal
> string "initDomStorage('page1')".

Oh right...I misread.

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

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl.cc:49: FilePath::StringType domain =
descriptor.partition_domain;
On 2012/11/03 00:36:24, nasko wrote:
> On 2012/11/02 21:56:13, awong wrote:
> > Do we know if partition_domain, etc., are UTF-8?  (Hopefully we can
guarantee
> > this).  If so, what about just doing
.Append(FilePath::FromUTF8Unsafe(domain))
> > below and dropping the #defines?
> > 
> > The #defines are kinda semantically wrong anyways since what we should be
> keying
> > off of is whether or not StringType is wchar_t.  Thus, it's nicer if we can
> hide
> > behind an abstraction layer rather than fork the type switching logic.
> 
> Yes, the partition_domain is even ASCII, since it is the id for the
> app/extension. The partition_name is UTF-8. I've added CHECK in the embedder
to
> enforce this.

Do we have a check that enforces ascii and is it important to keep it that
strict?  If not, I'd stick with UTF-8 so the string encodings are uniform across
the different arguments.

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

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:215: if (!partition_domain.empty()
&& !partition_name.empty())
On 2012/11/03 00:36:24, nasko wrote:
> On 2012/11/02 21:56:13, awong wrote:
> > Maybe move this into the calling function?
> > 
> > That way this function doesn't need to know about parsing the various names.
> 
> The caller of this is BrowserContext. Since the changes for on-disk storage
are
> more close to the actual storage partition than BrowserContext, I chose to
keep
> it here.

But this way the API is more complicated because the caller has doesn't quite
know whether in_memory will be changed out from under neath it.

I think it's cleaner to move all this in_memory resolution logic out of Get() so
Get() can stick with the simple rule of "whatever its arguments say, it's going
to do" ratehr than causing a behavior switch based on a member variable...

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

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:208: // This test DOM storage
isolation for packaged apps with webview tags. It loads
This test -> This tests

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:235: // Let's test the
expected values are present in the first tag, as they will
test the -> test that the

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:295: // an app with multiple
webview tags and each tag creates IndexedDB record,
creates -> creates an

https://codereview.chromium.org/11234032/diff/26009/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js
(right):

https://codereview.chromium.org/11234032/diff/26009/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js:12: //
The test uses three basic operations - open, read, write to verify proper
s/- open, read, write/-- open, read, write --/

[nasko, 2012-11-05 18:09:06]

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

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:208: // This test DOM storage
isolation for packaged apps with webview tags. It loads
On 2012/11/05 18:00:33, awong wrote:
> This test -> This tests

Done.

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:235: // Let's test the
expected values are present in the first tag, as they will
On 2012/11/05 18:00:33, awong wrote:
> test the -> test that the

Done.

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:295: // an app with multiple
webview tags and each tag creates IndexedDB record,
On 2012/11/05 18:00:33, awong wrote:
> creates -> creates an

Done.

https://codereview.chromium.org/11234032/diff/26009/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js
(right):

https://codereview.chromium.org/11234032/diff/26009/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js:12: //
The test uses three basic operations - open, read, write to verify proper
On 2012/11/05 18:00:33, awong wrote:
> s/- open, read, write/-- open, read, write --/

Done.

[Charlie Reis, 2012-11-05 18:15:37]

I started to review, but I might need to read the whole discussion to catch up. 
Can you answer the question about the x:y:z format below first?  Thanks!

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:478: bool in_memory;
Should probably initialize this, in case a bug later crops up in
GetStoragePartitionConfigForSite (e.g., return without setting it).

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:491: //
"app_id:in_memory:partition_name", where each of the three parts is
I'm not entirely clear on why we need this format.  I thought we were using site
URLs because they already know how to parse what we need.  (Seems safer to avoid
adding a custom parser to the browser process if we can.)

Wouldn't the site URL have enough information?

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:493: // string and the app_id
cannot contain the separator, it is safe to parse
What are the restrictions on the partition_name?  Can it contain the separator? 
(I suppose you could still safely parse it, but it's not as obvious by looking
at it.)

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:565: if
(site.SchemeIs(chrome::kGuestScheme)) {
Please add some comments for each of these blocks, plus an example guest URL to
show what you're pulling out of it.

[nasko, 2012-11-05 19:20:26]

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:478: bool in_memory;
On 2012/11/05 18:15:37, creis wrote:
> Should probably initialize this, in case a bug later crops up in
> GetStoragePartitionConfigForSite (e.g., return without setting it).

Done.

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:491: //
"app_id:in_memory:partition_name", where each of the three parts is
On 2012/11/05 18:15:37, creis wrote:
> I'm not entirely clear on why we need this format.  I thought we were using
site
> URLs because they already know how to parse what we need.  (Seems safer to
avoid
> adding a custom parser to the browser process if we can.)
> 
> Wouldn't the site URL have enough information?

I've had the same discussion with Albert. He feels strongly against using the
URL as identifier, as different parts of the code may start depending on the
fact that it is URL of specific format and start taking parts of it. This is the
reason for this separate way to identify them.

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:493: // string and the app_id
cannot contain the separator, it is safe to parse
On 2012/11/05 18:15:37, creis wrote:
> What are the restrictions on the partition_name?  Can it contain the
separator? 
> (I suppose you could still safely parse it, but it's not as obvious by looking
> at it.)

I do have a bug here, the user supplied partition value doesn't have any
limitations at this point in time. I have to account for that in the parsing and
not expect exactly 2 separators.

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:565: if
(site.SchemeIs(chrome::kGuestScheme)) {
On 2012/11/05 18:15:37, creis wrote:
> Please add some comments for each of these blocks, plus an example guest URL
to
> show what you're pulling out of it.

Done.

[awong, 2012-11-05 20:11:18]

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:519: StringTokenizer
t(partition_id, ":");
On 2012/11/02 21:56:13, awong wrote:
> What about just doing:
>  vector<std::string> parts;
>  StringSplit(partition_id, ":", &parts);
>  if (parts.size() != 3) {
>    NOTREACHED();
>    return false;
>  }
> 
>   if (!parts[0].empty()) {
>     // Code to test for the extension name.
>   }
> 
>   if (!parts[1].empty() && parts[1] != "in-memory") {
>     ...
>   }
> 
> 
> That seems simpler.
> 
> BTW, are we certain that we've disallowed embedded :s in the partition_name?

Is there a reason to prefer the tokenizer?

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

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:215: if (!partition_domain.empty()
&& !partition_name.empty())
On 2012/11/05 18:00:33, awong wrote:
> On 2012/11/03 00:36:24, nasko wrote:
> > On 2012/11/02 21:56:13, awong wrote:
> > > Maybe move this into the calling function?
> > > 
> > > That way this function doesn't need to know about parsing the various
names.
> > 
> > The caller of this is BrowserContext. Since the changes for on-disk storage
> are
> > more close to the actual storage partition than BrowserContext, I chose to
> keep
> > it here.
> 
> But this way the API is more complicated because the caller has doesn't quite
> know whether in_memory will be changed out from under neath it.
> 
> I think it's cleaner to move all this in_memory resolution logic out of Get()
so
> Get() can stick with the simple rule of "whatever its arguments say, it's
going
> to do" ratehr than causing a behavior switch based on a member variable...

For instance, doesn't content browser client ultimately own this logic?

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/26009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:491: //
"app_id:in_memory:partition_name", where each of the three parts is
On 2012/11/05 19:20:26, nasko wrote:
> On 2012/11/05 18:15:37, creis wrote:
> > I'm not entirely clear on why we need this format.  I thought we were using
> site
> > URLs because they already know how to parse what we need.  (Seems safer to
> avoid
> > adding a custom parser to the browser process if we can.)
> > 
> > Wouldn't the site URL have enough information?
> 
> I've had the same discussion with Albert. He feels strongly against using the
> URL as identifier, as different parts of the code may start depending on the
> fact that it is URL of specific format and start taking parts of it. This is
the
> reason for this separate way to identify them.

My objection was that GURL had too much validation logic. Could be convinced
otherwise though.

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:474: content::BrowserContext*
browser_context,
4-spaces, not 6.

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:492: // optional and the ':'
separators are mandatory. Since "in_memory" is fixed
in-memory

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:569: *in_memory = ((site.path()
== "/persist") ? false : true);
*in_memory = site.path() != "/persist";

[nasko, 2012-11-05 23:06:58]

Missed one file in the earlier iterations, now fixed.

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:519: StringTokenizer
t(partition_id, ":");
On 2012/11/05 20:11:18, awong wrote:
> On 2012/11/02 21:56:13, awong wrote:
> > What about just doing:
> >  vector<std::string> parts;
> >  StringSplit(partition_id, ":", &parts);
> >  if (parts.size() != 3) {
> >    NOTREACHED();
> >    return false;
> >  }
> > 
> >   if (!parts[0].empty()) {
> >     // Code to test for the extension name.
> >   }
> > 
> >   if (!parts[1].empty() && parts[1] != "in-memory") {
> >     ...
> >   }
> > 
> > 
> > That seems simpler.
> > 
> > BTW, are we certain that we've disallowed embedded :s in the partition_name?
> 
> Is there a reason to prefer the tokenizer?

Done.

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:564: bool* in_memory) {
On 2012/11/02 21:56:13, awong wrote:
> We need a highlevel comment that either explains what guests are versus
> extensions and why in_memory is only sane for guest, or points to overview
> documentation for browser tag that can let a reader explain this.
> 
> Otherwise, random engineer is going to be confused to death by this function
and
> will have no way to tell if the various assignment logic is.
> 
> This also makes me wonder, is there a unittest for this class?  Should we
create
> one?

Done.

https://codereview.chromium.org/11234032/diff/19009/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:569: net::UnescapeRule::NORMAL);
On 2012/11/02 21:56:13, awong wrote:
> Add comment that tells where to look for the authoritative location that
defines
> the format of this site.

I've added a verbose comment above, which describes the format.

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

https://codereview.chromium.org/11234032/diff/19009/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:215: if (!partition_domain.empty()
&& !partition_name.empty())
On 2012/11/05 20:11:18, awong wrote:
> On 2012/11/05 18:00:33, awong wrote:
> > On 2012/11/03 00:36:24, nasko wrote:
> > > On 2012/11/02 21:56:13, awong wrote:
> > > > Maybe move this into the calling function?
> > > > 
> > > > That way this function doesn't need to know about parsing the various
> names.
> > > 
> > > The caller of this is BrowserContext. Since the changes for on-disk
storage
> > are
> > > more close to the actual storage partition than BrowserContext, I chose to
> > keep
> > > it here.
> > 
> > But this way the API is more complicated because the caller has doesn't
quite
> > know whether in_memory will be changed out from under neath it.
> > 
> > I think it's cleaner to move all this in_memory resolution logic out of
Get()
> so
> > Get() can stick with the simple rule of "whatever its arguments say, it's
> going
> > to do" ratehr than causing a behavior switch based on a member variable...
> 
> For instance, doesn't content browser client ultimately own this logic?

This is temporary until you fix the on-disk code, so I don't think it makes
really big difference where it lives. I'll move it over to the content client.

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:474: content::BrowserContext*
browser_context,
On 2012/11/05 20:11:18, awong wrote:
> 4-spaces, not 6.

Done.

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:492: // optional and the ':'
separators are mandatory. Since "in_memory" is fixed
On 2012/11/05 20:11:18, awong wrote:
> in-memory

Done.

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:569: *in_memory = ((site.path()
== "/persist") ? false : true);
On 2012/11/05 20:11:18, awong wrote:
> *in_memory = site.path() != "/persist";

Done.

[awong, 2012-11-05 23:48:36]

Couple more minor comments.  Let's talk out the partition_id formatting
tomorrow.

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:494: // the string based on the
two separators.
Question: How strong is our belief that |app_id|will not contain colons, and
what is the damage if one slips through?

Is it strong enough that we don't need a CHECK() for assertion?

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:515: if (tokens.size() < 3) {
Should it be exactly 3?

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:577: *partition_name =
std::string();
partition_name->clear()?

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:583: *app_id = std::string();
->clear(), here and below?

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

https://codereview.chromium.org/11234032/diff/27027/content/browser/storage_p...
content/browser/storage_partition_impl.cc:49:
.AppendASCII(descriptor.partition_domain);
Do we want this to also be UTF8?  Having 2 different encodings in one
structure's fields seems like a recipe for confusion.

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

https://codereview.chromium.org/11234032/diff/27027/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:212: bool in_memory_only =
in_memory || browser_context_->IsOffTheRecord();
I would move this out too...

https://codereview.chromium.org/11234032/diff/27027/content/browser/storage_p...
File content/browser/storage_partition_impl_map.h (right):

https://codereview.chromium.org/11234032/diff/27027/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:54: // the |browser_context_| this
map is associated with.
I would prefer something that states the valid state transtitions.  Maybe

Set to true when the ResourceContext for the associated |browser_context_| is
initialized. Can never return to false.

[Charlie Reis, 2012-11-06 00:17:17]

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:561: // out of it. The format
is: guest://app_id/persist?partition_name
nit: chrome-guest

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

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:39: content::WebContents**
contents1,
These names are confusing until you see them in context below: it's not obvious
how they differ, nor that they're all guests.  Maybe the first two should be
cookie_contents1 and cookie_contents2, and you could mention in the comment
above that the first two guests will load pages that set cookies while the
second two guests use other storage features.

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:98: GetStoragePartition(),
I like that we're testing this, but isn't GetStoragePartition() going away?  How
do we plan to test it after that?

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:201: // The third tag should
not have any cookies as it is in separate partition.
nit: in a

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:220:
L"window.sessionStorage.getItem('foo') || 'badval')");
Can we change this to 'bar' to avoid any bugs confusing the two?

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:511: // Now, parse the three
parts of the partition ID, so we can verify them.
Yeah, let's chat about this in person tomorrow.  If we can avoid adding a custom
parser to the browser process, I'd sleep a little better.  :)

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (left):

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:216: // to the default profile
path.
Please keep this comment (minus the TODO).

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:249: // to the default profile
path.
Same.

https://codereview.chromium.org/11234032/diff/27027/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view_isolation/main.js
(right):

https://codereview.chromium.org/11234032/diff/27027/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_isolation/main.js:20:
"<object id='webview2' partition='foo' src=" + url3 +
Again, potential for confusion between all the 'foo's.  Can this be something
different?

https://codereview.chromium.org/11234032/diff/27027/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js
(right):

https://codereview.chromium.org/11234032/diff/27027/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js:64: var
partition = null;
Is this an IndexedDB term?  (I can't tell if it's referring to a
StoragePartition or not.)

https://codereview.chromium.org/11234032/diff/27027/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11234032/diff/27027/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:95:
CHECK(IsStringUTF8(storage_partition_id));
Sanity check: Is crashing the browser process the right response to this?  Or
should we be trying something like clearing it out and killing the renderer,
since the renderer is the one to blame?

https://codereview.chromium.org/11234032/diff/27027/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:110: SiteInstance*
guest_site_instance = SiteInstance::CreateForURL(
Not important for this CL, but if I understand correctly, this move from
WebContents to BPE is nice because it will let the BPE keep a single
BrowsingInstance for all its guest SiteInstances.  That will group them together
in the same process.

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

https://codereview.chromium.org/11234032/diff/27027/content/browser/storage_p...
content/browser/storage_partition_impl.cc:48: FilePath path =
FilePath(kStoragePartitionDirname).Append(kExtensionsDirname)
nit: I think the dot goes on this line in Chrome style.  (I always get confused
because WebKit style is the opposite.)

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

https://codereview.chromium.org/11234032/diff/27027/content/browser/storage_p...
content/browser/storage_partition_impl.h:26: // Note: It is equivalent, though
not identical to the same structure that
"Note:" makes me think this is a permanent situation, in which case it's hard to
follow.  Can you change it to a TODO?

https://codereview.chromium.org/11234032/diff/27027/content/browser/storage_p...
File content/browser/storage_partition_impl_map.h (right):

https://codereview.chromium.org/11234032/diff/27027/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:40: PartitionsMap;
nit: PartitionMap

https://codereview.chromium.org/11234032/diff/27027/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11234032/diff/27027/content/public/browser/co...
content/public/browser/content_browser_client.h:277: // a site.
Since this is the public header file that embedders rely on, we need a better
description of what partition_domain, partition_name, and in_memory mean. 
There's no way to tell what the difference between "domain" and "name" is here,
and why they're separate.

(Maybe a reference to the description in storage_partition_impl.h would help,
but that's not in content/public.)

[nasko, 2012-11-06 01:21:51]

I've addressed all comments, but two - the CHECK on partition_name and the need
of different format for partition id. Let's discuss tomorrow.

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:494: // the string based on the
two separators.
On 2012/11/05 23:48:37, awong wrote:
> Question: How strong is our belief that |app_id|will not contain colons, and
> what is the damage if one slips through?
> 
> Is it strong enough that we don't need a CHECK() for assertion?

The app id is the ascii representation of a public key hash. It should not have
any special characters in it.

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:515: if (tokens.size() < 3) {
On 2012/11/05 23:48:37, awong wrote:
> Should it be exactly 3?

Not if the partition name itself has ':'. Since we are not filtering that input
from the app, it can be "foo:bar:goog". This is the reason the partition name is
at the end of the string, since the other two strings are guaranteed to be
delimiter free.

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:577: *partition_name =
std::string();
On 2012/11/05 23:48:37, awong wrote:
> partition_name->clear()?

Done.

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:583: *app_id = std::string();
On 2012/11/05 23:48:37, awong wrote:
> ->clear(), here and below?

Done.

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (left):

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:216: // to the default profile
path.
On 2012/11/06 00:17:18, creis wrote:
> Please keep this comment (minus the TODO).

Done.

https://codereview.chromium.org/11234032/diff/27027/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:249: // to the default profile
path.
On 2012/11/06 00:17:18, creis wrote:
> Same.

Isn't the existing comment already doing similar job? I find having the two is
leading to duplication.

https://codereview.chromium.org/11234032/diff/27027/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view_isolation/main.js
(right):

https://codereview.chromium.org/11234032/diff/27027/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_isolation/main.js:20:
"<object id='webview2' partition='foo' src=" + url3 +
On 2012/11/06 00:17:18, creis wrote:
> Again, potential for confusion between all the 'foo's.  Can this be something
> different?

Done.

https://codereview.chromium.org/11234032/diff/27027/chrome/test/data/extensio...
File chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js
(right):

https://codereview.chromium.org/11234032/diff/27027/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/web_view_isolation/storage.js:64: var
partition = null;
On 2012/11/06 00:17:18, creis wrote:
> Is this an IndexedDB term?  (I can't tell if it's referring to a
> StoragePartition or not.)

Done.

https://codereview.chromium.org/11234032/diff/27027/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11234032/diff/27027/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:110: SiteInstance*
guest_site_instance = SiteInstance::CreateForURL(
On 2012/11/06 00:17:18, creis wrote:
> Not important for this CL, but if I understand correctly, this move from
> WebContents to BPE is nice because it will let the BPE keep a single
> BrowsingInstance for all its guest SiteInstances.  That will group them
together
> in the same process.

Yes. It is not done yet, I will do it in a follow up CL.

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

https://codereview.chromium.org/11234032/diff/27027/content/browser/storage_p...
content/browser/storage_partition_impl.cc:49:
.AppendASCII(descriptor.partition_domain);
On 2012/11/05 23:48:37, awong wrote:
> Do we want this to also be UTF8?  Having 2 different encodings in one
> structure's fields seems like a recipe for confusion.

This is known to be ASCII, since it is the app ID. I'd like to be more strict
here, as we have known constraints on the data.

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

https://codereview.chromium.org/11234032/diff/27027/content/browser/storage_p...
content/browser/storage_partition_impl.h:26: // Note: It is equivalent, though
not identical to the same structure that
On 2012/11/06 00:17:18, creis wrote:
> "Note:" makes me think this is a permanent situation, in which case it's hard
to
> follow.  Can you change it to a TODO?

Done.

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

https://codereview.chromium.org/11234032/diff/27027/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:212: bool in_memory_only =
in_memory || browser_context_->IsOffTheRecord();
On 2012/11/05 23:48:37, awong wrote:
> I would move this out too...

Done.

https://codereview.chromium.org/11234032/diff/27027/content/browser/storage_p...
File content/browser/storage_partition_impl_map.h (right):

https://codereview.chromium.org/11234032/diff/27027/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:54: // the |browser_context_| this
map is associated with.
On 2012/11/05 23:48:37, awong wrote:
> I would prefer something that states the valid state transtitions.  Maybe
> 
> Set to true when the ResourceContext for the associated |browser_context_| is
> initialized. Can never return to false.

Done.

https://codereview.chromium.org/11234032/diff/27027/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11234032/diff/27027/content/public/browser/co...
content/public/browser/content_browser_client.h:277: // a site.
On 2012/11/06 00:17:18, creis wrote:
> Since this is the public header file that embedders rely on, we need a better
> description of what partition_domain, partition_name, and in_memory mean. 
> There's no way to tell what the difference between "domain" and "name" is
here,
> and why they're separate.
> 
> (Maybe a reference to the description in storage_partition_impl.h would help,
> but that's not in content/public.)

Done.

[nasko, 2012-11-07 00:33:57]

Fixed the two outstanding issues - the custom format and the CHECK on partition
name.

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

http://codereview.chromium.org/11234032/diff/27027/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:511: // Now, parse the three
parts of the partition ID, so we can verify them.
On 2012/11/06 00:17:18, creis wrote:
> Yeah, let's chat about this in person tomorrow.  If we can avoid adding a
custom
> parser to the browser process, I'd sleep a little better.  :)

Done.

http://codereview.chromium.org/11234032/diff/27027/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/11234032/diff/27027/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:95:
CHECK(IsStringUTF8(storage_partition_id));
On 2012/11/06 00:17:18, creis wrote:
> Sanity check: Is crashing the browser process the right response to this?  Or
> should we be trying something like clearing it out and killing the renderer,
> since the renderer is the one to blame?

Done.

[awong, 2012-11-07 01:14:11]

looking pretty close. Last round of comments.

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

http://codereview.chromium.org/11234032/diff/30099/content/browser/browser_co...
content/browser/browser_context.cc:52: if (browser_context->IsOffTheRecord())
Add comment explaining why we force to true.

http://codereview.chromium.org/11234032/diff/30099/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/11234032/diff/30099/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:95: // in-memory
partition.
Not sure how I feel about this.  It's okay, but killing the renderer actually
feels better...

http://codereview.chromium.org/11234032/diff/30099/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:95: // in-memory
partition.
Not sure how I feel about this. If the renderer is misbehaving, shouldn't we
kill it rather than use fallback behavior which might mask the bug?

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

http://codereview.chromium.org/11234032/diff/30099/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:49:
.AppendASCII(descriptor.partition_domain);
What happens if we call AppendASCII with UTF-8 or something here?

Is there a possible attack if future code causes us to fail validation?

http://codereview.chromium.org/11234032/diff/30099/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/11234032/diff/30099/content/public/browser/con...
content/public/browser/content_browser_client.h:279: // from each other. The
|partition_domain| is an arbitrary string, specifying
Please state the formating constraints (eg., what's the encoding, can
partition_domain and name contain slashes, etc.)

http://codereview.chromium.org/11234032/diff/30099/content/public/browser/con...
content/public/browser/content_browser_client.h:283: // persisted, the
|in_memory| value should be set to true.
should -> will

[Charlie Reis, 2012-11-07 01:40:54]

Quick comments on content/public.  Rest of review on the way.

http://codereview.chromium.org/11234032/diff/30099/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/11234032/diff/30099/content/public/browser/con...
content/public/browser/content_browser_client.h:277: // Allow the embedder to
provide storage parititon description associated with
nit: Allows
nit: provide a
s/associated with/for/

http://codereview.chromium.org/11234032/diff/30099/content/public/browser/con...
content/public/browser/content_browser_client.h:278: // a site. Storage can be
split into partition domains, which are independent
This sentence is hard for me to understand.  Can we tie it back to the
description you mentioned on the line above?  Maybe something like:

A storage partition description includes a domain of the embedder's choice, an
optional name within that domain, and whether the partition is in-memory only.

http://codereview.chromium.org/11234032/diff/30099/content/public/browser/con...
content/public/browser/content_browser_client.h:283: // persisted, the
|in_memory| value should be set to true.
On 2012/11/07 01:14:11, awong wrote:
> should -> will

Hmm, I think I agree with Nasko's wording (should).  In a sense, this is
documentation for anyone implementing this interface, so they need to know that
they should set the value to true.

[awong, 2012-11-07 01:43:46]

http://codereview.chromium.org/11234032/diff/30099/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/11234032/diff/30099/content/public/browser/con...
content/public/browser/content_browser_client.h:283: // persisted, the
|in_memory| value should be set to true.
On 2012/11/07 01:40:54, creis wrote:
> On 2012/11/07 01:14:11, awong wrote:
> > should -> will
> 
> Hmm, I think I agree with Nasko's wording (should).  In a sense, this is
> documentation for anyone implementing this interface, so they need to know
that
> they should set the value to true.

I tend to think of function comments as hard specs and should feels weaker since
no implementation that leave it as false would be correct.  But don't feel
strongly enough to argue. Either way works. :)

[awong, 2012-11-07 02:15:42]

one more comment.  If you don't want to integrate this change, no worries. I'll
do it in the next CL.

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

http://codereview.chromium.org/11234032/diff/30099/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:43: FilePath
StoragePartitionImpl::GetStoragePartitionPath(
If we move this into SPIM, along with the descriptor, and make it so
SPI::Create() takes 
   BrowserContext* context, 
   bool in_memory,
   const FilePath& partition_path

I think the code gets cleaner.

[Charlie Reis, 2012-11-07 02:36:34]

I do think we're close, but some comments and questions below.  Thanks!

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

http://codereview.chromium.org/11234032/diff/30099/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:479: if
(site.SchemeIs(chrome::kGuestScheme)) {
Can you give example(s) in a comment?  This looks like the only place where we
would document what partition_id means for Chrome.

e.g., chrome-guest://app_id1/persist?foo, chrome-guest://app_id2/?

http://codereview.chromium.org/11234032/diff/30099/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:502: std::string* app_id,
Please leave this as partition_domain.  Coding style is to keep parameter names
the same between header and implementation, and I'd argue it makes it clearer
that we're choosing to assign the app ID to the partition domain slot.

http://codereview.chromium.org/11234032/diff/30099/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:511: //
guest://app_id/persist?partition_name
Awesome comment!
nit: chrome-guest://

http://codereview.chromium.org/11234032/diff/30099/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:534: if (extension &&
extension->is_storage_isolated()) {
Please add a comment here saying that storage-isolated extensions (e.g., apps)
just use the extension ID as the partition name.

http://codereview.chromium.org/11234032/diff/30099/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:542: app_id->clear();
Add a comment here saying that all other cases use the default storage partition
description.

http://codereview.chromium.org/11234032/diff/30099/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1632: // the correct scheme.
Without this check, guest:// schemes used by
nit: chrome-guest://

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

http://codereview.chromium.org/11234032/diff/30099/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:36: content::WebContents**
contents1,
I think you missed my comments in this file, perhaps because they were in an
earlier draft.  (Code review tip: it helps to step through the email instead of
the latest patch set, in case some new comments were in previous drafts.  In my
case, I'd already added a bunch of comments when you uploaded a new draft.)

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

http://codereview.chromium.org/11234032/diff/30099/content/browser/browser_co...
content/browser/browser_context.cc:40: StoragePartition*
GetStoragePartitionHelper(BrowserContext* browser_context,
nit: This sounds like we should return a Helper object.  Maybe
GetStoragePartitionFromConfig?

http://codereview.chromium.org/11234032/diff/30099/content/browser/browser_co...
content/browser/browser_context.cc:57: // until the on-disk storage code has
landed.
Can you add a mention of http://crbug.com/159464?

http://codereview.chromium.org/11234032/diff/30099/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/11234032/diff/30099/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:95: // in-memory
partition.
On 2012/11/07 01:14:11, awong wrote:
> Not sure how I feel about this. If the renderer is misbehaving, shouldn't we
> kill it rather than use fallback behavior which might mask the bug?

I don't know if we have access to BrowserMessageFilter::BadMessageReceived here
or just want to do something similar, but that's one way to kill the renderer
when it does something unexpected like this.

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

http://codereview.chromium.org/11234032/diff/30099/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:48: FilePath path =
FilePath(kStoragePartitionDirname).Append(kExtensionsDirname)
I hate to open this can of worms, but somehow we ended up with a
kExtensionsDirname in content.  Ugh.  (It's for the "ext" in the
Storage/ext/ABCDEF/ path.)

Maybe we need a separate bug for punting that to the embedder?  Albert mentioned
that it might even make sense to have "ext/ABCDEF" be the partition_domain, but
we haven't thought it through.

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

http://codereview.chromium.org/11234032/diff/30099/content/browser/storage_pa...
content/browser/storage_partition_impl.h:31: struct StoragePartitionDescriptor {
Sorry, more name questions.  Is this intentionally a different name than
"storage partition config," even though it has the same data?  Should we use the
same name for this and ContentBrowserClient::GetStoragePartitionConfigForSite?

http://codereview.chromium.org/11234032/diff/30099/content/browser/storage_pa...
File content/browser/storage_partition_impl_map.h (right):

http://codereview.chromium.org/11234032/diff/30099/content/browser/storage_pa...
content/browser/storage_partition_impl_map.h:40: PartitionsMap;
nit: PartitionMap.

(I mentioned this last time but it must have gotten missed.)

http://codereview.chromium.org/11234032/diff/30099/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/11234032/diff/30099/content/public/browser/con...
content/public/browser/content_browser_client.h:266: // Allow the embedder to
specify storage parititon id associated with a site.
Can you also update this to explain that partition_id is somehow a combination
of the partition_domain, partition_name, and in_memory info from below?  I'm
still not entirely clear how we expect those to be related in general (e.g., if
we're not talking about chrome-guest:// URLs), nor whether this code is going
away.

Maybe the right way is to say it's an embedder-specified string version of the
"storage partition config"?

http://codereview.chromium.org/11234032/diff/30099/content/public/browser/con...
content/public/browser/content_browser_client.h:277: // Allow the embedder to
provide storage parititon description associated with
On 2012/11/07 01:40:54, creis wrote:
> nit: Allows
> nit: provide a
> s/associated with/for/

Also, s/description/configuration/.  We've already got enough terms for it, so
let's stick with the one from the method name.  :)

(Unless we settle on StoragePartitionDescriptor, which appears to have the same
info...)  :-/

[nasko, 2012-11-07 17:47:01]

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

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:39: content::WebContents**
contents1,
On 2012/11/06 00:17:18, creis wrote:
> These names are confusing until you see them in context below: it's not
obvious
> how they differ, nor that they're all guests.  Maybe the first two should be
> cookie_contents1 and cookie_contents2, and you could mention in the comment
> above that the first two guests will load pages that set cookies while the
> second two guests use other storage features.

Done.

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:98: GetStoragePartition(),
On 2012/11/06 00:17:18, creis wrote:
> I like that we're testing this, but isn't GetStoragePartition() going away? 
How
> do we plan to test it after that?

We can use BrowserContext::GetStoragePartition, passing the WebContents'
SiteInstance.

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:201: // The third tag should
not have any cookies as it is in separate partition.
On 2012/11/06 00:17:18, creis wrote:
> nit: in a

Done.

https://codereview.chromium.org/11234032/diff/31024/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:220:
L"window.sessionStorage.getItem('foo') || 'badval')");
On 2012/11/06 00:17:18, creis wrote:
> Can we change this to 'bar' to avoid any bugs confusing the two?

Done.

https://codereview.chromium.org/11234032/diff/30099/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/30099/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:479: if
(site.SchemeIs(chrome::kGuestScheme)) {
On 2012/11/07 02:36:34, creis wrote:
> Can you give example(s) in a comment?  This looks like the only place where we
> would document what partition_id means for Chrome.
> 
> e.g., chrome-guest://app_id1/persist?foo, chrome-guest://app_id2/?

Done.

https://codereview.chromium.org/11234032/diff/30099/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:502: std::string* app_id,
On 2012/11/07 02:36:34, creis wrote:
> Please leave this as partition_domain.  Coding style is to keep parameter
names
> the same between header and implementation, and I'd argue it makes it clearer
> that we're choosing to assign the app ID to the partition domain slot.

Done.

https://codereview.chromium.org/11234032/diff/30099/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:511: //
guest://app_id/persist?partition_name
On 2012/11/07 02:36:34, creis wrote:
> Awesome comment!
> nit: chrome-guest://

Done.

https://codereview.chromium.org/11234032/diff/30099/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:534: if (extension &&
extension->is_storage_isolated()) {
On 2012/11/07 02:36:34, creis wrote:
> Please add a comment here saying that storage-isolated extensions (e.g., apps)
> just use the extension ID as the partition name.

Done.

https://codereview.chromium.org/11234032/diff/30099/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:542: app_id->clear();
On 2012/11/07 02:36:34, creis wrote:
> Add a comment here saying that all other cases use the default storage
partition
> description.

Done.

https://codereview.chromium.org/11234032/diff/30099/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1632: // the correct scheme.
Without this check, guest:// schemes used by
On 2012/11/07 02:36:34, creis wrote:
> nit: chrome-guest://

Done.

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

https://codereview.chromium.org/11234032/diff/30099/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:36: content::WebContents**
contents1,
On 2012/11/07 02:36:34, creis wrote:
> I think you missed my comments in this file, perhaps because they were in an
> earlier draft.  (Code review tip: it helps to step through the email instead
of
> the latest patch set, in case some new comments were in previous drafts.  In
my
> case, I'd already added a bunch of comments when you uploaded a new draft.)

Done.

https://codereview.chromium.org/11234032/diff/30099/content/browser/browser_c...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/11234032/diff/30099/content/browser/browser_c...
content/browser/browser_context.cc:40: StoragePartition*
GetStoragePartitionHelper(BrowserContext* browser_context,
On 2012/11/07 02:36:34, creis wrote:
> nit: This sounds like we should return a Helper object.  Maybe
> GetStoragePartitionFromConfig?

Done.

https://codereview.chromium.org/11234032/diff/30099/content/browser/browser_c...
content/browser/browser_context.cc:52: if (browser_context->IsOffTheRecord())
On 2012/11/07 01:14:11, awong wrote:
> Add comment explaining why we force to true.

Done.

https://codereview.chromium.org/11234032/diff/30099/content/browser/browser_c...
content/browser/browser_context.cc:57: // until the on-disk storage code has
landed.
On 2012/11/07 02:36:34, creis wrote:
> Can you add a mention of http://crbug.com/159464?

Done.

https://codereview.chromium.org/11234032/diff/30099/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11234032/diff/30099/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:95: // in-memory
partition.
On 2012/11/07 01:14:11, awong wrote:
> Not sure how I feel about this. If the renderer is misbehaving, shouldn't we
> kill it rather than use fallback behavior which might mask the bug?

As Charlie pointed in offline discussion, FilterURL already has similar behavior
of not killing renderer, but masking the value and letting it continue.

https://codereview.chromium.org/11234032/diff/30099/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:95: // in-memory
partition.
On 2012/11/07 02:36:34, creis wrote:
> On 2012/11/07 01:14:11, awong wrote:
> > Not sure how I feel about this. If the renderer is misbehaving, shouldn't we
> > kill it rather than use fallback behavior which might mask the bug?
> 
> I don't know if we have access to BrowserMessageFilter::BadMessageReceived
here
> or just want to do something similar, but that's one way to kill the renderer
> when it does something unexpected like this.

BadMessageReceived is a MessageFilter method that cannot be invoked without
having a filter object. It only does a KillProcess, so I've just done that.

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

https://codereview.chromium.org/11234032/diff/30099/content/browser/storage_p...
content/browser/storage_partition_impl.cc:43: FilePath
StoragePartitionImpl::GetStoragePartitionPath(
On 2012/11/07 02:15:42, awong wrote:
> If we move this into SPIM, along with the descriptor, and make it so
> SPI::Create() takes 
>    BrowserContext* context, 
>    bool in_memory,
>    const FilePath& partition_path
> 
> I think the code gets cleaner.

File a bug and we can move it later.

https://codereview.chromium.org/11234032/diff/30099/content/browser/storage_p...
content/browser/storage_partition_impl.cc:48: FilePath path =
FilePath(kStoragePartitionDirname).Append(kExtensionsDirname)
On 2012/11/07 02:36:34, creis wrote:
> I hate to open this can of worms, but somehow we ended up with a
> kExtensionsDirname in content.  Ugh.  (It's for the "ext" in the
> Storage/ext/ABCDEF/ path.)
> 
> Maybe we need a separate bug for punting that to the embedder?  Albert
mentioned
> that it might even make sense to have "ext/ABCDEF" be the partition_domain,
but
> we haven't thought it through.

Let's do it as a different bug. This one has already done a ton and is getting
hard to grasp it all.

https://codereview.chromium.org/11234032/diff/30099/content/browser/storage_p...
content/browser/storage_partition_impl.cc:49:
.AppendASCII(descriptor.partition_domain);
On 2012/11/07 01:14:11, awong wrote:
> What happens if we call AppendASCII with UTF-8 or something here?
> 
> Is there a possible attack if future code causes us to fail validation?

Done.

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

https://codereview.chromium.org/11234032/diff/30099/content/browser/storage_p...
content/browser/storage_partition_impl.h:31: struct StoragePartitionDescriptor {
On 2012/11/07 02:36:34, creis wrote:
> Sorry, more name questions.  Is this intentionally a different name than
> "storage partition config," even though it has the same data?  Should we use
the
> same name for this and ContentBrowserClient::GetStoragePartitionConfigForSite?

Done.

https://codereview.chromium.org/11234032/diff/30099/content/browser/storage_p...
File content/browser/storage_partition_impl_map.h (right):

https://codereview.chromium.org/11234032/diff/30099/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:40: PartitionsMap;
On 2012/11/07 02:36:34, creis wrote:
> nit: PartitionMap.
> 
> (I mentioned this last time but it must have gotten missed.)

Done.

https://codereview.chromium.org/11234032/diff/30099/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11234032/diff/30099/content/public/browser/co...
content/public/browser/content_browser_client.h:266: // Allow the embedder to
specify storage parititon id associated with a site.
On 2012/11/07 02:36:34, creis wrote:
> Can you also update this to explain that partition_id is somehow a combination
> of the partition_domain, partition_name, and in_memory info from below?  I'm
> still not entirely clear how we expect those to be related in general (e.g.,
if
> we're not talking about chrome-guest:// URLs), nor whether this code is going
> away.
> 
> Maybe the right way is to say it's an embedder-specified string version of the
> "storage partition config"?

Done.

https://codereview.chromium.org/11234032/diff/30099/content/public/browser/co...
content/public/browser/content_browser_client.h:277: // Allow the embedder to
provide storage parititon description associated with
On 2012/11/07 01:40:54, creis wrote:
> nit: Allows
> nit: provide a
> s/associated with/for/

Done.

https://codereview.chromium.org/11234032/diff/30099/content/public/browser/co...
content/public/browser/content_browser_client.h:277: // Allow the embedder to
provide storage parititon description associated with
On 2012/11/07 02:36:34, creis wrote:
> On 2012/11/07 01:40:54, creis wrote:
> > nit: Allows
> > nit: provide a
> > s/associated with/for/
> 
> Also, s/description/configuration/.  We've already got enough terms for it, so
> let's stick with the one from the method name.  :)
> 
> (Unless we settle on StoragePartitionDescriptor, which appears to have the
same
> info...)  :-/

Done.

https://codereview.chromium.org/11234032/diff/30099/content/public/browser/co...
content/public/browser/content_browser_client.h:278: // a site. Storage can be
split into partition domains, which are independent
On 2012/11/07 01:40:54, creis wrote:
> This sentence is hard for me to understand.  Can we tie it back to the
> description you mentioned on the line above?  Maybe something like:
> 
> A storage partition description includes a domain of the embedder's choice, an
> optional name within that domain, and whether the partition is in-memory only.

Done.

https://codereview.chromium.org/11234032/diff/30099/content/public/browser/co...
content/public/browser/content_browser_client.h:279: // from each other. The
|partition_domain| is an arbitrary string, specifying
On 2012/11/07 01:14:11, awong wrote:
> Please state the formating constraints (eg., what's the encoding, can
> partition_domain and name contain slashes, etc.)

Done.

https://codereview.chromium.org/11234032/diff/30099/content/public/browser/co...
content/public/browser/content_browser_client.h:283: // persisted, the
|in_memory| value should be set to true.
On 2012/11/07 01:14:11, awong wrote:
> should -> will

Done.

https://codereview.chromium.org/11234032/diff/30099/content/public/browser/co...
content/public/browser/content_browser_client.h:283: // persisted, the
|in_memory| value should be set to true.
On 2012/11/07 01:43:46, awong wrote:
> On 2012/11/07 01:40:54, creis wrote:
> > On 2012/11/07 01:14:11, awong wrote:
> > > should -> will
> > 
> > Hmm, I think I agree with Nasko's wording (should).  In a sense, this is
> > documentation for anyone implementing this interface, so they need to know
> that
> > they should set the value to true.
> 
> I tend to think of function comments as hard specs and should feels weaker
since
> no implementation that leave it as false would be correct.  But don't feel
> strongly enough to argue. Either way works. :)

Done.

[awong, 2012-11-07 18:14:30]

LGTM w/ nits

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

http://codereview.chromium.org/11234032/diff/21053/content/browser/browser_co...
content/browser/browser_context.cc:40: StoragePartition*
GetStoragePartitionFromConfig(
Can the parameters move back up to the preceding line?

http://codereview.chromium.org/11234032/diff/21053/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/11234032/diff/21053/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:88: // process.
process -> renderer process

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

http://codereview.chromium.org/11234032/diff/21053/content/browser/storage_pa...
content/browser/storage_partition_impl.h:86: // |partition_config|.
Add comment explaining the expected behavior for embedded path separators and
nulls in the config strings?

http://codereview.chromium.org/11234032/diff/21053/content/public/browser/con...
File content/public/browser/content_browser_client.cc (right):

http://codereview.chromium.org/11234032/diff/21053/content/public/browser/con...
content/public/browser/content_browser_client.cc:183: *partition_domain =
std::string();
->clear()

http://codereview.chromium.org/11234032/diff/21053/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/11234032/diff/21053/content/public/browser/con...
content/public/browser/content_browser_client.h:279: // site. A storage
partition description includes a domain of the embedder's
description -> configuration

[Charlie Reis, 2012-11-07 18:38:36]

Almost there.

nit: Change CL title and description to Webview tag

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

http://codereview.chromium.org/11234032/diff/21053/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:14: #include
"base/string_split.h"
Do we still need this?

http://codereview.chromium.org/11234032/diff/21053/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:481: if
(site.SchemeIs(chrome::kGuestScheme)) {
nit: No braces.

http://codereview.chromium.org/11234032/diff/21053/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:537: // Extensions, which have
storage isolation enabled (e.g., apps), use
nit: Extensions which have

(Saying "extensions, which have" suggests that all extensions are storage
isolated.)

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

http://codereview.chromium.org/11234032/diff/21053/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:33: // The |navigate_to_url|
paramter is used to navigate the main browser window.
nit: parameter

http://codereview.chromium.org/11234032/diff/21053/content/browser/browser_pl...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

http://codereview.chromium.org/11234032/diff/21053/content/browser/browser_pl...
content/browser/browser_plugin/browser_plugin_embedder.cc:90:
base::KillProcess(render_view_host->GetProcess()->GetHandle(),
Please add this line before the kill:
content::RecordAction(UserMetricsAction("BadMessageTerminate_BPE"));

That will let us keep track of whether this is a cause of renderer kills.

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

http://codereview.chromium.org/11234032/diff/21053/content/browser/storage_pa...
content/browser/storage_partition_impl.cc:32: // The code in GetPartitionPath()
constructs these path names.
nit: GetStoragePartitionPath()

Please also add a TODO to move extension-related paths out of the content
module.

[nasko, 2012-11-07 18:48:05]

https://codereview.chromium.org/11234032/diff/21053/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/21053/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:14: #include
"base/string_split.h"
On 2012/11/07 18:38:36, creis wrote:
> Do we still need this?

Done.

https://codereview.chromium.org/11234032/diff/21053/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:481: if
(site.SchemeIs(chrome::kGuestScheme)) {
On 2012/11/07 18:38:36, creis wrote:
> nit: No braces.

Done.

https://codereview.chromium.org/11234032/diff/21053/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:537: // Extensions, which have
storage isolation enabled (e.g., apps), use
On 2012/11/07 18:38:36, creis wrote:
> nit: Extensions which have
> 
> (Saying "extensions, which have" suggests that all extensions are storage
> isolated.)

Done.

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

https://codereview.chromium.org/11234032/diff/21053/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:33: // The |navigate_to_url|
paramter is used to navigate the main browser window.
On 2012/11/07 18:38:36, creis wrote:
> nit: parameter

Done.

https://codereview.chromium.org/11234032/diff/21053/content/browser/browser_c...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/11234032/diff/21053/content/browser/browser_c...
content/browser/browser_context.cc:40: StoragePartition*
GetStoragePartitionFromConfig(
On 2012/11/07 18:14:30, awong wrote:
> Can the parameters move back up to the preceding line?

The partition_domain parameter exceeds 80 cols once aligned with the opening
paren.

https://codereview.chromium.org/11234032/diff/21053/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/11234032/diff/21053/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:88: // process.
On 2012/11/07 18:14:30, awong wrote:
> process -> renderer process

Done.

https://codereview.chromium.org/11234032/diff/21053/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_embedder.cc:90:
base::KillProcess(render_view_host->GetProcess()->GetHandle(),
On 2012/11/07 18:38:36, creis wrote:
> Please add this line before the kill:
> content::RecordAction(UserMetricsAction("BadMessageTerminate_BPE"));
> 
> That will let us keep track of whether this is a cause of renderer kills.

Done.

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

https://codereview.chromium.org/11234032/diff/21053/content/browser/storage_p...
content/browser/storage_partition_impl.cc:32: // The code in GetPartitionPath()
constructs these path names.
On 2012/11/07 18:38:36, creis wrote:
> nit: GetStoragePartitionPath()
> 
> Please also add a TODO to move extension-related paths out of the content
> module.

Done.

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

https://codereview.chromium.org/11234032/diff/21053/content/browser/storage_p...
content/browser/storage_partition_impl.h:86: // |partition_config|.
On 2012/11/07 18:14:30, awong wrote:
> Add comment explaining the expected behavior for embedded path separators and
> nulls in the config strings?

Done.

https://codereview.chromium.org/11234032/diff/21053/content/public/browser/co...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/21053/content/public/browser/co...
content/public/browser/content_browser_client.cc:183: *partition_domain =
std::string();
On 2012/11/07 18:14:30, awong wrote:
> ->clear()

Done.

https://codereview.chromium.org/11234032/diff/21053/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11234032/diff/21053/content/public/browser/co...
content/public/browser/content_browser_client.h:279: // site. A storage
partition description includes a domain of the embedder's
On 2012/11/07 18:14:30, awong wrote:
> description -> configuration

Done.

[Charlie Reis, 2012-11-07 18:57:09]

Thanks!  LGTM with nit.

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

http://codereview.chromium.org/11234032/diff/28040/content/browser/storage_pa...
content/browser/storage_partition_impl.h:87: // embedded nuls, the values will
be truncated and only the portion prior to
nit: nulls

http://codereview.chromium.org/11234032/diff/28040/content/browser/storage_pa...
content/browser/storage_partition_impl.h:88: // the nul will be used.
nit: null

[nasko, 2012-11-07 19:25:14]

Adding OWNERS reviewers

kalman@ -> chrome/browser/extensions/
mirandac@ -> chrome/browser/profiles/
sky@ -> chrome/ and chrome/test

[not at google - send to devlin, 2012-11-07 19:28:47]

lgtm

[Miranda Callahan, 2012-11-07 19:29:43]

On 2012/11/07 19:28:47, kalman wrote:
> lgtm

profiles/* LGTM

[sky, 2012-11-07 22:21:05]

LGTM

https://codereview.chromium.org/11234032/diff/28040/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11234032/diff/28040/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:494: GURL
guest_url(partition_id);
What's with the white space here? How about:

return GURL(partition_id).is_valid();

[Charlie Reis, 2012-11-07 23:43:13]

Still LGTM after disabling SessionStorage test.

[commit-bot: I haz the power, 2012-11-08 04:05:45]

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

[commit-bot: I haz the power, 2012-11-08 08:31:55]

Change committed as 166639