Create swapped-out opener RVHs after a process swap.

Create swapped-out opener RVHs after a process swap.
This is required to support cross-process JavaScript calls, like postMessage.

BUG=99202
TEST=window.opener present after a cross-process navigation.

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

[Charlie Reis, 2012-04-30 20:34:38]

Matt, can you review?  This CL makes sure that RenderViews exist in any
situation that we might want to support cross-process scripting.  I'll add the
scripting support itself in a follow-up CL.

[Matt Perry, 2012-05-01 20:45:16]

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:67:
registrar_.Remove(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSING,
Is this needed? See comment in web_contents_impl.cc

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:338: registrar_.Remove(this,
content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
You don't need to explicitly remove the notification here, do you? registrar_
will do it upon destruction.

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2592: // SiteInstance as well.
I'm confused.. Doesn't this mean the opener RenderViews will all share a
SiteInstance? It seems like RenderViewHostManager assumes that there is only 1
RenderView per SiteInstance (e.g. the swapped_out_hosts_ map).

[Charlie Reis, 2012-05-01 21:25:53]

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:67:
registrar_.Remove(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSING,
On 2012/05/01 20:45:17, Matt Perry wrote:
> Is this needed? See comment in web_contents_impl.cc

Ah, this one's not needed if registrar_ takes care of this on destruction.  I'll
remove it.

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:338: registrar_.Remove(this,
content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
On 2012/05/01 20:45:17, Matt Perry wrote:
> You don't need to explicitly remove the notification here, do you? registrar_
> will do it upon destruction.

This one's different.  We're listening to when our *opener* WebContents is
closed, and then clearing our opener_ field.  That can happen at any time, and
it's important to realize we no longer have an opener after that point.

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2592: // SiteInstance as well.
On 2012/05/01 20:45:17, Matt Perry wrote:
> I'm confused.. Doesn't this mean the opener RenderViews will all share a
> SiteInstance? It seems like RenderViewHostManager assumes that there is only 1
> RenderView per SiteInstance (e.g. the swapped_out_hosts_ map).

There's only one RVH per SiteInstance, for a given tab.  This CL is making sure
that other related tabs *also* have a RVH for that SiteInstance, so that the
tabs can find each other in the renderer process.

Basic use case:
Tab 1 has page A1, opens Tab 2 with page A2.  Tab 2's opener_ is Tab 1.
User navigates Tab 2 to page B2 in a different SiteInstance.
We transparently need to create a swapped out RVH for SiteInstance B in Tab 1.
This will let page B2 say window.opener.postMessage(msg).

[Charlie Reis, 2012-05-01 21:41:46]

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:67:
registrar_.Remove(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSING,
On 2012/05/01 21:25:53, creis wrote:
> On 2012/05/01 20:45:17, Matt Perry wrote:
> > Is this needed? See comment in web_contents_impl.cc
> 
> Ah, this one's not needed if registrar_ takes care of this on destruction. 
I'll
> remove it.

Removed.

[Matt Perry, 2012-05-01 21:45:56]

lgtm

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:562:
RenderViewHostMap::iterator iter = swapped_out_hosts_.find(instance->GetId());
nit: you can use GetSwappedOutRenderViewHost() now.

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:338: registrar_.Remove(this,
content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
On 2012/05/01 21:25:53, creis wrote:
> On 2012/05/01 20:45:17, Matt Perry wrote:
> > You don't need to explicitly remove the notification here, do you?
registrar_
> > will do it upon destruction.
> 
> This one's different.  We're listening to when our *opener* WebContents is
> closed, and then clearing our opener_ field.  That can happen at any time, and
> it's important to realize we no longer have an opener after that point.

We're in the WebContents destructor. Are you saying the opener could be
destroyed before we reach the end of the method?

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2592: // SiteInstance as well.
On 2012/05/01 21:25:53, creis wrote:
> On 2012/05/01 20:45:17, Matt Perry wrote:
> > I'm confused.. Doesn't this mean the opener RenderViews will all share a
> > SiteInstance? It seems like RenderViewHostManager assumes that there is only
1
> > RenderView per SiteInstance (e.g. the swapped_out_hosts_ map).
> 
> There's only one RVH per SiteInstance, for a given tab.  This CL is making
sure
> that other related tabs *also* have a RVH for that SiteInstance, so that the
> tabs can find each other in the renderer process.
> 
> Basic use case:
> Tab 1 has page A1, opens Tab 2 with page A2.  Tab 2's opener_ is Tab 1.
> User navigates Tab 2 to page B2 in a different SiteInstance.
> We transparently need to create a swapped out RVH for SiteInstance B in Tab 1.
> This will let page B2 say window.opener.postMessage(msg).

Where I was getting confused was that each tab has its own RVHManager. I think
I've got it now.

[Charlie Reis, 2012-05-01 21:57:05]

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
content/browser/web_contents/render_view_host_manager.cc:562:
RenderViewHostMap::iterator iter = swapped_out_hosts_.find(instance->GetId());
On 2012/05/01 21:45:56, Matt Perry wrote:
> nit: you can use GetSwappedOutRenderViewHost() now.

Good catch.  Fixed.

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10171018/diff/16002/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:338: registrar_.Remove(this,
content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
On 2012/05/01 21:45:56, Matt Perry wrote:
> On 2012/05/01 21:25:53, creis wrote:
> > On 2012/05/01 20:45:17, Matt Perry wrote:
> > > You don't need to explicitly remove the notification here, do you?
> registrar_
> > > will do it upon destruction.
> > 
> > This one's different.  We're listening to when our *opener* WebContents is
> > closed, and then clearing our opener_ field.  That can happen at any time,
and
> > it's important to realize we no longer have an opener after that point.
> 
> We're in the WebContents destructor. Are you saying the opener could be
> destroyed before we reach the end of the method?

D'oh!  That's what I get for not looking closely.  (Thought we were talking
about OnWebContentsDestroyed.)

Removed.

[Charlie Reis, 2012-05-01 22:00:26]

John, can you review for content and content/public OWNERS approval?

Scott, can you review for chrome/browser/visitedlink OWNERS approval?

[sky, 2012-05-01 22:20:56]

LGTM

[jam, 2012-05-02 17:42:29]

https://chromiumcodereview.appspot.com/10171018/diff/37002/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10171018/diff/37002/content/renderer/r...
content/renderer/render_view_impl.cc:1538: true, /* is_renderer_created */
nit: here and in other places, I'm really not a huge fan of adding these kind of
comments. if someone wants to know what the parameters are, they can easily look
at the header. the way i think about these kind of things is what would the code
look like if we applied it consistently; then it seems that we'd end up of extra
comments that would get in the way of reading the code.

/personal opinion so feel free to ignore.

[Charlie Reis, 2012-05-02 17:52:28]

https://chromiumcodereview.appspot.com/10171018/diff/37002/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10171018/diff/37002/content/renderer/r...
content/renderer/render_view_impl.cc:1538: true, /* is_renderer_created */
On 2012/05/02 17:42:30, John Abd-El-Malek wrote:
> nit: here and in other places, I'm really not a huge fan of adding these kind
of
> comments. if someone wants to know what the parameters are, they can easily
look
> at the header. the way i think about these kind of things is what would the
code
> look like if we applied it consistently; then it seems that we'd end up of
extra
> comments that would get in the way of reading the code.
> 
> /personal opinion so feel free to ignore.

That's fair.  We certainly aren't consistent about it, and I agree it's easy to
look up in the header.  Removed here and in render_thread_impl.cc and
render_view_test.cc.

[jam, 2012-05-02 19:09:11]

lgtm