Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
912833002:
PlzNavigate: update Navigator tests post-removal of the RequestNavigation IPC (Closed)
|
Created:
5 years, 10 months ago by carlosk Modified:
5 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: update Navigator tests post-removal of the RequestNavigation IPC
Fixes and updates to NavigatorTestWithBrowserSideNavigation.
This is a pre-step for working on the navigation cancellation logic.
Several small things going on in this CL:
* Renamed methods, added and updated comments to make things clearer
* Added a couple new tests for renderer initiated navigations and
beforeUnload canceling the navigation.
* Replaced calls to TestRenderFrameHost::SendBeginNavigationWithURL with
TestRenderFrameHost::SendBeforeUnloadACK now that they represent
different things (beforeUnload reply vs renderer-initiated navigation
request)
* Added more checks in general but especially for the NavigationRequest
state, its browser_initiated flag
BUG=376014
Committed: https://crrev.com/e24d11781779d29d3e827d8762d5126173786136
Cr-Commit-Position: refs/heads/master@{#316200}
Patch Set 1 : #
Total comments: 32
Patch Set 2 : Changes from CR comments. #
Total comments: 9
Patch Set 3 : Minor changes from CR comments. #
Total comments: 12
Patch Set 4 : No more necromancer renderer-initiated navigation. #Patch Set 5 : Method renames and comment updates. #
Total comments: 2
Messages
Total messages: 22 (6 generated)
carlosk@chromium.org changed reviewers: + clamy@chromium.org, fdegans@chromium.org
carlosk@chromium.org changed required reviewers: + clamy@chromium.org
clamy@: PTAL. fdegans@: FYI. Details in the description.
Patchset #1 (id:1) has been deleted
Thanks! A few comments, mostly nits. Also, could you rename the issue to something along the lines of PlzNavigate: update NavigatorImpl tests following the removal of the RequestNavigation IPC (the important part is that the issue title should start with PlzNavigate: ). Also I think that the issue description should mention that it follows the refactoring that happened in https://codereview.chromium.org/872473003/. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (left): https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:118: nit: do not remove the empty line here. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:127: // PlzNavigate: Test a simple renderer initiated navigation starting with a nit: renderer-initiated. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:130: SimpleRendererInitiatedNavigation) { nit: I would rename the test RendererInitiatedNavigateAndCommit to match the other test. It would also be good to match the descriptions between the two tests. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:135: main_test_rfh()->SendBeginNavigationWithURL(kUrl); nit: Maybe a "// Start a renderer-initiated navigation." could be useful here? https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:144: // Request the RenderFrameHost to commit. I would rephrase that as: Have the current RenderFrameHost commit the navigation. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:151: // And commit provisional load. nit: maybe rephrase as The navigation commits. ? https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:162: // With PlzNavigate enabled a pending RenderFrameHost should never exist. nit: add an empty line above the comment. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:167: // BeforeUnloadACK. I would rephrase as: Test that a beforeUnload denial cancels the navigation. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:169: BeforeUnloadCancelledNavigation) { nit: maybe rename the test to BeforeUnloadDenialCancelNavigation. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:175: // Start a new navigation nit: add a . at the end of the comment. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:184: // Simulate a BeforeUnloadACK IPC with a signal to NOT proceed. nit: Simulate a beforeUnload denial. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:204: // Start a navigation at the subframe nit: add . at the end of the comment. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:220: // beforeUnload to run at the renderer. Please move this comment to where the previous removed comment was and add an empty line before it. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:221: EXPECT_EQ(NavigationRequest::STARTED, subframe_request->state()); Please move this expect below the call creating the subframe request (and I think that the assert can also go there). https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:253: // BeforeUnloadACK was received form the renderer so the navigation should nit: s/form/from https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:284: // As there's not live renderer the navigation should not wait for a response nit: s/not/no nit: s/response/beforeUnload ACK
Thanks! https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (left): https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:118: On 2015/02/10 15:41:22, clamy wrote: > nit: do not remove the empty line here. Done. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:127: // PlzNavigate: Test a simple renderer initiated navigation starting with a On 2015/02/10 15:41:22, clamy wrote: > nit: renderer-initiated. Done. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:130: SimpleRendererInitiatedNavigation) { On 2015/02/10 15:41:22, clamy wrote: > nit: I would rename the test RendererInitiatedNavigateAndCommit to match the > other test. It would also be good to match the descriptions between the two > tests. Done. As discussed offline I'm expanding the previous test to be a simple but complete browser-initiated navigation test. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:135: main_test_rfh()->SendBeginNavigationWithURL(kUrl); On 2015/02/10 15:41:22, clamy wrote: > nit: Maybe a "// Start a renderer-initiated navigation." could be useful here? Done. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:144: // Request the RenderFrameHost to commit. On 2015/02/10 15:41:22, clamy wrote: > I would rephrase that as: Have the current RenderFrameHost commit the > navigation. Done. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:151: // And commit provisional load. On 2015/02/10 15:41:22, clamy wrote: > nit: maybe rephrase as The navigation commits. ? Done. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:162: // With PlzNavigate enabled a pending RenderFrameHost should never exist. On 2015/02/10 15:41:22, clamy wrote: > nit: add an empty line above the comment. Done. In fact moved the comments to the previous test and removed them from here. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:167: // BeforeUnloadACK. On 2015/02/10 15:41:22, clamy wrote: > I would rephrase as: Test that a beforeUnload denial cancels the navigation. Done. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:169: BeforeUnloadCancelledNavigation) { On 2015/02/10 15:41:22, clamy wrote: > nit: maybe rename the test to BeforeUnloadDenialCancelNavigation. Done. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:175: // Start a new navigation On 2015/02/10 15:41:22, clamy wrote: > nit: add a . at the end of the comment. Done. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:184: // Simulate a BeforeUnloadACK IPC with a signal to NOT proceed. On 2015/02/10 15:41:22, clamy wrote: > nit: Simulate a beforeUnload denial. Done. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:204: // Start a navigation at the subframe On 2015/02/10 15:41:22, clamy wrote: > nit: add . at the end of the comment. Done. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:220: // beforeUnload to run at the renderer. On 2015/02/10 15:41:22, clamy wrote: > Please move this comment to where the previous removed comment was and add an > empty line before it. Done. But I moved a bit more things around so that the following line sticks together with the comment as they are closely related. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:221: EXPECT_EQ(NavigationRequest::STARTED, subframe_request->state()); On 2015/02/10 15:41:22, clamy wrote: > Please move this expect below the call creating the subframe request (and I > think that the assert can also go there). Yes, precisely. :) https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:253: // BeforeUnloadACK was received form the renderer so the navigation should On 2015/02/10 15:41:22, clamy wrote: > nit: s/form/from Done. https://codereview.chromium.org/912833002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:284: // As there's not live renderer the navigation should not wait for a response On 2015/02/10 15:41:22, clamy wrote: > nit: s/not/no > nit: s/response/beforeUnload ACK Done but mind that this whole test moved into the newly added test -- simple browser-initiated -- that starts from a non-live renderer and tests the same things.
Thanks! A few more comments and it should be good. https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:103: // PlzNavigate: Test a complete renderer-initiated navigation starting with a s/renderer-initiated/browser-initiated https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:112: int rfh_routing_id = main_test_rfh()->GetRoutingID(); I would prefer using the SiteInstance id here. https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:117: ASSERT_TRUE(GetLoaderForNavigationRequest(request)); nit: I think this ASSERT should be moved after the EXPECT_EQ(NavigationRequest::STARTED, request->state()); as the loader only gets initialized when the request starts. https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:1550: // If there's no live renderer or this is a subframe navigation then just Please remove this change. The subframe navigation case is a temporary one, as indicated by the TODO above the if condition.
Thanks! https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:103: // PlzNavigate: Test a complete renderer-initiated navigation starting with a On 2015/02/11 10:54:27, clamy wrote: > s/renderer-initiated/browser-initiated Done. https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:112: int rfh_routing_id = main_test_rfh()->GetRoutingID(); On 2015/02/11 10:54:27, clamy wrote: > I would prefer using the SiteInstance id here. Done. But could you explain why you think that's better? https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:117: ASSERT_TRUE(GetLoaderForNavigationRequest(request)); On 2015/02/11 10:54:27, clamy wrote: > nit: I think this ASSERT should be moved after the > EXPECT_EQ(NavigationRequest::STARTED, request->state()); as the loader only gets > initialized when the request starts. Done. https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:1550: // If there's no live renderer or this is a subframe navigation then just On 2015/02/11 10:54:27, clamy wrote: > Please remove this change. The subframe navigation case is a temporary one, as > indicated by the TODO above the if condition. OK, reverted as it's being updated anyway in another CL. I made this change due to me being confused when adding NavigationRequest::state checks to the subframe navigation in NavigatorTestWithBrowserSideNavigation::BeginNavigation. I expected it would have to wait for the beforeUnload ACK from the renderer but that was not the case.
Thanks! LGTM https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:112: int rfh_routing_id = main_test_rfh()->GetRoutingID(); On 2015/02/11 15:36:21, carlosk wrote: > On 2015/02/11 10:54:27, clamy wrote: > > I would prefer using the SiteInstance id here. > > Done. But could you explain why you think that's better? SiteInstance IDs are globally unique and are here to distinguish them. The routing id of a RFH is used for IPCs purposes, and is attributed by the RenderProcessHost. In tests, we use a MockRenderProcessHost instead of the RenderProcessHostImpl, so the routing id is not allocated in tests the same way as in the actual code. On top of that, routing ids are guaranteed to be unique per RenderProcessHost, but I am not sure they are globally unique.
clamy@chromium.org changed reviewers: + nasko@chromium.org
@nasko: PTAL
On 2015/02/11 16:12:17, clamy wrote: > @nasko: PTAL I wanted to raise a related concern. IMO it doesn't make sense that BeginNavigationParams::has_user_gesture is always set to false for browser-initiated navigation requests (see NavigationRequest::CreateBrowserInitiated). From previous discussions my understanding was that every browser-initiated navigation is considered user-initiated so this looks inconsistent right now. It would make sense if that attribute was *not* considered a synonym of "user-initiated". If that's true than we should update its comment in the .H file to reflect that. Otherwise we should at least add to that same comment that this property is only meaningful for renderer-initiated navigations. For now I avoided adding BeginNavigationParams::has_user_gesture EXPET-ations for browser-initiated navigations.
@carlosk: reading the other CL you sent me, I realized that there is a problem with the renderer-initiated simple test: it makes no sense for a non live renderer to start a navigation. This test should start from a live renderer that has committed a navigation.
Overall looks good, just a few comments and nits. https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:104: // non-live renderer. Do we have a test for the same, but starting with a live renderer? https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:109: EXPECT_FALSE(main_test_rfh()->IsRenderFrameLive()); Note: I wonder if this will conflict in functionality with a CL I'm working on: https://codereview.chromium.org/921443003/ We might need to explicitly set the RFH to non-live state. https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:114: SendRequestNavigation(node, kUrl); Shouldn't this method name change? We no longer send any IPCs for requesting navigations. https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:157: // non-live renderer. I'll echo clamy@'s comment here. How do we start a renderer-initiated navigation of the renderer isn't live?! https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:238: // beforeUnload to run at the renderer. FYI, this might change in the future. https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:506: // The navigation commits. nit: Let's match the style of the rest of the comments by saying what we are instructing the test to do: "Commit the navigation."
New patchsets have been uploaded after l-g-t-m from clamy@chromium.org
> https://codereview.chromium.org/912833002/diff/40001/content/browser/frame_ho... > content/browser/frame_host/navigator_impl_unittest.cc:112: int rfh_routing_id = > main_test_rfh()->GetRoutingID(); > On 2015/02/11 15:36:21, carlosk wrote: > > On 2015/02/11 10:54:27, clamy wrote: > > > I would prefer using the SiteInstance id here. > > > > Done. But could you explain why you think that's better? > > SiteInstance IDs are globally unique and are here to distinguish them. The > routing id of a RFH is used for IPCs purposes, and is attributed by the > RenderProcessHost. In tests, we use a MockRenderProcessHost instead of the > RenderProcessHostImpl, so the routing id is not allocated in tests the same way > as in the actual code. On top of that, routing ids are guaranteed to be unique > per RenderProcessHost, but I am not sure they are globally unique. Acknowledged and thanks! On 2015/02/11 17:03:46, clamy wrote: > @carlosk: reading the other CL you sent me, I realized that there is a problem > with the renderer-initiated simple test: it makes no sense for a non live > renderer to start a navigation. This test should start from a live renderer that > has committed a navigation. Yes, indeed. That didn't make any sense. :) clamy@, I'd like to ask you to PTAL at the Reload test. I'm a bit confused on how a reload request is treated and I don't follow the chain of calls driving the navigation there. More specifically, why is it not calling to TestNavigationURLLoader::CallOnResponseStarted? Isn't that necessary before the call to TestRenderFrameHost::SendNavigate? nasko@, could out also PTAL at comment https://codereview.chromium.org/912833002/#msg12 ? https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:104: // non-live renderer. On 2015/02/11 19:22:22, nasko wrote: > Do we have a test for the same, but starting with a live renderer? Yes, there's others below starting from a live one. For instance CrossSiteNavigation. https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:109: EXPECT_FALSE(main_test_rfh()->IsRenderFrameLive()); On 2015/02/11 19:22:22, nasko wrote: > Note: I wonder if this will conflict in functionality with a CL I'm working on: > https://codereview.chromium.org/921443003/ We might need to explicitly set the > RFH to non-live state. Acknowledged. https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:114: SendRequestNavigation(node, kUrl); On 2015/02/11 19:22:22, nasko wrote: > Shouldn't this method name change? We no longer send any IPCs for requesting > navigations. Indeed Send* seemed to imply an IPC that doesn't exist anymore for the request in a browser-initiated navigation. I the Send prefix from both SendRequestNavigation and SendRequestNavigationWithParameters. https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:157: // non-live renderer. On 2015/02/11 19:22:22, nasko wrote: > I'll echo clamy@'s comment here. How do we start a renderer-initiated navigation > of the renderer isn't live?! Done. https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:238: // beforeUnload to run at the renderer. On 2015/02/11 19:22:22, nasko wrote: > FYI, this might change in the future. Yes, I am aware. :) See my last comment reply in https://codereview.chromium.org/912833002/#msg8 https://codereview.chromium.org/912833002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl_unittest.cc:506: // The navigation commits. On 2015/02/11 19:22:22, nasko wrote: > nit: Let's match the style of the rest of the comments by saying what we are > instructing the test to do: "Commit the navigation." Done here and in a few other tests.
LGTM https://codereview.chromium.org/912833002/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/912833002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:161: contents()->NavigateAndCommit(kUrl1); FYI: this might not be needed after the changes in https://codereview.chromium.org/921443003/
Thanks. https://codereview.chromium.org/912833002/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl_unittest.cc (right): https://codereview.chromium.org/912833002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl_unittest.cc:161: contents()->NavigateAndCommit(kUrl1); On 2015/02/12 22:44:09, nasko wrote: > FYI: this might not be needed after the changes in > https://codereview.chromium.org/921443003/ I didn't understand what might not be needed here.
The CQ bit was checked by carlosk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/912833002/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e24d11781779d29d3e827d8762d5126173786136 Cr-Commit-Position: refs/heads/master@{#316200} |
