Move DidStartLoading, DidStopLoading, DidChangeLoadProgress to RFHI.

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"
 #include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_switches.h"
 
 namespace content {
 
 namespace {
 
 // This is a global map between frame_tree_node_ids and pointers to
 // FrameTreeNodes.
 typedef base::hash_map<int64, FrameTreeNode*> FrameTreeNodeIDMap;
 
 base::LazyInstance<FrameTreeNodeIDMap> g_frame_tree_node_id_map =
     LAZY_INSTANCE_INITIALIZER;
 
+// These values indicate the loading progress status. The minimum progress
+// value matches what Blink's ProgressTracker has traditionally used for a
+// minimum progress value.
+static const double kLoadingProgressNotStarted = 0.0;
+static const double kLoadingProgressMinimum = 0.1;
+static const double kLoadingProgressDone = 1.0;
+
 }  // namespace
 
-const double FrameTreeNode::kLoadingProgressNotStarted = 0.0;
-const double FrameTreeNode::kLoadingProgressMinimum = 0.1;
-const double FrameTreeNode::kLoadingProgressDone = 1.0;
-
 int64 FrameTreeNode::next_frame_tree_node_id_ = 1;
 
 // static
 FrameTreeNode* FrameTreeNode::GloballyFindByID(int64 frame_tree_node_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   FrameTreeNodeIDMap* nodes = g_frame_tree_node_id_map.Pointer();
   FrameTreeNodeIDMap::iterator it = nodes->find(frame_tree_node_id);
   return it == nodes->end() ? nullptr : it->second;
 }
 
@@ skipping 122 matching lines @@
   return current_frame_host->is_loading();
 }
 
 bool FrameTreeNode::CommitPendingSandboxFlags() {
   bool did_change_flags =
       effective_sandbox_flags_ != replication_state_.sandbox_flags;
   effective_sandbox_flags_ = replication_state_.sandbox_flags;
   return did_change_flags;
 }
 
+bool FrameTreeNode::HasStartedLoading() const {
+  return loading_progress_ != kLoadingProgressNotStarted;
+}
+
+void FrameTreeNode::ResetLoadingProgress() {
+  loading_progress_ = kLoadingProgressNotStarted;
+}
+
+void FrameTreeNode::DidStartLoading(bool to_different_document) {
+  // 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 && IsMainFrame())
+    frame_tree_->ResetLoadProgress();

[Fabrice (no longer in Chrome), 2015/04/14 16:54:11]

This part was problematic.
We have "FrameTree:ResetLoadProgress" and
"WebContentsImpl::ResetLoadProgressState". Most of the times, we will want to
call the latter that will clean the states both on FrameTree and WebContents.
This call will only reset the load progress logic in FrameTree. This is not
actually a problem because we're calling UpdateLoadProgress below that will
actually take care of cleaning the states in WebContents but it is not obvious
when reading the code.

[nasko, 2015/04/14 19:54:41]

If it isn't obvious, then it is a good candidate for an explicit comment
describing what is happening and how.

[Fabrice (no longer in Chrome), 2015/04/15 15:15:02]

I clarified the comments to ensure there is no confusion about the way things
are called and how WebContents ends up being notified.

+
+  // Notify the Navigator.

[nasko, 2015/04/14 19:54:40]

Navigator doesn't really need to know about this. The only reason I see for
these calls is to notify WebContents of the events through the delegate
interface. Let's think of how to structure this without going through Navigator.
For example, why not do this:

navigator()->delegate()->DidStartLoading(...)

Since this is exactly what the NavigatorImpl does anyway. I don't think this
level of indirection is needed here.

[Fabrice (no longer in Chrome), 2015/04/15 15:15:02]

The issue is that Navigator does not have a delegate() method. NavigatorImpl
does, but not InterstitialPageNavigatorImpl.
What we really want is call into WebContents. I suggest adding a
FrameTreeDelegate class that will be implemented by WebContentsImpl. With the
new methods in FrameTreeNode, we no longer need the interface in Navigator.

+  if (!frame_tree_->IsLoading())
+    navigator()->DidStartLoading(this, to_different_document);
+
+  // Notify the RenderFrameHostManager of the event.
+  render_manager()->OnDidStartLoading();
+
+  // Set initial load progress and update overall progress.
+  loading_progress_ = kLoadingProgressMinimum;
+  frame_tree_->UpdateLoadProgress();
+

[nasko, 2015/04/14 19:54:41]

nit: no need for empty line.

[Fabrice (no longer in Chrome), 2015/04/15 15:15:02]

Done.

+}
+
+void FrameTreeNode::DidStopLoading() {
+  // Notify the RenderFrameHostManager of the event.
+  render_manager()->OnDidStopLoading();
+
+  // Set final load progress and update overall progress.
+  loading_progress_ = kLoadingProgressDone;
+  frame_tree_->UpdateLoadProgress();
+
+  // Notify the Navigator.
+  if (!frame_tree_->IsLoading())
+    navigator()->DidStopLoading();
+}
+
+void FrameTreeNode::DidChangeLoadProgress(double load_progress) {
+  loading_progress_ = load_progress;
+  frame_tree_->UpdateLoadProgress();
+}
+
 }  // namespace content