Refactor the loading tracking logic in WebContentsImpl.

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/web_contents/web_contents_impl.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
@@ skipping 147 matching lines @@
 }
 
 // Helper function for retrieving all the sites in a frame tree.
 bool CollectSites(BrowserContext* context,
                   std::set<GURL>* sites,
                   FrameTreeNode* node) {
   sites->insert(SiteInstance::GetSiteForURL(context, node->current_url()));
   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) {
   on_frame.Run(node->current_frame_host());
   return true;
 }
 
 bool ForEachPendingFrameInternal(
     const base::Callback<void(RenderFrameHost*)>& on_frame,
     FrameTreeNode* node) {
@@ skipping 150 matching lines @@
                   this,
                   this,
                   this),
       is_loading_(false),
       is_load_to_different_document_(false),
       crashed_status_(base::TERMINATION_STATUS_STILL_RUNNING),
       crashed_error_code_(0),
       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),
       has_accessed_initial_document_(false),
       capturer_count_(0),
       should_normally_be_visible_(true),
       is_being_destroyed_(false),
       notify_disconnection_(false),
       dialog_manager_(NULL),
       is_showing_before_unload_dialog_(false),
@@ skipping 2473 matching lines @@
   FOR_EACH_OBSERVER(
       WebContentsObserver, observers_, DidFinishLoad(rfh, validated_url));
 }
 
 void WebContentsImpl::OnDidStartLoading(bool to_different_document) {
   if (!HasValidFrameSource())
     return;
 
   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);
   }
 
   // It is possible to get multiple calls to OnDidStartLoading that don't have
   // corresponding calls to OnDidStopLoading:
   // - With "swappedout://" URLs, this happens when a RenderView gets swapped
   //   out for a cross-process navigation, and it turns into a placeholder for
   //   one being rendered in a different process.
   // - Also, there might be more than one RenderFrameHost sharing the same
   //   FrameTreeNode (and thus sharing its ID) each sending a start.
   // - But in the future, once clamy@ moves navigation network requests to the
   //   browser process, there's a good chance that callbacks about starting and
   //   stopping will all be handled by the browser. When that happens, there
   //   should no longer be a start/stop call imbalance. TODO(avi): When this
   //   future arrives, update this code to not allow this case.
   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();
 }
 
 void WebContentsImpl::OnDidStopLoading() {
   if (!HasValidFrameSource())
     return;
 
   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);
 }
 
 void WebContentsImpl::OnDidChangeLoadProgress(double load_progress) {
   if (!HasValidFrameSource())
     return;
 
   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
   // might not execute a posted task in a timely manner so we make sure to
   // immediately send progress report if enough time has passed.
   base::TimeDelta min_delay =
       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,
                  loading_weak_factory_.GetWeakPtr()),
       min_delay);
 }
 
 void WebContentsImpl::OnGoToEntryAtOffset(int offset) {
   if (!delegate_ || delegate_->OnGoToEntryOffset(offset))
     controller_.GoToOffset(offset);
@@ skipping 440 matching lines @@
       Details<std::pair<NavigationEntry*, bool> >(&details));
 
   return true;
 }
 
 void WebContentsImpl::SendLoadProgressChanged() {
   loading_last_progress_update_ = base::TimeTicks::Now();
   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;
   DCHECK(progress <= 1.0);
 
   if (progress <= loading_total_progress_)
     return;
   loading_total_progress_ = progress;
 
   if (delegate_)
     delegate_->LoadProgressChanged(this, progress);
 }
 
 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();
 }
 
 void WebContentsImpl::NotifyViewSwapped(RenderViewHost* old_host,
                                         RenderViewHost* new_host) {
   // After sending out a swap notification, we need to send a disconnect
   // notification so that clients that pick up a pointer to |this| can NULL the
   // pointer.  See Bug 1230284.
   notify_disconnection_ = true;
   FOR_EACH_OBSERVER(WebContentsObserver, observers_,
@@ skipping 294 matching lines @@
 
   SetIsLoading(rvh, false, true, NULL);
   NotifyDisconnected();
   SetIsCrashed(status, error_code);
 
   // Reset the loading progress. TODO(avi): What does it mean to have a
   // "renderer crash" when there is more than one renderer process serving a
   // 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_,
                     RenderProcessGone(GetCrashedStatus()));
 }
 
 void WebContentsImpl::RenderViewDeleted(RenderViewHost* rvh) {
   FOR_EACH_OBSERVER(WebContentsObserver, observers_, RenderViewDeleted(rvh));
 }
 
@@ skipping 774 matching lines @@
   FrameTreeNode* node = frame_tree_.root();
   node->render_manager()->ResumeResponseDeferredAtStart();
 }
 
 void WebContentsImpl::SetForceDisableOverscrollContent(bool force_disable) {
   force_disable_overscroll_content_ = force_disable;
   if (view_)
     view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 }
 
+bool WebContentsImpl::IsTreeLoading() {
+  bool is_loading = false;
+  frame_tree_.ForEach(base::Bind(&IsNodeLoading, &is_loading));
+  return is_loading;
+}
+
 }  // namespace content