Move browser initiated navigation from RenderViewHost to RenderFrameHost.

Move browser initiated navigation from RenderViewHost to RenderFrameHost.

BUG=304341
TBR=jochen@chromium.org

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

[nasko, 2014-02-12 21:05:56]

Hey Charlie,
I think this CL is in a shape ready for reviewing. It moves ViewMsg_Navigate and
the Navigate* methods from RenderViewHost to RenderFrameHost.
Thanks!
Nasako

[Charlie Reis, 2014-02-13 01:23:53]

Nice.  I suppose there's still work to do before we can support navigations in
multiple frames at once, but this is a good step.

https://codereview.chromium.org/148083013/diff/330001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/navigation_tracker.cc (right):

https://codereview.chromium.org/148083013/diff/330001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/navigation_tracker.cc:142: // TODO(nasko):
Revisit case 3, since now unload handlers are run in the
Good point.  Can you put a link to Camille's bug here for context?

https://codereview.chromium.org/148083013/diff/330001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/148083013/diff/330001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:337: // Navigate in the desired
RenderViewHost.
nit: RenderFrameHost.

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

https://codereview.chromium.org/148083013/diff/330001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:18: #include
"content/common/view_messages.h"
Why do we need this?

https://codereview.chromium.org/148083013/diff/330001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:366:
TRACE_EVENT0("renderer_host", "RenderFrameHostImpl::Navigate");
Does this require any other bookkeeping to register a new type of event?  And
we're in the frame_host directory rather than renderer_host.

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

https://codereview.chromium.org/148083013/diff/330001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:106: // Load the specified
URL, this is a shortcut for Navigate().
nit: As long as we're moving it, let's change the comma to a semicolon to fix
the run-on sentence.

https://codereview.chromium.org/148083013/diff/330001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/148083013/diff/330001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:686:
scoped_ptr<FrameMsg_Navigate_Params> suspended_nav_params_;
Hmm, a bunch of this navigation state belongs on RFH, because it's possible to
have navigations on two separate frames at the same time.  I suppose that's
beyond the scope of this CL (meaning we'll still only support navigation in one
RFH at a time), but we should at least put TODOs on each bit of state that needs
to move.  I think that includes at least:

navigations_suspended_
suspended_nav_params_
rvh_state_
is_waiting_for_beforeunload_ack_
unload_ack_is_for_cross_site_transition_
send_should_close_start_time_
unload_event_monitor_timeout_
pending_shutdown_on_swap_out_

