Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1108283002:
PlzNavigate: Call DidStartLoading and DidStopLoading. (Closed)
|
Created:
5 years, 7 months ago by Fabrice (no longer in Chrome) Modified:
5 years, 7 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Call DidStartLoading and DidStopLoading.
This fixes many tests in content_unittests and content_browsertests.
BUG=439423, 470082, 475027
Committed: https://crrev.com/39ff038656968e925eebe5d1bbb4ccd92f31cc7b
Cr-Commit-Position: refs/heads/master@{#327535}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Review comments. #
Total comments: 2
Patch Set 3 : Commentses #
Total comments: 4
Patch Set 4 : Rephrase comment #
Total comments: 4
Patch Set 5 : Commentses. #
Messages
Total messages: 20 (3 generated)
fdegans@chromium.org changed reviewers: + carlosk@chromium.org, clamy@chromium.org, creis@chromium.org, nasko@chromium.org
PTAL. FYI, this fixes the following tests: content_unittests: NavigationControllerTest.LoadURL_AbortDoesntCancelPending NavigationControllerTest.LoadURL_IgnorePreemptsPending NavigationControllerTest.ShowRendererURLAfterFailUntilModified NavigationControllerTest.ShowRendererURLInNewTabUntilModified content_browsertests: DatabaseTest.ReloadPage FrameTreeBrowserTest.FrameTreeAfterCrash MediaCanPlayTypeTest.CodecSupportTest_Avc1Variants MediaCanPlayTypeTest.CodecSupportTest_Avc3Variants MediaCanPlayTypeTest.CodecSupportTest_AvcLevels MediaCanPlayTypeTest.CodecSupportTest_HLS MediaCanPlayTypeTest.CodecSupportTest_mp3 MediaCanPlayTypeTest.CodecSupportTest_mp4 MediaCanPlayTypeTest.CodecSupportTest_Mp4aVariants MediaCanPlayTypeTest.CodecSupportTest_ogg MediaCanPlayTypeTest.CodecSupportTest_wav MediaCanPlayTypeTest.CodecSupportTest_webm NavigationControllerBrowserTest.CorrectLengthWithNewTabNavigatingFromWebUI NavigationControllerBrowserTest.DontIgnoreBackAfterNavEntryLimit RenderFrameHostManagerTest.ClickLinkAfter204Error RenderFrameHostManagerTest.ForceSwapAfterWebUIBindings RenderFrameHostManagerTest.IgnoreRendererDebugURLsWhenCrashed ResourceDispatcherHostBrowserTest.CrossSiteAfterCrash ResourceDispatcherHostBrowserTest.CrossSiteNoUnloadOn204 ResourceFetcherTests.ResourceFetcher404 ResourceFetcherTests.ResourceFetcherDeletedInCallback ResourceFetcherTests.ResourceFetcherDidFail ResourceFetcherTests.ResourceFetcherDownload ResourceFetcherTests.ResourceFetcherPost ResourceFetcherTests.ResourceFetcherSetHeader ResourceFetcherTests.ResourceFetcherTimeout SessionHistoryTest.LocationReplace StatsTableBrowserTest.StartWithStatTable WebContentsImplBrowserTest.ChangeDisplayMode WebContentsViewAuraTest.ScreenshotForSwappedOutRenderViews
Thanks! One comment, and besides that the way we compute whether a FrameTreeNode is loading should be updated: it is no longer whether the current or pending RFH are loading, but whether we have a NavigationRequest or the current RFH is loading. https://codereview.chromium.org/1108283002/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1108283002/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:193: url::kJavaScriptScheme)) { Could you add a TODO to set the boolean properly once we recognize same-document navigation on the browser-side?
Thanks! I had to reset the navigation request earlier in ResetNavigationRequest. https://codereview.chromium.org/1108283002/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1108283002/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:193: url::kJavaScriptScheme)) { On 2015/04/28 11:42:56, clamy wrote: > Could you add a TODO to set the boolean properly once we recognize same-document > navigation on the browser-side? Done.
Thanks! Lgtm with one nit. https://codereview.chromium.org/1108283002/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1108283002/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:215: navigation_request_.reset(); nit: I wonder if this is clearer: if (navigation_request_) { navigation_request_.reset(); if (is_commit) return; DidStopLoading(); render_manager_.CleanUpNavigation(); }
Thanks! https://codereview.chromium.org/1108283002/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1108283002/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:215: navigation_request_.reset(); On 2015/04/28 14:46:50, clamy wrote: > nit: I wonder if this is clearer: > if (navigation_request_) { > navigation_request_.reset(); > if (is_commit) > return; > DidStopLoading(); > render_manager_.CleanUpNavigation(); > } I think it's clearer too. Done.
https://codereview.chromium.org/1108283002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/1108283002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:998: EXPECT_EQ(4, delegate->navigation_state_change_count()); Why do we have different value for the state changes here?
https://codereview.chromium.org/1108283002/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/1108283002/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:998: EXPECT_EQ(4, delegate->navigation_state_change_count()); On 2015/04/28 17:10:08, nasko wrote: > Why do we have different value for the state changes here? Because with PlzNavigate, the SendRendererInitiatedNavigationRequest call will stop the old navigation and start a new one, hence, 2 extra state changes. I think this makes more sense from the user's perspective as the throbber will reset.
LGTM once Nasko's happy. One question about the comment below. https://codereview.chromium.org/1108283002/diff/40001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1108283002/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:211: // cleanup performed since the navigation is still ongoing. If the reset What navigation tasks are still ongoing after commit? I generally consider that to be the end of navigation. Is it loading state? (Just wondering if we can clarify this comment.)
LGTM, provided we address Charlie's comment.
Thanks! https://codereview.chromium.org/1108283002/diff/40001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1108283002/diff/40001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:211: // cleanup performed since the navigation is still ongoing. If the reset On 2015/04/28 23:10:04, Charlie Reis wrote: > What navigation tasks are still ongoing after commit? I generally consider that > to be the end of navigation. Is it loading state? (Just wondering if we can > clarify this comment.) Yes it is the loading state. I rephrased the comment a bit. WDYT
Thanks! A comment about the comment :). https://codereview.chromium.org/1108283002/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1108283002/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:211: // cleanup performed since the load is still being tracked. If the reset Note that the reason render_manager_.CleanUpNavigation() is not called at commit time is due to the fact that we rely on RenderFrameHostManager::DidNavigate to perform the appropriate cleanup in that case.
Thanks! https://codereview.chromium.org/1108283002/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1108283002/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:211: // cleanup performed since the load is still being tracked. If the reset On 2015/04/29 10:01:20, clamy wrote: > Note that the reason render_manager_.CleanUpNavigation() is not called at commit > time is due to the fact that we rely on RenderFrameHostManager::DidNavigate to > perform the appropriate cleanup in that case. I changed the order a bit and provided clarifications for both cases. I feel it's more readable.
LGTM https://codereview.chromium.org/1108283002/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1108283002/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:211: // cleanup performed since the load is still being tracked. If the reset On 2015/04/29 14:32:24, Fabrice wrote: > On 2015/04/29 10:01:20, clamy wrote: > > Note that the reason render_manager_.CleanUpNavigation() is not called at > commit > > time is due to the fact that we rely on RenderFrameHostManager::DidNavigate to > > perform the appropriate cleanup in that case. > > I changed the order a bit and provided clarifications for both cases. I feel > it's more readable. Thanks! That looks much better!
LGTM https://codereview.chromium.org/1108283002/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1108283002/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:211: // cleanup performed since the load is still being tracked. If the reset On 2015/04/29 16:27:39, nasko wrote: > On 2015/04/29 14:32:24, Fabrice wrote: > > On 2015/04/29 10:01:20, clamy wrote: > > > Note that the reason render_manager_.CleanUpNavigation() is not called at > > commit > > > time is due to the fact that we rely on RenderFrameHostManager::DidNavigate > to > > > perform the appropriate cleanup in that case. > > > > I changed the order a bit and provided clarifications for both cases. I feel > > it's more readable. > > Thanks! That looks much better! Agreed, thanks.
The CQ bit was checked by fdegans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org Link to the patchset: https://codereview.chromium.org/1108283002/#ps80001 (title: "Commentses.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1108283002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/39ff038656968e925eebe5d1bbb4ccd92f31cc7b Cr-Commit-Position: refs/heads/master@{#327535} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
