Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
953503002:
PlzNavigate: test updates post beforeUnload IPC refactor. (Closed)
|
Created:
5 years, 10 months ago by carlosk Modified:
5 years, 9 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: test updates post beforeUnload IPC refactor.
This CL adapts several test classes with changes due now that
the IPCS for beforeUnload ack and renderer navigation requests are not
one and the same anymore.
BUG=440266
Committed: https://crrev.com/d6a4ef36ace903fd35bbe2e6ef2f87f690fb2bc9
Cr-Commit-Position: refs/heads/master@{#319049}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Move some methods around. #
Total comments: 10
Patch Set 3 : Rebase #Patch Set 4 : Minor changes from CR comments. #
Total comments: 9
Patch Set 5 : Minor changes from last round of comments. #
Total comments: 13
Patch Set 6 : Comment changes. #
Total comments: 3
Patch Set 7 : Addressed CR comments. #
Total comments: 1
Patch Set 8 : Rebase with small adaptation to support data URLs. #
Total comments: 2
Patch Set 9 : Reverted RFHM logic for new SiteInstance creation (moved to another CL). #Patch Set 10 : Minor comment fix. #
Total comments: 2
Patch Set 11 : Minor comment updates. #Messages
Total messages: 33 (8 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. This is a split off of http://crrev.com/946543003 that contains only the changes related to updating test code now that the IPCs for beforeUnload ACK and renderer-initated navigation requests are different. I repeated a few code comments with explanations from the other CL for consistency. https://codereview.chromium.org/953503002/diff/1/content/browser/frame_host/n... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/953503002/diff/1/content/browser/frame_host/n... content/browser/frame_host/navigation_controller_impl_unittest.cc:2630: controller.LoadURL(url2, Referrer(), ui::PAGE_TRANSITION_TYPED, Replaced this at it seemed the test was not using the intended URL. https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... content/test/test_render_frame_host.cc:242: CHECK(url_loader); Changed to CHECKs as ASSERTs will not trigger test failures when not at test-level frames.
Thanks! A few comments + suggestion. https://codereview.chromium.org/953503002/diff/1/content/browser/frame_host/n... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/953503002/diff/1/content/browser/frame_host/n... content/browser/frame_host/navigation_controller_impl_unittest.cc:250: void SimulateServerRedirect(TestRenderFrameHost* test_rfh, Now that I look at it, this code is very similar to TestRenderFrameHost::PrepareForCommit. So how about removing this helper function from NavigationControllerTest and have a PrepareForCommitWithServerRedirect method in TestRenderFrameHost? They could both use the same internal method that would check if we need a redirect, and if so do it. https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... content/test/test_render_frame_host.cc:229: // We may not have simulated the renderer response to the navigation request. nit: I think this should be rephrased as something like: We may be waiting for a beforeUnload ACK from the renderer. Simulate it now. (which is a better description of what we are doing). https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... content/test/test_render_frame_host.cc:242: CHECK(url_loader); On 2015/02/23 16:42:03, carlosk wrote: > Changed to CHECKs as ASSERTs will not trigger test failures when not at > test-level frames. Acknowledged. https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... content/test/test_render_frame_host.h:89: // PlzNavigate nit: move the comment above SendBeginNavigationWithURL and add an empty line before it. https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... content/test/test_render_frame_host.h:90: void SendRendererInitiatedNavigationRequest(const GURL& url, I think there should be a comment explaining what this function is doing. Something like: // Simulates the start of a renderer-initiated navigation. Currently this is a no-op in the browser. PlzNavigate: this simulates receiving a BeginNavigation IPC.
Patchset #2 (id:20001) has been deleted
Thanks. Beyond what's in the comments I also merged TestRenderFrameHost::SendRendererInitiatedNavigationRequest with TRFH::SendBeginNavigationWithURL as they were doing practically the same thing. Note: the diff against PS1 is all screwed up for part of test_render_frame_host.cc. You'll be better off looking at the regular diff. https://codereview.chromium.org/953503002/diff/1/content/browser/frame_host/n... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/953503002/diff/1/content/browser/frame_host/n... content/browser/frame_host/navigation_controller_impl_unittest.cc:250: void SimulateServerRedirect(TestRenderFrameHost* test_rfh, On 2015/02/24 13:05:33, clamy wrote: > Now that I look at it, this code is very similar to > TestRenderFrameHost::PrepareForCommit. So how about removing this helper > function from NavigationControllerTest and have a > PrepareForCommitWithServerRedirect method in TestRenderFrameHost? They could > both use the same internal method that would check if we need a redirect, and if > so do it. Done. This does looks better. https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... content/test/test_render_frame_host.cc:229: // We may not have simulated the renderer response to the navigation request. On 2015/02/24 13:05:34, clamy wrote: > nit: I think this should be rephrased as something like: > We may be waiting for a beforeUnload ACK from the renderer. Simulate it now. > > (which is a better description of what we are doing). Updated the comment. https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... content/test/test_render_frame_host.h:89: // PlzNavigate On 2015/02/24 13:05:34, clamy wrote: > nit: move the comment above SendBeginNavigationWithURL and add an empty line > before it. In fact I merged these two methods as they were practically the same anyway. And updated its comment. https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... content/test/test_render_frame_host.h:90: void SendRendererInitiatedNavigationRequest(const GURL& url, On 2015/02/24 13:05:34, clamy wrote: > I think there should be a comment explaining what this function is doing. > Something like: > // Simulates the start of a renderer-initiated navigation. Currently this is a > no-op in the browser. > PlzNavigate: this simulates receiving a BeginNavigation IPC. Done.
Thanks! It's nearly good. Also don't forget to fix the compilation error. https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... content/test/test_render_frame_host.h:89: // PlzNavigate On 2015/02/24 16:38:56, carlosk wrote: > On 2015/02/24 13:05:34, clamy wrote: > > nit: move the comment above SendBeginNavigationWithURL and add an empty line > > before it. > > In fact I merged these two methods as they were practically the same anyway. And > updated its comment. I think the merging is good but would prefer if the name was SendRendererInitiatedNavigationRequest instead of SendBeginNavigationWithURL. The second one implies that the only thing this method does is simulating a BeginNavigation IPC while it only does so if PlzNavigate is enabled. I also find that the other method name makes the NavigationController tests clearer. Not to mention that I'm not very keen on having a PlzNavigate IPC called in the middle of non PlzNavigate specific tests. https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... content/test/test_render_frame_host.cc:241: // Simulate a beforeUnload ACK from the renderer if it's being expected. nit: s/it's being expected/the browser is waiting for it. https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... content/test/test_render_frame_host.cc:248: CHECK(!redirect_url) << "Unable to simulate server redirect to \"" << I noticed you like to add log to the CHECKs. This is not usually done in the code though I cannot find an explicit thing against it in the style guide. However it seems we are removing all stream arguments when building an official release build (https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&sq=...). So maybe we should avoid it here and just add a comment explaining why the CHECK is there. Should it fail people will look at the code anyway and see the comment. https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... content/test/test_render_frame_host.h:89: // Normally this is a no-op. nit: I think you could add a sentence about the intention behind this method as I find that starting with this first sentence is a bit weird. https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... content/test/test_render_frame_host.h:90: // PlzNavigate: this method simulates receiving a navigation request IPC. s/navigation request/BeginNavigation
Thanks. https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/953503002/diff/1/content/test/test_render_fra... content/test/test_render_frame_host.h:89: // PlzNavigate On 2015/02/25 14:29:49, clamy wrote: > On 2015/02/24 16:38:56, carlosk wrote: > > On 2015/02/24 13:05:34, clamy wrote: > > > nit: move the comment above SendBeginNavigationWithURL and add an empty line > > > before it. > > > > In fact I merged these two methods as they were practically the same anyway. > And > > updated its comment. > > I think the merging is good but would prefer if the name was > SendRendererInitiatedNavigationRequest instead of SendBeginNavigationWithURL. > The second one implies that the only thing this method does is simulating a > BeginNavigation IPC while it only does so if PlzNavigate is enabled. I also find > that the other method name makes the NavigationController tests clearer. Not to > mention that I'm not very keen on having a PlzNavigate IPC called in the middle > of non PlzNavigate specific tests. Done. https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... content/test/test_render_frame_host.cc:241: // Simulate a beforeUnload ACK from the renderer if it's being expected. On 2015/02/25 14:29:49, clamy wrote: > nit: s/it's being expected/the browser is waiting for it. Done. https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... content/test/test_render_frame_host.cc:248: CHECK(!redirect_url) << "Unable to simulate server redirect to \"" << On 2015/02/25 14:29:49, clamy wrote: > I noticed you like to add log to the CHECKs. This is not usually done in the > code though I cannot find an explicit thing against it in the style guide. > However it seems we are removing all stream arguments when building an official > release build > (https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&sq=...). > So maybe we should avoid it here and just add a comment explaining why the CHECK > is there. Should it fail people will look at the code anyway and see the > comment. I think this still makes it easier when running a debug build as the message will appear in the log. But I agree that the person will look at the code anyway and see the comment, so... Done. https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... content/test/test_render_frame_host.h:89: // Normally this is a no-op. On 2015/02/25 14:29:49, clamy wrote: > nit: I think you could add a sentence about the intention behind this method as > I find that starting with this first sentence is a bit weird. I followed what was done in other similarly commented methods where we initially explain what it normally does and then after a PlzNavigate prefix explain what happens when browser side navigation is enabled. I improved it a little but I don't know how to change it further without being redundant with the PlzNavigate part. https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... content/test/test_render_frame_host.h:90: // PlzNavigate: this method simulates receiving a navigation request IPC. On 2015/02/25 14:29:49, clamy wrote: > s/navigation request/BeginNavigation Done.
Thanks! After further thinking I believe the url in the interstitial test should not be changed. https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... content/test/test_render_frame_host.cc:248: CHECK(!redirect_url) << "Unable to simulate server redirect to \"" << On 2015/02/25 16:08:00, carlosk wrote: > On 2015/02/25 14:29:49, clamy wrote: > > I noticed you like to add log to the CHECKs. This is not usually done in the > > code though I cannot find an explicit thing against it in the style guide. > > However it seems we are removing all stream arguments when building an > official > > release build > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&sq=...). > > So maybe we should avoid it here and just add a comment explaining why the > CHECK > > is there. Should it fail people will look at the code anyway and see the > > comment. > > I think this still makes it easier when running a debug build as the message > will appear in the log. But I agree that the person will look at the code anyway > and see the comment, so... Done. I would be fine if this was a DCHECK, but it is a CHECK therefore present in Release builds as well. https://codereview.chromium.org/953503002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/953503002/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:38: #include "content/test/test_navigation_url_loader.h" Remove this include which is now unnecessary. https://codereview.chromium.org/953503002/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:2601: controller.LoadURL(url2, Referrer(), ui::PAGE_TRANSITION_TYPED, After further thinking I think we should avoid doing this change here. It appears that this test has been here since 2008 (https://codereview.chromium.org/479) so I think that it is doing what is expected. My understanding is that we navigate to url1 and find that it is an interstitial, then the user proceed with it and we end up in url2. https://codereview.chromium.org/953503002/diff/80001/content/test/test_render... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/80001/content/test/test_render... content/test/test_render_frame_host.cc:263: // used and it was an error from the test author to a ask for a redirect. I would rephrase as something like: "If a server redirect was requested, the response to the navigation request should not have started." https://codereview.chromium.org/953503002/diff/80001/content/test/test_render... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/953503002/diff/80001/content/test/test_render... content/test/test_render_frame_host.h:89: // With the current navigation this method is a no-op. nit: "current navigation logic"
carlosk@chromium.org changed reviewers: + nasko@chromium.org
carlosk@chromium.org changed required reviewers: + nasko@chromium.org
Thanks for yet another round of comments! nasko@: PTAL. As this last round requested only minor changes I'm already adding nasko@ as a reviewer. https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/40001/content/test/test_render... content/test/test_render_frame_host.cc:248: CHECK(!redirect_url) << "Unable to simulate server redirect to \"" << On 2015/02/25 17:10:33, clamy wrote: > On 2015/02/25 16:08:00, carlosk wrote: > > On 2015/02/25 14:29:49, clamy wrote: > > > I noticed you like to add log to the CHECKs. This is not usually done in the > > > code though I cannot find an explicit thing against it in the style guide. > > > However it seems we are removing all stream arguments when building an > > official > > > release build > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&sq=...). > > > So maybe we should avoid it here and just add a comment explaining why the > > CHECK > > > is there. Should it fail people will look at the code anyway and see the > > > comment. > > > > I think this still makes it easier when running a debug build as the message > > will appear in the log. But I agree that the person will look at the code > anyway > > and see the comment, so... Done. > > I would be fine if this was a DCHECK, but it is a CHECK therefore present in > Release builds as well. Acknowledged. https://codereview.chromium.org/953503002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/953503002/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:38: #include "content/test/test_navigation_url_loader.h" On 2015/02/25 17:10:33, clamy wrote: > Remove this include which is now unnecessary. Done. https://codereview.chromium.org/953503002/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:2601: controller.LoadURL(url2, Referrer(), ui::PAGE_TRANSITION_TYPED, On 2015/02/25 17:10:33, clamy wrote: > After further thinking I think we should avoid doing this change here. It > appears that this test has been here since 2008 > (https://codereview.chromium.org/479) so I think that it is doing what is > expected. My understanding is that we navigate to url1 and find that it is an > interstitial, then the user proceed with it and we end up in url2. That doesn't seem to be the case. I read the description of what an interstitial page is and did a quick test in Chrome using a domain that has an un-trusted certificate: * I initially navigate to google.com (equivalent to |url1| above) * Then I type in the address of the site with the invalid certificate, say https://foo.org (equivalent to |url2|) * The navigation pauses and the interstitial page is presented *with https://foo.org shown in the omnibar* So the navigation must happen to the un-trusted page for the interstitial to be presented. The reason why this has been untouched since 2008 is most likely because this URL doesn't make any difference for this test success or failure. This makes me thing we should even add some EXPECT(s) here but I'm not sure of what to check. https://codereview.chromium.org/953503002/diff/80001/content/test/test_render... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/80001/content/test/test_render... content/test/test_render_frame_host.cc:263: // used and it was an error from the test author to a ask for a redirect. On 2015/02/25 17:10:33, clamy wrote: > I would rephrase as something like: > "If a server redirect was requested, the response to the navigation request > should not have started." Done. https://codereview.chromium.org/953503002/diff/80001/content/test/test_render... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/953503002/diff/80001/content/test/test_render... content/test/test_render_frame_host.h:89: // With the current navigation this method is a no-op. On 2015/02/25 17:10:33, clamy wrote: > nit: "current navigation logic" Done.
A bunch of mostly minor comments. The main issue we need to address is where CallOnResponseStarted belongs. https://codereview.chromium.org/953503002/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/953503002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:1241: main_test_rfh()->PrepareForCommitWithServerRedirect(url2); Why is this with server redirect? The previous code doesn't seem to have one. https://codereview.chromium.org/953503002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:1520: main_test_rfh()->PrepareForCommitWithServerRedirect(url3); Seeing a few of those makes me think even more that the CallOnResponseStarted doesn't belong in PrepareForCommit, but in a Commit method, which SendNavigation is the current approximation of. https://codereview.chromium.org/953503002/diff/100001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/100001/content/test/test_rende... content/test/test_render_frame_host.cc:259: // We have already simulated the IO thread commit. Only the This comment is confusing. Commit doesn't happen until we get CallOnResponseStarted, which is at the end of this method, yet the comment implies that it must have happened. Shouldn't it be conditional? https://codereview.chromium.org/953503002/diff/100001/content/test/test_rende... content/test/test_render_frame_host.cc:280: url_loader->CallOnResponseStarted(response, MakeEmptyStream()); This technically will commit, right? Does it belong here? https://codereview.chromium.org/953503002/diff/100001/content/test/test_rende... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/953503002/diff/100001/content/test/test_rende... content/test/test_render_frame_host.h:109: void PrepareForCommit(); Love that we no longer have the parameter! https://codereview.chromium.org/953503002/diff/100001/content/test/test_rende... content/test/test_render_frame_host.h:113: // also simulate a server redirect IPC to |redirect_url|. nit: Server redirect is not an IPC. https://codereview.chromium.org/953503002/diff/100001/content/test/test_rende... content/test/test_render_frame_host.h:124: // PlzNavigate: If |redirect_url| is not nullptr, it will be used to simulate Why not use const GURL& as the parameter and empty GURL to indicate no redirects are needed.
Thanks. To more clearly introduce the situation here I copied below a (slightly corrected) comment I wrote in the original CL (http://crrev.com/946543003): On 2015/02/20 19:16:51, carlosk wrote: > For each current call to SendNavigate its intent might be different: > * A reply from the renderer carrying on with a browser-initiated navigation > * Same as above but after one or more server redirects happened > * A renderer-initiated user-initiated navigation > * A renderer-initiated non-user-initiated navigation > > As each of those cases are handled differently in PlzNavigate world I did my > best to interpret each situation. I made comments on a few cases but please > let me know if you notice something I missed. You can see the "comments on a few cases" here: https://codereview.chromium.org/946543003/diff/20001/content/browser/frame_ho... https://codereview.chromium.org/953503002/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/953503002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:1241: main_test_rfh()->PrepareForCommitWithServerRedirect(url2); On 2015/02/26 01:27:42, nasko wrote: > Why is this with server redirect? The previous code doesn't seem to have one. Also from the original CL, this was my comment for this part: On 2015/02/20 19:16:51, carlosk wrote: > I guess that "a reload navigation produces a new page" means that the user did > hit Reload but the server redirected it to a new page. To justify this note that the browser starts a reload of |url1| but the renderer commits to |url2|. https://codereview.chromium.org/953503002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:1520: main_test_rfh()->PrepareForCommitWithServerRedirect(url3); On 2015/02/26 01:27:42, nasko wrote: > Seeing a few of those makes me think even more that the CallOnResponseStarted > doesn't belong in PrepareForCommit, but in a Commit method, which SendNavigation > is the current approximation of. If I understand correctly this would not work well for tests. CallOnResponseStarted simulates the response from the IO thread that the navigation has committed. This updates the states of browser side components and finally sends a FrameMsg_CommitNavigation IPC to the renderer. While SendNavigate simulates the followup response from the renderer, the FrameHostMsg_DidStartProvisionalLoadForFrame IPC, that triggers the OnDidStartProvisionalLoadForFrame chain of calls. I see value in having these calls being as they are so that we can properly test the state of browser side components in between those calls (like we do in navigator_impl_unittest.cc). That would be impossible if we made the change you suggested. Note: Now looking at the TestRenderFrameHost::SendNavigate* calls, those are damn confusing! Depending on which of them is called, the result calls can be very different... https://codereview.chromium.org/953503002/diff/100001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/100001/content/test/test_rende... content/test/test_render_frame_host.cc:259: // We have already simulated the IO thread commit. Only the On 2015/02/26 01:27:42, nasko wrote: > This comment is confusing. Commit doesn't happen until we get > CallOnResponseStarted, which is at the end of this method, yet the comment > implies that it must have happened. Shouldn't it be conditional? Yes, this was/is in the wrong place. I'm moving it inside the if-block. https://codereview.chromium.org/953503002/diff/100001/content/test/test_rende... content/test/test_render_frame_host.cc:280: url_loader->CallOnResponseStarted(response, MakeEmptyStream()); On 2015/02/26 01:27:42, nasko wrote: > This technically will commit, right? Does it belong here? Yes, see the reply to your comment in navigation_controller_impl_unittest.cc. https://codereview.chromium.org/953503002/diff/100001/content/test/test_rende... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/953503002/diff/100001/content/test/test_rende... content/test/test_render_frame_host.h:113: // also simulate a server redirect IPC to |redirect_url|. On 2015/02/26 01:27:42, nasko wrote: > nit: Server redirect is not an IPC. Indeed. Updated the comment. https://codereview.chromium.org/953503002/diff/100001/content/test/test_rende... content/test/test_render_frame_host.h:124: // PlzNavigate: If |redirect_url| is not nullptr, it will be used to simulate On 2015/02/26 01:27:42, nasko wrote: > Why not use const GURL& as the parameter and empty GURL to indicate no redirects > are needed. I am playing safe. I was afraid an empty GURL could be actually used for some weird reason. I also think a nullptr conveys a very clear message of the intent to ignore the redirect step.
Little addition to a topic we'd like to have your feedback on nasko@. https://codereview.chromium.org/953503002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/953503002/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl_unittest.cc:2601: controller.LoadURL(url2, Referrer(), ui::PAGE_TRANSITION_TYPED, On 2015/02/25 19:30:21, carlosk wrote: > On 2015/02/25 17:10:33, clamy wrote: > > After further thinking I think we should avoid doing this change here. It > > appears that this test has been here since 2008 > > (https://codereview.chromium.org/479) so I think that it is doing what is > > expected. My understanding is that we navigate to url1 and find that it is an > > interstitial, then the user proceed with it and we end up in url2. > > That doesn't seem to be the case. I read the description of what an interstitial > page is and did a quick test in Chrome using a domain that has an un-trusted > certificate: > * I initially navigate to http://google.com (equivalent to |url1| above) > * Then I type in the address of the site with the invalid certificate, say > https://foo.org (equivalent to |url2|) > * The navigation pauses and the interstitial page is presented *with > https://foo.org shown in the omnibar* > > So the navigation must happen to the un-trusted page for the interstitial to be > presented. > > The reason why this has been untouched since 2008 is most likely because this > URL doesn't make any difference for this test success or failure. This makes me > thing we should even add some EXPECT(s) here but I'm not sure of what to check. I forgot to add the link about interstitial pages I mentioned: http://www.chromium.org/developers/design-documents/safebrowsing#TOC-Safe-Bro...
Thanks! After seeing Nasko's comments, just one more on my part. https://codereview.chromium.org/953503002/diff/120001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/120001/content/test/test_rende... content/test/test_render_frame_host.cc:259: if (request->state() == NavigationRequest::RESPONSE_STARTED) { All considered, I think this if clause should be removed entirely. The only case I can see where we would end there is when the patch for data urls lands, and that is because no network request are made for data urls, not because the IO thread commit has already been simulated. It makes more sense to add it to that patch if needed.
On 2015/02/26 13:17:30, clamy wrote: > Thanks! After seeing Nasko's comments, just one more on my part. > > https://codereview.chromium.org/953503002/diff/120001/content/test/test_rende... > File content/test/test_render_frame_host.cc (right): > > https://codereview.chromium.org/953503002/diff/120001/content/test/test_rende... > content/test/test_render_frame_host.cc:259: if (request->state() == > NavigationRequest::RESPONSE_STARTED) { > All considered, I think this if clause should be removed entirely. The only case > I can see where we would end there is when the patch for data urls lands, and > that is because no network request are made for data urls, not because the IO > thread commit has already been simulated. It makes more sense to add it to that > patch if needed. Just an update: this comment by clamy@ was spot on. By removing this benevolent if-block 4 tests began crashing. I already fixed two of them, both due to lingering PlzNavigate issues, one regarding the test itself and the other related to the actual implementation. I still have to deal with the other two so the next PatchSet will come only next week.
As I mentioned before clamy@'s suggestion to remove that if-block from TestRenderFrameHost::PrepareForCommitWithServerRedirect caused four tests to fail: * NavigationControllerTest.ClearHistoryList * WebContentsImplTest.ActiveContentsCountChangeBrowsingInstance * WebContentsImplTest.ActiveContentsCountNavigate * WebContentsImplTest.CrossSiteBoundaries All but WebContentsImplTest.ActiveContentsCountChangeBrowsingInstance required just corrections to the tests themselves. That one though presented a situation where we were creating more SiteInstances per navigation than the current system. It doesn't seem like it would be a real-life problem as they were not incorrect per se. But as this was actually detected by the test and is arguably wasteful I updated the way new SiteInstances are created by RenderFrameHostManager, now routing them through the newly added CreateSiteInstanceForURL method. https://codereview.chromium.org/953503002/diff/120001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/120001/content/test/test_rende... content/test/test_render_frame_host.cc:259: if (request->state() == NavigationRequest::RESPONSE_STARTED) { On 2015/02/26 13:17:29, clamy wrote: > All considered, I think this if clause should be removed entirely. The only case > I can see where we would end there is when the patch for data urls lands, and > that is because no network request are made for data urls, not because the IO > thread commit has already been simulated. It makes more sense to add it to that > patch if needed. Done. https://codereview.chromium.org/953503002/diff/120001/content/test/test_rende... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/953503002/diff/120001/content/test/test_rende... content/test/test_render_frame_host.h:126: void PrepareForCommitInternal(const GURL* redirect_url); Eliminated this method in favor of calling PrepareForCommitWithServerRedirect with an empty URL. https://codereview.chromium.org/953503002/diff/140001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/140001/content/test/test_rende... content/test/test_render_frame_host.cc:260: // fully commit the navigation at this call to CallOnResponseStarted. Added comment as per nasko@'s request.
Thanks! Nice catch on the SiteInstance duplication problem. However I think it would be better to make a separate patch for it. You will need to rebase your patch on top of the data url patch that landed (and uses SendBeginNavigation in one of its unit tests).
Patchset #8 (id:160001) has been deleted
On 2015/03/02 16:53:53, clamy wrote: > Thanks! Nice catch on the SiteInstance duplication problem. However I think it > would be better to make a separate patch for it. You will need to rebase your > patch on top of the data url patch that landed (and uses SendBeginNavigation in > one of its unit tests). Done: https://codereview.chromium.org/967383002/ . I will remove the changes from this CL in the next patch set.
On 2015/03/02 18:38:18, carlosk wrote: > On 2015/03/02 16:53:53, clamy wrote: > > Thanks! Nice catch on the SiteInstance duplication problem. However I think it > > would be better to make a separate patch for it. You will need to rebase your > > patch on top of the data url patch that landed (and uses SendBeginNavigation > in > > one of its unit tests). > > Done: https://codereview.chromium.org/967383002/ . > I will remove the changes from this CL in the next patch set. Is it possible to rewrite the test to avoid this? If it cannot happen in "real life", then adding complexity to the live codepaths just for the sake of making a test pass is the wrong approach.
Thanks! Once you upload the patch set without the SiteInstance creation part it should be good. https://codereview.chromium.org/953503002/diff/180001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/180001/content/test/test_rende... content/test/test_render_frame_host.cc:247: // If a network request is not needed for this URL, Please finish this comment :).
On 2015/03/02 23:36:53, nasko wrote: > On 2015/03/02 18:38:18, carlosk wrote: > > On 2015/03/02 16:53:53, clamy wrote: > > > Thanks! Nice catch on the SiteInstance duplication problem. However I think > it > > > would be better to make a separate patch for it. You will need to rebase > your > > > patch on top of the data url patch that landed (and uses SendBeginNavigation > > in > > > one of its unit tests). > > > > Done: https://codereview.chromium.org/967383002/ . > > I will remove the changes from this CL in the next patch set. > > Is it possible to rewrite the test to avoid this? If it cannot happen in "real > life", then adding complexity to the live codepaths just for the sake of making > a test pass is the wrong approach. I'm pretty sure it is possible but mind that: * This test only fails if PlzNavigate is enabled. * I am assuming that the result form that change -- not creating extra SiteInstances when not needed, keeping the current behavior -- is desired. So I don't think we really need to worry about this test in this CL as it will be fixed, one way or the other, in the other one. I uploaded the new version of this CL without that change to RFHM.
Lgtm once you fix the comment issue (see previous patch set).
Thanks. https://codereview.chromium.org/953503002/diff/180001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/953503002/diff/180001/content/test/test_rende... content/test/test_render_frame_host.cc:247: // If a network request is not needed for this URL, On 2015/03/03 12:47:23, clamy wrote: > Please finish this comment :). Done.
LGTM with a note that we need follow up work. https://codereview.chromium.org/953503002/diff/220001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/953503002/diff/220001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:4433: switches::kEnableBrowserSideNavigation)) { I would've expected this CL to remove those conditional checks and always call PrepareForCommit without caring about BeforeUnload acks at all. As discussed over chat, let's look into how to improve those in a separate CL, as this code seems suboptimal.
Thanks. https://codereview.chromium.org/953503002/diff/220001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/953503002/diff/220001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl_unittest.cc:4433: switches::kEnableBrowserSideNavigation)) { On 2015/03/03 16:38:34, nasko wrote: > I would've expected this CL to remove those conditional checks and always call > PrepareForCommit without caring about BeforeUnload acks at all. As discussed > over chat, let's look into how to improve those in a separate CL, as this code > seems suboptimal. Acknowledged. The fix for that would add a lot more changes to this CL possibly going over non-content files. A new PlzNavigate issue was created to track this: http://crbug.com/463525.
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/953503002/#ps240001 (title: "Minor comment updates.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/953503002/240001
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/d6a4ef36ace903fd35bbe2e6ef2f87f690fb2bc9 Cr-Commit-Position: refs/heads/master@{#319049} |
