Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
715203004:
PlzNavigate: Add a browser test for basic navigations (Closed)
DescriptionPlzNavigate: Add a browser test for basic navigations
This CL adds a browsertest that checks that basic browser initiated navigations
work with browser side navigations enabled.
BUG=376014, 376006, 376091
Committed: https://crrev.com/cd857adbb07fb2e889fa24282648e1f64ede2c95
Cr-Commit-Position: refs/heads/master@{#313480}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Added a browser test for renderer initiated navigations #
Total comments: 13
Patch Set 3 : Folded the TestNavigationWebContentsObserver in TestNavigationObserver #
Total comments: 4
Patch Set 4 : Fixed nits #Patch Set 5 : Disabled the renderer initiated cross iste test on Win #Patch Set 6 : Fixed issue with cross-site navigation test #
Total comments: 2
Messages
Total messages: 30 (9 generated)
clamy@chromium.org changed reviewers: + carlosk@chromium.org, nasko@chromium.org
@carlosk, nasko: PTAL. This CL introduces a browsertest to exercise the full PlzNavigate codepath. https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... File content/browser/browser_side_navigation_browsertest.cc (right): https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... content/browser/browser_side_navigation_browsertest.cc:18: class BrowserSideNavigationWebContentsObserver : public WebContentsObserver { I was thinking maybe this class could be extracted to be a NavigationTestWebContentsObserver, that could be used both by SitePerProcessBrowserTest and BrowserSideNavigationBrowserTest. Wdyt? https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... content/browser/browser_side_navigation_browsertest.cc:77: IN_PROC_BROWSER_TEST_F(BrowserSideNavigationBrowserTest, This only test browser initiated navigations. For render intiated navigations, I am not sure how to simulate cliking on a link.
Thanks for adding this! https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... File content/browser/browser_side_navigation_browsertest.cc (right): https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... content/browser/browser_side_navigation_browsertest.cc:18: class BrowserSideNavigationWebContentsObserver : public WebContentsObserver { On 2014/11/19 16:41:04, clamy wrote: > I was thinking maybe this class could be extracted to be a > NavigationTestWebContentsObserver, that could be used both by > SitePerProcessBrowserTest and BrowserSideNavigationBrowserTest. Wdyt? Sure. I've created content_browser_test_utils_internal.h/cc for the purpose of sharing test utility code that is needed only internally in content/. It will be great to share code and not duplicate. If we feel/find there are uses outside of content, then move them over to the non-internal version. https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... content/browser/browser_side_navigation_browsertest.cc:77: IN_PROC_BROWSER_TEST_F(BrowserSideNavigationBrowserTest, On 2014/11/19 16:41:04, clamy wrote: > This only test browser initiated navigations. For render intiated navigations, I > am not sure how to simulate cliking on a link. We have a "framework" for testing this. Look at render_frame_host_manager_browsertest.cc, NoScriptAccessAfterSwapOut being one example. There is also some JS based functionality at the top of the file as well. Also, let's make it a different test case. https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... content/browser/browser_side_navigation_browsertest.cc:93: // Perform a cross-site navigation. Let's add a check that the previous site instance and the current one are different. This will ensure we are indeed doing cross-site navigation. https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... File content/browser/browser_side_navigation_browsertest.h (right): https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... content/browser/browser_side_navigation_browsertest.h:12: class BrowserSideNavigationBrowserTest : public ContentBrowserTest { Browser tests don't need separate headers, we just have a cc file and class definitions are inlined in the cc file.
Indeed it's good to have a bit more evidence that PlzNavigate is working as expected so far. I have no comments so far.
Thanks! I have added another test for renderer initiated navigations. https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... File content/browser/browser_side_navigation_browsertest.cc (right): https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... content/browser/browser_side_navigation_browsertest.cc:18: class BrowserSideNavigationWebContentsObserver : public WebContentsObserver { On 2014/11/19 18:05:56, nasko wrote: > On 2014/11/19 16:41:04, clamy wrote: > > I was thinking maybe this class could be extracted to be a > > NavigationTestWebContentsObserver, that could be used both by > > SitePerProcessBrowserTest and BrowserSideNavigationBrowserTest. Wdyt? > > Sure. I've created content_browser_test_utils_internal.h/cc for the purpose of > sharing test utility code that is needed only internally in content/. It will be > great to share code and not duplicate. If we feel/find there are uses outside of > content, then move them over to the non-internal version. Done. https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... content/browser/browser_side_navigation_browsertest.cc:77: IN_PROC_BROWSER_TEST_F(BrowserSideNavigationBrowserTest, On 2014/11/19 18:05:56, nasko wrote: > On 2014/11/19 16:41:04, clamy wrote: > > This only test browser initiated navigations. For render intiated navigations, > I > > am not sure how to simulate cliking on a link. > > We have a "framework" for testing this. Look at > render_frame_host_manager_browsertest.cc, NoScriptAccessAfterSwapOut being one > example. There is also some JS based functionality at the top of the file as > well. > Also, let's make it a different test case. Done. https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... content/browser/browser_side_navigation_browsertest.cc:93: // Perform a cross-site navigation. On 2014/11/19 18:05:56, nasko wrote: > Let's add a check that the previous site instance and the current one are > different. This will ensure we are indeed doing cross-site navigation. Done. https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... File content/browser/browser_side_navigation_browsertest.h (right): https://chromiumcodereview.appspot.com/715203004/diff/1/content/browser/brows... content/browser/browser_side_navigation_browsertest.h:12: class BrowserSideNavigationBrowserTest : public ContentBrowserTest { On 2014/11/19 18:05:56, nasko wrote: > Browser tests don't need separate headers, we just have a cc file and class > definitions are inlined in the cc file. Done (SitePerProcessBrowserTest has a header file though :) ).
https://chromiumcodereview.appspot.com/715203004/diff/20001/content/browser/b... File content/browser/browser_side_navigation_browsertest.cc (right): https://chromiumcodereview.appspot.com/715203004/diff/20001/content/browser/b... content/browser/browser_side_navigation_browsertest.cc:51: TestNavigationWebContentsObserver observer2(shell()->web_contents()); It will be a bit more readable if you scope each of those navigations and associated expectations in {}, this way you don't need to have multiple observerX variables and it will be clear that each block is independent navigation. https://chromiumcodereview.appspot.com/715203004/diff/20001/content/browser/b... content/browser/browser_side_navigation_browsertest.cc:79: GURL main_url1(embedded_test_server()->GetURL("/simple_links.html")); Any reason why the page that already exists doesn't work for these tests? https://chromiumcodereview.appspot.com/715203004/diff/20001/content/browser/b... content/browser/browser_side_navigation_browsertest.cc:96: WaitForLoadStop(shell()->web_contents()); alexmos@ has done some recent work to add return value to WaitForLoadStop. It will be good to add EXPECT_TRUE() around that call. https://chromiumcodereview.appspot.com/715203004/diff/20001/content/renderer/... File content/renderer/render_frame_impl.cc (right): https://chromiumcodereview.appspot.com/715203004/diff/20001/content/renderer/... content/renderer/render_frame_impl.cc:4172: Send(new FrameHostMsg_DidStartLoading(routing_id_, true)); Shouldn't this be fired off before the BeginNavigation IPC? https://chromiumcodereview.appspot.com/715203004/diff/20001/content/test/cont... File content/test/content_browser_test_utils_internal.h (right): https://chromiumcodereview.appspot.com/715203004/diff/20001/content/test/cont... content/test/content_browser_test_utils_internal.h:24: explicit TestNavigationWebContentsObserver(WebContents* web_contents); I wonder if it is useful to fold this functionality in the existing TestNavigationObserver class in content/public/test/test_navigation_observer.h. WDYT? https://chromiumcodereview.appspot.com/715203004/diff/20001/content/test/data... File content/test/data/simple_links.html (right): https://chromiumcodereview.appspot.com/715203004/diff/20001/content/test/data... content/test/data/simple_links.html:23: // Listen to incoming messages and reply to them. Is any of this code that follows actually used? If not, let's remove it.
Thanks! Here is a version with the TestNavigationWebContentsObserver folded in TestNavigationObserver. https://chromiumcodereview.appspot.com/715203004/diff/20001/content/browser/b... File content/browser/browser_side_navigation_browsertest.cc (right): https://chromiumcodereview.appspot.com/715203004/diff/20001/content/browser/b... content/browser/browser_side_navigation_browsertest.cc:51: TestNavigationWebContentsObserver observer2(shell()->web_contents()); On 2014/11/24 23:15:00, nasko wrote: > It will be a bit more readable if you scope each of those navigations and > associated expectations in {}, this way you don't need to have multiple > observerX variables and it will be clear that each block is independent > navigation. Done. https://chromiumcodereview.appspot.com/715203004/diff/20001/content/browser/b... content/browser/browser_side_navigation_browsertest.cc:79: GURL main_url1(embedded_test_server()->GetURL("/simple_links.html")); On 2014/11/24 23:15:00, nasko wrote: > Any reason why the page that already exists doesn't work for these tests? I wanted a simpler page with simple links, both same-site and cross-site. The page only add more complex cases (targeted, blank, etc...). Also it includes an iframe, which makes the test case more complicated. https://chromiumcodereview.appspot.com/715203004/diff/20001/content/browser/b... content/browser/browser_side_navigation_browsertest.cc:96: WaitForLoadStop(shell()->web_contents()); On 2014/11/24 23:15:00, nasko wrote: > alexmos@ has done some recent work to add return value to WaitForLoadStop. It > will be good to add EXPECT_TRUE() around that call. Done. https://chromiumcodereview.appspot.com/715203004/diff/20001/content/renderer/... File content/renderer/render_frame_impl.cc (right): https://chromiumcodereview.appspot.com/715203004/diff/20001/content/renderer/... content/renderer/render_frame_impl.cc:4172: Send(new FrameHostMsg_DidStartLoading(routing_id_, true)); On 2014/11/24 23:15:00, nasko wrote: > Shouldn't this be fired off before the BeginNavigation IPC? Done. https://chromiumcodereview.appspot.com/715203004/diff/20001/content/test/cont... File content/test/content_browser_test_utils_internal.h (right): https://chromiumcodereview.appspot.com/715203004/diff/20001/content/test/cont... content/test/content_browser_test_utils_internal.h:24: explicit TestNavigationWebContentsObserver(WebContents* web_contents); On 2014/11/24 23:15:00, nasko wrote: > I wonder if it is useful to fold this functionality in the existing > TestNavigationObserver class in content/public/test/test_navigation_observer.h. > WDYT? I think it would make sense. Especially as having two classes named TestNavigationObserver and TestNavigationWebContentsObserver might get confusing. Done. https://chromiumcodereview.appspot.com/715203004/diff/20001/content/test/data... File content/test/data/simple_links.html (right): https://chromiumcodereview.appspot.com/715203004/diff/20001/content/test/data... content/test/data/simple_links.html:23: // Listen to incoming messages and reply to them. On 2014/11/24 23:15:00, nasko wrote: > Is any of this code that follows actually used? If not, let's remove it. Done.
Found just a couple of minor things. LGTM after answering those. https://chromiumcodereview.appspot.com/715203004/diff/40001/content/browser/b... File content/browser/browser_side_navigation_browsertest.cc (right): https://chromiumcodereview.appspot.com/715203004/diff/40001/content/browser/b... content/browser/browser_side_navigation_browsertest.cc:42: GURL main_url(embedded_test_server()->GetURL("/title1.html")); nit: main_url now seems like a bad name choice as there are many of them in this test. https://chromiumcodereview.appspot.com/715203004/diff/40001/content/browser/b... content/browser/browser_side_navigation_browsertest.cc:114: // Go to the main link page again. This next step is resetting the test to its initial state. Would look better to me to split it in two different tests one for same and other for cross site. WDYT?
lgtm https://codereview.chromium.org/715203004/diff/20001/content/browser/browser_... File content/browser/browser_side_navigation_browsertest.cc (right): https://codereview.chromium.org/715203004/diff/20001/content/browser/browser_... content/browser/browser_side_navigation_browsertest.cc:79: GURL main_url1(embedded_test_server()->GetURL("/simple_links.html")); On 2014/11/26 12:47:42, clamy wrote: > On 2014/11/24 23:15:00, nasko wrote: > > Any reason why the page that already exists doesn't work for these tests? > > I wanted a simpler page with simple links, both same-site and cross-site. The > page only add more complex cases (targeted, blank, etc...). Also it includes an > iframe, which makes the test case more complicated. Acknowledged.
Thanks! https://chromiumcodereview.appspot.com/715203004/diff/40001/content/browser/b... File content/browser/browser_side_navigation_browsertest.cc (right): https://chromiumcodereview.appspot.com/715203004/diff/40001/content/browser/b... content/browser/browser_side_navigation_browsertest.cc:42: GURL main_url(embedded_test_server()->GetURL("/title1.html")); On 2014/11/26 15:02:53, carlosk wrote: > nit: main_url now seems like a bad name choice as there are many of them in this > test. Done. https://chromiumcodereview.appspot.com/715203004/diff/40001/content/browser/b... content/browser/browser_side_navigation_browsertest.cc:114: // Go to the main link page again. On 2014/11/26 15:02:53, carlosk wrote: > This next step is resetting the test to its initial state. Would look better to > me to split it in two different tests one for same and other for cross site. > WDYT? Done.
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/715203004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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/715203004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
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/715203004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...) Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
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/715203004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cd857adbb07fb2e889fa24282648e1f64ede2c95 Cr-Commit-Position: refs/heads/master@{#313480}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/884683002/ by dconnelly@chromium.org. The reason for reverting is: Broke the build: http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2... http://build.chromium.org/p/chromium.webkit/builders/Linux%20GN/builds/18976 http://build.chromium.org/p/chromium.webkit/builders/Win%20Builder/builds/73255.
Message was sent while issue was closed.
mostynb@opera.com changed reviewers: + mostynb@opera.com
Message was sent while issue was closed.
In commit cd857adbb07fb2e889fa24282648e1f64ede2c95 there was stray SitePerProcessWebContentsObserver left on content/browser/site_per_process_browsertest.cc line 927. But strangely, when viewing that entire file in the diff here, the line was changed also. Is this a rietveld bug, or a CQ bug? https://codereview.chromium.org/715203004/diff/100001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/715203004/diff/100001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:387: // We can't use a SitePerProcessWebContentsObserver to verify the URL here, Stray reference to SitePerProcessWebContentsObserver, which was removed. https://codereview.chromium.org/715203004/diff/100001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:855: // We can't use a SitePerProcessWebContentsObserver to verify the URL here, Stray reference to SitePerProcessWebContentsObserver, which was removed.
Message was sent while issue was closed.
Looking at tot locally, https://codereview.chromium.org/797813006 added a test yesterday night which uses the SitePerProcessWebContentsObserver. Funnily it does not show in the diff. I tried to submit that patch yesterday and got all bots green but the windows gpu one, so only this one was rerun this morning which does not compile the problematic file. So a lot of bad timing. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
