NoState Prefetch: nostate prefetch browser tests.

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 1ca412d4ec5ca41961f22eed0b8c2107fb2d02de..7c151cbfbb66ca80c1177d945c039d14996bd73d 100644
--- a/chrome/browser/prerender/prerender_browsertest.cc
+++ b/chrome/browser/prerender/prerender_browsertest.cc
@@ -25,7 +25,6 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/test/histogram_tester.h"
 #include "base/test/test_timeouts.h"
 #include "base/values.h"
 #include "build/build_config.h"
@@ -64,7 +63,6 @@
 #include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
-#include "chrome/grit/generated_resources.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/favicon/content/content_favicon_driver.h"
@@ -114,7 +112,6 @@
 #include "net/url_request/url_request_job.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
 
 using chrome_browser_net::NetworkPredictionOptions;
@@ -733,15 +730,6 @@ class PrerenderBrowserTest : public test_utils::PrerenderInProcessBrowserTest {
     EXPECT_TRUE(js_result);
   }
 
-  void UseHttpsSrcServer() {
-    if (https_src_server_)
-      return;
-    https_src_server_.reset(
-        new net::EmbeddedTestServer(net::EmbeddedTestServer::TYPE_HTTPS));
-    https_src_server_->ServeFilesFromSourceDirectory("chrome/test/data");
-    CHECK(https_src_server_->Start());
-  }
-
   void DisableJavascriptCalls() {
     call_javascript_ = false;
   }
@@ -840,15 +828,6 @@ class PrerenderBrowserTest : public test_utils::PrerenderInProcessBrowserTest {
     return history_list->GetSize();
   }
 
-  test_utils::FakeSafeBrowsingDatabaseManager*
-  GetFakeSafeBrowsingDatabaseManager() {
-    return static_cast<test_utils::FakeSafeBrowsingDatabaseManager*>(
-        safe_browsing_factory()
-            ->test_safe_browsing_service()
-            ->database_manager()
-            .get());
-  }
-
   void SetLoaderHostOverride(const std::string& host) {
     loader_host_override_ = host;
     host_resolver()->AddRule(host, "127.0.0.1");
@@ -909,20 +888,6 @@ class PrerenderBrowserTest : public test_utils::PrerenderInProcessBrowserTest {
         base::ASCIIToUTF16(javascript));
   }
 
-  // Returns a string for pattern-matching TaskManager tab entries.
-  base::string16 MatchTaskManagerTab(const char* page_title) {
-    return l10n_util::GetStringFUTF16(IDS_TASK_MANAGER_TAB_PREFIX,
-                                      base::ASCIIToUTF16(page_title));
-  }
-
-  // Returns a string for pattern-matching TaskManager prerender entries.
-  base::string16 MatchTaskManagerPrerender(const char* page_title) {
-    return l10n_util::GetStringFUTF16(IDS_TASK_MANAGER_PRERENDER_PREFIX,
-                                      base::ASCIIToUTF16(page_title));
-  }
-
-  const base::HistogramTester& histogram_tester() { return histogram_tester_; }
-
  private:
   // TODO(davidben): Remove this altogether so the tests don't globally assume
   // only one prerender.
