Move load progress tracking logic to the frame tree.

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 906e19a8051303833ff54cbacddf6a03dcfe3304..e41b558525122be861e4dbda8f6b364545e1dc8b 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -165,40 +165,6 @@ bool CollectSites(BrowserContext* context,
   return true;
 }
 
-// Helper function used with FrameTree::ForEach() for retrieving the total
-// loading progress and number of frames in a frame tree.
-bool CollectLoadProgress(double* progress,
-                         int* frame_count,
-                         FrameTreeNode* node) {
-  // Ignore the current frame if it has not started loading.
-  double frame_progress = node->GetLoadingProgress();
-  if (frame_progress == RenderFrameHostImpl::kLoadingProgressNotStarted)
-    return true;
-
-  // Collect progress.
-  *progress += node->GetLoadingProgress();
-  (*frame_count)++;
-  return true;
-}
-
-// Helper function used with FrameTree::ForEach() to check if at least one of
-// the nodes is loading.
-bool IsNodeLoading(bool* is_loading, FrameTreeNode* node) {
-  if (node->IsLoading()) {
-    // There is at least one node loading, so abort traversal.
-    *is_loading = true;
-    return false;
-  }
-  return true;
-}
-
-// Helper function used with FrameTree::ForEach() to reset the load progress.
-bool ResetLoadProgress(FrameTreeNode* node) {
-  node->current_frame_host()->set_loading_progress(
-      RenderFrameHostImpl::kLoadingProgressNotStarted);
-  return true;
-}
-
 bool ForEachFrameInternal(
     const base::Callback<void(RenderFrameHost*)>& on_frame,
     FrameTreeNode* node) {
@@ -260,12 +226,6 @@ bool SuddenTerminationAllowed(bool* sudden_termination_allowed,
   return false;
 }
 
-// Returns true if at least one of the nodes in the |frame_tree| is loading.
-bool IsFrameTreeLoading(FrameTree& frame_tree) {
-  bool is_loading = false;
-  frame_tree.ForEach(base::Bind(&IsNodeLoading, &is_loading));
-  return is_loading;
-}
 
 }  // namespace
 
@@ -2962,14 +2922,17 @@ void WebContentsImpl::OnDidStartLoading(bool to_different_document) {
     return;
   }
 
-  if (!IsFrameTreeLoading(frame_tree_))
+  if (!frame_tree_.IsLoading())
     DidStartLoading(rfh, to_different_document);
 
   rfh->set_is_loading(true);
-  rfh->set_loading_progress(RenderFrameHostImpl::kLoadingProgressMinimum);
+
+  FrameTreeNode* ftn = rfh->frame_tree_node();
+  if (ftn->render_manager()->current_frame_host() == rfh)

[clamy, 2015/03/24 14:52:23]

Should we reset the load progress even if its the pending frame host that starts
navigating? I know this is not what we do right now, I'm wondering if it makes
sense now that the value is in the FrameTreeNode.
If it doesn't, could you add a comment explaining that we only reset the loading
progress when the current RFH starts navigating.

[Fabrice (no longer in Chrome), 2015/03/24 16:28:50]

We probably should reset but that is going to make tracking things harder in the
other methods since these track the current load progress.
I'll just add a comment here.

+    ftn->set_loading_progress(FrameTreeNode::kLoadingProgressMinimum);
 
   // Notify the RenderFrameHostManager of the event.
-  rfh->frame_tree_node()->render_manager()->OnDidStartLoading();
+  ftn->render_manager()->OnDidStartLoading();
 
   SendLoadProgressChanged();
 }
@@ -2998,7 +2961,10 @@ void WebContentsImpl::OnDidStopLoading() {
   }
 
   rfh->set_is_loading(false);
-  rfh->set_loading_progress(RenderFrameHostImpl::kLoadingProgressDone);
+
+  FrameTreeNode* ftn = rfh->frame_tree_node();
+  if (ftn->render_manager()->current_frame_host() == rfh)

