Fixing visibility transitions for Instant.

chrome/browser/instant/instant_browsertest.cc

Index: chrome/browser/instant/instant_browsertest.cc
diff --git a/chrome/browser/instant/instant_browsertest.cc b/chrome/browser/instant/instant_browsertest.cc
index 9d7cc671acce0abb987728717c7f0cf903523905..e7111fa97cdc885ad8d315f5a39f6d388c1d77ca 100644
--- a/chrome/browser/instant/instant_browsertest.cc
+++ b/chrome/browser/instant/instant_browsertest.cc
@@ -59,6 +59,7 @@
 #define MAYBE_DontCrashOnBlockedJS DISABLED_DontCrashOnBlockedJS
 #define MAYBE_DontPersistSearchbox DISABLED_DontPersistSearchbox
 #define MAYBE_PreloadsInstant DISABLED_PreloadsInstant
+#define MAYBE_PageVisibilityTest DISABLED_PageVisibilityTest
 #define MAYBE_ExperimentEnabled DISABLED_ExperimentEnabled
 #else
 #define MAYBE_OnChangeEvent OnChangeEvent
@@ -80,6 +81,7 @@
 #define MAYBE_DontCrashOnBlockedJS DontCrashOnBlockedJS
 #define MAYBE_DontPersistSearchbox DontPersistSearchbox
 #define MAYBE_PreloadsInstant PreloadsInstant
+#define MAYBE_PageVisibilityTest PageVisibilityTest
 #define MAYBE_ExperimentEnabled ExperimentEnabled
 #endif
 
@@ -255,6 +257,14 @@ class InstantTest : public InProcessBrowserTest {
     return browser()->instant()->GetPreviewContents() != NULL;
   }
 
+  bool IsTabContentsPageVisibiltyVisible(TabContents* tab_contents) {

[sreeram, 2011/10/25 23:26:48]

Typo in "Visibilty". In any case, I'd recommend giving a simpler name:
    bool IsTabContentsVisible(...)
or even just
    bool IsVisible(...)

[Shishir, 2011/10/25 23:46:26]

Done.

+    std::string visibility;
+    if (!GetStringFromJavascript(tab_contents, "document.webkitVisibilityState",
+            &visibility))
+      return false;
+    return visibility == "visible";
+  }
+
   // Returns the state of the search box as a string. This consists of the
   // following:
   // window.chrome.sv
@@ -888,6 +898,7 @@ IN_PROC_BROWSER_TEST_F(InstantTest, MAYBE_PreloadsInstant) {
   // Instant should have a preview, but not display it.
   EXPECT_TRUE(HasPreview());
   EXPECT_FALSE(browser()->instant()->is_displayable());
+  EXPECT_FALSE(IsTabContentsPageVisibiltyVisible(tab_contents->tab_contents()));
 
   // Adding a new tab shouldn't delete (or recreate) the TabContentsWrapper.
   AddBlankTabAndShow(browser());
@@ -903,6 +914,50 @@ IN_PROC_BROWSER_TEST_F(InstantTest, MAYBE_PreloadsInstant) {
   EXPECT_TRUE(browser()->instant()->IsCurrent());
 }
 
+// Tests that the instant search page's visibility is set correctly.
+IN_PROC_BROWSER_TEST_F(InstantTest, MAYBE_PageVisibilityTest) {
+  ASSERT_TRUE(test_server()->Start());
+  EnableInstant();
+  ASSERT_NO_FATAL_FAILURE(SetupInstantProvider("search.html"));
+
+  // Initially navigate to the empty page which should be visible.
+  ASSERT_NO_FATAL_FAILURE(FindLocationBar());
+  location_bar_->location_entry()->SetUserText(ASCIIToUTF16(
+      test_server()->GetURL("files/instant/empty.html").spec()));
+  TabContents* initial_contents = browser()->GetSelectedTabContents();
+  TestNavigationObserver observer(content::Source<NavigationController>(
+      &initial_contents->controller()), NULL, 1);
+  SendKey(ui::VKEY_RETURN);
+  observer.WaitForObservation();

[sreeram, 2011/10/25 23:26:48]

This seems like a lot of trouble. Why not this?

ASSERT_TRUE(test_server()->Start());
EnableInstant();
SetupInstantProvider("search.html");

ui_test_utils::NavigateToURL(browser(), test_server()->GetURL("/"));
TabContents* initial_contents = browser()->GetSelectedTabContents();
EXPECT_TRUE(IsVisible(initial_contents));

FindLocationBar();
location_bar_->FocusLocation(false);
SetupLocationBar();
...

[Shishir, 2011/10/25 23:46:26]

Done.

+  EXPECT_TRUE(IsTabContentsPageVisibiltyVisible(initial_contents));
+
+  // Type something for instant to trigger and wait for preview to navigate.
+  location_bar_->FocusLocation(false);
+  SetupLocationBar();
+  SetupPreview();
+  SetLocationBarText("def");
+  TabContents* preview_contents =
+      browser()->instant()->GetPreviewContents()->tab_contents();
+  EXPECT_TRUE(IsTabContentsPageVisibiltyVisible(preview_contents));
+  EXPECT_FALSE(IsTabContentsPageVisibiltyVisible(initial_contents));
+
+  // Delete the user text we should show the previous page.
+  location_bar_->location_entry()->SetUserText(UTF8ToUTF16(""));
+  EXPECT_FALSE(IsTabContentsPageVisibiltyVisible(preview_contents));
+  EXPECT_TRUE(IsTabContentsPageVisibiltyVisible(initial_contents));
+
+  // Set the user text back and we should see the preview again.
+  location_bar_->location_entry()->SetUserText(UTF8ToUTF16("def"));
+  EXPECT_TRUE(IsTabContentsPageVisibiltyVisible(preview_contents));

[sreeram, 2011/10/25 23:26:48]

Just FYI: At first glance, I wouldn't have expected this to work, since we need
to wait for the renderer to send back a setSuggestions call before we'll show
instant; but your visibility check actually sends a message and waits for the
renderer to respond, so I think we end up processing the message loop.

[Shishir, 2011/10/25 23:46:26]

Do you suggest something else here to increase robustness.

[sreeram, 2011/10/26 04:29:52]

I would normally have said to put in a call to
WaitForMessageToBeProcessedByRenderer(), so that if ever the visibility check
becomes something like a simple function call on the browser thread, instead of
needing to go around to the renderer and back, the test won't break. But this
(visibility check implementation changing) is unlikely to happen, so don't worry
about it.

+  EXPECT_FALSE(IsTabContentsPageVisibiltyVisible(initial_contents));
+
+  // Commit the preview.
+  SendKey(ui::VKEY_RETURN);
+  EXPECT_EQ(preview_contents, browser()->GetSelectedTabContents());
+  EXPECT_TRUE(IsTabContentsPageVisibiltyVisible(preview_contents));
+}
+
+
 // Tests the INSTANT experiment of the field trial.
 class InstantFieldTrialInstantTest : public InstantTest {
  public: