Move load progress tracking logic to the frame tree.

Move load progress tracking logic to the frame tree.

BUG=470082
Committed: https://crrev.com/1d1635516c4f7a561f6fe8a08d1ee4c3602d8fdd
Cr-Commit-Position: refs/heads/master@{#322360}

[Fabrice (no longer in Chrome), 2015-03-24 12:39:45]

fdegans@chromium.org changed reviewers:
+ carlosk@chromium.org, clamy@chromium.org, creis@chromium.org,
nasko@chromium.org

[Fabrice (no longer in Chrome), 2015-03-24 12:39:45]

PTAL

As discussed, moving the load progress tracking to the frame tree node and the
frame tree as it makes more sense to track it here rather than be shared between
RFHI and WCI.

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

Thanks! It looks good overall, a few comments.

https://codereview.chromium.org/1015243004/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/1015243004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.h:133: // Returns this tree's total load
progress.
s/tree/FrameTree

https://codereview.chromium.org/1015243004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.h:136: // Resets the load progress on each
node of this frame tree.
s/frame tree/FrameTree

https://codereview.chromium.org/1015243004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.h:139: // Returns true if at least one of
the nodes in this frame tree is loading.
s/frame tree/FrameTree

https://codereview.chromium.org/1015243004/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1015243004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:36: // TODO(fdegans): Move these
values to the implementation when the relevant
Is this TODO still relevant?

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2931: if
(ftn->render_manager()->current_frame_host() == rfh)
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.

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2966: if
(ftn->render_manager()->current_frame_host() == rfh)
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.

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3015: if
(ftn->render_manager()->current_frame_host() != rfh)
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]

Thanks!

https://codereview.chromium.org/1015243004/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/1015243004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.h:133: // Returns this tree's total load
progress.
On 2015/03/24 14:52:23, clamy wrote:
> s/tree/FrameTree

Done.

https://codereview.chromium.org/1015243004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.h:136: // Resets the load progress on each
node of this frame tree.
On 2015/03/24 14:52:23, clamy wrote:
> s/frame tree/FrameTree

Done.

https://codereview.chromium.org/1015243004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.h:139: // Returns true if at least one of
the nodes in this frame tree is loading.
On 2015/03/24 14:52:23, clamy wrote:
> s/frame tree/FrameTree

Done.

https://codereview.chromium.org/1015243004/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1015243004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:36: // TODO(fdegans): Move these
values to the implementation when the relevant
On 2015/03/24 14:52:23, clamy wrote:
> Is this TODO still relevant?

No it isn't, unless we want to add interface methods to set the loading progress
to these specific values.
I think removing this TODO will be simpler and cleaner, doing that.

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2931: if
(ftn->render_manager()->current_frame_host() == rfh)
On 2015/03/24 14:52:23, clamy wrote:
> 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.

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.

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2966: if
(ftn->render_manager()->current_frame_host() == rfh)
On 2015/03/24 14:52:23, clamy wrote:
> 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.

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

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3015: if
(ftn->render_manager()->current_frame_host() != rfh)
On 2015/03/24 14:52:23, clamy wrote:
> Same here. I don't think we should be getting this from a RFH different from
our
> current one.

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

[nasko, 2015-03-24 17:26:12]

Just a few comments. Overall, let's not add behavior changes to CL that is just
moving code around.

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2966: if
(ftn->render_manager()->current_frame_host() == rfh)
On 2015/03/24 16:28:50, Fabrice wrote:
> On 2015/03/24 14:52:23, clamy wrote:
> > 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.
> 
> We can and we do. In fact, when we track the overall loading progress, we
track
> both pending and current RFH.

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?

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3015: if
(ftn->render_manager()->current_frame_host() != rfh)
On 2015/03/24 16:28:50, Fabrice wrote:
> On 2015/03/24 14:52:23, clamy wrote:
> > Same here. I don't think we should be getting this from a RFH different from
> our
> > current one.
> 
> Same as above, we still get these events for pending RFH.

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".

https://codereview.chromium.org/1015243004/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/1015243004/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree.h:136: // Resets the load progress on each
node of this FrameTree.
nit: s/each node of/all nodes in/.

https://codereview.chromium.org/1015243004/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1015243004/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:35: // minimum progress value.
nit: I still think we can hide those to be private/anonymous values in follow-up
CLs, so a TODO for that might be useful to have.

https://codereview.chromium.org/1015243004/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1015243004/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2932: if
(ftn->render_manager()->current_frame_host() == rfh)
I think this check might be problematic. In browser-initiated navigation,
DidStartLoading is issued browser-side, which will set the loading state to
true, but will not reset the loading progress if the navigation is cross-site
and we have a pending RFH. Later on, the pending RFH will send a
DidStartLoading, but the RFH will already be in loading state, so this method
will return early and the progress value won't be reset.

Since this CL is just moving code around, let's not make any functional changes.

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

