Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
761013003:
PlzNavigate: add support in several navigation controller unit tests (Closed)
|
Created:
6 years ago by clamy Modified:
6 years 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: add support in several navigation controller unit tests
This CL rewrites several NavigationControllerTests so that they use helper
functions working with PlzNavigate instead of being dependant on the current
implementation of navigation.
BUG=439423
Committed: https://crrev.com/7c5016ccbd307a4e2a2ca140cc0739f29f288bb0
Cr-Commit-Position: refs/heads/master@{#309207}
Patch Set 1 #Patch Set 2 : #
Total comments: 13
Patch Set 3 : Moved the functions to the TestRenderFrameHost #
Total comments: 24
Patch Set 4 : Addressed Nasko's comments #
Total comments: 14
Patch Set 5 : Fixed description of states #Patch Set 6 : Addressed Nasko's comments #
Messages
Total messages: 23 (4 generated)
clamy@chromium.org changed reviewers: + carlosk@chromium.org, nasko@chromium.org
@carlosk, nasko: PTAL. This is a refactoring of several NavigationControllerTests that are currently failing with PlzNavigate (see the FYI bot for more details: http://build.chromium.org/p/chromium.fyi/builders/Browser%20Side%20Navigation...). The main cause of failure is that they expect a pending RFH to be present, or that the DidCommitProvisionalLoad IPC is sufficient to represent navigation commit. The CL removes those assumptions by providing helper method in TestWebContents that will simulate the appropriate IPCs depending on whether browser side navigation is enabled.
Bunch of comments, but my main concern is losing the ability to navigate at the FTN/RFH level. https://chromiumcodereview.appspot.com/761013003/diff/20001/content/browser/f... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/20001/content/browser/f... content/browser/frame_host/navigation_controller_impl_unittest.cc:379: contents()->CommitNavigationWithPageID(0, url1, false); Why do we have false in this case? https://chromiumcodereview.appspot.com/761013003/diff/20001/content/browser/f... content/browser/frame_host/navigation_controller_impl_unittest.cc:801: LoadCommittedDetails details; drive-by nit: It looks like details isn't used anymore and can be cleaned up. It appears in a few places in this file. https://chromiumcodereview.appspot.com/761013003/diff/20001/content/browser/f... content/browser/frame_host/navigation_controller_impl_unittest.cc:826: contents()->CommitPendingNavigationWithBindings(2, false); As I'm reading through these changes, it seems like we are losing the capability to perform those navigation operations on the FTN/RFH level. If we want to test subframe navigations, which I believe we will want to add more tests for, the way the code in this patch is written won't allow for it. https://chromiumcodereview.appspot.com/761013003/diff/20001/content/test/test... File content/test/test_web_contents.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/20001/content/test/test... content/test/test_web_contents.cc:149: // (old_rfh != rfh). Why have in comment the same condition you are testing in code right after? https://chromiumcodereview.appspot.com/761013003/diff/20001/content/test/test... content/test/test_web_contents.cc:235: void TestWebContents::ProceedNavigationWithRendererResponse(const GURL& url) { Does it matter that it is "WithRendererResponse"? https://chromiumcodereview.appspot.com/761013003/diff/20001/content/test/test... content/test/test_web_contents.cc:271: void TestWebContents::CommitPendingNavigationNoLiveRenderer() { This method is almost identical to the latter portion of TestWebContents::NavigateAndCommit. Is there any reason to have so much code that is the same be duplicated? https://chromiumcodereview.appspot.com/761013003/diff/20001/content/test/test... content/test/test_web_contents.cc:281: // (old_rfh != rfh). nit: Another instance of comment and code being the same. https://chromiumcodereview.appspot.com/761013003/diff/20001/content/test/test... content/test/test_web_contents.cc:330: void TestWebContents::CommitNavigationWithParams( Shouldn't this method name start with CommitPendingNavigation? It seems all others have the pending part in the name. https://chromiumcodereview.appspot.com/761013003/diff/20001/content/test/test... content/test/test_web_contents.cc:336: // If we are doing a cross-site navigation, this simulates the current RVH nit: Did you mean RFH?
In that case, I could introduce these methods on the tree node level. That would require making a TestFrameTreeNode class I think (probably along with a FrameTreeNodeFactory and TestFrameTreeNodeFactory). Is that ok with you?
On 2014/12/11 13:01:18, clamy wrote: > In that case, I could introduce these methods on the tree node level. That would > require making a TestFrameTreeNode class I think (probably along with a > FrameTreeNodeFactory and TestFrameTreeNodeFactory). Is that ok with you? Can't we put those methods on the TestRenderFrameHost? In the methods, we can reach up to the FTN and/or RFHM and drive any interaction from there. The alternative is a static method that takes the RFH as input, but it doesn't seem much different other than technicalities.
Patchset #3 (id:40001) has been deleted
Thanks! I rewrote the patch entirely to have it use TestRenderFrameHost. One of the tricky part is that we don't have the pending RFH before the IO stack commits, hence the calls to SimulateIOThread in the tests. https://chromiumcodereview.appspot.com/761013003/diff/20001/content/browser/f... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/20001/content/browser/f... content/browser/frame_host/navigation_controller_impl_unittest.cc:801: LoadCommittedDetails details; On 2014/12/11 00:51:45, nasko wrote: > drive-by nit: It looks like details isn't used anymore and can be cleaned up. It > appears in a few places in this file. Done. https://chromiumcodereview.appspot.com/761013003/diff/20001/content/browser/f... content/browser/frame_host/navigation_controller_impl_unittest.cc:826: contents()->CommitPendingNavigationWithBindings(2, false); On 2014/12/11 00:51:45, nasko wrote: > As I'm reading through these changes, it seems like we are losing the capability > to perform those navigation operations on the FTN/RFH level. If we want to test > subframe navigations, which I believe we will want to add more tests for, the > way the code in this patch is written won't allow for it. Done. https://chromiumcodereview.appspot.com/761013003/diff/20001/content/test/test... File content/test/test_web_contents.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/20001/content/test/test... content/test/test_web_contents.cc:149: // (old_rfh != rfh). On 2014/12/11 00:51:45, nasko wrote: > Why have in comment the same condition you are testing in code right after? Done. https://chromiumcodereview.appspot.com/761013003/diff/20001/content/test/test... content/test/test_web_contents.cc:336: // If we are doing a cross-site navigation, this simulates the current RVH On 2014/12/11 00:51:45, nasko wrote: > nit: Did you mean RFH? Done. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_controller_impl_unittest.cc:764: contents()->GetMainFrame()->SendNavigate(3, kNewURL); I modified this line because the description of the test is "This will happen if the user types in a URL to somewhere slow, and then navigates the current page before the typed URL commits." Therefore the renderer navigating should be the current one, not the pending one. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_request.h:30: enum NavigationState { I actually plan to use this enum for things other than tests in the future. Also it helps when checking that navigation proceeds correctly.
This looks much better! Few more comments. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_controller_impl_unittest.cc:419: if (base::CommandLine::ForCurrentProcess()->HasSwitch( This code pattern appears few times in this file. Why not abstract this out in a method on the TestRFH? https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_controller_impl_unittest.cc:3925: const IPC::Message* message = process()->sink().GetFirstMessageMatching( It seems that this branching logic can be put in a couple of methods - one to get the IPC message and one to get the URL. This will make the body of the test case a bit more streamlined and easier to read. Granted, it is only used in one test, so I'd leave it up to you. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... File content/browser/frame_host/navigation_request.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_request.cc:61: state_ = RESPONSE_STARTED; DCHECK that the request has started? https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_request.cc:67: state_ = FAILED; Similarly here, we shouldn't reach this code unless the state is STARTED, right? https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_request.h:30: enum NavigationState { On 2014/12/15 17:01:39, clamy wrote: > I actually plan to use this enum for things other than tests in the future. Also > it helps when checking that navigation proceeds correctly. Let's put a comment explaining the goal/usage of it and the states. Some are clear, but some could mean different things, so documenting precisely the meaning will be very useful. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_request.h:58: void SetWaitingForRendererResponse() { Do we not expect to have other states being set externally? https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... File content/test/test_render_frame_host.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... content/test/test_render_frame_host.cc:216: // PlzNavigate: if there is a live renderer, send its response to the Is the condition "live renderer"? The check below is about state of the request. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... File content/test/test_render_frame_host.h (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... content/test/test_render_frame_host.h:101: void SimulateIOThread(const GURL& url); This is a bit too specific of a name for unit tests to call. Can we abstract the name somehow to a higher level concept that happens regardless of the mode of operation? I like the above "SendRendererResponse...", which is clear as to what it does and in response to what event. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... content/test/test_render_frame_host.h:103: nit: no need for empty line here, rather above "private:"
Thanks! Please find my answer to a few comments below. I will try to upload a new patchset tomorrow. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_request.h:58: void SetWaitingForRendererResponse() { On 2014/12/16 01:40:26, nasko wrote: > Do we not expect to have other states being set externally? No the other states are set because of the interactions with the IO thread (beginning the request on the IO thread, and the response being ready to commit or the request failing). https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... File content/test/test_render_frame_host.h (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... content/test/test_render_frame_host.h:101: void SimulateIOThread(const GURL& url); On 2014/12/16 01:40:26, nasko wrote: > This is a bit too specific of a name for unit tests to call. Can we abstract the > name somehow to a higher level concept that happens regardless of the mode of > operation? I like the above "SendRendererResponse...", which is clear as to what > it does and in response to what event. Would the name MakeNavigationReadyForCommit be better? I could also modify the function so that it does the logic found in the NavigationControllerTests (ie if PlzNavigate what this function currently does otherwise simulate the BeforeUnload ack).
creis@chromium.org changed reviewers: + creis@chromium.org
https://codereview.chromium.org/761013003/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/761013003/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:764: contents()->GetMainFrame()->SendNavigate(3, kNewURL); On 2014/12/15 17:01:39, clamy wrote: > I modified this line because the description of the test is "This will happen if > the user types in a URL to somewhere slow, and then navigates the current page > before the typed URL commits." Therefore the renderer navigating should be the > current one, not the pending one. Yes, I appear to have broken this in https://codereview.chromium.org/6973073, as part of a more general attempt to update tests that did cross-process navigations. I think I missed the fact that this commit was intended to come from the old renderer, rather than the new one.
https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_request.h:58: void SetWaitingForRendererResponse() { On 2014/12/16 19:19:05, clamy wrote: > On 2014/12/16 01:40:26, nasko wrote: > > Do we not expect to have other states being set externally? > > No the other states are set because of the interactions with the IO thread > (beginning the request on the IO thread, and the response being ready to commit > or the request failing). Acknowledged. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... File content/test/test_render_frame_host.h (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... content/test/test_render_frame_host.h:101: void SimulateIOThread(const GURL& url); On 2014/12/16 19:19:05, clamy wrote: > On 2014/12/16 01:40:26, nasko wrote: > > This is a bit too specific of a name for unit tests to call. Can we abstract > the > > name somehow to a higher level concept that happens regardless of the mode of > > operation? I like the above "SendRendererResponse...", which is clear as to > what > > it does and in response to what event. > > Would the name MakeNavigationReadyForCommit be better? PrepareForCommit? Or yours works too. > I could also modify the > function so that it does the logic found in the NavigationControllerTests (ie if > PlzNavigate what this function currently does otherwise simulate the > BeforeUnload ack). Yes, I think I made such a comment in the actual unit tests file, as the pattern was used in a few places.
Thanks! Here is the new patchset. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_controller_impl_unittest.cc:419: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2014/12/16 01:40:26, nasko wrote: > This code pattern appears few times in this file. Why not abstract this out in a > method on the TestRFH? Done. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_controller_impl_unittest.cc:3925: const IPC::Message* message = process()->sink().GetFirstMessageMatching( On 2014/12/16 01:40:26, nasko wrote: > It seems that this branching logic can be put in a couple of methods - one to > get the IPC message and one to get the URL. This will make the body of the test > case a bit more streamlined and easier to read. > Granted, it is only used in one test, so I'd leave it up to you. Done. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... File content/browser/frame_host/navigation_request.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_request.cc:61: state_ = RESPONSE_STARTED; On 2014/12/16 01:40:26, nasko wrote: > DCHECK that the request has started? Done. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_request.cc:67: state_ = FAILED; On 2014/12/16 01:40:26, nasko wrote: > Similarly here, we shouldn't reach this code unless the state is STARTED, right? Done. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/browser/f... content/browser/frame_host/navigation_request.h:30: enum NavigationState { On 2014/12/16 01:40:26, nasko wrote: > On 2014/12/15 17:01:39, clamy wrote: > > I actually plan to use this enum for things other than tests in the future. > Also > > it helps when checking that navigation proceeds correctly. > > Let's put a comment explaining the goal/usage of it and the states. Some are > clear, but some could mean different things, so documenting precisely the > meaning will be very useful. Done. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... File content/test/test_render_frame_host.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... content/test/test_render_frame_host.cc:216: // PlzNavigate: if there is a live renderer, send its response to the On 2014/12/16 01:40:26, nasko wrote: > Is the condition "live renderer"? The check below is about state of the request. Done. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... File content/test/test_render_frame_host.h (right): https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... content/test/test_render_frame_host.h:101: void SimulateIOThread(const GURL& url); On 2014/12/17 00:55:02, nasko wrote: > On 2014/12/16 19:19:05, clamy wrote: > > On 2014/12/16 01:40:26, nasko wrote: > > > This is a bit too specific of a name for unit tests to call. Can we abstract > > the > > > name somehow to a higher level concept that happens regardless of the mode > of > > > operation? I like the above "SendRendererResponse...", which is clear as to > > what > > > it does and in response to what event. > > > > Would the name MakeNavigationReadyForCommit be better? > > PrepareForCommit? Or yours works too. > > > I could also modify the > > function so that it does the logic found in the NavigationControllerTests (ie > if > > PlzNavigate what this function currently does otherwise simulate the > > BeforeUnload ack). > > Yes, I think I made such a comment in the actual unit tests file, as the pattern > was used in a few places. Done. https://chromiumcodereview.appspot.com/761013003/diff/60001/content/test/test... content/test/test_render_frame_host.h:103: On 2014/12/16 01:40:26, nasko wrote: > nit: no need for empty line here, rather above "private:" Done.
This looks awesome! Few comments and I think we will be good to go. https://chromiumcodereview.appspot.com/761013003/diff/80001/content/browser/f... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/80001/content/browser/f... content/browser/frame_host/navigation_controller_impl_unittest.cc:778: main_test_rfh()->SendRendererResponseToNavigation(true, kExistingURL2); Isn't this now PrepareForCommit? https://chromiumcodereview.appspot.com/761013003/diff/80001/content/browser/f... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/761013003/diff/80001/content/browser/f... content/browser/frame_host/navigation_request.h:36: // renderer when the request is created, this stage is skipped. The no live renderer is only for browser-initiated navigations, right? Also, when we optimize out roundtrip because no beforeunload handlers are registered, we will skip it too, right? Just want to make sure I understand it correctly. https://chromiumcodereview.appspot.com/761013003/diff/80001/content/browser/f... content/browser/frame_host/navigation_request.h:72: state_ = WAITING_FOR_RENDERER_RESPONSE; nit: DCHECK we aren't already in this state? https://chromiumcodereview.appspot.com/761013003/diff/80001/content/test/test... File content/test/test_render_frame_host.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/80001/content/test/test... content/test/test_render_frame_host.cc:131: PrepareForCommit(params->url); Do we only need to do this in PlzNavigate and not in other cases? https://chromiumcodereview.appspot.com/761013003/diff/80001/content/test/test... content/test/test_render_frame_host.cc:212: void TestRenderFrameHost::SendRendererResponseToNavigation(bool proceed, What's the difference between this method and PrepareForCommit?
On 2014/12/17 19:55:38, nasko wrote: > This looks awesome! Few comments and I think we will be good to go. Awesome indeed! It will make writing new PlzNavigate tests a bliss. LGTM.
Thanks! https://chromiumcodereview.appspot.com/761013003/diff/80001/content/browser/f... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/80001/content/browser/f... content/browser/frame_host/navigation_controller_impl_unittest.cc:778: main_test_rfh()->SendRendererResponseToNavigation(true, kExistingURL2); On 2014/12/17 19:55:38, nasko wrote: > Isn't this now PrepareForCommit? I think we can use either of them. After SendRendererResponseToNavigation, the request is started. After PrepareForCommit, the request has received the response and is waiting for the renderer to commit it. https://chromiumcodereview.appspot.com/761013003/diff/80001/content/browser/f... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/761013003/diff/80001/content/browser/f... content/browser/frame_host/navigation_request.h:36: // renderer when the request is created, this stage is skipped. On 2014/12/17 19:55:38, nasko wrote: > The no live renderer is only for browser-initiated navigations, right? Also, > when we optimize out roundtrip because no beforeunload handlers are registered, > we will skip it too, right? > Just want to make sure I understand it correctly. This stage only happens in browser initiated navigations. Yes I think we will be able to optimize this step out for cross-site navigations where the old page has no BeforeUnload handler registered. https://chromiumcodereview.appspot.com/761013003/diff/80001/content/browser/f... content/browser/frame_host/navigation_request.h:72: state_ = WAITING_FOR_RENDERER_RESPONSE; On 2014/12/17 19:55:38, nasko wrote: > nit: DCHECK we aren't already in this state? In fact it is even DCHECK that the state is NOT_STARTED. https://chromiumcodereview.appspot.com/761013003/diff/80001/content/test/test... File content/test/test_render_frame_host.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/80001/content/test/test... content/test/test_render_frame_host.cc:131: PrepareForCommit(params->url); On 2014/12/17 19:55:38, nasko wrote: > Do we only need to do this in PlzNavigate and not in other cases? I think so. The tests call SendBeforeUnloadACK explicitely in cross-site navigations cases. However, with PlzNavigate, a browser-initiated same-site navigation needs the call to PrepareForCommit (not the case currently, you don't need the BeforeUnload ack then). https://chromiumcodereview.appspot.com/761013003/diff/80001/content/test/test... content/test/test_render_frame_host.cc:212: void TestRenderFrameHost::SendRendererResponseToNavigation(bool proceed, On 2014/12/17 19:55:38, nasko wrote: > What's the difference between this method and PrepareForCommit? In PlzNavigate, this function gets the NavigationRequest to the state STARTED. The other one gets it to the state RESPONSE_STARTED.
https://chromiumcodereview.appspot.com/761013003/diff/80001/content/test/test... File content/test/test_render_frame_host.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/80001/content/test/test... content/test/test_render_frame_host.cc:131: PrepareForCommit(params->url); On 2014/12/18 13:54:42, clamy wrote: > On 2014/12/17 19:55:38, nasko wrote: > > Do we only need to do this in PlzNavigate and not in other cases? > > I think so. The tests call SendBeforeUnloadACK explicitely in cross-site > navigations cases. However, with PlzNavigate, a browser-initiated same-site > navigation needs the call to PrepareForCommit (not the case currently, you don't > need the BeforeUnload ack then). Why not eliminate all calls to SendBeforeUnloadACK from the tests and replace them with PrepareForCommit, since this is exactly what sending the ack is - preparing the state so we can commit the navigation. With that, all tests will look like LoadURL/Go*, PrepareForCommit, SendNavigate (which we can just rename to Commit or something similar). https://chromiumcodereview.appspot.com/761013003/diff/80001/content/test/test... content/test/test_render_frame_host.cc:212: void TestRenderFrameHost::SendRendererResponseToNavigation(bool proceed, On 2014/12/18 13:54:42, clamy wrote: > On 2014/12/17 19:55:38, nasko wrote: > > What's the difference between this method and PrepareForCommit? > > In PlzNavigate, this function gets the NavigationRequest to the state STARTED. > The other one gets it to the state RESPONSE_STARTED. I think the only call to SendRendererResponseToNavigation in the unittests will be just fine with calling PrepareForCommit, won't it?
Thanks! Here is the new version, which should be simpler. https://chromiumcodereview.appspot.com/761013003/diff/80001/content/test/test... File content/test/test_render_frame_host.cc (right): https://chromiumcodereview.appspot.com/761013003/diff/80001/content/test/test... content/test/test_render_frame_host.cc:131: PrepareForCommit(params->url); On 2014/12/18 15:53:34, nasko wrote: > On 2014/12/18 13:54:42, clamy wrote: > > On 2014/12/17 19:55:38, nasko wrote: > > > Do we only need to do this in PlzNavigate and not in other cases? > > > > I think so. The tests call SendBeforeUnloadACK explicitely in cross-site > > navigations cases. However, with PlzNavigate, a browser-initiated same-site > > navigation needs the call to PrepareForCommit (not the case currently, you > don't > > need the BeforeUnload ack then). > > Why not eliminate all calls to SendBeforeUnloadACK from the tests and replace > them with PrepareForCommit, since this is exactly what sending the ack is - > preparing the state so we can commit the navigation. > With that, all tests will look like LoadURL/Go*, PrepareForCommit, SendNavigate > (which we can just rename to Commit or something similar). Done. https://chromiumcodereview.appspot.com/761013003/diff/80001/content/test/test... content/test/test_render_frame_host.cc:212: void TestRenderFrameHost::SendRendererResponseToNavigation(bool proceed, On 2014/12/18 15:53:34, nasko wrote: > On 2014/12/18 13:54:42, clamy wrote: > > On 2014/12/17 19:55:38, nasko wrote: > > > What's the difference between this method and PrepareForCommit? > > > > In PlzNavigate, this function gets the NavigationRequest to the state STARTED. > > The other one gets it to the state RESPONSE_STARTED. > > I think the only call to SendRendererResponseToNavigation in the unittests will > be just fine with calling PrepareForCommit, won't it? Done.
Awesome! LGTM
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/761013003/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7c5016ccbd307a4e2a2ca140cc0739f29f288bb0 Cr-Commit-Position: refs/heads/master@{#309207} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
