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 d2215b13cec355ba1f4de2eea91e71129366ebb7..0f88860d310fd58d2e90d18f7ae215bd02e001c6 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -165,6 +165,23 @@ bool CollectSites(BrowserContext* context,
   return true;
 }
 
+// Helper function for retrieving the total loading progress

[Charlie Reis, 2015/02/13 01:01:51]

nit: Please try to wrap lines closer to 80 chars for consistency with other
code.

[carlosk, 2015/02/13 15:05:41]

Note: git cl format does *not* do a good job when dealing with comments.

[Fabrice (no longer in Chrome), 2015/02/13 18:04:25]

Done.

+// and number of frames in a frame tree.
+bool CollectProgress(double* progress, int* frame_count, FrameTreeNode* node) {

[Charlie Reis, 2015/02/13 01:01:51]

nit: This name is a bit ambiguous.  Maybe CollectLoadProgress?

[Fabrice (no longer in Chrome), 2015/02/13 18:04:24]

Done.

+  *progress += node->get_loading_progress();
+  (*frame_count)++;
+  return true;
+}
+
+// Helper function to check if a node is loading.

[carlosk, 2015/02/13 15:05:41]

I'd suggest: Used with FrameTree::ForEach() to check it at least one node is
loading.
And something in these lines for the previous method.

[Fabrice (no longer in Chrome), 2015/02/13 18:04:25]

That's exactly what is being done. These are the functions called by ForEach on
each node.

[carlosk, 2015/02/16 11:40:21]

I was suggesting a new way to phrase your comment. :)

[Fabrice (no longer in Chrome), 2015/02/20 12:58:58]

Done.

+bool IsNodeLoading(bool* is_loading, FrameTreeNode* node) {
+  if (node->is_loading()) {
+    *is_loading = true;
+    return false;

[Charlie Reis, 2015/02/13 01:01:51]

nit: Put a comment here saying that this aborts the traversal, since otherwise
it looks a bit odd to return false here.

[Fabrice (no longer in Chrome), 2015/02/13 18:04:25]

Done.

+  }
+  return true;
+}
+
 bool ForEachFrameInternal(
     const base::Callback<void(RenderFrameHost*)>& on_frame,
     FrameTreeNode* node) {
@@ -335,7 +352,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),
@@ -2829,13 +2845,11 @@ 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();
 
   // 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);
   }
 
@@ -2854,17 +2868,15 @@ 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 (!IsTreeLoading())
     DidStartLoading(rfh, to_different_document);
 
-  ++loading_frames_in_progress_;
   rfh->frame_tree_node()->set_is_loading(true);
 
   // Notify the RenderFrameHostManager of the event.
   rfh->frame_tree_node()->render_manager()->OnDidStartLoading();
 
-  loading_progresses_[render_frame_id] = kMinimumLoadingProgress;
+  rfh->frame_tree_node()->set_loading_progress(kMinimumLoadingProgress);
   SendLoadProgressChanged();
 }
 
@@ -2874,28 +2886,25 @@ void WebContentsImpl::OnDidStopLoading() {
 
   RenderFrameHostImpl* rfh =
       static_cast<RenderFrameHostImpl*>(render_frame_message_source_);
-  int64 render_frame_id = rfh->frame_tree_node()->frame_tree_node_id();
-  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();
-  }
+  // Load stopped while the load was still being tracked.

[Charlie Reis, 2015/02/13 01:01:51]

This comment doesn't make sense if it's not within the if.  It's possible the
load isn't still being tracked.

It's also not clear to me why the if was removed.  How is that case handled now?

[Fabrice (no longer in Chrome), 2015/02/13 18:04:25]

My understanding of this test is that it really should have been a DCHECK all
along but I am not certain. I am replacing it with just that right now. It
appears the pdf plugin issue has been fixed some time ago too.