[clamy, 2015/03/24 14:52:23]

Can we get a DidStopLoading IPC from the pending RFH? I don't think so, but I
may be wrong. In that case, the if condition is unnecessary and should be
replaced with a DCHECK.

[Fabrice (no longer in Chrome), 2015/03/24 16:28:50]

We can and we do. In fact, when we track the overall loading progress, we track
both pending and current RFH.

[nasko, 2015/03/24 17:26:11]

I'm not sure we can get a DidStopLoading from the pending RFH, as it won't have
even committed a navigation. Do we have a test that does this?

[Fabrice (no longer in Chrome), 2015/03/25 13:25:12]

I have found that DownloadContentTest.RemoveDownload and
SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs in
content_browsertests do this.

+    ftn->set_loading_progress(FrameTreeNode::kLoadingProgressDone);
 
   // TODO(erikchen): Remove ScopedTracker below once crbug.com/465796 is
   // fixed.
@@ -3020,9 +2986,9 @@ void WebContentsImpl::OnDidStopLoading() {
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "465796 WebContentsImpl::OnDidStopLoading::NotifyRenderManager"));
   // Notify the RenderFrameHostManager of the event.
-  rfh->frame_tree_node()->render_manager()->OnDidStopLoading();
+  ftn->render_manager()->OnDidStopLoading();
 
