Browser Test (Prerendering) for Favicon

chrome/browser/prerender/prerender_browsertest.cc

Index: chrome/browser/prerender/prerender_browsertest.cc
===================================================================
--- chrome/browser/prerender/prerender_browsertest.cc	(revision 88510)
+++ chrome/browser/prerender/prerender_browsertest.cc	(working copy)
@@ -7,7 +7,9 @@
 #include "base/command_line.h"
 #include "base/path_service.h"
 #include "base/string_util.h"
+#include "base/test/test_timeouts.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
+#include "chrome/browser/favicon/favicon_tab_helper.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prerender/prerender_contents.h"
 #include "chrome/browser/prerender/prerender_manager.h"
@@ -381,9 +383,7 @@
 
   void OpenDestUrlInNewWindowViaJs() const {
     // Make sure in navigating we have a URL to use in the PrerenderManager.
-    TestPrerenderContents* prerender_contents =
-        static_cast<TestPrerenderContents*>(
-            prerender_manager()->FindEntry(dest_url_));
+    TestPrerenderContents* prerender_contents = GetPrerenderContents();
     ASSERT_TRUE(prerender_contents != NULL);
     prerender_contents->set_quit_message_loop_on_destruction(false);
 
@@ -395,7 +395,7 @@
     EXPECT_TRUE(open_window_result);
 
     // If the prerender contents has not been destroyed, run message loop.
-    if (prerender_manager()->FindEntry(dest_url_) != NULL) {
+    if (GetPrerenderContents() != NULL) {
       prerender_contents->set_quit_message_loop_on_destruction(true);
       ui_test_utils::RunMessageLoop();
     }
@@ -403,9 +403,7 @@
 
   void OpenDestUrlInNewWindowViaClick() const {
     // Make sure in navigating we have a URL to use in the PrerenderManager.
-    TestPrerenderContents* prerender_contents =
-        static_cast<TestPrerenderContents*>(
-            prerender_manager()->FindEntry(dest_url_));
+    TestPrerenderContents* prerender_contents = GetPrerenderContents();
     ASSERT_TRUE(prerender_contents != NULL);
     prerender_contents->set_quit_message_loop_on_destruction(false);
 
@@ -417,7 +415,7 @@
     EXPECT_TRUE(click_prerendered_link_result);
 
     // If the prerender contents has not been destroyed, run message loop.
-    if (prerender_manager()->FindEntry(dest_url_) != NULL) {
+    if (GetPrerenderContents() != NULL) {
       prerender_contents->set_quit_message_loop_on_destruction(true);
       ui_test_utils::RunMessageLoop();
     }
@@ -465,6 +463,11 @@
     return safe_browsing_factory_->most_recent_service();
   }
 
+  TestPrerenderContents* GetPrerenderContents() const {
+    return static_cast<TestPrerenderContents*>(
+        prerender_manager()->FindEntry(dest_url_));
+  }
+
  private:
   void PrerenderTestURLImpl(
       const GURL& url,
@@ -510,12 +513,9 @@
     // handle browser navigation directly.
     browser()->OpenURL(src_url, GURL(), CURRENT_TAB, PageTransition::TYPED);
 
-    TestPrerenderContents* prerender_contents = NULL;
     ui_test_utils::RunMessageLoop();
 
-    prerender_contents =
-        static_cast<TestPrerenderContents*>(
-            prerender_manager()->FindEntry(dest_url_));
+    TestPrerenderContents* prerender_contents = GetPrerenderContents();
 
     if (ShouldRenderPrerenderedPageCorrectly(expected_final_status)) {
       ASSERT_TRUE(prerender_contents != NULL);
@@ -539,7 +539,7 @@
 
   void NavigateToURLImpl(const GURL& dest_url) const {
     // Make sure in navigating we have a URL to use in the PrerenderManager.
-    EXPECT_TRUE(prerender_manager()->FindEntry(dest_url_) != NULL);
+    EXPECT_TRUE(GetPrerenderContents() != NULL);
 
     // ui_test_utils::NavigateToURL waits until DidStopLoading is called on
     // the current tab.  As that tab is going to end up deleted, and may never
@@ -553,7 +553,7 @@
     ui_test_utils::RunMessageLoop();
 
     // Make sure the PrerenderContents found earlier was used or removed.
-    EXPECT_TRUE(prerender_manager()->FindEntry(dest_url_) == NULL);
+    EXPECT_TRUE(GetPrerenderContents() == NULL);
 
     if (call_javascript_) {
       // Check if page behaved as expected when actually displayed.
@@ -1340,4 +1340,24 @@
   NavigateToDestURL();
 }
 
+// Checks that the favicon is properly loaded on prerender.
+IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderFavicon) {
+  PrerenderTestURL("files/prerender/prerender_favicon.html",
+                   FINAL_STATUS_USED,
+                   1);
+  TestPrerenderContents* prerender_contents = GetPrerenderContents();
+  ASSERT_TRUE(prerender_contents != NULL);
+  prerender_contents->set_quit_message_loop_on_destruction(false);
+  // The Favicon should show within two seconds of navigating to the page,
+  // otherwise something is wrong.
+  MessageLoopForUI::current()->PostDelayedTask(

[cbentzel, 2011/06/09 15:41:04]

I agree with Pawel that this is pretty hairy. 

Could you do something like the TitleWatcher in
http://codereview.chromium.org/7046053/ which listens for the FAVICON_CHANGED
event instead? 

[Paweł Hajdan Jr., 2011/06/09 19:29:20]

Yes, that's what I'd like to see. Please do that. Also, I'm slightly annoyed
that the change got committed with unresolved comments.

Just a note: my comments about this code are not about flakiness, at least not
directly. Waiting for a fixed amount of time and waiting for an event with a
possibly similar timeout are essentially equally solid.

However, waiting for a fixed amount of time is just wrong. It makes the test
unnecessarily slow, and might encourage a bad test writing style. It does
happen, and it does matter. When most Sleeps disappeared from the codebase,
people stopped adding them to new tests (yes, I'm just watching _every_ test
change, so this is all based on facts and data). This PostDelayedTask is
essentially a Sleep, and that's what makes me unhappy.

Finally, thank you for dealing with flaky tests - I appreciate that effort very
much, and the goal of my comments is just to further increase the quality of the
tests.

+      FROM_HERE,
+      new MessageLoop::QuitTask(),
+      TestTimeouts::action_timeout_ms());
+  NavigateToDestURL();
+  ASSERT_TRUE(TabContentsWrapper::GetCurrentWrapperForContents(
+      browser()->GetSelectedTabContents())->favicon_tab_helper()
+              ->FaviconIsValid());
+}
+
 }  // namespace prerender