Pick frame to navigate in the browser process.

Move frame_to_navigate logic to the browser process.

Layout tests can choose a frame by name to navigate, using
testRunner.queueLoad(url, name).  This frame should be
found in the browser process rather than RenderFrameImpl.

This is useful cleanup in general, but also makes it possible
for layout tests to target OOPIFs.

TBR=dcheng,sky
BUG=477150
TEST=Layout tests continue to pass

Committed: https://crrev.com/6a93a81c92cddec263040f5970113f126f733f4f
Cr-Commit-Position: refs/heads/master@{#326928}

[Charlie Reis, 2015-04-23 19:59:06]

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

[Charlie Reis, 2015-04-23 20:05:37]

Alex, can you take a look?

This was a TODO I wanted to kill in RenderFrameImpl::OnNavigate, now that we
track frame names in the browser process.  Should be a useful step toward
getting layout tests to work with OOPIFs as well.

Note that I was able to get by with a simple version of FindByName, since no
tests needed to pass things like _parent or _self to queueLoad.  There's a
successful linux_blink_rel try job in patch 2.

[alexmos, 2015-04-23 23:32:54]

LGTM.  The named frame lookup will also come in handy to implement postMessages
to named frames in AwContents::PostMessageToFrame for 389721.  Also a couple of
minor questions below.

https://codereview.chromium.org/1104603002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1104603002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:146: if (name.empty())
Even though it's not needed for layout tests, would it make sense to add
pseudoname support while we're here, so that it's ready when we later use this
in something like AwContents::PostMessageToFrame?  It seems that the
name.empty() case isn't intuitive unless you know that it's mimicking part of
the "return root" case in findFrameForNavigation, and the _current/_self/etc.
checks seem easy to add to bring it on par.  But it also seems fine to add these
later when they're needed.

https://codereview.chromium.org/1104603002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/1104603002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_unittest.cc:241: EXPECT_EQ(grandchild,
frame_tree->FindByName("grandchild"));
Would it be worth also checking that lookups for non-existing names/IDs fail
properly?

https://codereview.chromium.org/1104603002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1104603002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:742:
entry->set_frame_tree_node_id(params.frame_tree_node_id);
Could it ever be the case that params specify both frame_name and
frame_tree_node_id?  Probably not given that the frame_name is only used in
layout tests, but just wanted to double-check that frame_tree_node_id overriding
frame_name is desired.

[Charlie Reis, 2015-04-24 18:30:53]

creis@chromium.org changed reviewers:
+ dcheng@chromium.org, sky@chromium.org

[Charlie Reis, 2015-04-24 18:30:54]

[+sky, dcheng]

Thanks!

sky@, can you review content_serialized_navigation_builder.cc for OWNERS?

dcheng@, can you review the line removal from frame_messages.h for IPC OWNERS?

https://codereview.chromium.org/1104603002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1104603002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:146: if (name.empty())
On 2015/04/23 23:32:54, alexmos wrote:
> Even though it's not needed for layout tests, would it make sense to add
> pseudoname support while we're here, so that it's ready when we later use this
> in something like AwContents::PostMessageToFrame?  It seems that the
> name.empty() case isn't intuitive unless you know that it's mimicking part of
> the "return root" case in findFrameForNavigation, and the _current/_self/etc.
> checks seem easy to add to bring it on par.  But it also seems fine to add
these
> later when they're needed.

I think it's better to add them later, since the API needed for that is a bit
different.

More specifically, supporting pseudonames means the call needs to live on
FrameTreeNode rather than FrameTree, since "_self" and "_parent" only make sense
if you know what node you're starting from.  That version of the function will
also need to look through all the other frames in the BrowsingInstance, but only
if we don't find the frame in the current tree.

In that sense, I imagine we'll have a separate FrameTreeNode::FindByName which
first handles the pseudo names, then looks in its own tree using this as a
helper function, and then either consults a BrowsingInstance-wide map or
iterates over other frame trees.  This will be a useful helper function, but
let's avoid adding the rest until it's needed.

https://codereview.chromium.org/1104603002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/1104603002/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_unittest.cc:241: EXPECT_EQ(grandchild,
frame_tree->FindByName("grandchild"));
On 2015/04/23 23:32:54, alexmos wrote:
> Would it be worth also checking that lookups for non-existing names/IDs fail
> properly?

Good point.  Done.

https://codereview.chromium.org/1104603002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1104603002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:742:
entry->set_frame_tree_node_id(params.frame_tree_node_id);
On 2015/04/23 23:32:54, alexmos wrote:
> Could it ever be the case that params specify both frame_name and
> frame_tree_node_id?  Probably not given that the frame_name is only used in
> layout tests, but just wanted to double-check that frame_tree_node_id
overriding
> frame_name is desired.

It's theoretically possible to set both in a test, but it seems ok for the
frame_tree_node_id to take precedence if you specify it.  I'm not concerned for
the long term, since NavigationEntryImpl::set_frame_tree_node_id is going away
in the session history work and this will behave differently.

[Charlie Reis, 2015-04-24 22:16:40]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[Charlie Reis, 2015-04-24 22:16:40]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/1104603002/#ps60001
(title: "Add test cases")

[commit-bot: I haz the power, 2015-04-24 22:20:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1104603002/60001

[commit-bot: I haz the power, 2015-04-24 22:26:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-24 22:26:06]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2015-04-24 23:11:23]

TBR'ing dcheng and sky, since those files are simple line removals for now-dead
code.

[Charlie Reis, 2015-04-24 23:11:29]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-04-24 23:12:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1104603002/60001

[commit-bot: I haz the power, 2015-04-24 23:13:31]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-24 23:14:30]

Patchset 4 (id:??) landed as
https://crrev.com/6a93a81c92cddec263040f5970113f126f733f4f
Cr-Commit-Position: refs/heads/master@{#326928}

[dcheng, 2015-04-24 23:33:28]

rs lgtm for ipc changes