Plumb opener information when creating RenderFrames and RenderFrameProxies for subframes.

Plumb opener information when creating RenderFrames and
RenderFrameProxies for subframes.

Frame opener updates (via window.open) make it possible for subframes
to have an opener.  Currently, when a subframe gains an opener, this
will be forwarded to the browser process and all of the renderer
processes where the subframe has a RenderFrame or a RenderFrameProxy.
Openers are also supposed to be preserved across cross-process frame
navigations.  Currently, this is done correctly for top-level frames
(where opener information is sent as part of ViewMsg_New), but not for
subframes: FrameMsg_NewFrame and FrameMsg_NewFrameProxy currently
don't pass any information about the frame's opener.

For example, suppose a frame F1 has two subframes, F2 and F3, all at
site A.  Suppose F1 executes a window.open('',F2_name) to update F2's
opener to F1.  Now suppose F2 navigates to B.  F2's frame in B should
have an opener that points to F1's proxy in B, but currently that's
not the case.  Likewise, suppose F3 navigates to C.  F2's proxy in C
should have an opener that points to F1's proxy in C.

This CL plumbs opener information through FrameMsg_NewFrame(Proxy) to
make these scenarios work correctly.

BUG=225940
Committed: https://crrev.com/a181efc0fa9b899a2a1f3a69053a1adb0b8683c1
Cr-Commit-Position: refs/heads/master@{#347072}

[alexmos, 2015-08-26 16:42:31]

alexmos@chromium.org changed reviewers:
+ creis@chromium.org

[alexmos, 2015-08-26 16:42:32]

Charlie, please take a look at this when you have a chance.  This is hopefully
the last of the update opener CLs.  The plumbing here is pretty straightforward
(although there are lots of affected files).  Another approach would've been to
use the UpdateOpener IPC after creating subframes with openers, but I was
concerned about race conditions.

[Charlie Reis, 2015-08-28 23:27:38]

Nice.  LGTM with nits.  Sorry for the delay!

https://codereview.chromium.org/1312643002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1312643002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:604: int opener_routing_id)
{
nit: The ordering here and below is a bit confusing.  Let's make them
consistent.

https://codereview.chromium.org/1312643002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:622:
params.opener_routing_id = opener_routing_id;
Maybe it should go: routing_id, proxy, opener, parent, previous_sibling?

Proxy corresponds to the same frame as routing_id (which we're about to
replace), and the others are all relationships to other frames.

It would be nice for this to be true throughout this CL, though that may cause
conflicts with dcheng's RenderWidget CL.  (Well, I guess it will conflict either
way.)

https://codereview.chromium.org/1312643002/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1312643002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3054: "foo.com",
"/frame_tree/page_with_two_frames.html");
nit: Let's use Nick's new cross_site_iframe_factory.html (from
https://codereview.chromium.org/1320023003/).

[alexmos, 2015-08-31 23:54:27]

alexmos@chromium.org changed reviewers:
+ nick@chromium.org

[alexmos, 2015-08-31 23:54:28]

+nick for the cross_site_iframe_factory question below.

https://codereview.chromium.org/1312643002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1312643002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:604: int opener_routing_id)
{
On 2015/08/28 23:27:38, Charlie Reis wrote:
> nit: The ordering here and below is a bit confusing.  Let's make them
> consistent.

Done.

https://codereview.chromium.org/1312643002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:622:
params.opener_routing_id = opener_routing_id;
On 2015/08/28 23:27:38, Charlie Reis wrote:
> Maybe it should go: routing_id, proxy, opener, parent, previous_sibling?
> 
> Proxy corresponds to the same frame as routing_id (which we're about to
> replace), and the others are all relationships to other frames.
> 
> It would be nice for this to be true throughout this CL, though that may cause
> conflicts with dcheng's RenderWidget CL.  (Well, I guess it will conflict
either
> way.)

Done.  I changed it to follow your suggested order here and elsewhere for
NewFrame.  I similarly fixed the order for NewFrameProxy plumbing to be
"routing_id, view_id, opener, parent".  

I'm actually a bit unsure whether we should put opener before or after parent in
both cases.  Parent seems more important and will be specified in more cases
(whereas opener will be non-null in fairly rare situations), so maybe it should
go first?

https://codereview.chromium.org/1312643002/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1312643002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3054: "foo.com",
"/frame_tree/page_with_two_frames.html");
On 2015/08/28 23:27:38, Charlie Reis wrote:
> nit: Let's use Nick's new cross_site_iframe_factory.html (from
> https://codereview.chromium.org/1320023003/).

I hit a problem with this in that I need the subframes to have names ('frame1'
is used below), which cross_site_iframe_factory doesn't assign currently.  We
could modify cross_site_iframe_factory to assign names, but frame names are
supposed to be global, so following the pattern that it uses for assigning IDs
may result in duplicate names across levels and possible confusion in tests.  I
left it as-is for now.

Nick, is there a good way to set a unique name for each frame in
cross_site_iframe_factory?

[alexmos, 2015-08-31 23:55:07]

alexmos@chromium.org changed reviewers:
+ nasko@chromium.org

[alexmos, 2015-08-31 23:55:08]

Nasko, could you please review the IPC changes?

[Charlie Reis, 2015-09-01 22:04:26]

LGTM

https://codereview.chromium.org/1312643002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1312643002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:622:
params.opener_routing_id = opener_routing_id;
On 2015/08/31 23:54:28, alexmos wrote:
> On 2015/08/28 23:27:38, Charlie Reis wrote:
> > Maybe it should go: routing_id, proxy, opener, parent, previous_sibling?
> > 
> > Proxy corresponds to the same frame as routing_id (which we're about to
> > replace), and the others are all relationships to other frames.
> > 
> > It would be nice for this to be true throughout this CL, though that may
cause
> > conflicts with dcheng's RenderWidget CL.  (Well, I guess it will conflict
> either
> > way.)
> 
> Done.  I changed it to follow your suggested order here and elsewhere for
> NewFrame.  I similarly fixed the order for NewFrameProxy plumbing to be
> "routing_id, view_id, opener, parent".  
> 
> I'm actually a bit unsure whether we should put opener before or after parent
in
> both cases.  Parent seems more important and will be specified in more cases
> (whereas opener will be non-null in fairly rare situations), so maybe it
should
> go first?