-  if (!IsFrameTreeLoading(frame_tree_)) {
+  if (!frame_tree_.IsLoading()) {
     // TODO(erikchen): Remove ScopedTracker below once crbug.com/465796 is
     // fixed.
     tracked_objects::ScopedTracker tracking_profile4(
@@ -3044,8 +3010,12 @@ void WebContentsImpl::OnDidChangeLoadProgress(double load_progress) {
 
   RenderFrameHostImpl* rfh =
       static_cast<RenderFrameHostImpl*>(render_frame_message_source_);
+  FrameTreeNode* ftn = rfh->frame_tree_node();
+
+  if (ftn->render_manager()->current_frame_host() != rfh)

[clamy, 2015/03/24 14:52:23]

Same here. I don't think we should be getting this from a RFH different from our
current one.

[Fabrice (no longer in Chrome), 2015/03/24 16:28:50]

Same as above, we still get these events for pending RFH.

[nasko, 2015/03/24 17:26:11]

Really? Real progress cannot be made before we commit a navigation. Maybe you
are seeing the equivalent of "let's set our initial progress that we've started
doing something".

[Fabrice (no longer in Chrome), 2015/03/25 13:25:12]

I have found that the following tests send the event for the pending RFH:
    BrowserSideNavigationBrowserTest.BrowserInitiatedNavigations
    CrossProcessFrameTreeBrowserTest.CreateCrossProcessSubframeProxies
    CrossProcessFrameTreeBrowserTest.OriginSetOnCrossProcessNavigations
    CrossSiteTransferTest.ReplaceEntryCrossProcessThenTransfer
    CrossSiteTransferTest.ReplaceEntryCrossProcessTwice
    DownloadContentTest.RemoveDownload
    FrameTreeBrowserTest.NavigateWithLeftoverFrames
    FrameTreeBrowserTest.OriginSetOnNavigation
    NavigationControllerBrowserTest.CorrectLengthWithNewTabNavigatingFromWebUI
    RFHMProcessPerTabTest.BackFromWebUI
    RenderFrameHostManagerTest.AllowTargetedNavigationsAfterSwap
    RenderFrameHostManagerTest.AllowTargetedNavigationsInOpenerAfterSwap
    RenderFrameHostManagerTest.BackForwardNotStale
    RenderFrameHostManagerTest.DisownOpener
    RenderFrameHostManagerTest.DontPreemptNavigationWithFrameTreeUpdate
    RenderFrameHostManagerTest.ForceSwapAfterWebUIBindings
    RenderFrameHostManagerTest.LeakingRenderViewHosts
    RenderFrameHostManagerTest.NoScriptAccessAfterSwapOut
    RenderFrameHostManagerTest.ProcessExitWithSwappedOutViews
    RenderFrameHostManagerTest.RestoreFileAccessForHistoryNavigation
    RenderFrameHostManagerTest.RestoreSubframeFileAccessForHistoryNavigation
    RenderFrameHostManagerTest.SupportCrossProcessPostMessage
    RenderFrameHostManagerTest.SupportCrossProcessPostMessageWithMessagePort
    RenderFrameHostManagerTest.SwappedOutViewHasCorrectVisibilityState
    RenderFrameHostManagerTest.WebUIGetsBindings
    RenderViewHostTest.BaseURLParam
    ResourceDispatcherHostBrowserTest.CrossSiteNavigationNonBuffered
    ServiceWorkerBrowserTest.ResponseFromHTTPSServiceWorkerIsMarkedAsSecure
    SitePerProcessAccessibilityBrowserTest.CrossSiteIframeAccessibility
    SitePerProcessBrowserTest.CrossSiteDidStopLoading
    SitePerProcessBrowserTest.CrossSiteIframe
    SitePerProcessBrowserTest.DynamicSandboxFlags
    SitePerProcessBrowserTest.DynamicSandboxFlagsRemoteToLocal
    SitePerProcessBrowserTest.DynamicSandboxFlagsRendererInitiatedNavigation
    SitePerProcessBrowserTest.DynamicWindowName
    SitePerProcessBrowserTest.KillingRendererClearsDescendantProxies
    SitePerProcessBrowserTest.LoadEventForwarding
    SitePerProcessBrowserTest.NavigateRemoteFrame
    SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs
    SitePerProcessBrowserTest.NavigateRemoteFrameToKilledProcess
    SitePerProcessBrowserTest.NavigateRemoteFrameToKilledProcessWithSubtree
    SitePerProcessBrowserTest.NavigateWithSiblingRemoteFrame
    SitePerProcessBrowserTest.OriginReplication
    SitePerProcessBrowserTest.ProxyCreationSkipsSubtree
    SitePerProcessBrowserTest.SandboxFlagsReplication
    SitePerProcessBrowserTest.WindowNameReplication
    SitePerProcessDevToolsBrowserTest.CrossSiteIframeAgentHost
    WebContentsImplBrowserTest.GetSizeForNewRenderView
    WebContentsImplBrowserTest.LoadProgressAfterInterruptedNav
    WebContentsImplBrowserTest.RenderFrameCreatedCorrectProcessForObservers
    WebContentsViewAuraTest.ScreenshotForSwappedOutRenderViews
    WebUIMojoTest.EndToEndPing

+    return;
 
-  rfh->set_loading_progress(load_progress);
+  ftn->set_loading_progress(load_progress);
 
   // We notify progress change immediately for the first and last updates.
   // Also, since the message loop may be pretty busy when a page is loaded, it
@@ -3519,13 +3489,8 @@ bool WebContentsImpl::UpdateTitleForEntry(NavigationEntryImpl* entry,
 
 void WebContentsImpl::SendLoadProgressChanged() {
   loading_last_progress_update_ = base::TimeTicks::Now();
-  double progress = 0.0;
-  int frame_count = 0;
+  double progress = frame_tree_.GetLoadProgress();
 
-  frame_tree_.ForEach(
-      base::Bind(&CollectLoadProgress, &progress, &frame_count));
-  if (frame_count != 0)
-    progress /= frame_count;
   DCHECK_LE(progress, 1.0);
 
   if (progress <= loading_total_progress_)
@@ -3537,7 +3502,7 @@ void WebContentsImpl::SendLoadProgressChanged() {
 }
 
 void WebContentsImpl::ResetLoadProgressState() {
-  frame_tree_.ForEach(base::Bind(&ResetLoadProgress));
+  frame_tree_.ResetLoadProgress();
   loading_total_progress_ = 0.0;
   loading_weak_factory_.InvalidateWeakPtrs();
   loading_last_progress_update_ = base::TimeTicks();