Move DidStartLoading, DidStopLoading, DidChangeLoadProgress to RFHI.

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 d4cc6f50c5f352ef7c908d8f5601785b97335ec6..604c3ce8aea5c2f078b1c7595201648abbbbdfff 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -385,6 +385,10 @@ bool RenderFrameHostImpl::OnMessageReceived(const IPC::Message &msg) {
     // The following message is synthetic and doesn't come from RenderFrame, but
     // from RenderProcessHost.
     IPC_MESSAGE_HANDLER(FrameHostMsg_RenderProcessGone, OnRenderProcessGone)
+    IPC_MESSAGE_HANDLER(FrameHostMsg_DidStartLoading, OnDidStartLoading)
+    IPC_MESSAGE_HANDLER(FrameHostMsg_DidStopLoading, OnDidStopLoading)
+    IPC_MESSAGE_HANDLER(FrameHostMsg_DidChangeLoadProgress,
+                        OnDidChangeLoadProgress)
 #if defined(OS_MACOSX) || defined(OS_ANDROID)
     IPC_MESSAGE_HANDLER(FrameHostMsg_ShowPopup, OnShowPopup)
     IPC_MESSAGE_HANDLER(FrameHostMsg_HidePopup, OnHidePopup)
@@ -842,11 +846,11 @@ void RenderFrameHostImpl::OnDidCommitProvisionalLoad(const IPC::Message& msg) {
 }
 
 void RenderFrameHostImpl::OnDidDropNavigation() {
-  // At the end of Navigate(), the delegate's DidStartLoading is called to force
-  // the spinner to start, even if the renderer didn't yet begin the load. If it
-  // turns out that the renderer dropped the navigation, we need to turn off the
-  // spinner.
-  delegate_->DidStopLoading();
+  // At the end of Navigate(), the Navigator's DidStartLoading is called to
+  // force the spinner to start, even if the renderer didn't yet begin the load.
+  // If it turns out that the renderer dropped the navigation, the spinner needs
+  // to be turned off.
+  frame_tree_node_->navigator()->DidStopLoading();

[Fabrice (no longer in Chrome), 2015/04/14 16:54:11]

We have to go through the navigator here because calling
FrameTreeNode::DidStopLoading will not necessarily end up calling
WebContentsImpl::DidStopLoading.
That is also why we introduced the "page_start_loading_with_frame" argument on
the other CL.

[nasko, 2015/04/14 19:54:41]

I don't quite follow. During Navigate() we simulate DidStartLoading. If this is
FTN level operation, than it makes sense that undoing its work is calling
DidStopLoading on the FTN as well.
Why would we expect that WebContentsImpl::DidStopLoading is called for every
invocation of this code? Can't we have two sibling iframes start loading and
stop loading through those paths and we only need to do one call to
WebContentsImpl::DidStopLoading.

[Fabrice (no longer in Chrome), 2015/04/15 15:15:03]

I think this is going to break some tests, trying it in next CL.

[nasko, 2015/04/15 15:39:28]

Different CL or a new patchset?

[Fabrice (no longer in Chrome), 2015/04/15 17:46:05]

New patchset, sorry.

 }
 
 RenderWidgetHostImpl* RenderFrameHostImpl::GetRenderWidgetHost() {
@@ -1403,6 +1407,45 @@ void RenderFrameHostImpl::OnToggleFullscreen(bool enter_fullscreen) {
   render_view_host_->WasResized();
 }
 
+void RenderFrameHostImpl::OnDidStartLoading(bool to_different_document) {
+  // Any main frame load to a new document should reset the load since it will
+  // replace the current page and any frames.
+  if (to_different_document && !GetParent())
+    is_loading_ = false;
+
+  // This method should never be called when the frame is loading.
+  // Unfortunately, it can happen if a history navigation happens during a
+  // BeforeUnload or Unload event.
+  // TODO(fdegans): Change this to a DCHECK after LoadEventProgress has been
+  // refactored in Blink. See crbug.com/466089
+  if (is_loading_) {
+    LOG(WARNING) << "OnDidStartLoading was called twice.";
+    return;
+  }
+
+  frame_tree_node_->DidStartLoading(to_different_document);

[Fabrice (no longer in Chrome), 2015/04/14 16:54:11]

Here and below, it is not clear to me that this will end up calling into
WebContents and "doing the right thing". We're adding a new level of indirection
and increasing code complexity.
I had to reread what I was doing multiple times while checking tests were still
passing locally and found it confusing. I cannot imagine how it will be for
someone who has not written this part of the code.

[nasko, 2015/04/14 19:54:41]

It is the job of the FTN to ensure that it does the right thing. The RFH only
needs to notify its FTN that it has entered loading state.
I actually find this a bit easier to reason about, as no RFH on its own knows
what "the right thing" to do is. It is only the FrameTree and its FTNs, having a
global picture of all loads that can determine it. So doing something in RFH
here is very counterintuitive to me.
It will be useful to know what made it hard for you to understand what is going
on. Could you elaborate on that?
Code needs to be easy to reason about if we want to have a complex piece of
software, such as Chrome, to be maintainable.

[Fabrice (no longer in Chrome), 2015/04/15 15:15:03]

The main issue is you now have to consider 4 different files when trying to
debug this piece of code. However, it is now made simpler because we consider
we're notifying the WebContents from FrameTree and FrameTreeNode while RFHI
simply notifies FrameTreeNode.
There are also two distinct "call stacks" that both end up in WebContents. One
is about the loading status and the other about the load progress. The first
goes "RFHI -> FTN -> WebContents" while the latter goes "RFHI -> FTN -> FT ->
WebContents". I have changed the comments to make that clearer.

+  is_loading_ = true;
+}
+
+void RenderFrameHostImpl::OnDidStopLoading() {
+  // This method should never be called when the frame is not loading.
+  // Unfortunately, it can happen if a history navigation happens during a
+  // BeforeUnload or Unload event.
+  // TODO(fdegans): Change this to a DCHECK after LoadEventProgress has been
+  // refactored in Blink. See crbug.com/466089
+  if (!is_loading_) {
+    LOG(WARNING) << "OnDidStopLoading was called twice.";
+    return;
+  }
+
+  is_loading_ = false;
+  frame_tree_node_->DidStopLoading();

[Fabrice (no longer in Chrome), 2015/04/14 16:54:11]

Same as above, it is not clear we'll end up in WebContents.

[nasko, 2015/04/14 19:54:41]

Why do we care to know here whether we end up in WebContents or not? RFH on its
own can never know that.

[Fabrice (no longer in Chrome), 2015/04/15 15:15:03]

Acknowledged.

+}
+
+void RenderFrameHostImpl::OnDidChangeLoadProgress(double load_progress) {
+  frame_tree_node_->DidChangeLoadProgress(load_progress);
+}
+
 #if defined(OS_MACOSX) || defined(OS_ANDROID)
 void RenderFrameHostImpl::OnShowPopup(
     const FrameHostMsg_ShowPopup_Params& params) {
@@ -1573,7 +1616,7 @@ void RenderFrameHostImpl::Navigate(
   // Blink doesn't send throb notifications for JavaScript URLs, so we
   // don't want to either.
   if (!common_params.url.SchemeIs(url::kJavaScriptScheme))
-    delegate_->DidStartLoading(this, true);
+    frame_tree_node_->navigator()->DidStartLoading(frame_tree_node_, true);

[Fabrice (no longer in Chrome), 2015/04/14 16:54:11]

Same as above, we need to go through the Navigator.

[nasko, 2015/04/14 19:54:41]

I don't think we need to. FTN is the object which has view of all RFHs
representing that frame, so it can work together with the FrameTree to figure
out what the global loading state is.

[Fabrice (no longer in Chrome), 2015/04/15 15:15:03]

Same as above, trying it in the next patch set.

 }
 
 void RenderFrameHostImpl::NavigateToURL(const GURL& url) {