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 102 matching lines @@
 #endif
 
 #if defined(OS_MACOSX)
 #include "base/mac/foundation_util.h"
 #endif
 
 namespace content {
 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";
 
 #if defined(OS_ANDROID)
 const char kWebContentsAndroidKey[] = "web_contents_android";
 #endif  // OS_ANDROID
 
 base::LazyInstance<std::vector<WebContentsImpl::CreatedCallback> >
 g_created_callbacks = LAZY_INSTANCE_INITIALIZER;
 
 static int StartDownload(RenderFrameHost* rfh,
@@ skipping 18 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 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) {
+  *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;
+}
+
 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 2476 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);
+    rfh->set_is_loading(false);
+    rfh->set_loading_progress(RenderFrameHostImpl::kLoadingProgressNotStarted);
   }
 
-  // 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;
+  // This method should never be called when the frame is loading.
+  DCHECK(!rfh->is_loading());
 
-  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->set_is_loading(true);
+  rfh->set_loading_progress(RenderFrameHostImpl::kLoadingProgressMinimum);
 
   // Notify the RenderFrameHostManager of the event.
   rfh->frame_tree_node()->render_manager()->OnDidStartLoading();
 
-  loading_progresses_[render_frame_id] = 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();
-  }
+  // This method should never be called when the frame is not loading.
+  DCHECK(rfh->is_loading());
+  rfh->set_is_loading(false);
+  rfh->set_loading_progress(RenderFrameHostImpl::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);
 }
 
 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->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.
@@ skipping 461 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;
-  }
-  if (frame_count == 0)
-    return;
+  frame_tree_.ForEach(
+      base::Bind(&CollectLoadProgress, &progress, &frame_count));
+  DCHECK_GT(frame_count, 0);
+
   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/26 16:05:00]

As mentionned offline, we want to keep the current behavior intact. Therefore I
think you should replace this line with resetting the load progress of the
current RFH in each FTN to kLoadingProgressNotStarted. Then you would need not
to account for frames with a load progress of not started in the count.

Other option, you add a boolean to the RFH/FTN indicating whether it should be
taken into account in the total load computation. It is reset to false for all
frames there, and set to true when a frame starts loading.

[Fabrice (no longer in Chrome), 2015/02/27 17:45:25]

I like the first option because it fixes a whole bunch of problems, including
some we have with the current implementation. I'm going to do that, thanks a
lot!

   loading_total_progress_ = 0.0;
   loading_weak_factory_.InvalidateWeakPtrs();
   loading_last_progress_update_ = base::TimeTicks();
 }
 
+bool WebContentsImpl::IsFrameTreeLoading() {
+  bool is_loading = false;
+  frame_tree_.ForEach(base::Bind(&IsNodeLoading, &is_loading));
+  return is_loading;
+}
+
 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_,
                     RenderViewHostChanged(old_host, new_host));
 
   // TODO(avi): Remove. http://crbug.com/170921
@@ skipping 291 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 772 matching lines @@
   node->render_manager()->ResumeResponseDeferredAtStart();
 }
 
 void WebContentsImpl::SetForceDisableOverscrollContent(bool force_disable) {
   force_disable_overscroll_content_ = force_disable;
   if (view_)
     view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 }
 
 }  // namespace content