Refactor the loading tracking logic in WebContentsImpl.

content/browser/frame_host/frame_tree_node.cc

 // Copyright 2013 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/frame_host/frame_tree_node.h"
 
 #include <queue>
 
 #include "base/command_line.h"
 #include "base/stl_util.h"
@@ skipping 16 matching lines @@
                              const std::string& name)
     : frame_tree_(frame_tree),
       navigator_(navigator),
       render_manager_(this,
                       render_frame_delegate,
                       render_view_delegate,
                       render_widget_delegate,
                       manager_delegate),
       frame_tree_node_id_(next_frame_tree_node_id_++),
       parent_(NULL),
-      replication_state_(name),
-      is_loading_(false) {
+      replication_state_(name) {
 }
 
 FrameTreeNode::~FrameTreeNode() {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableBrowserSideNavigation)) {
     navigator_->CancelNavigation(this);
   }
 }
 
 bool FrameTreeNode::IsMainFrame() const {
@@ skipping 59 matching lines @@
     return false;
 
   for (FrameTreeNode* node = parent(); node; node = node->parent()) {
     if (node == other)
       return true;
   }
 
   return false;
 }
 
+bool FrameTreeNode::IsLoading() const {
+  RenderFrameHostImpl* current_frame_host =
+      render_manager_.current_frame_host();
+  RenderFrameHostImpl* pending_frame_host =
+      render_manager_.pending_frame_host();
+
+  DCHECK(current_frame_host != nullptr);
+  if (pending_frame_host != nullptr)
+    return current_frame_host->is_loading() && pending_frame_host->is_loading();

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

... s/&&/||/
I find it worrisome that none of our tests seem too bothered by it.

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

Done.
I will add a test for this case.

+  else
+    return current_frame_host->is_loading();
+}
+
+double FrameTreeNode::GetLoadingProgress() const {
+  RenderFrameHostImpl* current_frame_host =
+      render_manager_.current_frame_host();
+  RenderFrameHostImpl* pending_frame_host =
+      render_manager_.pending_frame_host();
+
+  DCHECK(current_frame_host != nullptr);
+  if (pending_frame_host != nullptr)
+    return (current_frame_host->loading_progress() +
+            pending_frame_host->loading_progress()) / 2;

[Charlie Reis, 2015/02/24 04:55:00]

This is not correct.  Why would the overall load progress be the average of two
different frames?  For example, we make an effort to ensure that load progress
never goes backwards, and this would fail that if a pending RFH were introduced
in the middle of a load (e.g., for a redirect).

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

The loading progress can in fact go backwards but it is simply ignored. The way
we track this has always been an estimate anyway so I am not sure why it would
be a problem now.

[Charlie Reis, 2015/02/25 00:07:20]

To be clear: it's important for UX that the load progress never goes backwards,
but I agree that WebContentsImpl::SendLoadProgressChanged() enforces that.

+  else
+    return current_frame_host->loading_progress();
+}
+
 }  // namespace content