Prerendered tabs use the same SessionStorage namespace as the tab that triggered the prerender.

Prerendered tabs use the same SessionStorage namespace as the tab that triggered the prerender.

If the prerendered page is swapped in to a tab which has a different namespace, it will be canceled.

BUG=80679
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108342

[cbentzel, 2011-10-26 20:32:03]

This CL will share the sessionStorage namespace of the origin with the
prerendered tab. When considering whether the prerendered tab should be swapped
in, make sure that the namespace match.

This means that the prerendered page needs to be swapped in to the same tab that
generated it. It may remove the need for all of the window.opener logic as a
result, but I haven't done it in this CL.

I will try to add an omnibox-triggered-prerender-browsertest to this CL before
landing - one doesn't exist currently.

Also, in another CL I may cancel a prerender if it happens to access
sessionStorage or localStorage while prerendered, to minimize confusion. I'll
hold off for that.

In terms of implementation, the one thing I don't like about this CL is that it
needs to retain a reference to a SessionStorageNamespace object. I think it
would be cleaner if the int64 namespace id were passed into the
PrerenderManager/PrerenderContents and the SessionStorageNamespace would change
to have a constructor which used an id. Let me know what you think.

[cbentzel, 2011-10-26 20:48:05]

Actually - this doesn't work for the omnibox case so you may want to hold off
reviewing.

[cbentzel, 2011-10-26 21:48:29]

Omnibox works now, but still is lacking a browsertest. 

shishir: I mainly wanted you to look at this due to the impact on window.opener

mmenke: Review all of it.

dominich: Please look at the omnibox change. I'll need to 
probably get pkasting's blessing as well.

tburkard: FYI, please review if interested.

http://codereview.chromium.org/8392041/diff/10001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/8392041/diff/10001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:177: NULL);
Note: This works because AddPrerender will rediscover the
SessionStorageNamespace using child_id_ and route_id_. It also may be better to
pass in directly.