Couple of comments on the pending RFH events for now, we do have tests sending
these events. It is possible they could all be wrong, though.
I am going to move back tracking of the load_progress per RFH.

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2966: if
(ftn->render_manager()->current_frame_host() == rfh)
On 2015/03/24 17:26:11, nasko wrote:
> On 2015/03/24 16:28:50, Fabrice wrote:
> > On 2015/03/24 14:52:23, clamy wrote:
> > > 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.
> > 
> > We can and we do. In fact, when we track the overall loading progress, we
> track
> > both pending and current RFH.
> 
> 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?

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

https://codereview.chromium.org/1015243004/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3015: if
(ftn->render_manager()->current_frame_host() != rfh)
On 2015/03/24 17:26:11, nasko wrote:
> On 2015/03/24 16:28:50, Fabrice wrote:
> > On 2015/03/24 14:52:23, clamy wrote:
> > > Same here. I don't think we should be getting this from a RFH different
from
> > our
> > > current one.
> > 
> > Same as above, we still get these events for pending RFH.
> 
> 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".

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

[Fabrice (no longer in Chrome), 2015-03-25 17:35:01]

Thanks to clamy@ and nasko@ for the review!
After doing some local tests, it is totally possible for a pending frame to send
DidStopLoading / DidChangeLoadProgress events. However, only one of them will
send subsequent DidChangeLoadProgress events, so I'm removing the checks.

https://codereview.chromium.org/1015243004/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/1015243004/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree.h:136: // Resets the load progress on each
node of this FrameTree.
On 2015/03/24 17:26:11, nasko wrote:
> nit: s/each node of/all nodes in/.

Done.

https://codereview.chromium.org/1015243004/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1015243004/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:35: // minimum progress value.
On 2015/03/24 17:26:11, nasko wrote:
> nit: I still think we can hide those to be private/anonymous values in
follow-up
> CLs, so a TODO for that might be useful to have.

The issue is that right now, we need to access these values from FrameTree and I
do not think that is going to change.

https://codereview.chromium.org/1015243004/diff/60001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1015243004/diff/60001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2932: if
(ftn->render_manager()->current_frame_host() == rfh)
On 2015/03/24 17:26:11, nasko wrote:
> I think this check might be problematic. In browser-initiated navigation,
> DidStartLoading is issued browser-side, which will set the loading state to
> true, but will not reset the loading progress if the navigation is cross-site
> and we have a pending RFH. Later on, the pending RFH will send a
> DidStartLoading, but the RFH will already be in loading state, so this method
> will return early and the progress value won't be reset.
> 
> Since this CL is just moving code around, let's not make any functional
changes.

Following our offline conversation, I removed the checks in this file.

[nasko, 2015-03-25 22:48:03]

Thanks for doing the verification! LGTM

In general, it is useful to separate rebase into a separate patchset, so it
doesn't interfere when reviewing diffs between two different patchsets.

https://codereview.chromium.org/1015243004/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1015243004/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:35: // minimum progress value.
On 2015/03/25 17:35:01, Fabrice wrote:
> On 2015/03/24 17:26:11, nasko wrote:
> > nit: I still think we can hide those to be private/anonymous values in
> follow-up
> > CLs, so a TODO for that might be useful to have.
> 
> The issue is that right now, we need to access these values from FrameTree and
I
> do not think that is going to change.

It can be hidden from FrameTree, as it will not need to tell FTN anything. Once
we move the IPC to be handled by RFH, it will be telling FTN about its status.
This will be more clear once we do that work though.

[Fabrice (no longer in Chrome), 2015-03-26 11:05:18]

The CQ bit was checked by fdegans@chromium.org

[Fabrice (no longer in Chrome), 2015-03-26 11:05:19]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1015243004/#ps100001
(title: "Readding TODO")

[Fabrice (no longer in Chrome), 2015-03-26 11:05:24]

https://codereview.chromium.org/1015243004/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1015243004/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:35: // minimum progress value.
On 2015/03/25 22:48:03, nasko wrote:
> On 2015/03/25 17:35:01, Fabrice wrote:
> > On 2015/03/24 17:26:11, nasko wrote:
> > > nit: I still think we can hide those to be private/anonymous values in
> > follow-up
> > > CLs, so a TODO for that might be useful to have.
> > 
> > The issue is that right now, we need to access these values from FrameTree
and
> I
> > do not think that is going to change.
> 
> It can be hidden from FrameTree, as it will not need to tell FTN anything.
Once
> we move the IPC to be handled by RFH, it will be telling FTN about its status.
> This will be more clear once we do that work though.

I'm readding the TODO, we'll see if we can remove it after moving the IPCs.

[commit-bot: I haz the power, 2015-03-26 11:05:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1015243004/100001

[commit-bot: I haz the power, 2015-03-26 11:58:45]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-26 12:00:00]

Patchset 6 (id:??) landed as
https://crrev.com/1d1635516c4f7a561f6fe8a08d1ee4c3602d8fdd
Cr-Commit-Position: refs/heads/master@{#322360}