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 3c46724086182f256112f0cb2c3841faf15eb3fa..01b0d9fe914bdc6a066992fb0bfe939bc0c5feb5 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -122,9 +122,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";
 
@@ -164,6 +161,27 @@ bool CollectSites(BrowserContext* context,
   return true;
 }
 
+// Helper function for retrieving the total loading progress and number of

[Charlie Reis, 2015/02/20 23:42:12]

nit: Helper function used with FrameTree::ForEach() for...

[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.

[Charlie Reis, 2015/02/20 23:42:12]

nit: This is a run-on sentence.  Use a semicolon or "loading, so abort"

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

Done.

+    *is_loading = true;
+    return false;
+  }
+  return true;
+}
+
 bool ForEachFrameInternal(
     const base::Callback<void(RenderFrameHost*)>& on_frame,
     FrameTreeNode* node) {
@@ -334,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),
@@ -2831,14 +2848,14 @@ 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);
+    rfh->frame_tree_node()->set_loading_progress(

[clamy, 2015/02/20 14:00:24]

It seems a bit weird to have a call to ResetLoadProgessState and then reset the
load progress again on the frame tree node. Would it be possible to have it in
ResetLoadProgressState?

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

Answered in another comment in patch set 4, ResetLoadProgressState only resets
the loading progress tracking done in WebContentsImpl, so this is correct.

+        FrameTreeNode::kNotStartedLoadingProgress);

[Charlie Reis, 2015/02/20 23:42:12]

It looks wrong to me to have this here at all.  We might return early on line
2874, which means we won't set it to kMinimumLoadingProgress even though we're
loading.

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

Latter patch sets no longer return early, so it is no longer relevant.

   }
 
   // It is possible to get multiple calls to OnDidStartLoading that don't have
@@ -2856,17 +2873,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::kMinimumLoadingProgress);
 
   // Notify the RenderFrameHostManager of the event.
   rfh->frame_tree_node()->render_manager()->OnDidStartLoading();
 
-  loading_progresses_[render_frame_id] = kMinimumLoadingProgress;
   SendLoadProgressChanged();
 }
 
@@ -2876,28 +2892,27 @@ 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.

[Charlie Reis, 2015/02/20 23:42:12]

Same nit about run-on sentence.

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

Done.

+  if (rfh->frame_tree_node()->current_frame_host() != rfh)

[clamy, 2015/02/20 14:00:24]

We want to remove mention of the RenderFrameHost from the DidStart/StopLoading
functions in favor of the FrameTreeNode.

Additionally, the rfh you get here is the one that sent the IPC. However, we
want to merge this IPC handler with the DidStopLoading function, so this would
completely break.

[Charlie Reis, 2015/02/20 23:42:12]

Let's not worry about the RFH->FTN switch or OnDidStop + DidStop merge just yet.
 I'd like to make sure we get something working for this case first.

That said, we don't have a similar check in OnDidStartLoading, so I'm not sure
why this is needed here.

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

This is no longer relevant as we now track the loading per-frame.

+    return;
+
+  // This method should never be called when the frame is not loading.
+  DCHECK(rfh->frame_tree_node()->is_loading());
   rfh->frame_tree_node()->set_is_loading(false);
+  rfh->frame_tree_node()->set_loading_progress(
+      FrameTreeNode::kDoneLoadingProgress);
 
-  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();
-  }
+  // 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);
 }
 
@@ -2907,9 +2922,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
@@ -3391,14 +3405,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;

[carlosk, 2015/02/23 10:41:15]

You *might* be able replace this if-check with |(progress == 0)| as this *seems*
to reflect what this early return used to do within the new implementation
context (meaning no nodes are loading).

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

The loading progress tracking that is done here is inaccurate at best, it is an
estimate. This new way is no better nor worse. I do not think this test is
necessary anyway because the test on line 3405 will return early if |(progress
== 0.0)| anyway.

[carlosk, 2015/03/02 14:17:28]

Acknowledged.

+  frame_tree_.ForEach(
+      base::Bind(&CollectLoadProgress, &progress, &frame_count));
   progress /= frame_count;

[Charlie Reis, 2015/02/20 23:42:12]

Hmm, it's a little subtle to assume that we will always have at least one frame
in the tree, even though that should be true.

We should put a DCHECK_GT(frame_count, 0) here to show that's our expectation. 
(A DCHECK should be sufficient, since we'll crash either way in release builds
if we get it wrong.)

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

Done.

   DCHECK(progress <= 1.0);
 
@@ -3411,7 +3419,6 @@ void WebContentsImpl::SendLoadProgressChanged() {
 }
 
 void WebContentsImpl::ResetLoadProgressState() {
-  loading_progresses_.clear();

[clamy, 2015/02/20 14:00:24]

Is it okay to not reset the load progress for each of the FrameTreeNodes here
(as an equivalent of resetting the map)?

[Charlie Reis, 2015/02/20 23:42:12]

No.  We should be resetting each node in the tree here.

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

I do not think we should actually, because this method is called in multiple
places. From what I can see, this is merely a "helper" method to reset the parts
of the loading progress tracking logic done in WebContentsImpl.

   loading_total_progress_ = 0.0;
   loading_weak_factory_.InvalidateWeakPtrs();
   loading_last_progress_update_ = base::TimeTicks();
@@ -3728,7 +3735,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_,
@@ -4512,4 +4518,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