Move load progress tracking logic to the frame tree.

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 147 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 used with FrameTree::ForEach() for retrieving the total
-// loading progress and number of frames in a frame tree.
-bool CollectLoadProgress(double* progress,
-                         int* frame_count,
-                         FrameTreeNode* node) {
-  // Ignore the current frame if it has not started loading.
-  double frame_progress = node->GetLoadingProgress();
-  if (frame_progress == RenderFrameHostImpl::kLoadingProgressNotStarted)
-    return true;
-
-  // Collect progress.
-  *progress += node->GetLoadingProgress();
-  (*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->IsLoading()) {
-    // There is at least one node loading, so abort traversal.
-    *is_loading = true;
-    return false;
-  }
-  return true;
-}
-
-// Helper function used with FrameTree::ForEach() to reset the load progress.
-bool ResetLoadProgress(FrameTreeNode* node) {
-  node->current_frame_host()->set_loading_progress(
-      RenderFrameHostImpl::kLoadingProgressNotStarted);
-  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 41 matching lines @@
 // termination is not allowed for the current RenderFrameHost of
 // |frame_tree_node|. Used with FrameTree::ForEach.
 bool SuddenTerminationAllowed(bool* sudden_termination_allowed,
                               FrameTreeNode* frame_tree_node) {
   if (frame_tree_node->current_frame_host()->SuddenTerminationAllowed())
     return true;
   *sudden_termination_allowed = false;
   return false;
 }
 
-// Returns true if at least one of the nodes in the |frame_tree| is loading.
-bool IsFrameTreeLoading(FrameTree& frame_tree) {
-  bool is_loading = false;
-  frame_tree.ForEach(base::Bind(&IsNodeLoading, &is_loading));
-  return is_loading;
-}
 
 }  // namespace
 
 WebContents* WebContents::Create(const WebContents::CreateParams& params) {
   return WebContentsImpl::CreateWithOpener(
       params, static_cast<WebContentsImpl*>(params.opener));
 }
 
 WebContents* WebContents::CreateWithSessionStorage(
     const WebContents::CreateParams& params,
@@ skipping 2676 matching lines @@
   // This method should never be called when the frame is loading.
   // Unfortunately, it can happen if a history navigation happens during a
   // BeforeUnload or Unload event.
   // TODO(fdegans): Change this to a DCHECK after LoadEventProgress has been
   // refactored in Blink. See crbug.com/466089
   if (rfh->is_loading()) {
     LOG(WARNING) << "OnDidStartLoading was called twice.";
     return;
   }
 
-  if (!IsFrameTreeLoading(frame_tree_))
+  if (!frame_tree_.IsLoading())
     DidStartLoading(rfh, to_different_document);
 
   rfh->set_is_loading(true);
-  rfh->set_loading_progress(RenderFrameHostImpl::kLoadingProgressMinimum);
+
+  FrameTreeNode* ftn = rfh->frame_tree_node();
+  if (ftn->render_manager()->current_frame_host() == rfh)

[clamy, 2015/03/24 14:52:23]

Should we reset the load progress even if its the pending frame host that starts
navigating? I know this is not what we do right now, I'm wondering if it makes
sense now that the value is in the FrameTreeNode.
If it doesn't, could you add a comment explaining that we only reset the loading
progress when the current RFH starts navigating.

[Fabrice (no longer in Chrome), 2015/03/24 16:28:50]

We probably should reset but that is going to make tracking things harder in the
other methods since these track the current load progress.
I'll just add a comment here.

+    ftn->set_loading_progress(FrameTreeNode::kLoadingProgressMinimum);
 
   // Notify the RenderFrameHostManager of the event.
-  rfh->frame_tree_node()->render_manager()->OnDidStartLoading();
+  ftn->render_manager()->OnDidStartLoading();
 
   SendLoadProgressChanged();
 }
 
 void WebContentsImpl::OnDidStopLoading() {
   // TODO(erikchen): Remove ScopedTracker below once crbug.com/465796 is
   // fixed.
   tracked_objects::ScopedTracker tracking_profile1(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "465796 WebContentsImpl::OnDidStopLoading::Start"));
 
   if (!HasValidFrameSource())
     return;
 
   RenderFrameHostImpl* rfh =
       static_cast<RenderFrameHostImpl*>(render_frame_message_source_);
 
   // This method should never be called when the frame is not loading.
   // Unfortunately, it can happen if a history navigation happens during a
   // BeforeUnload or Unload event.
   // TODO(fdegans): Change this to a DCHECK after LoadEventProgress has been
   // refactored in Blink. See crbug.com/466089
   if (!rfh->is_loading()) {
     LOG(WARNING) << "OnDidStopLoading was called twice.";
     return;
   }
 
   rfh->set_is_loading(false);
