Move DidFailProvisionalLoad handling from RenderView(Host) to RenderFrame(Host).

Move DidFailProvisionalLoad handling from RenderView(Host) to RenderFrame(Host).

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

[Charlie Reis, 2014-01-06 18:13:35]

Nice.  I'll take a look to see if we can get rid of some of these hacks, but
this seems like a good start for moving it over.  A few comments and nits below.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl_unittest.cc:966:
FrameHostMsg_DidFailProvisionalLoadWithError(0,  // routing_id
nit: This had the wrong indent before, so we should fix it.  Same below.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
content/browser/frame_host/navigator_delegate.h:29: // A provisional load in the
RenderFrameHost failed.
in the RenderFrameHost -> in |render_frame_host|

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:121: //
RenderFrameHostManager::RendererAbortedProvisionalLoad that is commented
We can remove this mention of RFHM::RendererAbortedProvisionalLoad now that it's
gone.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:130: RenderFrameHostManager*
root_rfhm = render_frame_host->frame_tree_node()
Nit: I wonder if there's a better way to format this line without all the
newline+arrows, though it's not a big deal.  Maybe we could grab the root and
then check root->render_manager()->interstitial_page() != NULL?

(Side note: I'd love to find a way to remove this whole hack, but that's better
to do after we move it.)

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:137: }
Let's put the useful part of the RFHM::RendererAbortedProvisionalLoad comment
here.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:58: FrameTreeNode*
frame_tree_node() { return frame_tree_node_; }
Do we need to expose FrameTreeNode yet?  Looks like we're just using it to get
to FrameTree, which we could expose instead.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:121: // Reference to the
whole frame tree that this RenderFrameHost belongs too.
nit: too -> to

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:361: // (Navigate,
DidNavigate, etc), which will clean it up properly.
While it makes sense to remove this method, I think we want to keep the first
part of this comment in the call site (NavigatorImpl).  It's another thing that
needs to be fixed, but not in this CL.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:366: // the response is
not a download.
This is an ancient TODO from 2008, when I was loosely planning to move all the
swapping logic to the equivalent of CSRH.  I think we're planning to keep the
logic where it is for browser-initiated navigations (including typed URLs to
cross-site downloads), so let's go ahead and remove the TODO.

Perhaps we should replace it with a simpler:
TODO(creis): Find a way to cancel any pending RFH here.

https://codereview.chromium.org/118553006/diff/2/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/118553006/diff/2/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2218: /*
Don't forget to delete this now that it's moved.

https://codereview.chromium.org/118553006/diff/2/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/118553006/diff/2/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:875:
render_view_->renderer_preferences_.accept_languages,
We're relying a lot on being friends with RenderViewImpl here and below.  What's
the plan for removing that?  Will renderer_preferences_ be accessible?  Will
pending_navigation_params_ move entirely to here?

(It's ok if we don't do all of that at once, but I'd like to make sure we have a
path for removing the friend keyword.)

[nasko, 2014-01-06 18:47:02]

Thanks for the review Charlie!
Still need to look at one failing content_browsertest case.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl_unittest.cc:966:
FrameHostMsg_DidFailProvisionalLoadWithError(0,  // routing_id
On 2014/01/06 18:13:35, Charlie Reis (OOO) wrote:
> nit: This had the wrong indent before, so we should fix it.  Same below.

Done.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
content/browser/frame_host/navigator_delegate.h:29: // A provisional load in the
RenderFrameHost failed.
On 2014/01/06 18:13:35, Charlie Reis (OOO) wrote:
> in the RenderFrameHost -> in |render_frame_host|

Done.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:121: //
RenderFrameHostManager::RendererAbortedProvisionalLoad that is commented
On 2014/01/06 18:13:35, Charlie Reis (OOO) wrote:
> We can remove this mention of RFHM::RendererAbortedProvisionalLoad now that
it's
> gone.

Done.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:130: RenderFrameHostManager*
root_rfhm = render_frame_host->frame_tree_node()
On 2014/01/06 18:13:35, Charlie Reis (OOO) wrote:
> Nit: I wonder if there's a better way to format this line without all the
> newline+arrows, though it's not a big deal.  Maybe we could grab the root and
> then check root->render_manager()->interstitial_page() != NULL?
> 
> (Side note: I'd love to find a way to remove this whole hack, but that's
better
> to do after we move it.)

Done.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:137: }
On 2014/01/06 18:13:35, Charlie Reis (OOO) wrote:
> Let's put the useful part of the RFHM::RendererAbortedProvisionalLoad comment
> here.

