Move DidStartLoading, DidStopLoading, DidChangeLoadProgress to RFHI.

content/browser/frame_host/frame_tree_node.cc

Index: content/browser/frame_host/frame_tree_node.cc
diff --git a/content/browser/frame_host/frame_tree_node.cc b/content/browser/frame_host/frame_tree_node.cc
index daf4a71255d6b8a9c6f30493364f25644dd18620..9a8d4cab6a2fbc9b4b605936d7b5c28e64ed0c9b 100644
--- a/content/browser/frame_host/frame_tree_node.cc
+++ b/content/browser/frame_host/frame_tree_node.cc
@@ -26,11 +26,14 @@ typedef base::hash_map<int64, FrameTreeNode*> FrameTreeNodeIDMap;
 base::LazyInstance<FrameTreeNodeIDMap> g_frame_tree_node_id_map =
     LAZY_INSTANCE_INITIALIZER;
 
-}  // namespace
+// 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;
 
-const double FrameTreeNode::kLoadingProgressNotStarted = 0.0;
-const double FrameTreeNode::kLoadingProgressMinimum = 0.1;
-const double FrameTreeNode::kLoadingProgressDone = 1.0;
+}  // namespace
 
 int64 FrameTreeNode::next_frame_tree_node_id_ = 1;
 
@@ -174,4 +177,49 @@ bool FrameTreeNode::CommitPendingSandboxFlags() {
   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