Add "frame to navigate" to NavigationEntry and plumb it to the RenderViewImpl

Add "frame to navigate" to NavigationEntry and plumb it to the RenderViewImpl

This allows for navigating a specific frame from the browser. This is required
for running layout tests.

BUG=171391
TEST=RenderViewImplTest.NavigateFrame
R=creis@chromium.org,tsepez@chromium.org

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

[jochen (gone - plz use gerrit), 2013-01-22 20:01:37]

Tom, can you rubber stamp the IPC change

John. please review the rest

[Tom Sepez, 2013-01-22 20:04:02]

+creis - Charlie, is there a reason we disallow this kind of navigation?

[Tom Sepez, 2013-01-22 20:05:28]

The messages themselves LGTM, but wait for confirmation from charlie that this
does not open a new kind of hole.

[jam, 2013-01-22 20:20:18]

i defer my lgtm to creis

[Charlie Reis, 2013-01-23 01:38:45]

Yeah, we haven't needed to navigate subframes from the browser process before,
but I can see that this is useful for tests.  (It might be useful for
out-of-process iframes as well, but we would need IDs rather than names.  No
need to worry about that yet, though.)

I have a few questions below, but I haven't been able to think of any major
problems.  We already have to track renderer-initiated subframe navigations, so
this shouldn't affect the bookkeeping too much.

https://codereview.chromium.org/12052023/diff/1010/content/browser/web_conten...
File content/browser/web_contents/navigation_entry_impl.h (right):

https://codereview.chromium.org/12052023/diff/1010/content/browser/web_conten...
content/browser/web_contents/navigation_entry_impl.h:264: // If not empty, the
name of the frame to navigate.
Should this be persisted?  (See the warning above.)

I would worry about a restored tab sending the navigation to the main frame
instead of the intended subframe, even if this is only used in tests.

https://codereview.chromium.org/12052023/diff/1010/content/public/browser/nav...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/12052023/diff/1010/content/public/browser/nav...
content/public/browser/navigation_entry.h:188: virtual void
SetFrameToNavigate(const std::string& frame_name) = 0;
I suppose this is a name rather than a frame ID because of how the layout tests
work?  An ID could probably locate any frame rather than just the named ones,
but maybe that's not useful.

https://codereview.chromium.org/12052023/diff/1010/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12052023/diff/1010/content/renderer/render_vi...
content/renderer/render_view_impl.cc:1140: return;
Is there any way to trigger a navigation error instead of returning early?  It's
quite feasible for the frame to not be found (e.g., has just been deleted), and
that leaves the browser in a provisional load state without knowing it will
never finish.  Even if this is just used for tests, that seems like it could
lead to tests that easily time out.

https://codereview.chromium.org/12052023/diff/1010/content/renderer/render_vi...
content/renderer/render_view_impl.cc:1200: if (frame->isViewSourceModeEnabled())
Do subframes return true here when the main frame is in view-source mode?

[jochen (gone - plz use gerrit), 2013-01-23 08:22:42]

https://codereview.chromium.org/12052023/diff/1010/content/browser/web_conten...
File content/browser/web_contents/navigation_entry_impl.h (right):

https://codereview.chromium.org/12052023/diff/1010/content/browser/web_conten...
content/browser/web_contents/navigation_entry_impl.h:264: // If not empty, the
name of the frame to navigate.
On 2013/01/23 01:38:45, creis wrote:
> Should this be persisted?  (See the warning above.)
> 
> I would worry about a restored tab sending the navigation to the main frame
> instead of the intended subframe, even if this is only used in tests.

I'd rather not persist test-only data. I added a DCHECK to the session restore
system to make sure this doesn't happen accidentally.

https://codereview.chromium.org/12052023/diff/1010/content/public/browser/nav...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/12052023/diff/1010/content/public/browser/nav...
content/public/browser/navigation_entry.h:188: virtual void
SetFrameToNavigate(const std::string& frame_name) = 0;
On 2013/01/23 01:38:45, creis wrote:
> I suppose this is a name rather than a frame ID because of how the layout
tests
> work?  An ID could probably locate any frame rather than just the named ones,
> but maybe that's not useful.

Yes, it's the (unique) name. In a layout test, we can't hard code the frame id
since they're generated on the fly.

https://codereview.chromium.org/12052023/diff/1010/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12052023/diff/1010/content/renderer/render_vi...
content/renderer/render_view_impl.cc:1140: return;
On 2013/01/23 01:38:45, creis wrote:
> Is there any way to trigger a navigation error instead of returning early? 
It's
> quite feasible for the frame to not be found (e.g., has just been deleted),
and
> that leaves the browser in a provisional load state without knowing it will
> never finish.  Even if this is just used for tests, that seems like it could
> lead to tests that easily time out.

