Prerender: Remove MatchComplete

chrome/browser/prerender/prerender_browsertest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stddef.h>
 #include <deque>
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 649 matching lines @@
   void OnPrerenderStop(PrerenderContents* contents) override {
     DCHECK(contents_);
     contents_ = NULL;
     stop_loop_.Quit();
     // If there is a WaitForLoads call and it has yet to see the expected number
     // of loads, stop the loop so the test fails instead of timing out.
     if (load_waiter_)
       load_waiter_->Quit();
   }
 
-  void OnPrerenderCreatedMatchCompleteReplacement(
-      PrerenderContents* contents,
-      PrerenderContents* replacement) override {}
-
  private:
   TestPrerenderContents* contents_;
   int number_of_loads_;
 
   int expected_number_of_loads_;
   std::unique_ptr<base::RunLoop> load_waiter_;
 
   base::RunLoop create_loop_;
   base::RunLoop start_loop_;
   base::RunLoop stop_loop_;
@@ skipping 2609 matching lines @@
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, ControlGroupRendererInitiated) {
   RestorePrerenderMode restore_prerender_mode;
   PrerenderManager::SetMode(
       PrerenderManager::PRERENDER_MODE_EXPERIMENT_CONTROL_GROUP);
   DisableJavascriptCalls();
   PrerenderTestURL("/prerender/prerender_xhr_put.html",
                    FINAL_STATUS_WOULD_HAVE_BEEN_USED, 0);
   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");
   PrerenderTestURL("/prerender/prerender_referrer_policy.html",
                    FINAL_STATUS_USED, 1);
   NavigateToDestURL();
 }
 
 // Checks that the referrer policy is used when prerendering on HTTPS.
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
@@ skipping 104 matching lines @@
   NavigateToDestURL();
   channel_close_watcher.WaitForChannelClose();
 
   ASSERT_TRUE(IsEmptyPrerenderLinkManager());
   ASSERT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
 // 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(
       test_data_dir_.AppendASCII("streams_private/handle_mime_type"));
   ASSERT_TRUE(extension);
   EXPECT_EQ(std::string(extension_misc::kMimeHandlerPrivateTestExtensionId),
             extension->id());
   MimeTypesHandler* handler = MimeTypesHandler::GetHandler(extension);
   ASSERT_TRUE(handler);
   EXPECT_TRUE(handler->CanHandleMIMEType("application/msword"));
 
   PrerenderTestURL("/prerender/document.doc", FINAL_STATUS_DOWNLOAD, 0);
 
   // 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());
 }
 
 // Checks that non-http/https/chrome-extension subresource cancels the
 // prerender.
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
                        PrerenderCancelSubresourceUnsupportedScheme) {
   GURL image_url = GURL("invalidscheme://www.google.com/test.jpg");
   base::StringPairs replacement_text;
   replacement_text.push_back(
@@ skipping 150 matching lines @@
   // Mock out requests to the Web Store.
   base::FilePath file(GetTestPath("prerender_page.html"));
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&CreateMockInterceptorOnIO, GURL(webstore_url), file));
 
   PrerenderTestURL(CreateClientRedirect(webstore_url),
                    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) {
   DisableJavascriptCalls();
 
   // The prerender will not completely load until after the swap, so wait for a
   // title change before calling DidPrerenderPass.
   std::unique_ptr<TestPrerender> prerender = PrerenderTestURL(
       "/prerender/prerender_deferred_image.html", FINAL_STATUS_USED, 0);
   WaitForASCIITitle(prerender->contents()->prerender_contents(), kReadyTitle);
@@ skipping 69 matching lines @@
 // Checks that deferred redirects in the main frame are followed, even
 // with a double-redirect.
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
                        PrerenderDeferredMainFrameAfterRedirect) {
   DisableJavascriptCalls();
   PrerenderTestURL(CreateServerRedirect("/prerender/image-deferred.png"),
                    FINAL_STATUS_USED, 1);
   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",
                    FINAL_STATUS_APP_TERMINATING, 1);
 
   content::OpenURLParams params(dest_url(), Referrer(), CURRENT_TAB,
                                 ui::PAGE_TRANSITION_TYPED, false);
   params.extra_headers = "X-Custom-Header: 42\r\n";
   NavigateToURLWithParams(params, false);
 }
@@ skipping 305 matching lines @@
 
   NavigateToDestURL();
   EXPECT_EQ(1U, task_manager.tasks().size());
 }
 
 }  // namespace
 
 #endif  // defined(ENABLE_TASK_MANAGER)
 
 }  // namespace prerender