Done.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:58: FrameTreeNode*
frame_tree_node() { return frame_tree_node_; }
On 2014/01/06 18:13:35, Charlie Reis (OOO) wrote:
> Do we need to expose FrameTreeNode yet?  Looks like we're just using it to get
> to FrameTree, which we could expose instead.

Exposing FrameTreeNode seems more logical, since it is the object closest to RFH
and allows us to get to FrameTree if needed. Conceptually, RFH knows which
FrameTreeNode it belongs to, so it makes sense to expose it. I also believe we
will need it for other cases in the near future.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:121: // Reference to the
whole frame tree that this RenderFrameHost belongs too.
On 2014/01/06 18:13:35, Charlie Reis (OOO) wrote:
> nit: too -> to

Done.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (left):

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:361: // (Navigate,
DidNavigate, etc), which will clean it up properly.
On 2014/01/06 18:13:35, Charlie Reis (OOO) wrote:
> While it makes sense to remove this method, I think we want to keep the first
> part of this comment in the call site (NavigatorImpl).  It's another thing
that
> needs to be fixed, but not in this CL.

Done.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:366: // the response is
not a download.
On 2014/01/06 18:13:35, Charlie Reis (OOO) wrote:
> This is an ancient TODO from 2008, when I was loosely planning to move all the
> swapping logic to the equivalent of CSRH.  I think we're planning to keep the
> logic where it is for browser-initiated navigations (including typed URLs to
> cross-site downloads), so let's go ahead and remove the TODO.
> 
> Perhaps we should replace it with a simpler:
> TODO(creis): Find a way to cancel any pending RFH here.

Done.

https://codereview.chromium.org/118553006/diff/2/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/118553006/diff/2/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2218: /*
On 2014/01/06 18:13:35, Charlie Reis (OOO) wrote:
> Don't forget to delete this now that it's moved.

Done.

https://codereview.chromium.org/118553006/diff/2/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/118553006/diff/2/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:875:
render_view_->renderer_preferences_.accept_languages,
On 2014/01/06 18:13:35, Charlie Reis (OOO) wrote:
> We're relying a lot on being friends with RenderViewImpl here and below. 
What's
> the plan for removing that?  Will renderer_preferences_ be accessible?  Will
> pending_navigation_params_ move entirely to here?
> 
> (It's ok if we don't do all of that at once, but I'd like to make sure we have
a
> path for removing the friend keyword.)

I don't have a very concrete long term plan yet, just a rough idea. All
navigation related state will be better understood once we start moving session
history to the browser, so things like page_id_ will be better understood at
that time.

For things like renderer_preferences_, that are page global, they will move to
whatever is the PageContext equivalent in content. The same thing that will have
to hold the FrameTree on the renderer side and other moved-out-of-Blink
functionality.

[Charlie Reis, 2014-01-06 19:07:17]

Thanks!  LGTM with one nit.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:119: // We can't tell this apart,
so we think we're tearing down the current page
Please keep the first sentence (and put it at the end of the previous
paragraph).

nit: later one -> later on.

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:58: FrameTreeNode*
frame_tree_node() { return frame_tree_node_; }
On 2014/01/06 18:47:03, nasko wrote:
> On 2014/01/06 18:13:35, Charlie Reis (OOO) wrote:
> > Do we need to expose FrameTreeNode yet?  Looks like we're just using it to
get
> > to FrameTree, which we could expose instead.
> 
> Exposing FrameTreeNode seems more logical, since it is the object closest to
RFH
> and allows us to get to FrameTree if needed. Conceptually, RFH knows which
> FrameTreeNode it belongs to, so it makes sense to expose it. I also believe we
> will need it for other cases in the near future.

