prerender: Add NavigationOrSwapObserver in browsertest.

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 6e4c235b41595ffb6eb907f13d6a1879e6ce65bb..1b4051a14dac0fab003782b2cf1af4c9000dd885 100644
--- a/chrome/browser/prerender/prerender_browsertest.cc
+++ b/chrome/browser/prerender/prerender_browsertest.cc
@@ -43,6 +43,7 @@
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension_constants.h"
@@ -61,6 +62,7 @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
@@ -87,6 +89,7 @@ using content::Referrer;
 using content::RenderViewHost;
 using content::RenderWidgetHost;
 using content::WebContents;
+using content::WebContentsObserver;
 
 // Prerender tests work as follows:
 //
@@ -209,6 +212,46 @@ class ChannelDestructionWatcher {
   DISALLOW_COPY_AND_ASSIGN(ChannelDestructionWatcher);
 };
 
+// A simple observer to wait on either a load or a swap of a WebContents.
+class NavigationOrSwapObserver : public WebContentsObserver,
+                                 public TabStripModelObserver {
+ public:
+  // Waits for either a load or a swap of |tab_strip_model|'s active
+  // WebContents.
+  explicit NavigationOrSwapObserver(TabStripModel* tab_strip_model)
+      : WebContentsObserver(tab_strip_model->GetActiveWebContents()),
+        tab_strip_model_(tab_strip_model) {
+    tab_strip_model_->AddObserver(this);
+  }
+
+  ~NavigationOrSwapObserver() {
+    tab_strip_model_->RemoveObserver(this);
+  }
+
+  void Wait() {
+    loop_.Run();
+  }
+
+  // WebContentsObserver implementation:
+  virtual void DidStopLoading(RenderViewHost* render_view_host) OVERRIDE {
+    loop_.Quit();
+  }
+
+  // TabStripModelObserver implementation:
+  virtual void TabReplacedAt(TabStripModel* tab_strip_model,
+                             WebContents* old_contents,
+                             WebContents* new_contents,
+                             int index) OVERRIDE {
+    if (old_contents != web_contents())

[mmenke, 2014/01/07 15:09:17]

Does this ever happen?  If not, could use an EXPECT_EQ

[davidben, 2014/01/07 17:31:34]

Hrm. Doesn't currently happen, but I was thinking this would just apply to a
particular WebContents. Perhaps we want to test two simultaneous prerenders or
something. I've tweaked it to pass both TabStripModel and WebContents as
parameter and added a comment to make it clear it only works for CURRENT_TAB.

I'm not sure why we even have NavigateToURLWithDisposition; nothing ever passes
a different disposition and we don't even prerender in that case. Anyway, I've
added a TODO for now. We can make it use OpenURL's return value when all swaps
become async. Though, even that's not enough for NEW_WINDOW. We'd either need to
static_cast the delegate to a Browser* or call chrome::Navigate instead with a
NavigateParams and look at params.browser. I think chrome::Navigate will fill
that in.

[mmenke, 2014/01/07 17:44:50]

NavigateToDestURLInNewTab()?  There used to be a test that did a background tab
that way, too.

[davidben, 2014/01/07 18:57:14]

Well, NavigateToDestURLInNewTab creates a new tab and then uses CURRENT_TAB. The
background tab one is gone now I guess. git log -S comes up with
http://codereview.chromium.org/8392041.

Right now MaybeUsePrerenderedPage doesn't run on non-CURRENT_TAB navigations at
all.

+      return;
+    loop_.Quit();
+  }
+
+ private:
+  TabStripModel* tab_strip_model_;
+  base::RunLoop loop_;
+};
+
 // PrerenderContents that stops the UI message loop on DidStopLoading().
 class TestPrerenderContents : public PrerenderContents {
  public:
@@ -621,20 +664,33 @@ class NeverStartURLRequestJob : public net::URLRequestJob {
 class NeverStartProtocolHandler
     : public net::URLRequestJobFactory::ProtocolHandler {
  public:
-  NeverStartProtocolHandler() {}
+  explicit NeverStartProtocolHandler(const base::FilePath& file)
+      : file_(file),
+        first_run_(true) {
+  }
   virtual ~NeverStartProtocolHandler() {}
 
   virtual net::URLRequestJob* MaybeCreateJob(
       net::URLRequest* request,
       net::NetworkDelegate* network_delegate) const OVERRIDE {
-    return new NeverStartURLRequestJob(request, network_delegate);
+    if (first_run_) {

[mmenke, 2014/01/07 15:09:17]

While this isn't the most complicated class, this first run behavior isn't clear
from the class name.  Suggest either a short comment, or making a name that
encompasses this behavior (NeverStartFirstRequestProtocolHandler?)

[davidben, 2014/01/07 17:31:34]

Done.

+      first_run_ = false;
+      return new NeverStartURLRequestJob(request, network_delegate);
+    }
+    return new content::URLRequestMockHTTPJob(request, network_delegate, file_);
   }
+ private:

[mmenke, 2014/01/07 15:09:17]

nit:  Blank line before private.

[davidben, 2014/01/07 17:31:34]

Done.

+  base::FilePath file_;
+  mutable bool first_run_;
 };
 
-void CreateNeverStartProtocolHandlerOnIO(const GURL& url) {
+// Makes |url| never respond on the first load, and then with the contents of
+// |file| afterwards.
+void CreateNeverStartProtocolHandlerOnIO(const GURL& url,
+                                         const base::FilePath& file) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   scoped_ptr<net::URLRequestJobFactory::ProtocolHandler> never_respond_handler(
-      new NeverStartProtocolHandler());
+      new NeverStartProtocolHandler(file));
   net::URLRequestFilter::GetInstance()->AddUrlProtocolHandler(
       url, never_respond_handler.Pass());
 }
@@ -838,9 +894,6 @@ class PrerenderBrowserTest : virtual public InProcessBrowserTest {
     // due to session storage namespace mismatch. The merge is only kicked off
     // asynchronously.
     NavigateToDestURLWithDisposition(CURRENT_TAB, false);
-    // Run the message loop, waiting for the merge to complete, the swapin to
-    // be reattempted, and to eventually succeed.
-    content::RunMessageLoop();
   }
 
   // Opens the url in a new tab, with no opener.
@@ -1152,6 +1205,24 @@ class PrerenderBrowserTest : virtual public InProcessBrowserTest {
     GetPrerenderManager()->mutable_config().max_bytes = 1000 * 1024 * 1024;
   }
 
+  bool DidPrerenderPass(WebContents* web_contents) const {
+    bool prerender_test_result = false;
+    CHECK(content::ExecuteScriptAndExtractBool(
+        web_contents,
+        "window.domAutomationController.send(DidPrerenderPass())",
+        &prerender_test_result));
+    return prerender_test_result;
+  }
+
+  bool DidDisplayPass(WebContents* web_contents) const {
+    bool display_test_result = false;
+    CHECK(content::ExecuteScriptAndExtractBool(
+        web_contents,
+        "window.domAutomationController.send(DidDisplayPass())",
+        &display_test_result));

[mmenke, 2014/01/07 15:09:17]

Do we need these CHECKs?  Seems like we can just return false.

[davidben, 2014/01/07 17:31:34]

Done. I think it's worthwhile to be able to distinguish "DidDisplayPass()
returned false" from "something went horribly wrong and we couldn't run JS at
all", but there are DLOG(ERROR)s, so we're probably good?

[mmenke, 2014/01/07 17:44:50]

You could return a testing::AssertionResult, with different text in the two
cases, if you really wanted.  Or could just stick with the CHECK, if you're that
concerned about it.

+    return display_test_result;
+  }
+
  protected:
   bool autostart_test_server_;
 
@@ -1238,12 +1309,7 @@ class PrerenderBrowserTest : virtual public InProcessBrowserTest {
         prerender_contents->WaitForPrerenderToHaveReadyTitleIfRequired();
 
         // Check if page behaves as expected while in prerendered state.
-        bool prerender_test_result = false;
-        ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
-            prerender_contents->GetRenderViewHostMutable(),
-            "window.domAutomationController.send(DidPrerenderPass())",
-            &prerender_test_result));
-        EXPECT_TRUE(prerender_test_result);
+        EXPECT_TRUE(DidPrerenderPass(prerender_contents->prerender_contents()));
       }
 
       // Test that the referring page received events.