https://codereview.chromium.org/148083013/diff/330001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/148083013/diff/330001/content/common/frame_me...
content/common/frame_messages.h:25:
IPC_ENUM_TRAITS_MAX_VALUE(FrameMsg_Navigate_Type::Value,
This wasn't a MAX_VALUE before.  Just making sure it stays valid?

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
content/renderer/render_frame_impl.cc:159: // NOTE(rogerm): We intentionally
perform an invalid heap access here in
Wow, I didn't realize we had these in the codebase.  Pretty amusing.  At least
they're ifdef'd out.

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
content/renderer/render_frame_impl.cc:211: static void
NotifyTimezoneChange(blink::WebFrame* frame) {
We shouldn't have two copies of this.  Can you make it accessible from
render_view_impl.cc similar to IsReload?

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
content/renderer/render_frame_impl.cc:579: frame =
render_view_->webview()->findFrameByName(
Add a TODO to move the lookup to the browser process?  (We won't need to pass
params.frame_to_navigate soon.)  :)

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
File content/renderer/render_view_impl.h (left):

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
content/renderer/render_view_impl.h:810: static blink::WebReferrerPolicy
GetReferrerPolicyFromRequest(
Why is this no longer needed?  Dead code?

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
content/renderer/render_view_impl.h:928: void OnNavigate(const
FrameMsg_Navigate_Params& params);
Maybe we should move this out of the IPC handlers section, now that it's just
for notifying observers?

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
content/renderer/render_view_impl.h:1182: scoped_ptr<FrameMsg_Navigate_Params>
pending_navigation_params_;
TODO to move this?

[nasko, 2014-02-13 17:05:26]

Fixes for all comments.

https://codereview.chromium.org/148083013/diff/330001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/navigation_tracker.cc (right):

https://codereview.chromium.org/148083013/diff/330001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/navigation_tracker.cc:142: // TODO(nasko):
Revisit case 3, since now unload handlers are run in the
On 2014/02/13 01:23:53, Charlie Reis wrote:
> Good point.  Can you put a link to Camille's bug here for context?

Done.

https://codereview.chromium.org/148083013/diff/330001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/148083013/diff/330001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:337: // Navigate in the desired
RenderViewHost.
On 2014/02/13 01:23:53, Charlie Reis wrote:
> nit: RenderFrameHost.

Done.

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

https://codereview.chromium.org/148083013/diff/330001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:18: #include
"content/common/view_messages.h"
On 2014/02/13 01:23:53, Charlie Reis wrote:
> Why do we need this?

Done.

https://codereview.chromium.org/148083013/diff/330001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:366:
TRACE_EVENT0("renderer_host", "RenderFrameHostImpl::Navigate");
On 2014/02/13 01:23:53, Charlie Reis wrote:
> Does this require any other bookkeeping to register a new type of event?  And
> we're in the frame_host directory rather than renderer_host.

Nothing else is required, this is used for about:tracing. Fixed the renderer
host.

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

https://codereview.chromium.org/148083013/diff/330001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:106: // Load the specified
URL, this is a shortcut for Navigate().
On 2014/02/13 01:23:53, Charlie Reis wrote:
> nit: As long as we're moving it, let's change the comma to a semicolon to fix
> the run-on sentence.

Done.

https://codereview.chromium.org/148083013/diff/330001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/148083013/diff/330001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:686:
scoped_ptr<FrameMsg_Navigate_Params> suspended_nav_params_;
On 2014/02/13 01:23:53, Charlie Reis wrote:
> Hmm, a bunch of this navigation state belongs on RFH, because it's possible to
> have navigations on two separate frames at the same time.  I suppose that's
> beyond the scope of this CL (meaning we'll still only support navigation in
one
> RFH at a time), but we should at least put TODOs on each bit of state that
needs
> to move.  I think that includes at least:
> 
> navigations_suspended_
> suspended_nav_params_
> rvh_state_
> is_waiting_for_beforeunload_ack_
> unload_ack_is_for_cross_site_transition_
> send_should_close_start_time_
> unload_event_monitor_timeout_
> pending_shutdown_on_swap_out_

I think a lot of other state will have to move as well, so I didn't mark them
explicitly. The ones you point out are navigational ones, so I've added TODOs.

https://codereview.chromium.org/148083013/diff/330001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/148083013/diff/330001/content/common/frame_me...
content/common/frame_messages.h:25:
IPC_ENUM_TRAITS_MAX_VALUE(FrameMsg_Navigate_Type::Value,
On 2014/02/13 01:23:53, Charlie Reis wrote:
> This wasn't a MAX_VALUE before.  Just making sure it stays valid?

Yes, this is a security hardening, as we are specifying what the max value of
the enum is and strict validation can be performed. See crbug.com/246708, where
tsepez@ has been going through all enums to convert them. This is my little drop
in the bucket towards the end goal ; ).

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
content/renderer/render_frame_impl.cc:159: // NOTE(rogerm): We intentionally
perform an invalid heap access here in
On 2014/02/13 01:23:53, Charlie Reis wrote:
> Wow, I didn't realize we had these in the codebase.  Pretty amusing.  At least
> they're ifdef'd out.

Done.

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
content/renderer/render_frame_impl.cc:211: static void
NotifyTimezoneChange(blink::WebFrame* frame) {
On 2014/02/13 01:23:53, Charlie Reis wrote:
> We shouldn't have two copies of this.  Can you make it accessible from
> render_view_impl.cc similar to IsReload?

Done.

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
content/renderer/render_frame_impl.cc:579: frame =
render_view_->webview()->findFrameByName(
On 2014/02/13 01:23:53, Charlie Reis wrote:
> Add a TODO to move the lookup to the browser process?  (We won't need to pass
> params.frame_to_navigate soon.)  :)

Done.

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
File content/renderer/render_view_impl.h (left):

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
content/renderer/render_view_impl.h:810: static blink::WebReferrerPolicy
GetReferrerPolicyFromRequest(
On 2014/02/13 01:23:53, Charlie Reis wrote:
> Why is this no longer needed?  Dead code?

Jochen removed the implementation in
https://codereview.chromium.org/135973009/diff/530001/content/renderer/render...,
but I didn't catch it at the time and I'm removing it as part of this CL.

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
content/renderer/render_view_impl.h:928: void OnNavigate(const
FrameMsg_Navigate_Params& params);
On 2014/02/13 01:23:53, Charlie Reis wrote:
> Maybe we should move this out of the IPC handlers section, now that it's just
> for notifying observers?

Done.

https://codereview.chromium.org/148083013/diff/330001/content/renderer/render...
content/renderer/render_view_impl.h:1182: scoped_ptr<FrameMsg_Navigate_Params>
pending_navigation_params_;
On 2014/02/13 01:23:53, Charlie Reis wrote:
> TODO to move this?

Done.

[nasko, 2014-02-13 17:06:43]

Adding jochen@ for OWNERS review of chrome/.

[Charlie Reis, 2014-02-13 19:27:20]

Looks like there might be a merge conflict, but otherwise I think it's about
ready to go.

https://codereview.chromium.org/148083013/diff/540001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/148083013/diff/540001/content/renderer/render...
content/renderer/render_frame_impl.cc:211: static void
NotifyTimezoneChange(blink::WebFrame* frame) {
Don't forget to remove this copy.

https://codereview.chromium.org/148083013/diff/540001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/148083013/diff/540001/content/renderer/render...
content/renderer/render_view_impl.h:1033: // TODO(nasko): Remove this method,
once all callers are migrated to use
This comment isn't quite right-- the only caller is RenderFrame, right?  We want
to remove this once we move this event to RenderFrameObserver.

[nasko, 2014-02-13 19:37:24]

Fixes and rebased.

https://codereview.chromium.org/148083013/diff/540001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/148083013/diff/540001/content/renderer/render...
content/renderer/render_frame_impl.cc:211: static void
NotifyTimezoneChange(blink::WebFrame* frame) {
On 2014/02/13 19:27:21, Charlie Reis wrote:
> Don't forget to remove this copy.

Doh! Done.

https://codereview.chromium.org/148083013/diff/540001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/148083013/diff/540001/content/renderer/render...
content/renderer/render_view_impl.h:1033: // TODO(nasko): Remove this method,
once all callers are migrated to use
On 2014/02/13 19:27:21, Charlie Reis wrote:
> This comment isn't quite right-- the only caller is RenderFrame, right?  We
want
> to remove this once we move this event to RenderFrameObserver.

You would think : ). Unit tests call directly into this method through
TestRenderViewHost.

[nasko, 2014-02-13 19:44:05]

On 2014/02/13 19:37:24, nasko wrote:
> Fixes and rebased.
> 
>
https://codereview.chromium.org/148083013/diff/540001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/148083013/diff/540001/content/renderer/render...
> content/renderer/render_frame_impl.cc:211: static void
> NotifyTimezoneChange(blink::WebFrame* frame) {
> On 2014/02/13 19:27:21, Charlie Reis wrote:
> > Don't forget to remove this copy.
> 
> Doh! Done.
> 
>
https://codereview.chromium.org/148083013/diff/540001/content/renderer/render...
> File content/renderer/render_view_impl.h (right):
> 
>
https://codereview.chromium.org/148083013/diff/540001/content/renderer/render...
> content/renderer/render_view_impl.h:1033: // TODO(nasko): Remove this method,
> once all callers are migrated to use
> On 2014/02/13 19:27:21, Charlie Reis wrote:
> > This comment isn't quite right-- the only caller is RenderFrame, right?  We
> want
> > to remove this once we move this event to RenderFrameObserver.
> 
> You would think : ). Unit tests call directly into this method through
> TestRenderViewHost.

And my comment here is wrong. It refers to browser side and this is renderer
side. Fixed the comment.

[Charlie Reis, 2014-02-13 19:49:10]

Thanks, LGTM.

[nasko, 2014-02-13 20:01:05]

Moved jochen@ to TBR, as the chrome/ change is just comments.

[nasko, 2014-02-13 20:01:23]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-02-13 20:12:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/148083013/710001

[commit-bot: I haz the power, 2014-02-13 22:17:28]

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

[commit-bot: I haz the power, 2014-02-13 22:17:29]

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

[nasko, 2014-02-14 00:11:27]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-02-14 00:13:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/148083013/710001

[commit-bot: I haz the power, 2014-02-14 02:02:24]

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

[commit-bot: I haz the power, 2014-02-14 02:02:25]

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

[jochen (gone - plz use gerrit), 2014-02-14 09:43:37]

chrome/ lgtm

[nasko, 2014-02-15 06:57:48]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-02-15 06:58:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/148083013/1470001

[commit-bot: I haz the power, 2014-02-15 08:36:38]

Change committed as 251563