-  rfh->set_loading_progress(RenderFrameHostImpl::kLoadingProgressDone);
+
+  FrameTreeNode* ftn = rfh->frame_tree_node();
+  if (ftn->render_manager()->current_frame_host() == rfh)

[clamy, 2015/03/24 14:52:23]

Can we get a DidStopLoading IPC from the pending RFH? I don't think so, but I
may be wrong. In that case, the if condition is unnecessary and should be
replaced with a DCHECK.

[Fabrice (no longer in Chrome), 2015/03/24 16:28:50]

We can and we do. In fact, when we track the overall loading progress, we track
both pending and current RFH.

[nasko, 2015/03/24 17:26:11]

I'm not sure we can get a DidStopLoading from the pending RFH, as it won't have
even committed a navigation. Do we have a test that does this?

[Fabrice (no longer in Chrome), 2015/03/25 13:25:12]

I have found that DownloadContentTest.RemoveDownload and
SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs in
content_browsertests do this.

+    ftn->set_loading_progress(FrameTreeNode::kLoadingProgressDone);
 
   // TODO(erikchen): Remove ScopedTracker below once crbug.com/465796 is
   // fixed.
   tracked_objects::ScopedTracker tracking_profile2(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "465796 "
           "WebContentsImpl::OnDidStopLoading::SendLoadProgressChanged"));
 
   // Update progress based on this frame's completion.
   SendLoadProgressChanged();
 
   // Then clean-up the states.
   if (loading_total_progress_ == 1.0)
     ResetLoadProgressState();
 
   // TODO(erikchen): Remove ScopedTracker below once crbug.com/465796 is
   // fixed.
   tracked_objects::ScopedTracker tracking_profile3(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "465796 WebContentsImpl::OnDidStopLoading::NotifyRenderManager"));
   // Notify the RenderFrameHostManager of the event.
-  rfh->frame_tree_node()->render_manager()->OnDidStopLoading();
+  ftn->render_manager()->OnDidStopLoading();
 
