Move SessionStorageNamespace entirely into NavigationController and support StoragePartitions.

Move SessionStorageNamespace entirely into NavigationController and support StoragePartitions.

Previously, WebContents, NavigationController, and RenderViewHost all exposed
APIs that allowed one to retrieve the SessionStorageNamespace associated with
them. This is confusing because there were too many ways to access the object.
After this change, the SessionStorageNamespace is only exposed by the
NavigationController.

Conceptually a SessionStorageNamespace belongs to a WebContents but we store
it in NavigationController because on tab duplication, the
NavigationController becomes the authoritative state of the tab.

Also, to support StoragePartitions, the NavigationController
now maintains a map of partition_id -> SessionStorageNamespace. Someone
requesting a SessionStorageNamespace must either know which StoragePartition
they are coming from, or which child process they are acting on behalf of.


This change also changes the way TabContents and WebContents are created.
1) We now have explicitly separate creation methods for prepopulating
   with SessionStorage and creating with an opener.
2) Some of the WebContentImpl construct has been moved into an Init()
   function to avoid accidental calls to virtual functions by subobjects.

TBR-ing all the directories where I just remove a NULL.

TBR=sky,stevenjb,dominich,brettw,satorux,kalman
BUG=85121
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151379

[awong, 2012-08-01 22:28:35]

Privatizing constructors for fun an profit!

The interesting changes are in WebContentsImpl and NavigationControllerImpl. 
Most of the other files are removals of a "NULL" parameter.

[dominich, 2012-08-01 22:59:26]

https://chromiumcodereview.appspot.com/10831116/diff/2001/chrome/browser/pred...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

https://chromiumcodereview.appspot.com/10831116/diff/2001/chrome/browser/pred...
chrome/browser/predictors/autocomplete_action_predictor.cc:168:
content::SessionStorageNamespaceMap::const_iterator it =
drive-by: If this is done a lot, maybe the NavigationController should expose a
method. Or the SessionStorageNamespaceMap should be more than a typedef and can
expose a GetDefaultNamespace method.

[awong, 2012-08-01 23:37:55]

https://chromiumcodereview.appspot.com/10831116/diff/2001/chrome/browser/pred...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

https://chromiumcodereview.appspot.com/10831116/diff/2001/chrome/browser/pred...
chrome/browser/predictors/autocomplete_action_predictor.cc:168:
content::SessionStorageNamespaceMap::const_iterator it =
On 2012/08/01 22:59:26, dominich wrote:
> drive-by: If this is done a lot, maybe the NavigationController should expose
a
> method. Or the SessionStorageNamespaceMap should be more than a typedef and
can
> expose a GetDefaultNamespace method.
> 

Actually, this is mostly transitional. I a followup CL that will take this out.
The idea is to completely remove the concept of "default" namespace.

[Charlie Reis, 2012-08-02 23:06:47]

This mostly looks good.  I think it's an improvement to have more explicit ways
to create WebContents, and good catch on the constructor issues.

https://chromiumcodereview.appspot.com/10831116/diff/2001/chrome/browser/pred...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

https://chromiumcodereview.appspot.com/10831116/diff/2001/chrome/browser/pred...
chrome/browser/predictors/autocomplete_action_predictor.cc:168:
content::SessionStorageNamespaceMap::const_iterator it =
On 2012/08/01 23:37:56, awong wrote:
> On 2012/08/01 22:59:26, dominich wrote:
> > drive-by: If this is done a lot, maybe the NavigationController should
expose
> a
> > method. Or the SessionStorageNamespaceMap should be more than a typedef and
> can
> > expose a GetDefaultNamespace method.
> > 
> 
> Actually, this is mostly transitional. I a followup CL that will take this
out.
> The idea is to completely remove the concept of "default" namespace.

Then this needs a TODO.