There is no code path to report an error back. If the passed URL is invalid for
example, we just crash.

Since that's the same as DRT does, we could just CHECK(false) as well.

https://codereview.chromium.org/12052023/diff/1010/content/renderer/render_vi...
content/renderer/render_view_impl.cc:1200: if (frame->isViewSourceModeEnabled())
On 2013/01/23 01:38:45, creis wrote:
> Do subframes return true here when the main frame is in view-source mode?

a view-source page can't have frames

[Charlie Reis, 2013-01-23 18:38:17]

Thanks for the session restore check.  LGTM with the changes below, since I want
to make it clear that this is only safe to use within tests.

https://codereview.chromium.org/12052023/diff/1010/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12052023/diff/1010/content/renderer/render_vi...
content/renderer/render_view_impl.cc:1140: return;
On 2013/01/23 08:22:42, jochen wrote:
> On 2013/01/23 01:38:45, creis wrote:
> > Is there any way to trigger a navigation error instead of returning early? 
> It's
> > quite feasible for the frame to not be found (e.g., has just been deleted),
> and
> > that leaves the browser in a provisional load state without knowing it will
> > never finish.  Even if this is just used for tests, that seems like it could
> > lead to tests that easily time out.
> 
> There is no code path to report an error back. If the passed URL is invalid
for
> example, we just crash.
> 
> Since that's the same as DRT does, we could just CHECK(false) as well.

Ok, since it's just for tests, that seems like it will do, and it's probably
better than a timeout.  (Otherwise, this is a totally legitimate case to hit, so
a CHECK would be the wrong thing to do.)

https://codereview.chromium.org/12052023/diff/6003/chrome/browser/sessions/se...
File chrome/browser/sessions/session_types.cc (right):

https://codereview.chromium.org/12052023/diff/6003/chrome/browser/sessions/se...
chrome/browser/sessions/session_types.cc:53:
DCHECK(entry.GetFrameToNavigate().empty());
Please make this a CHECK.  It could be a security issue if we navigate the wrong
frame in the wild.

https://codereview.chromium.org/12052023/diff/6003/content/browser/web_conten...
File content/browser/web_contents/navigation_entry_impl.h (right):

https://codereview.chromium.org/12052023/diff/6003/content/browser/web_conten...
content/browser/web_contents/navigation_entry_impl.h:265: // persisted.
Please elaborate: ", because it is currently only used in tests."

I want to make a point of it here, because there's a good chance I'll be looking
for something similar for out-of-process iframes, but this won't be right
without some extra work.

https://codereview.chromium.org/12052023/diff/6003/content/public/browser/nav...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/12052023/diff/6003/content/public/browser/nav...
content/public/browser/navigation_entry.h:187: // navigated. This is currently
not persisted in session restore.
Again, please mention ", since it is only used in tests."

[jochen (gone - plz use gerrit), 2013-01-23 19:52:45]

https://codereview.chromium.org/12052023/diff/6003/chrome/browser/sessions/se...
File chrome/browser/sessions/session_types.cc (right):

https://codereview.chromium.org/12052023/diff/6003/chrome/browser/sessions/se...
chrome/browser/sessions/session_types.cc:53:
DCHECK(entry.GetFrameToNavigate().empty());
On 2013/01/23 18:38:17, creis wrote:
> Please make this a CHECK.  It could be a security issue if we navigate the
wrong
> frame in the wild.

Done.

https://codereview.chromium.org/12052023/diff/6003/content/browser/web_conten...
File content/browser/web_contents/navigation_entry_impl.h (right):

https://codereview.chromium.org/12052023/diff/6003/content/browser/web_conten...
content/browser/web_contents/navigation_entry_impl.h:265: // persisted.
On 2013/01/23 18:38:17, creis wrote:
> Please elaborate: ", because it is currently only used in tests."
> 
> I want to make a point of it here, because there's a good chance I'll be
looking
> for something similar for out-of-process iframes, but this won't be right
> without some extra work.

Done.

https://codereview.chromium.org/12052023/diff/6003/content/public/browser/nav...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/12052023/diff/6003/content/public/browser/nav...
content/public/browser/navigation_entry.h:187: // navigated. This is currently
not persisted in session restore.
On 2013/01/23 18:38:17, creis wrote:
> Again, please mention ", since it is only used in tests."

Done.

[commit-bot: I haz the power, 2013-01-23 19:53:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/12052023/5004

[commit-bot: I haz the power, 2013-01-23 21:44:57]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...