Refactor the loading tracking logic in WebContentsImpl.

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 ed679224852e6b4631a4335b32c695015be47e3e..96097b44dd958875192e00c8e938d21af3084bd9 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -121,9 +121,6 @@ namespace {
 
 const int kMinimumDelayBetweenLoadingUpdatesMS = 100;
 
-// This matches what Blink's ProgressTracker has traditionally used for a
-// minimum progress value.
-const double kMinimumLoadingProgress = 0.1;
 
 const char kDotGoogleDotCom[] = ".google.com";
 
@@ -163,6 +160,27 @@ bool CollectSites(BrowserContext* context,
   return true;
 }
 
+// Helper function for retrieving the total loading progress and number of

[nasko, 2015/02/23 16:54:43]

nit: I like the "used with FrameTree::ForEach()" part of the comment in the
function below. It will be nice to add it here too.

[Fabrice (no longer in Chrome), 2015/02/23 20:02:06]

Done.

+// frames in a frame tree.
+bool CollectLoadProgress(double* progress,
+                         int* frame_count,
+                         FrameTreeNode* node) {
+  *progress += node->loading_progress();
+  (*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->is_loading()) {
+    // There is at least one node loading, abort traversal.
+    *is_loading = true;
+    return false;
+  }
+  return true;
+}
+
 bool ForEachFrameInternal(
     const base::Callback<void(RenderFrameHost*)>& on_frame,
     FrameTreeNode* node) {
@@ -333,7 +351,6 @@ WebContentsImpl::WebContentsImpl(BrowserContext* browser_context,
       waiting_for_response_(false),
       load_state_(net::LOAD_STATE_IDLE, base::string16()),
       loading_total_progress_(0.0),
-      loading_frames_in_progress_(0),
       upload_size_(0),
       upload_position_(0),
       displayed_insecure_content_(false),
@@ -2830,14 +2847,22 @@ void WebContentsImpl::OnDidStartLoading(bool to_different_document) {
 
   RenderFrameHostImpl* rfh =
       static_cast<RenderFrameHostImpl*>(render_frame_message_source_);
-  int64 render_frame_id = rfh->frame_tree_node()->frame_tree_node_id();
+
+  // Update pending rfh in the frame tree node if necessary.
+  if (rfh->frame_tree_node()->pending_frame_host() != rfh) {

[nasko, 2015/02/23 16:54:43]

Pending RFH has a very precise meaning and it doesn't match what this code is
doing. Maybe rename it to "loading_frame_host"?

[Fabrice (no longer in Chrome), 2015/02/23 20:02:06]

Done.

+    rfh->frame_tree_node()->set_pending_frame_host(rfh);
+    rfh->frame_tree_node()->set_is_loading(false);
+    rfh->frame_tree_node()->set_loading_progress(
+        FrameTreeNode::kLoadingProgressNotStarted);
+  }
 
   // Any main frame load to a new document should reset the load progress, since
   // it will replace the current page and any frames.
   if (to_different_document && !rfh->GetParent()) {
     ResetLoadProgressState();
-    loading_frames_in_progress_ = 0;
     rfh->frame_tree_node()->set_is_loading(false);
+    rfh->frame_tree_node()->set_loading_progress(
+        FrameTreeNode::kLoadingProgressNotStarted);
   }
 
   // It is possible to get multiple calls to OnDidStartLoading that don't have
@@ -2855,17 +2880,16 @@ void WebContentsImpl::OnDidStartLoading(bool to_different_document) {
   if (rfh->frame_tree_node()->is_loading())
     return;
 
-  DCHECK_GE(loading_frames_in_progress_, 0);
-  if (loading_frames_in_progress_ == 0)
+  if (!IsFrameTreeLoading())
     DidStartLoading(rfh, to_different_document);
 
-  ++loading_frames_in_progress_;
   rfh->frame_tree_node()->set_is_loading(true);
+  rfh->frame_tree_node()->set_loading_progress(
+      FrameTreeNode::kLoadingProgressMinimum);
 
   // Notify the RenderFrameHostManager of the event.
   rfh->frame_tree_node()->render_manager()->OnDidStartLoading();
 
-  loading_progresses_[render_frame_id] = kMinimumLoadingProgress;
   SendLoadProgressChanged();
 }
 
@@ -2875,28 +2899,28 @@ void WebContentsImpl::OnDidStopLoading() {
 
   RenderFrameHostImpl* rfh =
       static_cast<RenderFrameHostImpl*>(render_frame_message_source_);
-  int64 render_frame_id = rfh->frame_tree_node()->frame_tree_node_id();
+
+  // StopLoading was called for an older navigation, ignore.
+  if (rfh->frame_tree_node()->pending_frame_host() != rfh)
+    return;
+
+  DCHECK(rfh->frame_tree_node()->is_loading());
   rfh->frame_tree_node()->set_is_loading(false);
 
-  if (loading_progresses_.find(render_frame_id) != loading_progresses_.end()) {
-    // Load stopped while we were still tracking load.  Make sure we update
-    // progress based on this frame's completion.
-    loading_progresses_[render_frame_id] = 1.0;
-    SendLoadProgressChanged();
-    // Then we clean-up our states.
-    if (loading_total_progress_ == 1.0)
-      ResetLoadProgressState();
-  }
+  // This method should never be called when the frame is not loading.

[nasko, 2015/02/23 16:54:43]

A DCHECK is warranted every time we have a "should never ..."

[Fabrice (no longer in Chrome), 2015/02/23 20:02:06]

This got shuffled around, the comment is at the proper line again.
Thanks.

+  rfh->frame_tree_node()->set_loading_progress(
+      FrameTreeNode::kLoadingProgressDone);
+
+  // Update progress based on this frame's completion.
+  SendLoadProgressChanged();
+  // Then clean-up the states.
+  if (loading_total_progress_ == 1.0)
+    ResetLoadProgressState();
 
   // Notify the RenderFrameHostManager of the event.
   rfh->frame_tree_node()->render_manager()->OnDidStopLoading();
 
-  // TODO(japhet): This should be a DCHECK, but the pdf plugin sometimes
-  // calls DidStopLoading() without a matching DidStartLoading().
-  if (loading_frames_in_progress_ == 0)
-    return;
-  --loading_frames_in_progress_;
-  if (loading_frames_in_progress_ == 0)
+  if (!IsFrameTreeLoading())
     DidStopLoading(rfh);
 }
 
@@ -2906,9 +2930,8 @@ void WebContentsImpl::OnDidChangeLoadProgress(double load_progress) {
 
   RenderFrameHostImpl* rfh =
       static_cast<RenderFrameHostImpl*>(render_frame_message_source_);
-  int64 render_frame_id = rfh->frame_tree_node()->frame_tree_node_id();
 
-  loading_progresses_[render_frame_id] = load_progress;
+  rfh->frame_tree_node()->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
@@ -3390,14 +3413,8 @@ void WebContentsImpl::SendLoadProgressChanged() {
   double progress = 0.0;
   int frame_count = 0;
 
-  for (LoadingProgressMap::iterator it = loading_progresses_.begin();
-       it != loading_progresses_.end();
-       ++it) {
-    progress += it->second;
-    ++frame_count;
-  }
-  if (frame_count == 0)
-    return;
+  frame_tree_.ForEach(
+      base::Bind(&CollectLoadProgress, &progress, &frame_count));
   progress /= frame_count;
   DCHECK(progress <= 1.0);
 
@@ -3410,7 +3427,6 @@ void WebContentsImpl::SendLoadProgressChanged() {
 }
 
 void WebContentsImpl::ResetLoadProgressState() {
-  loading_progresses_.clear();
   loading_total_progress_ = 0.0;
   loading_weak_factory_.InvalidateWeakPtrs();
   loading_last_progress_update_ = base::TimeTicks();
@@ -3727,7 +3743,6 @@ void WebContentsImpl::RenderViewTerminated(RenderViewHost* rvh,
   // webpage? Once this function is called at a more granular frame level, we
   // probably will need to more granularly reset the state here.
   ResetLoadProgressState();
-  loading_frames_in_progress_ = 0;
 
   FOR_EACH_OBSERVER(WebContentsObserver,
                     observers_,
@@ -4519,4 +4534,10 @@ void WebContentsImpl::SetForceDisableOverscrollContent(bool force_disable) {
     view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 }
 
+bool WebContentsImpl::IsFrameTreeLoading() {
+  bool is_loading = false;
+  frame_tree_.ForEach(base::Bind(&IsNodeLoading, &is_loading));
+  return is_loading;
+}
+
 }  // namespace content