I suppose it could also go: routing_id, proxy, parent, previous_sibling, opener.
 The main thing is that routing_id and proxy should be adjacent (since they
refer to the same frame), and parent and previous_sibling should be adjacent
(since they both refer to frame tree relationships).  I could see opener as
state of the frame (thus before the parent/sibling) or less common than frame
tree neighbors (thus after the parent/sibling).

https://codereview.chromium.org/1312643002/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1312643002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:3054: "foo.com",
"/frame_tree/page_with_two_frames.html");
On 2015/08/31 23:54:28, alexmos wrote:
> On 2015/08/28 23:27:38, Charlie Reis wrote:
> > nit: Let's use Nick's new cross_site_iframe_factory.html (from
> > https://codereview.chromium.org/1320023003/).
> 
> I hit a problem with this in that I need the subframes to have names ('frame1'
> is used below), which cross_site_iframe_factory doesn't assign currently.  We
> could modify cross_site_iframe_factory to assign names, but frame names are
> supposed to be global, so following the pattern that it uses for assigning IDs
> may result in duplicate names across levels and possible confusion in tests. 
I
> left it as-is for now.
> 
> Nick, is there a good way to set a unique name for each frame in
> cross_site_iframe_factory?

Ah, no need, then.  We can change it if the factory ever gets that feature.

https://codereview.chromium.org/1312643002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1312643002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:197: int opener_routing_id);
Don't forget to update this order as well.

[alexmos, 2015-09-01 23:51:02]

Thanks for reviewing!

https://codereview.chromium.org/1312643002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1312643002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:622:
params.opener_routing_id = opener_routing_id;
On 2015/09/01 22:04:26, Charlie Reis (OOO till 9-8) wrote:
> On 2015/08/31 23:54:28, alexmos wrote:
> > On 2015/08/28 23:27:38, Charlie Reis wrote:
> > > Maybe it should go: routing_id, proxy, opener, parent, previous_sibling?
> > > 
> > > Proxy corresponds to the same frame as routing_id (which we're about to
> > > replace), and the others are all relationships to other frames.
> > > 
> > > It would be nice for this to be true throughout this CL, though that may
> cause
> > > conflicts with dcheng's RenderWidget CL.  (Well, I guess it will conflict
> > either
> > > way.)
> > 
> > Done.  I changed it to follow your suggested order here and elsewhere for
> > NewFrame.  I similarly fixed the order for NewFrameProxy plumbing to be
> > "routing_id, view_id, opener, parent".  
> > 
> > I'm actually a bit unsure whether we should put opener before or after
parent
> in
> > both cases.  Parent seems more important and will be specified in more cases
> > (whereas opener will be non-null in fairly rare situations), so maybe it
> should
> > go first?
> 
> I suppose it could also go: routing_id, proxy, parent, previous_sibling,
opener.
>  The main thing is that routing_id and proxy should be adjacent (since they
> refer to the same frame), and parent and previous_sibling should be adjacent
> (since they both refer to frame tree relationships).  I could see opener as
> state of the frame (thus before the parent/sibling) or less common than frame
> tree neighbors (thus after the parent/sibling).

Good reasoning -- I agree both orders make sense.  I don't have a strong
preference, so I'll keep the current order and see if Nasko has thoughts when he
does the IPC review.

https://codereview.chromium.org/1312643002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1312643002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:197: int opener_routing_id);
On 2015/09/01 22:04:26, Charlie Reis (OOO till 9-8) wrote:
> Don't forget to update this order as well.

Done.

[ncarter (slow), 2015-09-02 16:50:18]

nick@chromium.org changed reviewers:
- nick@chromium.org

[nasko, 2015-09-02 17:27:45]

LGTM once parameter order is fixed.

https://codereview.chromium.org/1312643002/diff/100001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1312643002/diff/100001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1636: int render_view_routing_id,
This doesn't match the header file, which seems to have them in the wrong order.

[alexmos, 2015-09-02 22:41:19]

https://codereview.chromium.org/1312643002/diff/100001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1312643002/diff/100001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1636: int render_view_routing_id,
On 2015/09/02 17:27:44, nasko (slow to review) wrote:
> This doesn't match the header file, which seems to have them in the wrong
order.

Done.  Thanks for catching this!

[alexmos, 2015-09-03 00:12:39]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2015-09-03 00:12:40]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1312643002/#ps120001
(title: "Fix param ordering in RenderThreadImpl::OnCreateNewFrameProxy")

[alexmos, 2015-09-03 00:13:07]

The CQ bit was unchecked by alexmos@chromium.org

[alexmos, 2015-09-03 00:13:32]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-09-03 00:31:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1312643002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1312643002/120001

[commit-bot: I haz the power, 2015-09-03 00:39:12]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-09-03 00:39:44]

Patchset 7 (id:??) landed as
https://crrev.com/a181efc0fa9b899a2a1f3a69053a1adb0b8683c1
Cr-Commit-Position: refs/heads/master@{#347072}