That's fair.  Let's leave it as is.

https://codereview.chromium.org/118553006/diff/2/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/118553006/diff/2/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:875:
render_view_->renderer_preferences_.accept_languages,
On 2014/01/06 18:47:03, nasko wrote:
> On 2014/01/06 18:13:35, Charlie Reis (OOO) wrote:
> > We're relying a lot on being friends with RenderViewImpl here and below. 
> What's
> > the plan for removing that?  Will renderer_preferences_ be accessible?  Will
> > pending_navigation_params_ move entirely to here?
> > 
> > (It's ok if we don't do all of that at once, but I'd like to make sure we
have
> a
> > path for removing the friend keyword.)
> 
> I don't have a very concrete long term plan yet, just a rough idea. All
> navigation related state will be better understood once we start moving
session
> history to the browser, so things like page_id_ will be better understood at
> that time.
> 
> For things like renderer_preferences_, that are page global, they will move to
> whatever is the PageContext equivalent in content. The same thing that will
have
> to hold the FrameTree on the renderer side and other moved-out-of-Blink
> functionality.

Ok.  We can try to nail down these plans in the near future.

[nasko, 2014-01-06 19:16:30]

Fixed.
Thanks!

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/118553006/diff/2/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:119: // We can't tell this apart,
so we think we're tearing down the current page
On 2014/01/06 19:07:17, Charlie Reis (OOO) wrote:
> Please keep the first sentence (and put it at the end of the previous
> paragraph).
> 
> nit: later one -> later on.

Done.

[commit-bot: I haz the power, 2014-01-06 20:10:33]

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

[commit-bot: I haz the power, 2014-01-06 20:10:46]

Failed to apply patch for content/browser/frame_host/render_frame_host_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/frame_host/render_frame_host_impl.cc
  Hunk #1 FAILED at 115.
  Hunk #2 succeeded at 173 with fuzz 1 (offset 19 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
content/browser/frame_host/render_frame_host_impl.cc.rej

Patch:       content/browser/frame_host/render_frame_host_impl.cc
Index: content/browser/frame_host/render_frame_host_impl.cc
diff --git a/content/browser/frame_host/render_frame_host_impl.cc
b/content/browser/frame_host/render_frame_host_impl.cc
index
620d87dc08004cbd4e42842132f61186c9543e5b..4a1cb8a4d1b4131f50527329a8ec24721850a48b
100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -115,6 +115,8 @@ bool RenderFrameHostImpl::OnMessageReceived(const
IPC::Message &msg) {
     IPC_MESSAGE_HANDLER(FrameHostMsg_Detach, OnDetach)
     IPC_MESSAGE_HANDLER(FrameHostMsg_DidStartProvisionalLoadForFrame,
                         OnDidStartProvisionalLoadForFrame)
+    IPC_MESSAGE_HANDLER(FrameHostMsg_DidFailProvisionalLoadWithError,
+                        OnDidFailProvisionalLoadWithError)
   IPC_END_MESSAGE_MAP_EX()
 
   if (!msg_is_ok) {
@@ -154,4 +156,9 @@ void RenderFrameHostImpl::OnDidStartProvisionalLoadForFrame(
       this, frame_id, parent_frame_id, is_main_frame, url);
 }
 
+void RenderFrameHostImpl::OnDidFailProvisionalLoadWithError(
+    const FrameHostMsg_DidFailProvisionalLoadWithError_Params& params) {
+  frame_tree_node_->navigator()->DidFailProvisionalLoadWithError(this, params);
+}
+
 }  // namespace content

[commit-bot: I haz the power, 2014-01-06 20:48:24]

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

[commit-bot: I haz the power, 2014-01-06 23:57:17]

Change committed as 243208