Prerender: Remove MatchComplete

chrome/browser/prerender/prerender_browsertest.cc

Index: chrome/browser/prerender/prerender_browsertest.cc
diff --git a/chrome/browser/prerender/prerender_browsertest.cc b/chrome/browser/prerender/prerender_browsertest.cc
index 9eec60fa294f02a3d36490018784ad8e1cc5b9c2..f418c096bcb0fd79dd7f57cc6114873d7d2816ab 100644
--- a/chrome/browser/prerender/prerender_browsertest.cc
+++ b/chrome/browser/prerender/prerender_browsertest.cc
@@ -667,10 +667,6 @@ class TestPrerender : public PrerenderContents::Observer,
       load_waiter_->Quit();
   }
 
-  void OnPrerenderCreatedMatchCompleteReplacement(
-      PrerenderContents* contents,
-      PrerenderContents* replacement) override {}
-
  private:
   TestPrerenderContents* contents_;
   int number_of_loads_;
@@ -3300,35 +3296,6 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, ControlGroupRendererInitiated) {
   OpenDestURLViaClick();
 }
 
-// Make sure that the MatchComplete dummy works in the normal case.  Once
-// a prerender is cancelled because of a script, a dummy must be created to
-// account for the MatchComplete case, and it must have a final status of
-// FINAL_STATUS_WOULD_HAVE_BEEN_USED.
-IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, MatchCompleteDummy) {
-  RestorePrerenderMode restore_prerender_mode;
-  PrerenderManager::SetMode(
-      PrerenderManager::PRERENDER_MODE_EXPERIMENT_MATCH_COMPLETE_GROUP);
-
-  std::vector<FinalStatus> expected_final_status_queue;
-  expected_final_status_queue.push_back(FINAL_STATUS_INVALID_HTTP_METHOD);
-  expected_final_status_queue.push_back(FINAL_STATUS_WOULD_HAVE_BEEN_USED);
-  PrerenderTestURL("/prerender/prerender_xhr_put.html",
-                   expected_final_status_queue, 1);
-  histogram_tester().ExpectTotalCount("Prerender.none_PerceivedPLT", 1);
-  histogram_tester().ExpectTotalCount("Prerender.none_PerceivedPLTMatched", 0);
-  histogram_tester().ExpectTotalCount(
-      "Prerender.none_PerceivedPLTMatchedComplete", 0);
-  histogram_tester().ExpectTotalCount(
-      "Prerender.websame_PrerenderNotSwappedInPLT", 1);
-
-  NavigateToDestURL();
-  histogram_tester().ExpectTotalCount("Prerender.websame_PerceivedPLT", 1);
-  histogram_tester().ExpectTotalCount("Prerender.websame_PerceivedPLTMatched",
-                                      0);
-  histogram_tester().ExpectTotalCount(
-      "Prerender.websame_PerceivedPLTMatchedComplete", 1);
-}
-
 // Checks that the referrer policy is used when prerendering.
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderReferrerPolicy) {
   set_loader_path("/prerender/prerender_loader_with_referrer_policy.html");
@@ -3453,10 +3420,6 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTestWithExtensions, TabsApi) {
 // Test that prerenders abort when navigating to a stream.
 // See chrome/browser/extensions/api/streams_private/streams_private_apitest.cc
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTestWithExtensions, StreamsTest) {
-  RestorePrerenderMode restore_prerender_mode;
-  PrerenderManager::SetMode(
-      PrerenderManager::PRERENDER_MODE_EXPERIMENT_MATCH_COMPLETE_GROUP);
-
   ASSERT_TRUE(StartEmbeddedTestServer());
 
   const extensions::Extension* extension = LoadExtension(
@@ -3472,10 +3435,14 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTestWithExtensions, StreamsTest) {
 
   // Sanity-check that the extension would have picked up the stream in a normal
   // navigation had prerender not intercepted it.
-  // streams_private/handle_mime_type reports success if it has handled the
-  // application/msword type.
+  // The extension streams_private/handle_mime_type reports success if it has
+  // handled the application/msword type.
+  // Note: NavigateToDestURL() cannot be used because of the assertion shecking
+  //     for non-null PrerenderContents.
+  content::OpenURLParams params(dest_url(), Referrer(), CURRENT_TAB,
+      ui::PAGE_TRANSITION_TYPED, false);
   extensions::ResultCatcher catcher;
-  NavigateToDestURL();
+  current_browser()->OpenURL(params);

[mmenke, 2016/05/04 19:21:52]

Can you just use NavigateToURL(browser(), dest_url())?

Not strictly needed, since the catcher does its own waiting, but it's a little
simpler than using OpenURLParams.

[pasko, 2016/05/09 16:07:51]

Done. Thank you, I did not know about the existence of
ui_test_utils::NavigateToURL. The implementation looks a bit different than
NavigateToURLImpl here, but duplicated in parts, which confuses me. Do you know
the history behind the two?

[mmenke, 2016/05/09 20:04:38]

OpenURL is the production code method to start a navigation.

NavigateToURL is the test method to navigate to a page and spin the message loop
until the navigation has finished.  Since this code uses catcher.GetNextResult()
on the next line, the waiting isn't strictly needed, but it doesn't hurt, and
it's a simpler API.

[pasko, 2016/05/10 13:52:26]

Acknowledged. Thank you.

   EXPECT_TRUE(catcher.GetNextResult());
 }
 
@@ -3646,28 +3613,6 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
                    FINAL_STATUS_OPEN_URL, 1);
 }
 
