Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1800123002:
PlzNavigate: fix several failing NavigationControllerTests (Closed)
|
Created:
4 years, 9 months ago by clamy Modified:
4 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, carlosk Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: fix several failing NavigationControllerTests
https://codereview.chromium.org/1777903003 introduced several calls to
TestRenderFrameHost::PrepareForCommit without calling
TestRenderFrameHost::SendRendererInitiatedNavigationRequest when simulating
renderer initiated navigations. This makes them fail with PlzNavigate enabled.
This CL fixes this fact.
BUG=439423
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/7b0202139ab83ec27b73ac1ddddcacf77d5e2de6
Cr-Commit-Position: refs/heads/master@{#381726}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase + addressed comments #
Messages
Total messages: 17 (9 generated)
Description was changed from ========== PlzNavigate: fix several failing NavigationControllerTests https://codereview.chromium.org/1777903003 introduced several calls to TestRenderFrameHost::PrepareForCommit without calling TestRenderFrameHost::SendRendererInitiatedNavigationRequest when simulating renderer initiated navigations. This makes them fail with PlzNavigate enabled. This CL fixes this fact. BUG=439423 ========== to ========== PlzNavigate: fix several failing NavigationControllerTests https://codereview.chromium.org/1777903003 introduced several calls to TestRenderFrameHost::PrepareForCommit without calling TestRenderFrameHost::SendRendererInitiatedNavigationRequest when simulating renderer initiated navigations. This makes them fail with PlzNavigate enabled. This CL fixes this fact. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNavigate: fix several failing NavigationControllerTests https://codereview.chromium.org/1777903003 introduced several calls to TestRenderFrameHost::PrepareForCommit without calling TestRenderFrameHost::SendRendererInitiatedNavigationRequest when simulating renderer initiated navigations. This makes them fail with PlzNavigate enabled. This CL fixes this fact. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: fix several failing NavigationControllerTests https://codereview.chromium.org/1777903003 introduced several calls to TestRenderFrameHost::PrepareForCommit without calling TestRenderFrameHost::SendRendererInitiatedNavigationRequest when simulating renderer initiated navigations. This makes them fail with PlzNavigate enabled. This CL fixes this fact. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
@nasko:PTAL @creis: FYI https://codereview.chromium.org/1800123002/diff/1/testing/buildbot/filters/br... File testing/buildbot/filters/browser-side-navigation.linux.content_unittests.filter (right): https://codereview.chromium.org/1800123002/diff/1/testing/buildbot/filters/br... testing/buildbot/filters/browser-side-navigation.linux.content_unittests.filter:1: -NavigationControllerTest.BackSubframe This test is failing on PlzNavigate because of an issue with the test itself. The first call to GoBack does not actually trigger a load in subframe as the test seems to expect but in the main frame instead. I verified that this is also true in site-per-process mode.
Sorry about that! I was trying to improve these tests for PlzNavigate rather than make them regress, and I'm sure that I ran them with --enable-browser-side-navigation at one point to confirm. Must have been an earlier patchset, or maybe I had a typo in the flag name. I'll mention that it's not clear from the documentation of these methods when they need to be called. Maybe their comments can be updated to mention calling them together? (Also, would the tests have passed in PlzNavigate if I hadn't called either of them at all?) Anyway, LGTM with nits, though I'm concerned about csharrison's DontDiscardWrongPendingEntry test failing. Is that also related to my CL? https://codereview.chromium.org/1800123002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/1800123002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_unittest.cc:2393: subframe->SendRendererInitiatedNavigationRequest(url2, false); Why are some of these before the params declaration (above) and others just before PrepareForCommit (here and below)? Let's be consistent if we can, and it might be nice to have them all together like this. https://codereview.chromium.org/1800123002/diff/1/testing/buildbot/filters/br... File testing/buildbot/filters/browser-side-navigation.linux.content_unittests.filter (right): https://codereview.chromium.org/1800123002/diff/1/testing/buildbot/filters/br... testing/buildbot/filters/browser-side-navigation.linux.content_unittests.filter:1: -NavigationControllerTest.BackSubframe On 2016/03/15 15:31:30, clamy wrote: > This test is failing on PlzNavigate because of an issue with the test itself. > The first call to GoBack does not actually trigger a load in subframe as the > test seems to expect but in the main frame instead. I verified that this is also > true in site-per-process mode. Oh, that might be https://crbug.com/586324. I'm glad you caught that in the test-- it'll make it easy to update the test to make it fail in --site-per-process. I'll try to get that one fixed soon, now that Lukasz has replicated unique name. https://codereview.chromium.org/1800123002/diff/1/testing/buildbot/filters/br... testing/buildbot/filters/browser-side-navigation.linux.content_unittests.filter:2: -NavigationControllerTest.DontDiscardWrongPendingEntry Why is this one failing?
Thanks! I reckon that the method description could be improved. Without the methods, the tests would pass in PlzNavigate, since we would assume on receiving DidCommitProvisionalLoad that it was a synchronous navigation in the renderer. This would be wrong, so it's better to call the methods to properly simulate what's happening. As explained in the comment, even without your CL I don't think that Charles' test woudl work with PlzNavigate. In PlzNavigate, it's not possible for the browser to have a mismatched pending entry when it knows about the navigation failure (this is because we kill all previosu navigation when we make a new one, so the old one cannot reset the entry anymore). https://codereview.chromium.org/1800123002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl_unittest.cc (right): https://codereview.chromium.org/1800123002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl_unittest.cc:2393: subframe->SendRendererInitiatedNavigationRequest(url2, false); On 2016/03/16 17:08:41, Charlie Reis wrote: > Why are some of these before the params declaration (above) and others just > before PrepareForCommit (here and below)? Let's be consistent if we can, and it > might be nice to have them all together like this. Done. https://codereview.chromium.org/1800123002/diff/1/testing/buildbot/filters/br... File testing/buildbot/filters/browser-side-navigation.linux.content_unittests.filter (right): https://codereview.chromium.org/1800123002/diff/1/testing/buildbot/filters/br... testing/buildbot/filters/browser-side-navigation.linux.content_unittests.filter:2: -NavigationControllerTest.DontDiscardWrongPendingEntry On 2016/03/16 17:08:41, Charlie Reis wrote: > Why is this one failing? Charles' test is simulating an interleaving of events that can't happen in PlzNavigate as it is. I need to take a closer look as to whether it can happen at all, but I suspect this is not the case. This is due to us discarding the entry at a different point in time.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/1800123002/#ps20001 (title: "Rebase + addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1800123002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800123002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
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/1800123002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1800123002/20001
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: fix several failing NavigationControllerTests https://codereview.chromium.org/1777903003 introduced several calls to TestRenderFrameHost::PrepareForCommit without calling TestRenderFrameHost::SendRendererInitiatedNavigationRequest when simulating renderer initiated navigations. This makes them fail with PlzNavigate enabled. This CL fixes this fact. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: fix several failing NavigationControllerTests https://codereview.chromium.org/1777903003 introduced several calls to TestRenderFrameHost::PrepareForCommit without calling TestRenderFrameHost::SendRendererInitiatedNavigationRequest when simulating renderer initiated navigations. This makes them fail with PlzNavigate enabled. This CL fixes this fact. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: fix several failing NavigationControllerTests https://codereview.chromium.org/1777903003 introduced several calls to TestRenderFrameHost::PrepareForCommit without calling TestRenderFrameHost::SendRendererInitiatedNavigationRequest when simulating renderer initiated navigations. This makes them fail with PlzNavigate enabled. This CL fixes this fact. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: fix several failing NavigationControllerTests https://codereview.chromium.org/1777903003 introduced several calls to TestRenderFrameHost::PrepareForCommit without calling TestRenderFrameHost::SendRendererInitiatedNavigationRequest when simulating renderer initiated navigations. This makes them fail with PlzNavigate enabled. This CL fixes this fact. BUG=439423 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/7b0202139ab83ec27b73ac1ddddcacf77d5e2de6 Cr-Commit-Position: refs/heads/master@{#381726} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7b0202139ab83ec27b73ac1ddddcacf77d5e2de6 Cr-Commit-Position: refs/heads/master@{#381726} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
