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..c31dc9faf81669f077603f8729fc63221b869b7e 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,18 @@ 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);
+
+  // The loading progress is only reset when the current RFH starts navigating.
+  FrameTreeNode* ftn = rfh->frame_tree_node();
+  if (ftn->render_manager()->current_frame_host() == rfh)

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

I think this check might be problematic. In browser-initiated navigation,
DidStartLoading is issued browser-side, which will set the loading state to
true, but will not reset the loading progress if the navigation is cross-site
and we have a pending RFH. Later on, the pending RFH will send a
DidStartLoading, but the RFH will already be in loading state, so this method
will return early and the progress value won't be reset.

Since this CL is just moving code around, let's not make any functional changes.

[Fabrice (no longer in Chrome), 2015/03/25 17:35:01]

Following our offline conversation, I removed the checks in this file.

+    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 +2962,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)
+    ftn->set_loading_progress(FrameTreeNode::kLoadingProgressDone);
 
   // TODO(erikchen): Remove ScopedTracker below once crbug.com/465796 is
   // fixed.
@@ -3020,9 +2987,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 +3011,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)
+    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 +3490,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 +3503,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();