-// Checks that canceling a MatchComplete dummy doesn't result in two
-// stop events.
-IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, CancelMatchCompleteDummy) {
-  RestorePrerenderMode restore_prerender_mode;
-  PrerenderManager::SetMode(
-      PrerenderManager::PRERENDER_MODE_EXPERIMENT_MATCH_COMPLETE_GROUP);
-
-  std::vector<FinalStatus> expected_final_status_queue;
-  expected_final_status_queue.push_back(FINAL_STATUS_JAVASCRIPT_ALERT);
-  expected_final_status_queue.push_back(FINAL_STATUS_CANCELLED);
-  ScopedVector<TestPrerender> prerenders =
-      PrerenderTestURL("/prerender/prerender_alert_before_onload.html",
-                       expected_final_status_queue, 0);
-
-  // Cancel the MatchComplete dummy.
-  GetPrerenderManager()->CancelAllPrerenders();
-  prerenders[1]->WaitForStop();
-
-  // Check the referring page only got one copy of the event.
-  EXPECT_FALSE(HadPrerenderEventErrors());
-}
-
 // Checks that a deferred redirect to an image is not loaded until the page is
 // visible. Also test the right histogram events are emitted in this case.
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderDeferredImage) {
@@ -3757,17 +3702,6 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
   NavigateToDestURL();
 }
 
-// Checks that deferred redirects in a synchronous XHR abort the
-// prerender.
-IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderDeferredSynchronousXHR) {
-  RestorePrerenderMode restore_prerender_mode;
-  PrerenderManager::SetMode(
-      PrerenderManager::PRERENDER_MODE_EXPERIMENT_MATCH_COMPLETE_GROUP);

[mmenke, 2016/05/04 19:21:52]

Why does this test require this call?  Seems like the two lines should work,
regardless.

[pasko, 2016/05/09 16:07:51]

As in the StreamsTest, we cannot use NavigateToDestURL() because it expects a
non-null GetPrerenderContents(). I think the lines were added as a quick fix
without investigating the reasons. The patchset says "Update tests":

https://codereview.chromium.org/550453002

I can make it a ui_test_utils::NavigateToURL(current_browser(), dest_url()), but
it seems to require to set up PrerenderResourceThrottle, which is not done in
prerender_browsertest, afaict.

PrerenderResourceThrottleTest.RedirectSyncXHR seems to cover the exact case we
wanted to cover here, and does it with PrerenderResourceThrottle, so I prefer to
remove this test.

[mmenke, 2016/05/09 20:04:38]

I want to try and get some idea what's going on there before signing off on this
CL - I generally prefer integration tests, because of cobbled together a lot of
these network APIs are, and because the underlying implementation of some of
this stuff changes over time, and if we ever removed the throttle, this test
should still pass.

[pasko, 2016/05/10 13:52:26]

The argument for more integration-kind of tests is something I did not think
about, and it is good.

Also, I was wrong when I said that PrerenderResourceThrottle is not set up with
prerender_browsertest. I checked more carefully and saw that it actually sets
FINAL_STATUS_BAD_DEFERRED_REDIRECT in this test.

So I restored the test. Thanks.

-  PrerenderTestURL("/prerender/prerender_deferred_sync_xhr.html",
-                   FINAL_STATUS_BAD_DEFERRED_REDIRECT, 0);
-  NavigateToDestURL();
-}
-
 // Checks that prerenders are not swapped for navigations with extra headers.
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderExtraHeadersNoSwap) {
   PrerenderTestURL("/prerender/prerender_page.html",