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/profiler/scoped_tracker.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.
+const double kLoadingProgressNotStarted = 0.0;
+const double kLoadingProgressMinimum = 0.1;
+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 {

[Charlie Reis, 2015/04/15 23:37:53]

This should be in the .h file as has_started_loading().

[Fabrice (no longer in Chrome), 2015/04/16 13:55:25]

Then we'd have to move the kLoadingProgress constants back to the header, which
we didn't want to do.

[nasko, 2015/04/16 16:23:48]

It still should be hacker_cased though, as it just returns a simple value
without doing any work.

[Fabrice (no longer in Chrome), 2015/04/16 16:33:30]

Done.

+  return loading_progress_ != kLoadingProgressNotStarted;
+}
+
+void FrameTreeNode::ResetLoadingProgress() {

[Charlie Reis, 2015/04/15 23:37:53]

Same: reset_loading_progress().

[Fabrice (no longer in Chrome), 2015/04/16 13:55:25]

Same as above.

[nasko, 2015/04/16 16:23:48]

Again, hacker_cased.

[Fabrice (no longer in Chrome), 2015/04/16 16:33:30]

Done.

+  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. The WebContents will
+  // be notified when UpdateLoadProgress is called.
+  if (to_different_document && IsMainFrame())
+    frame_tree_->ResetLoadProgress();
+
+  // Notify the WebContents.
+  if (!frame_tree_->IsLoading())
+    navigator()->GetDelegate()->DidStartLoading(this, to_different_document);
+
+  // Notify the RenderFrameHostManager of the event.

[nasko, 2015/04/15 19:17:05]

I wonder if it will be more consistent if all notifications are done after the
state on this FTN is changed to indicate it has started loading. If we don't,
for example the code in RFHM asking this FTN for HasStartedLoading will return
false, which will be contradictory.

The notification to WebContents is definitely gated on checking IsLoading(), so
that check must be done before changing the state of the FTN, but maybe that
result can be captured in a local variable and referenced later on.

[Fabrice (no longer in Chrome), 2015/04/16 13:55:25]

I am not sure what you are referring to, afaict RFHM does not check the calling
FTN status, same for the WebContents. Tests assume calls
DidStartLoading/DidStopLoading and update of the load progress are done in a
specific order (see comments in the observer in
web_contents_impl_browsertest.cc) so that order cannot be changed.
On the other hand, the RFHM call seems to be independent so it can be done
before or after in both cases. I am changing it to be done after.
The is_loading tracking is done on the RFH. If we wanted to do that, we'd need
to pass an extra argument to FTN and WebContents, which we wanted to avoid.

[nasko, 2015/04/16 16:23:48]

My concerns are addressed by the usage of DidChangeLoadProgress earlier in the
method.

+  render_manager()->OnDidStartLoading();
+
+  // Set initial load progress and update overall progress. This will notify
+  // the WebContents of the load progress change.
+  loading_progress_ = kLoadingProgressMinimum;
+  frame_tree_->UpdateLoadProgress();

[Charlie Reis, 2015/04/15 23:37:53]

We should probably use DidChangeLoadProgress here rather than duplicating the
code.  (This will help if we later add another statement to this pattern.)

[Fabrice (no longer in Chrome), 2015/04/16 13:55:25]

Done.
Also updated the comments.

+}
+
+void FrameTreeNode::DidStopLoading() {
+  // TODO(erikchen): Remove ScopedTracker below once crbug.com/465796 is fixed.
+  tracked_objects::ScopedTracker tracking_profile1(
+      FROM_HERE_WITH_EXPLICIT_FUNCTION(
+          "465796 FrameTreeNode::DidStopLoading::Start"));
+
+  // Notify the RenderFrameHostManager of the event.
+  render_manager()->OnDidStopLoading();

[nasko, 2015/04/15 19:17:05]

Same concern as above, should we change the state before notifying.

[Fabrice (no longer in Chrome), 2015/04/16 13:55:25]

Moved that at the end of the method.

+
+  // TODO(erikchen): Remove ScopedTracker below once crbug.com/465796 is fixed.
+  tracked_objects::ScopedTracker tracking_profile2(
+      FROM_HERE_WITH_EXPLICIT_FUNCTION(
+          "465796 FrameTreeNode::DidStopLoading::UpdateLoadProgress"));
+
+  // Set final load progress and update overall progress. This will notify
+  // the WebContents of the load progress change.
+  loading_progress_ = kLoadingProgressDone;
+  frame_tree_->UpdateLoadProgress();

[Charlie Reis, 2015/04/15 23:37:53]

Same here: call DidChangeLoadProgress.

[Fabrice (no longer in Chrome), 2015/04/16 13:55:25]

Done.

+
+  // TODO(erikchen): Remove ScopedTracker below once crbug.com/465796 is fixed.
+  tracked_objects::ScopedTracker tracking_profile3(
+      FROM_HERE_WITH_EXPLICIT_FUNCTION(
+          "465796 FrameTreeNode::DidStopLoading::WCIDidStopLoading"));
+
+  // Notify the WebContents.
+  if (!frame_tree_->IsLoading())
+    navigator()->GetDelegate()->DidStopLoading();
+
+  // TODO(erikchen): Remove ScopedTracker below once crbug.com/465796 is fixed.
+  tracked_objects::ScopedTracker tracking_profile4(
+      FROM_HERE_WITH_EXPLICIT_FUNCTION(
+          "465796 FrameTreeNode::DidStopLoading::End"));
+}
+
+void FrameTreeNode::DidChangeLoadProgress(double load_progress) {
+  loading_progress_ = load_progress;
+  frame_tree_->UpdateLoadProgress();
+}
+
 }  // namespace content