[dominich, 2011-10-26 21:57:55]

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:281: SessionStorageNamespace*
session_storage_namespace) {
A SessionStorageNamespace really doesn't make sense here. We might have one, but
it won't have anything in it, so passing one in and storing it is redundant.

Could we instead store NULL (or a null id) and check for that when checking
equivalence?

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:556: if
(old_render_view_host->session_storage_namespace()->id() !=
As you're checking ids here, why not just store the id instead of a pointer to
the namespace?

[cbentzel, 2011-10-26 22:03:48]

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:281: SessionStorageNamespace*
session_storage_namespace) {
On 2011/10/26 21:57:56, dominich wrote:
> A SessionStorageNamespace really doesn't make sense here. We might have one,
but
> it won't have anything in it, so passing one in and storing it is redundant.
> 
> Could we instead store NULL (or a null id) and check for that when checking
> equivalence?

Actually, a namespace makes sense here. In this case, it is simply for the tab
rather than the tab+origin of the domain.

We also need to establish the namespace at the time that the RenderView for the
prerendered page is created. If it's not done, it will use a different one than
the tab that could be swapped in.

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:556: if
(old_render_view_host->session_storage_namespace()->id() !=
On 2011/10/26 21:57:56, dominich wrote:
> As you're checking ids here, why not just store the id instead of a pointer to
> the namespace?

I mentioned that earlier in the CL as a possibility. It will require changes to
SessionStorageNamespace. 

Basically, there are two things we are using the SessionStorageNamespace for:

  - Comparing id's at the time that the prerendered view is swapped in.
  - Sending down a namespace to the prerendered RenderView at the time of it's
creation, which needs to match.

We could probably just hold onto an id if we changed the SessionStorageNamespace
to have a public constructor which takes an id at the time we create the
prerendered TabContents.

[dominich, 2011-10-26 22:21:24]

On 2011/10/26 22:03:48, cbentzel wrote:
>
http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
> File chrome/browser/prerender/prerender_manager.cc (right):
> 
>
http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
> chrome/browser/prerender/prerender_manager.cc:281: SessionStorageNamespace*
> session_storage_namespace) {
> On 2011/10/26 21:57:56, dominich wrote:
> > A SessionStorageNamespace really doesn't make sense here. We might have one,
> but
> > it won't have anything in it, so passing one in and storing it is redundant.
> > 
> > Could we instead store NULL (or a null id) and check for that when checking
> > equivalence?
> 
> Actually, a namespace makes sense here. In this case, it is simply for the tab
> rather than the tab+origin of the domain.
> 
> We also need to establish the namespace at the time that the RenderView for
the
> prerendered page is created. If it's not done, it will use a different one
than
> the tab that could be swapped in.

As per IM conversation, preserved here for posterity:

I understand why this is wanted, however I am concerned that this is a special
case fix rather than one to apply to the general case. It used to be a
reasonable assumption that a tab_contents was a persistent object matching the
lifetime of a Tab in the chrome interface. Things owned by that tab_contents
would have the lifetime of the Tab and thus things like the
SessionStorageNamespace that are per tab could safely live there. Now these
features are coming along that are swapping out tabs but keeping the Tab
persistent in the interface, like Prerender and Instant (which may also suffer
from the same problem, by the way).

I think we need a general solution to split the notion of the tab into the parts
that are persistent as far as the UI, and thus the user, is concerned, and those
parts that are swapped in and out such as the RenderViewHost.

Right now, only Instant and Prerender have this issue, and we should be able to
consolidate the tab swapping such that both of these features have the same code
path. In this case, it's not clear that a general solution has any great
benefits over the specific one here. But we do have a conceptual shift, and
there may be other features down the road that want to monkey around with tab
contents but retain the per-Tab appearance for the user. A general solution will
be better for future development.

> 
>
http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
> chrome/browser/prerender/prerender_manager.cc:556: if
> (old_render_view_host->session_storage_namespace()->id() !=
> On 2011/10/26 21:57:56, dominich wrote:
> > As you're checking ids here, why not just store the id instead of a pointer
to
> > the namespace?
> 
> I mentioned that earlier in the CL as a possibility. It will require changes
to
> SessionStorageNamespace. 
> 
> Basically, there are two things we are using the SessionStorageNamespace for:
> 
>   - Comparing id's at the time that the prerendered view is swapped in.
>   - Sending down a namespace to the prerendered RenderView at the time of it's
> creation, which needs to match.
> 
> We could probably just hold onto an id if we changed the
SessionStorageNamespace
> to have a public constructor which takes an id at the time we create the
> prerendered TabContents.

I don't know about the SessionStorageNamespace, but if it's trivial to create
one given an ID, holding that ID would be better. Reduced dependencies and
reduction of references to refcounted objects are my main concerns.

[mmenke, 2011-10-27 14:26:34]

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:1043:
tab->tab_contents()->render_view_host()->session_storage_namespace());
nit:  4 space indent

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:219: const RenderViewHost*
source_render_view_host) {
Is there a reason we can't just prevent |source_render_view_host| from being
NULL here, and obtain the session storage namespace directly from it?  Then we
also wouldn't need to keep our own reference to it around, either.  We could
just access it through prerender_contents.

Think it would simplify this CL, and the fewer things that own references to the
namespace, the better, in my opinion.

[cbentzel, 2011-10-27 17:14:02]

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:219: const RenderViewHost*
source_render_view_host) {
On 2011/10/27 14:26:34, Matt Menke wrote:
> Is there a reason we can't just prevent |source_render_view_host| from being
> NULL here, and obtain the session storage namespace directly from it?  

|source_render_view_host| is NULL when doing an omnibox triggered prerender.

I am changing this CL to pass int64's around (Well,
SessionStorageNamespace::IDs) around to simplify this.

[mmenke, 2011-10-27 17:21:35]

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/8392041/diff/6002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:219: const RenderViewHost*
source_render_view_host) {
On 2011/10/27 17:14:02, cbentzel wrote:
> On 2011/10/27 14:26:34, Matt Menke wrote:
> > Is there a reason we can't just prevent |source_render_view_host| from being
> > NULL here, and obtain the session storage namespace directly from it?  
> 
> |source_render_view_host| is NULL when doing an omnibox triggered prerender.
> 
> I am changing this CL to pass int64's around (Well,
> SessionStorageNamespace::IDs) around to simplify this.

Right, but we get the session storage namespace from a RenderView...Which we
could just pass along instead of the namespace.  The id would work fine, too.

[cbentzel, 2011-10-27 19:57:43]

It turns out we need to use the exact same SessionStorageNamespace object for
the prerendered contents as the original tab, instead of constructing a separate
SessionStorageNamespace object with the same id.

Why? 

Because when the SessionStorageNamespace's refcount goes to 0 it ends up
deleting the key->value dictionary for that SessionStorageNamespace, as well as
remove it from the overall SessionStorage directory.

So, the int64 id approach is a no-go. It's possible we can pass the
RenderViewHost around for a portion of time rather than the
SessionStorageNamespace, and I'll give that a shot.

I should also add a debug check to SessionStorageNamespace to make sure that
only one is ever created for a <WebKitContext, id> pair. 

So, when I was just passing the ID's around

[cbentzel, 2011-10-31 17:05:57]

I added a browser test to validate that the sessionStorage namespace is the same
when swapping in a prerendered page.

Also, this uses SessionStorageNamespace objects rather than ids. It turns out
that the SessionStorageNamespace destructor will deregister the namespace
associated with it - so the reference counting is used not only to make sure
that memory is freed, but also to manage the lifetime of other resources.

Please take another look.

[mmenke, 2011-11-02 02:27:55]

Figured I'd wait for the net-internals fix before reviewing.

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:1045: match.destination_url,
current_host->session_storage_namespace());
nit:  4 space indent

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (left):

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:814:
PrerenderTestURL("files/prerender/prerender_visibility_hidden.html",
nit:  The file this references is no longer needed.

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:824:
PrerenderTestURL("files/prerender/prerender_visibility_hidden_quick.html",
nit:  The file this references is no longer needed.

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:62: SessionStorageNamespace*
session_storage_namespace,
nit:  Think we should forward declare this, as it's not clear where we're
currently pulling it in from.

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:398:
data.contents_->StartPrerendering(source_render_view_host);
Is there a reason we can't just pass in sesion_storage_namespace here instead? 
Don't think PrerenderContents needs to own a ref to it.

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:106: // current contents.
"Destination tab rather than the current contents"?  What does that mean?

http://codereview.chromium.org/8392041/diff/24001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_loader_with_session_storage.html
(right):

http://codereview.chromium.org/8392041/diff/24001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_loader_with_session_storage.html:24: '<link
rel="prerender" href="REPLACE_WITH_PRERENDER_URL"/>');
nit:  4-space indent.

[cbentzel, 2011-11-02 15:57:08]

Newest patch fixes the PrerenderManager unittest and the NetInternalsTest.

It also incorporates some small fixes that you suggested, as well as changing
when SessionStorageNamespace gets passed to PrerenderContents which simplifies
things greatly.

I also played with passing in a RenderViewHost* into the AddPrerender calls
instead of the (process_id, route_id) pair and session_storage_namespace. This
looks like it may be promising, but will require a number of other changes. I'll
continue giving that a shot.

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:1045: match.destination_url,
current_host->session_storage_namespace());
On 2011/11/02 02:27:55, Matt Menke wrote:
> nit:  4 space indent

Done.

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (left):

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:814:
PrerenderTestURL("files/prerender/prerender_visibility_hidden.html",
On 2011/11/02 02:27:55, Matt Menke wrote:
> nit:  The file this references is no longer needed.

Done.

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:824:
PrerenderTestURL("files/prerender/prerender_visibility_hidden_quick.html",
On 2011/11/02 02:27:55, Matt Menke wrote:
> nit:  The file this references is no longer needed.

Done.

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:398:
data.contents_->StartPrerendering(source_render_view_host);
On 2011/11/02 02:27:55, Matt Menke wrote:
> Is there a reason we can't just pass in sesion_storage_namespace here instead?

> Don't think PrerenderContents needs to own a ref to it.

Ah, great suggestion! This made the code much simpler.

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/8392041/diff/24001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:106: // current contents.
On 2011/11/02 02:27:55, Matt Menke wrote:
> "Destination tab rather than the current contents"?  What does that mean?

Fixed comment - thanks for pointing this out.

http://codereview.chromium.org/8392041/diff/24001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_loader_with_session_storage.html
(right):

http://codereview.chromium.org/8392041/diff/24001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_loader_with_session_storage.html:24: '<link
rel="prerender" href="REPLACE_WITH_PRERENDER_URL"/>');
On 2011/11/02 02:27:55, Matt Menke wrote:
> nit:  4-space indent.

Done.

[mmenke, 2011-11-02 16:19:16]

Couple more nits.  Only one potentially significant issue.

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:549: // If the sessionStorage
namespaces don't match, don't swap the site in and
nit:  "session storage namespaces" or "SessionStorageNamespaces".  Also "site"
seems a little odd here, I'd suggest "TabContents" or "prerender" or some such
instead.

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:551: RenderViewHost*
old_render_view_host = tab_contents->render_view_host();
What happens if the old tab has crashed?  I think old_render_view_host may be
NULL, and we probably don't want to crash the browser here in a fit of pique. 
May be a little silly, but once we have omnibox prerender tests, may want to add
one for that, too.

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:107: const GURL& url,
nit:  4-space indent.

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/ui/webui/net...
File chrome/browser/ui/webui/net_internals_ui_browsertest.cc (right):

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_internals_ui_browsertest.cc:120: 
nit:  Blank line isn't needed here.

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_internals_ui_browsertest.cc:162: RenderViewHost*
host = browser_->GetTabContentsAt(1)->render_view_host();
nit:  Should indent two spaces here.

http://codereview.chromium.org/8392041/diff/32001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_loader_with_session_storage.html
(right):

http://codereview.chromium.org/8392041/diff/32001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_loader_with_session_storage.html:30: return
window.sessionStorage.counter == "2";
nit:  Single quotes in Javascript.

[cbentzel, 2011-11-02 16:55:51]

I'll fix the nits you point out - but the issue you raised is a non-issue.

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:551: RenderViewHost*
old_render_view_host = tab_contents->render_view_host();
On 2011/11/02 16:19:17, Matt Menke wrote:
> What happens if the old tab has crashed?  I think old_render_view_host may be
> NULL, and we probably don't want to crash the browser here in a fit of pique. 
> May be a little silly, but once we have omnibox prerender tests, may want to
add
> one for that, too.

It's old_render_view_host->view() which could be NULL if the render process
crashes, not old_render_view_host.

old_render_view_host is generally non-NULL whenever tab_contents is non-NULL.
There are two exceptions: during the construction phase of the TabContents
(before render_view_host_manager_.Init() is called) and during the destruction
phase (in render_view_host_manager_'s destructor when it NULLs out the value).
Neither is applicable here.

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/ui/webui/net...
File chrome/browser/ui/webui/net_internals_ui_browsertest.cc (right):

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_internals_ui_browsertest.cc:162: RenderViewHost*
host = browser_->GetTabContentsAt(1)->render_view_host();
On 2011/11/02 16:19:17, Matt Menke wrote:
> nit:  Should indent two spaces here.

Oi. Do you know of anything for VS which indents correctly? I got very used to
emacs which does.

[cbentzel, 2011-11-02 17:13:49]

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:549: // If the sessionStorage
namespaces don't match, don't swap the site in and
On 2011/11/02 16:19:17, Matt Menke wrote:
> nit:  "session storage namespaces" or "SessionStorageNamespaces".  Also "site"
> seems a little odd here, I'd suggest "TabContents" or "prerender" or some such
> instead.

Removed the "don't swap the site in" and just said "cancel the prerender".

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/ui/webui/net...
File chrome/browser/ui/webui/net_internals_ui_browsertest.cc (right):

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_internals_ui_browsertest.cc:162: RenderViewHost*
host = browser_->GetTabContentsAt(1)->render_view_host();
On 2011/11/02 16:19:17, Matt Menke wrote:
> nit:  Should indent two spaces here.

Done.

http://codereview.chromium.org/8392041/diff/32001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_loader_with_session_storage.html
(right):

http://codereview.chromium.org/8392041/diff/32001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_loader_with_session_storage.html:30: return
window.sessionStorage.counter == "2";
On 2011/11/02 16:19:17, Matt Menke wrote:
> nit:  Single quotes in Javascript.

Done.

[mmenke, 2011-11-02 17:44:07]

LGTM

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:551: RenderViewHost*
old_render_view_host = tab_contents->render_view_host();
On 2011/11/02 16:55:52, cbentzel wrote:
> On 2011/11/02 16:19:17, Matt Menke wrote:
> > What happens if the old tab has crashed?  I think old_render_view_host may
be
> > NULL, and we probably don't want to crash the browser here in a fit of
pique. 
> > May be a little silly, but once we have omnibox prerender tests, may want to
> add
> > one for that, too.
> 
> It's old_render_view_host->view() which could be NULL if the render process
> crashes, not old_render_view_host.
> 
> old_render_view_host is generally non-NULL whenever tab_contents is non-NULL.
> There are two exceptions: during the construction phase of the TabContents
> (before render_view_host_manager_.Init() is called) and during the destruction
> phase (in render_view_host_manager_'s destructor when it NULLs out the value).
> Neither is applicable here.
> 

You're right.  As you say, I was getting the RenderWidgetHostView confused with
the RenderWidgetHost.

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/ui/webui/net...
File chrome/browser/ui/webui/net_internals_ui_browsertest.cc (right):

http://codereview.chromium.org/8392041/diff/32001/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_internals_ui_browsertest.cc:162: RenderViewHost*
host = browser_->GetTabContentsAt(1)->render_view_host();
On 2011/11/02 16:55:52, cbentzel wrote:
> On 2011/11/02 16:19:17, Matt Menke wrote:
> > nit:  Should indent two spaces here.
> 
> Oi. Do you know of anything for VS which indents correctly? I got very used to
> emacs which does. 

Visual studio indents correctly, it's just emacs and the Google C++ style guide
that are wrong.  (Ok, I admit it... Actually, I prefer the emacs style here)

Unfortunately, I don't know of any way to get Visual Studio to indent like emacs
automatically in cases like this.  I generally just use shift-home-delete, and
then press tab until I have the right number of spaces.

[cbentzel, 2011-11-02 17:47:32]

+pkasting: chrome/browser/autocomplete/autocomplete_edit.cc
+jam: content/browser/renderer_host/render_view_host.h

[Peter Kasting, 2011-11-02 17:53:50]

LGTM

http://codereview.chromium.org/8392041/diff/31003/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/8392041/diff/31003/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:1043: if (current_host) {
Can this actually be NULL (maybe for about:blank)?  If not we shouldn't have the
conditional check.

[cbentzel, 2011-11-02 18:14:14]

http://codereview.chromium.org/8392041/diff/31003/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/8392041/diff/31003/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:1043: if (current_host) {
On 2011/11/02 17:53:56, Peter Kasting wrote:
> Can this actually be NULL (maybe for about:blank)?  If not we shouldn't have
the
> conditional check.

There's two periods during the TabContents constructor and destructor where it
can be NULL, but not at this point. I'll remove the check. Thanks.

[tburkard, 2011-11-02 18:36:16]

LGTM.

So this will be the first step which means we will only be able to swap into the
originating TabContents basically.

I guess down the road, we will look at whether we can hot swap session storage
based on whether it has been accessed or not?

Thanks.

[cbentzel, 2011-11-02 18:46:53]

On 2011/11/02 18:36:16, tburkard wrote:
> LGTM.
> 
> So this will be the first step which means we will only be able to swap into
the
> originating TabContents basically.
> 
> I guess down the road, we will look at whether we can hot swap session storage
> based on whether it has been accessed or not?

Yes. Let's look at how common this is. It will make the DOM storage code a bit
more complex so I want to hold off until it's justified.

[jam, 2011-11-02 19:08:58]

content lgtm

[commit-bot: I haz the power, 2011-11-02 19:24:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/8392041/26004

[commit-bot: I haz the power, 2011-11-02 20:28:39]

Change committed as 108342