@@ -1305,7 +1371,10 @@ class PrerenderBrowserTest : virtual public InProcessBrowserTest {
     // Navigate to the prerendered URL, but don't run the message loop. Browser
     // issued navigations to prerendered pages will synchronously swap in the
     // prerendered page.

[mmenke, 2014/01/07 15:09:17]

This comment is no longer correct.

[davidben, 2014/01/07 17:31:34]

Done.

+    NavigationOrSwapObserver swap_observer(
+        current_browser()->tab_strip_model());
     WebContents* target_web_contents = current_browser()->OpenURL(params);
+    swap_observer.Wait();
 
     if (web_contents && expect_swap_to_succeed) {
       EXPECT_EQ(web_contents, target_web_contents);
@@ -1313,12 +1382,7 @@ class PrerenderBrowserTest : virtual public InProcessBrowserTest {
         if (page_load_observer.get())
           page_load_observer->Wait();
 
-        bool display_test_result = false;
-        ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
-            web_contents,
-            "window.domAutomationController.send(DidDisplayPass())",
-            &display_test_result));
-        EXPECT_TRUE(display_test_result);
+        EXPECT_TRUE(DidDisplayPass(web_contents));
       }
     }
   }
@@ -1552,9 +1616,11 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderNoCommitNoSwap) {
   // Navigate to a page that triggers a prerender for a URL that never commits.
   const GURL kNoCommitUrl("http://never-respond.example.com");
+  base::FilePath file(FILE_PATH_LITERAL(
+      "chrome/test/data/prerender/prerender_page.html"));
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::Bind(&CreateNeverStartProtocolHandlerOnIO, kNoCommitUrl));
+      base::Bind(&CreateNeverStartProtocolHandlerOnIO, kNoCommitUrl, file));
   DisableJavascriptCalls();
   PrerenderTestURL(kNoCommitUrl,
                    FINAL_STATUS_NAVIGATION_UNCOMMITTED,
@@ -1568,9 +1634,11 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderNoCommitNoSwap) {
 IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderNoCommitNoSwap2) {
   // Navigate to a page that then navigates to a URL that never commits.
   const GURL kNoCommitUrl("http://never-respond.example.com");
+  base::FilePath file(FILE_PATH_LITERAL(
+      "chrome/test/data/prerender/prerender_page.html"));
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::Bind(&CreateNeverStartProtocolHandlerOnIO, kNoCommitUrl));
+      base::Bind(&CreateNeverStartProtocolHandlerOnIO, kNoCommitUrl, file));
   DisableJavascriptCalls();
   PrerenderTestURL(CreateClientRedirect(kNoCommitUrl.spec()),
                    FINAL_STATUS_APP_TERMINATING, 1);
@@ -1652,12 +1720,7 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderNaClPluginDisabled) {
   //                loading.  It would be great if we could avoid that.
   WebContents* web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
-  bool display_test_result = false;
-  ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
-      web_contents,
-      "window.domAutomationController.send(DidDisplayPass())",
-      &display_test_result));
-  EXPECT_TRUE(display_test_result);
+  EXPECT_TRUE(DidDisplayPass(web_contents));
 }
 
 // Checks that plugins in an iframe are not loaded while a page is