https://chromiumcodereview.appspot.com/10831116/diff/2002/chrome/browser/inst...
File chrome/browser/instant/instant_loader.cc (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/chrome/browser/inst...
chrome/browser/instant/instant_loader.cc:99: // but instead I will whack it in a
followup CL.
Not sure we need to mention the obsolete CL.  Instead, just say a bit more on
why it's wrong and what you plan to do.  (Removing the default namespace?)

https://chromiumcodereview.appspot.com/10831116/diff/2002/chrome/browser/ui/b...
File chrome/browser/ui/browser_tabstrip.h (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/chrome/browser/ui/b...
chrome/browser/ui/browser_tabstrip.h:80: TabContents*
TabContentsFactoryWithSessionStorage(
Move Factory to the end of the name.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/dom...
File content/browser/dom_storage/session_storage_namespace_impl.h (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/dom...
content/browser/dom_storage/session_storage_namespace_impl.h:19: class
CONTENT_EXPORT SessionStorageNamespaceImpl
Just checking: do we need this?

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

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
Extra space

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1220: //
TODO(ajwong): Do we need to handle SessionStorage here?
I would think so.  We want all the SessionStorageNamespaces for the entries we
just copied over, right?

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1265: if
(!session_storage_namespace) {
nit: No braces

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1297: if (it !=
session_storage_namespace_map_.end()) {
nit: No braces

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl_unittest.cc:2147:
other_controller.GetSessionStorageNamespaceMap();
nit: indent

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl_unittest.cc:2156:
EXPECT_TRUE(other != other_session_storage_namespace_map.end());
EXPECT_NE?

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:85:
site_instance->GetProcess()->GetID())));
Are we sure we want to call GetProcess() this early?  (This will create a
process if we don't have one yet.)  Is it earlier than we used to?

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
File content/browser/web_contents/test_web_contents.h (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/test_web_contents.h:98: 
No newline here?

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

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:1234:
->GetStoragePartitionIdForChildProcess(GetBrowserContext(),
I think we usually put the arrow on the previous line, right?

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/public/brow...
File content/public/browser/navigation_controller.h (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/public/brow...
content/public/browser/navigation_controller.h:28: // Used to store the mapping
of a StoragePartion id to SessionStorageNamespace.
StoragePartition

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/public/brow...
content/public/browser/navigation_controller.h:30: SessionStorageNamespaceMap;
Topic for discussion: would it be better for this to be exposed as its own type,
rather than having several places in the code depend on navigation_controller.h
to get this typedef?

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/public/brow...
content/public/browser/navigation_controller.h:236: // The session storage
namespace that all child render views of a renderer
nits: all child RenderViews in the renderer process with the given ID should
use.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/public/brow...
File content/public/browser/web_contents.h (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/public/brow...
content/public/browser/web_contents.h:59: CONTENT_EXPORT static WebContents*
CreateWithSessionStorage(
Please add a comment saying when callers should use this version rather than the
default.

[sreeram, 2012-08-02 23:44:42]

https://chromiumcodereview.appspot.com/10831116/diff/2002/chrome/browser/inst...
File chrome/browser/instant/instant_loader.cc (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/chrome/browser/inst...
chrome/browser/instant/instant_loader.cc:99: // but instead I will whack it in a
followup CL.
On 2012/08/02 23:06:47, creis wrote:
> Not sure we need to mention the obsolete CL.  Instead, just say a bit more on
> why it's wrong and what you plan to do.  (Removing the default namespace?)

That CL got rolled over into http://codereview.chromium.org/10836031/, so yes,
this code is still on track to be removed.

[awong, 2012-08-03 00:31:03]

Addressed most comments. The one about creating the process too early, I wasn't
sure how to handle.

https://chromiumcodereview.appspot.com/10831116/diff/2002/chrome/browser/inst...
File chrome/browser/instant/instant_loader.cc (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/chrome/browser/inst...
chrome/browser/instant/instant_loader.cc:99: // but instead I will whack it in a
followup CL.
On 2012/08/02 23:44:43, sreeram wrote:
> On 2012/08/02 23:06:47, creis wrote:
> > Not sure we need to mention the obsolete CL.  Instead, just say a bit more
on
> > why it's wrong and what you plan to do.  (Removing the default namespace?)
> 
> That CL got rolled over into http://codereview.chromium.org/10836031/, so yes,
> this code is still on track to be removed.

Updated comment to refer to sreeram's new CL. Hopefully sreeram's CL gets
checked in first and then this will just go away-Away-AWAY!!! :D

https://chromiumcodereview.appspot.com/10831116/diff/2002/chrome/browser/ui/b...
File chrome/browser/ui/browser_tabstrip.h (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/chrome/browser/ui/b...
chrome/browser/ui/browser_tabstrip.h:80: TabContents*
TabContentsFactoryWithSessionStorage(
On 2012/08/02 23:06:47, creis wrote:
> Move Factory to the end of the name.

Done.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/dom...
File content/browser/dom_storage/session_storage_namespace_impl.h (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/dom...
content/browser/dom_storage/session_storage_namespace_impl.h:19: class
CONTENT_EXPORT SessionStorageNamespaceImpl
On 2012/08/02 23:06:47, creis wrote:
> Just checking: do we need this?

Yep...but only for the constructor. Moved.

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

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2012/08/02 23:06:47, creis wrote:
> Extra space

Done.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1220: //
TODO(ajwong): Do we need to handle SessionStorage here?
On 2012/08/02 23:06:47, creis wrote:
> I would think so.  We want all the SessionStorageNamespaces for the entries we
> just copied over, right?

I'm actually not sure....the prerenderer and instant versions should have been
created off our web contents should we should have exactly the same session
storage state?  I mean, otherwise, our session storage was broken previously.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1265: if
(!session_storage_namespace) {
On 2012/08/02 23:06:47, creis wrote:
> nit: No braces

Done.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1297: if (it !=
session_storage_namespace_map_.end()) {
On 2012/08/02 23:06:47, creis wrote:
> nit: No braces

Done.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl_unittest.cc:2147:
other_controller.GetSessionStorageNamespaceMap();
On 2012/08/02 23:06:47, creis wrote:
> nit: indent

Done.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl_unittest.cc:2156:
EXPECT_TRUE(other != other_session_storage_namespace_map.end());
On 2012/08/02 23:06:47, creis wrote:
> EXPECT_NE?

Done.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:85:
site_instance->GetProcess()->GetID())));
On 2012/08/02 23:06:47, creis wrote:
> Are we sure we want to call GetProcess() this early?  (This will create a
> process if we don't have one yet.)  Is it earlier than we used to?

I bet it probably is earlier. But I don't know how else to solve this since our
principal is the child process id. I guess we could make RVH take a reference to
the controller and then request the storage namespace lazily. Would that work
better?

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
File content/browser/web_contents/test_web_contents.h (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/test_web_contents.h:98: 
On 2012/08/02 23:06:47, creis wrote:
> No newline here?

Done.

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

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/web_contents_impl.cc:1234:
->GetStoragePartitionIdForChildProcess(GetBrowserContext(),
On 2012/08/02 23:06:47, creis wrote:
> I think we usually put the arrow on the previous line, right?

I'm actually not sure...for gmock, they put the . on the following line. 

I changed it since I don't think the style guide specifies.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/public/brow...
File content/public/browser/navigation_controller.h (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/public/brow...
content/public/browser/navigation_controller.h:28: // Used to store the mapping
of a StoragePartion id to SessionStorageNamespace.
On 2012/08/02 23:06:47, creis wrote:
> StoragePartition

Done.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/public/brow...
content/public/browser/navigation_controller.h:30: SessionStorageNamespaceMap;
On 2012/08/02 23:06:47, creis wrote:
> Topic for discussion: would it be better for this to be exposed as its own
type,
> rather than having several places in the code depend on
navigation_controller.h
> to get this typedef?

General rule is not to go to large lengths to avoid header dependencies, but I
don't feel too strongly about it.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/public/brow...
content/public/browser/navigation_controller.h:236: // The session storage
namespace that all child render views of a renderer
On 2012/08/02 23:06:47, creis wrote:
> nits: all child RenderViews in the renderer process with the given ID should
> use.

Done.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/public/brow...
File content/public/browser/web_contents.h (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/public/brow...
content/public/browser/web_contents.h:59: CONTENT_EXPORT static WebContents*
CreateWithSessionStorage(
On 2012/08/02 23:06:47, creis wrote:
> Please add a comment saying when callers should use this version rather than
the
> default.

Done.

[Charlie Reis, 2012-08-03 22:11:28]

Two quick comments, then I'll do another pass.

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

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1220: //
TODO(ajwong): Do we need to handle SessionStorage here?
On 2012/08/03 00:31:04, awong wrote:
> On 2012/08/02 23:06:47, creis wrote:
> > I would think so.  We want all the SessionStorageNamespaces for the entries
we
> > just copied over, right?
> 
> I'm actually not sure....the prerenderer and instant versions should have been
> created off our web contents should we should have exactly the same session
> storage state?  I mean, otherwise, our session storage was broken previously.

Should be easy to check manually, right?  Just set a breakpoint here and see if
the current tab's map already has the namespaces for the newly copied entries.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:85:
site_instance->GetProcess()->GetID())));
On 2012/08/03 00:31:04, awong wrote:
> On 2012/08/02 23:06:47, creis wrote:
> > Are we sure we want to call GetProcess() this early?  (This will create a
> > process if we don't have one yet.)  Is it earlier than we used to?
> 
> I bet it probably is earlier. But I don't know how else to solve this since
our
> principal is the child process id. I guess we could make RVH take a reference
to
> the controller and then request the storage namespace lazily. Would that work
> better?


Looks like it's about the same time.  Before, we called it in
RenderViewHostImpl's constructor, which happens during this Create call.  So,
there's not much difference.

[awong, 2012-08-04 01:01:32]

Comments addressed, unittests added, scary comment about all the
WithSessionStorage factories added, cause really, don't call those unless you
know what you're doing. :)

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

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/navigation_controller_impl.cc:1220: //
TODO(ajwong): Do we need to handle SessionStorage here?
On 2012/08/03 22:11:28, creis wrote:
> On 2012/08/03 00:31:04, awong wrote:
> > On 2012/08/02 23:06:47, creis wrote:
> > > I would think so.  We want all the SessionStorageNamespaces for the
entries
> we
> > > just copied over, right?
> > 
> > I'm actually not sure....the prerenderer and instant versions should have
been
> > created off our web contents should we should have exactly the same session
> > storage state?  I mean, otherwise, our session storage was broken
previously.
> 
> Should be easy to check manually, right?  Just set a breakpoint here and see
if
> the current tab's map already has the namespaces for the newly copied entries.

At this point, I think I understand instant and prerender enough to know that we
should *not* copy over the session storage map.  On construction, they should
already have the right thing.

Let me address this in the prerender CL. I've added a note on my little
vim-checklist-file and removed this TODO.

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10831116/diff/2002/content/browser/web...
content/browser/web_contents/render_view_host_manager.cc:85:
site_instance->GetProcess()->GetID())));
On 2012/08/03 22:11:28, creis wrote:
> On 2012/08/03 00:31:04, awong wrote:
> > On 2012/08/02 23:06:47, creis wrote:
> > > Are we sure we want to call GetProcess() this early?  (This will create a
> > > process if we don't have one yet.)  Is it earlier than we used to?
> > 
> > I bet it probably is earlier. But I don't know how else to solve this since
> our
> > principal is the child process id. I guess we could make RVH take a
reference
> to
> > the controller and then request the storage namespace lazily. Would that
work
> > better?
> 
> 
> Looks like it's about the same time.  Before, we called it in
> RenderViewHostImpl's constructor, which happens during this Create call.  So,
> there's not much difference.

Your instincts were right. This doesn't work because the extension service
hasn't had a chance to "observer" the creation of this object meaning that
lookup functions break.

Instead, I've reimplemented to work based on site instance.  We need to better
understand our principal for isolation, but let's move that to another CL. This
one is getting unwieldy.

[Charlie Reis, 2012-08-06 20:38:30]

Mostly nits at this point.  This looks good once the tests are working.

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

https://chromiumcodereview.appspot.com/10831116/diff/11006/chrome/browser/chr...
chrome/browser/chrome_content_browser_client.cc:419:
GetInstalledAppForRenderer(child_process_id);
Huh, this API doesn't make sense.  There can be multiple apps in the same
process.  I've filed http://crbug.com/140856.

As you mentioned offline, it does work for this case, because isolated apps
won't share a process.  Please add a comment to that effect.

https://chromiumcodereview.appspot.com/10831116/diff/11006/chrome/browser/chr...
chrome/browser/chrome_content_browser_client.cc:442:
GetExtensionOrAppByURL(ExtensionURLInfo(instance->GetSite()));
These two functions are identical except in how they acquire their Extension. 
Can you use a helper function instead?

https://chromiumcodereview.appspot.com/10831116/diff/11006/chrome/browser/chr...
chrome/browser/chrome_content_browser_client.cc:457: if (partition_id.empty()) {
nit: No braces.

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

https://chromiumcodereview.appspot.com/10831116/diff/11006/chrome/browser/chr...
chrome/browser/chrome_content_browser_client.h:40: virtual std::string
GetStoragePartitionIdForSiteInstance(
Do we need the "ForX" in these two names, since child_process_id and
SiteInstance are already in the parameter list?

https://chromiumcodereview.appspot.com/10831116/diff/11006/chrome/browser/pre...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

https://chromiumcodereview.appspot.com/10831116/diff/11006/chrome/browser/pre...
chrome/browser/predictors/autocomplete_action_predictor.cc:170: if (it !=
session_storage_namespace_map.end()) {
nit: No braces.

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

https://chromiumcodereview.appspot.com/10831116/diff/11006/content/browser/br...
content/browser/browser_context.cc:166: DOMStorageContext*
BrowserContext::GetDOMStorageContextByPartitionId(
Is this worth exposing, as opposed to just calling
GetStoragePartitionByPartitionId(...)->dom_storage_context()?

[awong, 2012-08-08 18:08:27]

YES! WE'RE GREEN!

Charlie, I made two non-trivial changes since your last review.  Specifically:

- I added a GetDefaultSSN() function NavController.  This is facilitate
migration.

- There was a major bug in RenderMessageFilter which is fixed.

-Albert

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

http://codereview.chromium.org/10831116/diff/11006/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:442:
GetExtensionOrAppByURL(ExtensionURLInfo(instance->GetSite()));
On 2012/08/06 20:38:30, creis wrote:
> These two functions are identical except in how they acquire their Extension. 
> Can you use a helper function instead?

I'm not sure how I'd extract something that wouldn't look almost the same.

http://codereview.chromium.org/10831116/diff/11006/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:457: if (partition_id.empty()) {
On 2012/08/06 20:38:30, creis wrote:
> nit: No braces.

Done.

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

http://codereview.chromium.org/10831116/diff/11006/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.h:40: virtual std::string
GetStoragePartitionIdForSiteInstance(
On 2012/08/06 20:38:30, creis wrote:
> Do we need the "ForX" in these two names, since child_process_id and
> SiteInstance are already in the parameter list?

The style guide doesn't like overloading. :-/

Also, I want one of these to go away.  But I need to get further in the
refactoring first.

http://codereview.chromium.org/10831116/diff/11006/chrome/browser/predictors/...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

http://codereview.chromium.org/10831116/diff/11006/chrome/browser/predictors/...
chrome/browser/predictors/autocomplete_action_predictor.cc:170: if (it !=
session_storage_namespace_map.end()) {
On 2012/08/06 20:38:30, creis wrote:
> nit: No braces.

Done.

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

http://codereview.chromium.org/10831116/diff/11006/content/browser/browser_co...
content/browser/browser_context.cc:166: DOMStorageContext*
BrowserContext::GetDOMStorageContextByPartitionId(
On 2012/08/06 20:38:30, creis wrote:
> Is this worth exposing, as opposed to just calling
> GetStoragePartitionByPartitionId(...)->dom_storage_context()?

No it's not worth exposing.  But the other API doesn't exist yet. :)

That was precisely my next CL.

[michaeln, 2012-08-08 22:28:52]

http://codereview.chromium.org/10831116/diff/19178/chrome/browser/predictors/...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

http://codereview.chromium.org/10831116/diff/19178/chrome/browser/predictors/...
chrome/browser/predictors/autocomplete_action_predictor.cc:169:
session_storage_namespace_map.find("");
Is the default namespace always the correct answer here? Do isolatedapps live in
tabs with an omnibox with which users can navigate to other chrome-extension
urls that corresponds to the isolatedapp?

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
File content/browser/web_contents/interstitial_page_impl.cc (right):

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
content/browser/web_contents/interstitial_page_impl.cc:496:
BrowserContext::GetDOMStorageContextByPartitionId(browser_context,
Can this be simplified to GetDefaultDOMStorageContext?

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:1307:
session_storage_namespace_map_[partition_id] = session_storage_namespace;
Seems like having additional SSN's come into existence long after tab creation
will affect the sequence of events around the session save/restore logic (in
addition to the bundle of data to be saved/restored per tab). Marja should
definitely be in the loop.

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.h (right):

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.h:122:
content::SessionStorageNamespace* GetSessionStorageNamespace(
The asymmetry between the getter and the setter is weird.

[michaeln, 2012-08-09 00:22:50]

Also, there's enough moving around in this CL that it probably will have some
sort of impact on crbug/138152, hopefully for the better.

[awong, 2012-08-09 01:51:43]

Thanks for the comments Michael.

I think most of them will be addressed in the followup CLs so I'd rather leave
this CL as is.

As for affecting the DuplicateTab() issue, we can cross our fingers, but my gut
reaction is that this won't change much. :(

http://codereview.chromium.org/10831116/diff/19178/chrome/browser/predictors/...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

http://codereview.chromium.org/10831116/diff/19178/chrome/browser/predictors/...
chrome/browser/predictors/autocomplete_action_predictor.cc:169:
session_storage_namespace_map.find("");
On 2012/08/08 22:28:53, michaeln wrote:
> Is the default namespace always the correct answer here? Do isolatedapps live
in
> tabs with an omnibox with which users can navigate to other chrome-extension
> urls that corresponds to the isolatedapp?

This gets fixed here:
  http://codereview.chromium.org/10826110/

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
File content/browser/web_contents/interstitial_page_impl.cc (right):

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
content/browser/web_contents/interstitial_page_impl.cc:496:
BrowserContext::GetDOMStorageContextByPartitionId(browser_context,
On 2012/08/08 22:28:53, michaeln wrote:
> Can this be simplified to GetDefaultDOMStorageContext?

The goal is to completely remove GetDefaultDOMStorageContext() which should
happen in about 2 more CLs so I'd rather not make that change.

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.cc:1307:
session_storage_namespace_map_[partition_id] = session_storage_namespace;
On 2012/08/08 22:28:53, michaeln wrote:
> Seems like having additional SSN's come into existence long after tab creation
> will affect the sequence of events around the session save/restore logic (in
> addition to the bundle of data to be saved/restored per tab). Marja should
> definitely be in the loop.

Yep.  This is being tracked in another CL:
  http://codereview.chromium.org/10850010/

Originally, this CL had everything but I broke it up into 3 pieces.  Maybe I
should have done 4, but alas. :-/

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
File content/browser/web_contents/navigation_controller_impl.h (right):

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
content/browser/web_contents/navigation_controller_impl.h:122:
content::SessionStorageNamespace* GetSessionStorageNamespace(
On 2012/08/08 22:28:53, michaeln wrote:
> The asymmetry between the getter and the setter is weird.

Do you mean that the setter should take a SiteInstance?  I can add that as a
TODO, but would rather do a cleanup since this CL has already churned a lot.

[sreeram, 2012-08-09 15:31:25]

FYI: I just submitted http://crrev.com/150795, which changes the Instant stuff a
lot, so you'll need a rebaseline.

[awong, 2012-08-09 19:08:05]

Sreeram: I've rebased on top of the current index implementation and "fixed" the
code as much as possible to preserve current intent.  Can you review the instant
related files?

Michael: Can you review the webkit/dom_storage and content/*/dom_storage/*
changes?  I added a IsFromContext() function to safeguard the window.open()
codepath.

Charlie: you get the rest :)

[sreeram, 2012-08-09 19:20:04]

http://codereview.chromium.org/10831116/diff/169/chrome/browser/instant/insta...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/10831116/diff/169/chrome/browser/instant/insta...
chrome/browser/instant/instant_controller.cc:96:
session_storage_map2.find(it1->first);
Since the map is a std::map, with string keys, iteration order is well defined.
So why not just iterate them both in parallel, checking that the keys and values
are the same?

[awong, 2012-08-10 00:01:21]

Implemented sreeram's idea.

[sreeram, 2012-08-10 19:04:12]

LGTM for chrome/browser/instant/...

[michaeln, 2012-08-10 19:54:54]

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
File content/browser/web_contents/interstitial_page_impl.cc (right):

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
content/browser/web_contents/interstitial_page_impl.cc:493: site_instance);
This recipe for getting the default partition and then its DOMStorageContext is
what i was referring to. Kind of ugly. Why bother with the temp site_instance?

[awong, 2012-08-10 21:11:53]

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
File content/browser/web_contents/interstitial_page_impl.cc (right):

http://codereview.chromium.org/10831116/diff/19178/content/browser/web_conten...
content/browser/web_contents/interstitial_page_impl.cc:493: site_instance);
On 2012/08/10 19:54:55, michaeln wrote:
> This recipe for getting the default partition and then its DOMStorageContext
is
> what i was referring to. Kind of ugly. Why bother with the temp site_instance?

Ack...this is a straight up bug.

The site_instance here should be passed down below in the new
RenderViewHostImpl() call.  There shouldn't be a "throwaway" site instance. I
will fix that.

[Charlie Reis, 2012-08-13 19:39:21]

LGTM with the requests below.

Also, you've got a typo in the CL description: the first sentence of the last
paragraph should say "are created."

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

http://codereview.chromium.org/10831116/diff/11006/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:442:
GetExtensionOrAppByURL(ExtensionURLInfo(instance->GetSite()));
On 2012/08/08 18:08:28, awong wrote:
> On 2012/08/06 20:38:30, creis wrote:
> > These two functions are identical except in how they acquire their
Extension. 
> > Can you use a helper function instead?
> 
> I'm not sure how I'd extract something that wouldn't look almost the same.

Even if we're just sharing the last few lines and the intro comment (using a
helper function that takes in an Extension and returns a partition ID), I think
that's worthwhile.  This is a policy that might change over time, and we should
avoid letting it get stale.

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

http://codereview.chromium.org/10831116/diff/4608/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:443: if (extension &&
extension->is_storage_isolated()) {
As mentioned, please share from here down in a helper function, and move the
comment above to it.

nit: No braces on one line if.

http://codereview.chromium.org/10831116/diff/4608/content/browser/web_content...
File content/browser/web_contents/navigation_controller_impl.h (right):

http://codereview.chromium.org/10831116/diff/4608/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl.h:121: // process with
the given ID should use.
This comment is stale, since we now use the SiteInstance rather than process ID.

[awong, 2012-08-13 21:29:19]

Comments addressed.

Michael, did you have further comments? I'm itching to get this checked in.

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

http://codereview.chromium.org/10831116/diff/11006/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:442:
GetExtensionOrAppByURL(ExtensionURLInfo(instance->GetSite()));
On 2012/08/13 19:39:21, creis wrote:
> On 2012/08/08 18:08:28, awong wrote:
> > On 2012/08/06 20:38:30, creis wrote:
> > > These two functions are identical except in how they acquire their
> Extension. 
> > > Can you use a helper function instead?
> > 
> > I'm not sure how I'd extract something that wouldn't look almost the same.
> 
> Even if we're just sharing the last few lines and the intro comment (using a
> helper function that takes in an Extension and returns a partition ID), I
think
> that's worthwhile.  This is a policy that might change over time, and we
should
> avoid letting it get stale.

Done.

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

http://codereview.chromium.org/10831116/diff/4608/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.cc:443: if (extension &&
extension->is_storage_isolated()) {
On 2012/08/13 19:39:22, creis wrote:
> As mentioned, please share from here down in a helper function, and move the
> comment above to it.
> 
> nit: No braces on one line if.

Done.

http://codereview.chromium.org/10831116/diff/4608/content/browser/web_content...
File content/browser/web_contents/navigation_controller_impl.h (right):

http://codereview.chromium.org/10831116/diff/4608/content/browser/web_content...
content/browser/web_contents/navigation_controller_impl.h:121: // process with
the given ID should use.
On 2012/08/13 19:39:22, creis wrote:
> This comment is stale, since we now use the SiteInstance rather than process
ID.

Done.

[michaeln, 2012-08-13 21:49:13]

lgtm