+  // Make sure the progress is updated based on this frame's completion.
+  rfh->frame_tree_node()->set_loading_progress(1.0);
+  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
+  // TODO(japhet): This test should not exist, but the pdf plugin sometimes

[nasko, 2015/02/13 00:16:29]

nit: s/test/check/

[Fabrice (no longer in Chrome), 2015/02/13 18:04:25]

Done.

   // calls DidStopLoading() without a matching DidStartLoading().
-  if (loading_frames_in_progress_ == 0)
+  if (!IsTreeLoading())
     return;
-  --loading_frames_in_progress_;
-  if (loading_frames_in_progress_ == 0)
+
+  rfh->frame_tree_node()->set_is_loading(false);
+  if (!IsTreeLoading())

[carlosk, 2015/02/13 15:05:41]

Suggestion: IsTreeLoading could return the number of loading nodes. This maps
seamlessly to the current |bool| and would allow you to not have to call it
twice here.

[Fabrice (no longer in Chrome), 2015/02/13 18:04:25]

This is no longer necessary since I am removing the above test.

     DidStopLoading(rfh);
 }
 
@@ -2905,9 +2914,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
@@ -2917,17 +2925,12 @@ void WebContentsImpl::OnDidChangeLoadProgress(double load_progress) {
       base::TimeDelta::FromMilliseconds(kMinimumDelayBetweenLoadingUpdatesMS);
   if (load_progress == 1.0 || loading_last_progress_update_.is_null() ||
       base::TimeTicks::Now() - loading_last_progress_update_ > min_delay) {
-    // If there is a pending task to send progress, it is now obsolete.
-    loading_weak_factory_.InvalidateWeakPtrs();

[Charlie Reis, 2015/02/13 01:01:51]

Why is this safe to remove?  Is the factory still needed?

[Fabrice (no longer in Chrome), 2015/02/13 18:04:25]

Because I fail to merge my local branches.
Fixed, sorry and thanks!

     SendLoadProgressChanged();
     if (loading_total_progress_ == 1.0)
       ResetLoadProgressState();
     return;
   }
 
-  if (loading_weak_factory_.HasWeakPtrs())
-    return;
-
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&WebContentsImpl::SendLoadProgressChanged,
@@ -3388,12 +3391,7 @@ 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;
-  }
+  frame_tree_.ForEach(base::Bind(&CollectProgress, &progress, &frame_count));
   if (frame_count == 0)

[carlosk, 2015/02/13 15:05:41]

With your implementation it seems that this if would never be triggered. The
previous implementation seemed to have in the map only the nodes that were
loading whereas your CollectProgress function will return the tree node count
(which is always >= 1).

It seems this would also affect the calculated |progress| value but I'd think
that was not really important and even that the new calculation would be
"better".

[Fabrice (no longer in Chrome), 2015/02/20 12:58:58]

Done.
But will need to double-check if we're doing the right thing.

     return;
   progress /= frame_count;
@@ -3408,9 +3406,7 @@ void WebContentsImpl::SendLoadProgressChanged() {
 }
 
 void WebContentsImpl::ResetLoadProgressState() {
-  loading_progresses_.clear();

[clamy, 2015/02/13 12:13:43]

Is it okay to not reset the loading progress of each FrameTreeNode here? (as an
equivalent of resetting the map).

[Fabrice (no longer in Chrome), 2015/02/24 13:14:46]

Answered in patch set 4.

   loading_total_progress_ = 0.0;
-  loading_weak_factory_.InvalidateWeakPtrs();
   loading_last_progress_update_ = base::TimeTicks();
 }
 
@@ -3725,7 +3721,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_,
@@ -4520,4 +4515,10 @@ void WebContentsImpl::SetForceDisableOverscrollContent(bool force_disable) {
     view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 }
 
+bool WebContentsImpl::IsTreeLoading() {
+  bool is_loading = false;
+  frame_tree_.ForEach(base::Bind(&IsNodeLoading, &is_loading));
+  return is_loading;
+}
+
 }  // namespace content