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 104 matching lines @@
 
 #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;
 
@@ skipping 19 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 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) {
   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->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
   // 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 (!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();
 }
 
 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();
+
+  // 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);
 }
 
 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.
@@ 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;

[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);
 
   if (progress <= loading_total_progress_)
     return;

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

This is a minor/usability issue and should be sorted out later.

This early return here stops the progress bar to go back in time which might be
good from a usability PoV. But it might also affect a lot the "precision" or our
loading tracking as this function is called very early in the loading process.

Imagine this case: we're loading a document with multiple iframes and a simple
one starts early and reports huge progress quickly before other iframes had a
chance to be added (*). Progress got to, say, 90% and it will not move even
though the others, more complex iframes will only later begin to report their
progress.

Am I missing something here or this sounds correct?

(*) If all iframes of a document are all added at once this is not an issue. But
I'm guessing that's not the case.

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

I did not want to change the logic in here so I let it to be the same way as
before. I am going to guess that in most cases iframes are added early enough
that it is not going to be a problem. And I think that having a loading progress
tracker that goes backwards would be confusing for users.
The way we are tracking it right now does have the side effect to favor the
"main" frame progression, but I think this is actually a good thing as this is
probably the load progress most users will care about.

   loading_total_progress_ = progress;
 
   if (delegate_)
     delegate_->LoadProgressChanged(this, progress);
 }
 
 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();
 }
 
 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.
@@ skipping 296 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 763 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::IsFrameTreeLoading() {
+  bool is_loading = false;
+  frame_tree_.ForEach(base::Bind(&IsNodeLoading, &is_loading));
+  return is_loading;
+}
+
 }  // namespace content