@@ -943,34 +908,18 @@ class PrerenderBrowserTest : public test_utils::PrerenderInProcessBrowserTest {
     net::test_server::GetFilePathWithReplacements(
         loader_path_, replacement_text, &replacement_path);
 
-    const net::EmbeddedTestServer* src_server = embedded_test_server();
-    if (https_src_server_)
-      src_server = https_src_server_.get();
-    GURL loader_url = src_server->GetURL(
-        replacement_path + "&" + loader_query_);
+    GURL loader_url =
+        src_server()->GetURL(replacement_path + "&" + loader_query_);
 
     GURL::Replacements loader_replacements;
     if (!loader_host_override_.empty())
       loader_replacements.SetHostStr(loader_host_override_);
     loader_url = loader_url.ReplaceComponents(loader_replacements);
 
-    CHECK(!expected_final_status_queue.empty());
-    ScopedVector<TestPrerender> prerenders;
-    for (size_t i = 0; i < expected_final_status_queue.size(); i++) {
-      prerenders.push_back(
-          prerender_contents_factory()
-              ->ExpectPrerenderContents(expected_final_status_queue[i])
-              .release());
-    }
+    ScopedVector<TestPrerender> prerenders = NavigateWithPrerenders(
+        loader_url, expected_final_status_queue, expected_number_of_loads);
 
     FinalStatus expected_final_status = expected_final_status_queue.front();
-
-    // Navigate to the loader URL and then wait for the first prerender to be
-    // created.
-    ui_test_utils::NavigateToURL(current_browser(), loader_url);
-    prerenders[0]->WaitForCreate();
-    prerenders[0]->WaitForLoads(expected_number_of_loads);
-
     if (ShouldAbortPrerenderBeforeSwap(expected_final_status)) {
       // The prerender will abort on its own. Assert it does so correctly.
       prerenders[0]->WaitForStop();
@@ -1056,13 +1005,11 @@ class PrerenderBrowserTest : public test_utils::PrerenderInProcessBrowserTest {
   }
 
   GURL dest_url_;
-  std::unique_ptr<net::EmbeddedTestServer> https_src_server_;
   bool call_javascript_;
   bool check_load_events_;
   std::string loader_host_override_;
   std::string loader_path_;
   std::string loader_query_;
-  base::HistogramTester histogram_tester_;
 };
 
 // Checks that a page is correctly prerendered in the case of a
@@ -1663,8 +1610,10 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderReferrer) {
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
                        PrerenderNoSSLReferrer) {

[pasko, 2016/09/22 18:14:04]

nit: fits one line

[mattcary, 2016/09/23 09:00:15]

Done. Strange that git cl format didn't catch that. Oh, I bet this wasn't
actually in my CL, so git didn't touch it. Anyway, fixed now.

   UseHttpsSrcServer();
-  PrerenderTestURL("/prerender/prerender_no_referrer.html", FINAL_STATUS_USED,
-                   1);
+  // Use non-https url for first page.

[pasko, 2016/09/22 18:14:04]

I am confused. Is this comment applying to the line below it? Does it say
something extra on top of the test description? The latter is just 3-4 lines
above...

[mattcary, 2016/09/23 09:00:15]

The test description says that the source page is HTTPS. This is clarifying that
the first prerendered page is *not* https, which is why we have to create the
URL directly rather than just
 PrerenderTestUrl("/prerender/prerender_no_referrer.html", ...)

[pasko, 2016/09/30 18:29:20]

OK, I think it would be more readable like that:
===
  // Use http:// url for the prerendered page.
  GURL url(
      embedded_test_server()->GetURL("/prerender/prerender_no_referrer.html"));

  UseHttpsSrcServer();
  PrerenderTestURL(url, FINAL_STATUS_USED, 1);
  NavigateToDestURL();
===

This would:
* by using extra spacing suggest which range of lines the comment corresponds to
* avoid ambiguity of 'first' that could refer to the page issuing the prerender
* more precise with 'http://' compared to 'non-https' (I know, my nits are
always great)

[mattcary, 2016/10/03 10:58:34]

Done, with slight modification.

+  GURL url(
+      embedded_test_server()->GetURL("/prerender/prerender_no_referrer.html"));
+  PrerenderTestURL(url, FINAL_STATUS_USED, 1);
   NavigateToDestURL();
 }
 
@@ -2149,14 +2098,13 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderXhrDelete) {
 
 // Checks that a top-level page which would trigger an SSL error is canceled.
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderSSLErrorTopLevel) {
+  // We only send the loaded page, not the loader, through SSL.

[pasko, 2016/09/22 18:14:04]

Isn't this a subset of what the comment two lines above says? When we say about
something 'cancelled' in context of prerender, it is assumed to be about the
prerendered page.

[mattcary, 2016/09/23 09:00:15]

It's further detail that explains further why this test differs from other
tests, in particular why we are instantiating a separate EmbeddedTestServer here
rather than just calling UseHttpsSrcServer(). That's a key detail that probably
won't be obvious to someone changing this test in the future.

[pasko, 2016/09/30 18:29:20]

I see. I agree that manipulations in this test are somewhat surprising compared
to other tests, so extra commenting looks good.

For me confusing parts were:
1. use of 'we'
2. big overlap between the two comments begged for more efficiency in comments
:)
3. ambiguity in the term 'loader'

To fix the above my suggestion would be:

// Sets up initial page on HTTP and checks that an SSL error on the prerendered
page cancels the prerender.

[mattcary, 2016/10/03 10:58:34]

Done, thanks, I am still quite sloppy on use of "we".

   net::EmbeddedTestServer https_server(net::EmbeddedTestServer::TYPE_HTTPS);
   https_server.SetSSLConfig(net::EmbeddedTestServer::CERT_MISMATCHED_NAME);
   https_server.ServeFilesFromSourceDirectory("chrome/test/data");
   ASSERT_TRUE(https_server.Start());
   GURL https_url = https_server.GetURL("/prerender/prerender_page.html");
-  PrerenderTestURL(https_url,
-                   FINAL_STATUS_SSL_ERROR,
-                   0);
+  PrerenderTestURL(https_url, FINAL_STATUS_SSL_ERROR, 0);
 }
 
 // Checks that an SSL error that comes from a subresource does not cancel