-  if (!IsFrameTreeLoading(frame_tree_)) {
+  if (!frame_tree_.IsLoading()) {
     // TODO(erikchen): Remove ScopedTracker below once crbug.com/465796 is
     // fixed.
     tracked_objects::ScopedTracker tracking_profile4(
         FROM_HERE_WITH_EXPLICIT_FUNCTION(
             "465796 WebContentsImpl::OnDidStopLoading::WCIDidStopLoading"));
     DidStopLoading();
   }
 
   // TODO(erikchen): Remove ScopedTracker below once crbug.com/465796 is
   // fixed.
   tracked_objects::ScopedTracker tracking_profile4(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "465796 WebContentsImpl::OnDidStopLoading::End"));
 }
 
 void WebContentsImpl::OnDidChangeLoadProgress(double load_progress) {
   if (!HasValidFrameSource())
     return;
 
   RenderFrameHostImpl* rfh =
       static_cast<RenderFrameHostImpl*>(render_frame_message_source_);
+  FrameTreeNode* ftn = rfh->frame_tree_node();
 
-  rfh->set_loading_progress(load_progress);
+  if (ftn->render_manager()->current_frame_host() != rfh)

[clamy, 2015/03/24 14:52:23]

Same here. I don't think we should be getting this from a RFH different from our
current one.

[Fabrice (no longer in Chrome), 2015/03/24 16:28:50]

Same as above, we still get these events for pending RFH.

[nasko, 2015/03/24 17:26:11]

Really? Real progress cannot be made before we commit a navigation. Maybe you
are seeing the equivalent of "let's set our initial progress that we've started
doing something".

[Fabrice (no longer in Chrome), 2015/03/25 13:25:12]

I have found that the following tests send the event for the pending RFH:
    BrowserSideNavigationBrowserTest.BrowserInitiatedNavigations
    CrossProcessFrameTreeBrowserTest.CreateCrossProcessSubframeProxies
    CrossProcessFrameTreeBrowserTest.OriginSetOnCrossProcessNavigations
    CrossSiteTransferTest.ReplaceEntryCrossProcessThenTransfer
    CrossSiteTransferTest.ReplaceEntryCrossProcessTwice
    DownloadContentTest.RemoveDownload
    FrameTreeBrowserTest.NavigateWithLeftoverFrames
    FrameTreeBrowserTest.OriginSetOnNavigation
    NavigationControllerBrowserTest.CorrectLengthWithNewTabNavigatingFromWebUI
    RFHMProcessPerTabTest.BackFromWebUI
    RenderFrameHostManagerTest.AllowTargetedNavigationsAfterSwap
    RenderFrameHostManagerTest.AllowTargetedNavigationsInOpenerAfterSwap
    RenderFrameHostManagerTest.BackForwardNotStale
    RenderFrameHostManagerTest.DisownOpener
    RenderFrameHostManagerTest.DontPreemptNavigationWithFrameTreeUpdate
    RenderFrameHostManagerTest.ForceSwapAfterWebUIBindings
    RenderFrameHostManagerTest.LeakingRenderViewHosts
    RenderFrameHostManagerTest.NoScriptAccessAfterSwapOut
    RenderFrameHostManagerTest.ProcessExitWithSwappedOutViews
    RenderFrameHostManagerTest.RestoreFileAccessForHistoryNavigation
    RenderFrameHostManagerTest.RestoreSubframeFileAccessForHistoryNavigation
    RenderFrameHostManagerTest.SupportCrossProcessPostMessage
    RenderFrameHostManagerTest.SupportCrossProcessPostMessageWithMessagePort
    RenderFrameHostManagerTest.SwappedOutViewHasCorrectVisibilityState
    RenderFrameHostManagerTest.WebUIGetsBindings
    RenderViewHostTest.BaseURLParam
    ResourceDispatcherHostBrowserTest.CrossSiteNavigationNonBuffered
    ServiceWorkerBrowserTest.ResponseFromHTTPSServiceWorkerIsMarkedAsSecure
    SitePerProcessAccessibilityBrowserTest.CrossSiteIframeAccessibility
    SitePerProcessBrowserTest.CrossSiteDidStopLoading
    SitePerProcessBrowserTest.CrossSiteIframe
    SitePerProcessBrowserTest.DynamicSandboxFlags
    SitePerProcessBrowserTest.DynamicSandboxFlagsRemoteToLocal
    SitePerProcessBrowserTest.DynamicSandboxFlagsRendererInitiatedNavigation
    SitePerProcessBrowserTest.DynamicWindowName
    SitePerProcessBrowserTest.KillingRendererClearsDescendantProxies
    SitePerProcessBrowserTest.LoadEventForwarding
    SitePerProcessBrowserTest.NavigateRemoteFrame
    SitePerProcessBrowserTest.NavigateRemoteFrameToBlankAndDataURLs
    SitePerProcessBrowserTest.NavigateRemoteFrameToKilledProcess
    SitePerProcessBrowserTest.NavigateRemoteFrameToKilledProcessWithSubtree
    SitePerProcessBrowserTest.NavigateWithSiblingRemoteFrame
    SitePerProcessBrowserTest.OriginReplication
    SitePerProcessBrowserTest.ProxyCreationSkipsSubtree
    SitePerProcessBrowserTest.SandboxFlagsReplication
    SitePerProcessBrowserTest.WindowNameReplication
    SitePerProcessDevToolsBrowserTest.CrossSiteIframeAgentHost
    WebContentsImplBrowserTest.GetSizeForNewRenderView
    WebContentsImplBrowserTest.LoadProgressAfterInterruptedNav
    WebContentsImplBrowserTest.RenderFrameCreatedCorrectProcessForObservers
    WebContentsViewAuraTest.ScreenshotForSwappedOutRenderViews
    WebUIMojoTest.EndToEndPing

+    return;
+
+  ftn->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 453 matching lines @@
   NotificationService::current()->Notify(
       NOTIFICATION_WEB_CONTENTS_TITLE_UPDATED,
       Source<WebContents>(this),
       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;
+  double progress = frame_tree_.GetLoadProgress();
 
-  frame_tree_.ForEach(
-      base::Bind(&CollectLoadProgress, &progress, &frame_count));
-  if (frame_count != 0)
-    progress /= frame_count;
   DCHECK_LE(progress, 1.0);
 
   if (progress <= loading_total_progress_)
     return;
   loading_total_progress_ = progress;
 
   if (delegate_)
     delegate_->LoadProgressChanged(this, progress);
 }
 
 void WebContentsImpl::ResetLoadProgressState() {
-  frame_tree_.ForEach(base::Bind(&ResetLoadProgress));
+  frame_tree_.ResetLoadProgress();
   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 1097 matching lines @@
   node->render_manager()->ResumeResponseDeferredAtStart();
 }
 
 void WebContentsImpl::SetForceDisableOverscrollContent(bool force_disable) {
   force_disable_overscroll_content_ = force_disable;
   if (view_)
     view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 }
 
 }  // namespace content