Merge 69043 - Fixes bug in instant where we wouldn't initially tell the page the

real bounds of the omnibox. The bug occurred because we didn't
initially send down the bounds so that when the init script ran it
sent a bounds of 0x0. When the browser then processed the response
that the page supported instant it incorrectly assumed the page was
told the right bounds.

BUG=65463
TEST=see bug (specifcally comment 5). this is now covered by an
     interactive ui test as well.

Review URL: http://codereview.chromium.org/5697003

TBR=sky@chromium.org
Review URL: http://codereview.chromium.org/6022001

git-svn-id: svn://svn.chromium.org/chrome/branches/597/src@69560 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 76 insertions, 16 deletions

diff --git a/chrome/browser/instant/instant_browsertest.cc b/chrome/browser/instant/instant_browsertest.cc
index daf4b96..29ea476 100644
--- a/chrome/browser/instant/instant_browsertest.cc
+++ b/chrome/browser/instant/instant_browsertest.cc
@@ -84,8 +84,8 @@ class InstantTest : public InProcessBrowserTest {
     EXPECT_TRUE(browser()->instant()->IsShowingInstant());
     EXPECT_FALSE(browser()->instant()->is_active());
 
-    // When the page loads, the initial searchBox values are set and no events
-    // have been called.
+    // When the page loads, the initial searchBox values are set and only a
+    // resize will have been sent.
     EXPECT_NO_FATAL_FAILURE(CheckBoolValueFromJavascript(
         true, "window.chrome.sv", preview_));
     EXPECT_NO_FATAL_FAILURE(CheckIntValueFromJavascript(
@@ -95,7 +95,7 @@ class InstantTest : public InProcessBrowserTest {
     EXPECT_NO_FATAL_FAILURE(CheckIntValueFromJavascript(
         0, "window.onchangecalls", preview_));
     EXPECT_NO_FATAL_FAILURE(CheckIntValueFromJavascript(
-        0, "window.onresizecalls", preview_));
+        1, "window.onresizecalls", preview_));
     EXPECT_NO_FATAL_FAILURE(CheckStringValueFromJavascript(
         "a", "window.chrome.searchBox.value", preview_));
     EXPECT_NO_FATAL_FAILURE(CheckBoolValueFromJavascript(
@@ -243,6 +243,29 @@ IN_PROC_BROWSER_TEST_F(InstantTest, NonSearchToSearchDoesntSupportInstant) {
   EXPECT_FALSE(browser()->instant()->is_active());
 }
 
+// Verifies the page was told a non-zero height.
+// TODO: when we nuke the old api and fix 66104, this test should load
+// search.html.
+IN_PROC_BROWSER_TEST_F(InstantTest, ValidHeight) {
+  ASSERT_TRUE(test_server()->Start());
+  ASSERT_NO_FATAL_FAILURE(SetupInstantProvider("old_api.html"));
+  ASSERT_NO_FATAL_FAILURE(SetLocationBarText(L"a"));
+  // The preview should be active.
+  ASSERT_TRUE(browser()->instant()->is_active());
+  // And the height should be valid.
+  TabContents* tab = browser()->instant()->GetPreviewContents()->tab_contents();
+  ASSERT_NO_FATAL_FAILURE(
+      CheckBoolValueFromJavascript(true, "window.validHeight", tab));
+
+  // Check that searchbox height was also set.
+  std::wstring script =
+      L"window.domAutomationController.send(window.chrome.searchBox.height)";
+  int height;
+  ASSERT_TRUE(ui_test_utils::ExecuteJavaScriptAndExtractInt(
+                  tab->render_view_host(), std::wstring(), script, &height));
+  EXPECT_GT(height, 0);
+}
+
 // Verify that the onsubmit event is dispatched upon pressing enter.
 // TODO(sky): Disabled, http://crbug.com/62940.
 IN_PROC_BROWSER_TEST_F(InstantTest, DISABLED_OnSubmitEvent) {
diff --git a/chrome/browser/instant/instant_controller.cc b/chrome/browser/instant/instant_controller.cc
index 8747e9f..c1fddcd 100644
--- a/chrome/browser/instant/instant_controller.cc
+++ b/chrome/browser/instant/instant_controller.cc
@@ -189,8 +189,10 @@ void InstantController::SetOmniboxBounds(const gfx::Rect& bounds) {
   if (omnibox_bounds_ == bounds)
     return;
 
+  // Always track the omnibox bounds. That way if Update is later invoked the
+  // bounds are in sync.
+  omnibox_bounds_ = bounds;
   if (loader_manager_.get()) {
-    omnibox_bounds_ = bounds;
     if (loader_manager_->current_loader())
       loader_manager_->current_loader()->SetOmniboxBounds(bounds);
     if (loader_manager_->pending_loader())
diff --git a/chrome/browser/instant/instant_loader.cc b/chrome/browser/instant/instant_loader.cc
index f97d190..d0a2b6e 100644
--- a/chrome/browser/instant/instant_loader.cc
+++ b/chrome/browser/instant/instant_loader.cc
@@ -58,10 +58,12 @@ const char kPreviewHeaderValue[] = "preview";
 // instant after it has loaded.
 class InstantLoader::FrameLoadObserver : public NotificationObserver {
  public:
-  FrameLoadObserver(TabContents* tab_contents,
+  FrameLoadObserver(InstantLoader* loader,
+                    TabContents* tab_contents,
                     const string16& text,
                     bool verbatim)
-      : tab_contents_(tab_contents),
+      : loader_(loader),
+        tab_contents_(tab_contents),
         text_(text),
         verbatim_(verbatim),
         unique_id_(tab_contents_->controller().pending_entry()->unique_id()) {
@@ -88,6 +90,7 @@ class InstantLoader::FrameLoadObserver : public NotificationObserver {
             active_entry->unique_id() != unique_id_) {
           return;
         }
+        loader_->SendBoundsToPage(true);
         tab_contents_->render_view_host()->DetermineIfPageSupportsInstant(
             text_, verbatim_);
         break;
@@ -99,6 +102,8 @@ class InstantLoader::FrameLoadObserver : public NotificationObserver {
   }
 
  private:
+  InstantLoader* loader_;
+
   // The TabContents we're listening for changes on.
   TabContents* tab_contents_;
 
@@ -386,13 +391,10 @@ class InstantLoader::TabContentsDelegateImpl : public TabContentsDelegate {
         page_id != source->controller().GetActiveEntry()->page_id())
       return;
 
-    if (result) {
-      gfx::Rect bounds = loader_->GetOmniboxBoundsInTermsOfPreview();
-      loader_->last_omnibox_bounds_ = loader_->omnibox_bounds_;
+    if (result)
       loader_->PageFinishedLoading();
-    } else {
+    else
       loader_->PageDoesntSupportInstant(user_typed_before_load_);
-    }
   }
 
  private:
@@ -560,8 +562,10 @@ void InstantLoader::Update(TabContentsWrapper* tab_contents,
       preview_contents_->controller().LoadURL(
           instant_url, GURL(), transition_type);
       frame_load_observer_.reset(
-          new FrameLoadObserver(preview_contents()->tab_contents(),
-                                user_text_, verbatim));
+          new FrameLoadObserver(this,
+                                preview_contents()->tab_contents(),
+                                user_text_,
+                                verbatim));
     }
   } else {
     DCHECK(template_url_id_ == 0);
@@ -717,6 +721,10 @@ void InstantLoader::Observe(NotificationType type,
 
 void InstantLoader::PageFinishedLoading() {
   frame_load_observer_.reset();
+
+  // Send the bounds of the omnibox down now.
+  SendBoundsToPage(false);
+
   // Wait for the user input before showing, this way the page should be up to
   // date by the time we show it.
 }
@@ -750,12 +758,16 @@ void InstantLoader::PageDoesntSupportInstant(bool needs_reload) {
 }
 
 void InstantLoader::ProcessBoundsChange() {
+  SendBoundsToPage(false);
+}
+
+void InstantLoader::SendBoundsToPage(bool force_if_waiting) {
   if (last_omnibox_bounds_ == omnibox_bounds_)
     return;
 
-  last_omnibox_bounds_ = omnibox_bounds_;
   if (preview_contents_.get() && is_showing_instant() &&
-      !is_waiting_for_load()) {
+      (force_if_waiting || !is_waiting_for_load())) {
+    last_omnibox_bounds_ = omnibox_bounds_;
     preview_contents_->render_view_host()->SearchBoxResize(
         GetOmniboxBoundsInTermsOfPreview());
   }
diff --git a/chrome/browser/instant/instant_loader.h b/chrome/browser/instant/instant_loader.h
index bae1a89..8be48af 100644
--- a/chrome/browser/instant/instant_loader.h
+++ b/chrome/browser/instant/instant_loader.h
@@ -127,9 +127,14 @@ class InstantLoader : public NotificationObserver {
   // the page needs to be reloaded.
   void PageDoesntSupportInstant(bool needs_reload);
 
-  // Invoked from the timer to update the bounds of the omnibox.
+  // Invokes |SetBoundsToPage(false)|. This is called from the timer.
   void ProcessBoundsChange();
 
+  // Notifes the page of the omnibox bounds. If |force_if_loading| is true the
+  // bounds are sent down even if we're waiting on the load, otherwise if we're
+  // waiting on the load and |force_if_loading| is false this does nothing.
+  void SendBoundsToPage(bool force_if_loading);
+
   // Creates and sets the preview TabContentsWrapper.
   void CreatePreviewContents(TabContentsWrapper* tab_contents);
 
diff --git a/chrome/test/data/instant/old_api.html b/chrome/test/data/instant/old_api.html
new file mode 100644
index 0000000..7f3d006
--- /dev/null
+++ b/chrome/test/data/instant/old_api.html
@@ -0,0 +1,18 @@
+<html>
+<body>
+<script>
+window.chrome.sv = true;
+
+window.validHeight = false;
+
+window.chrome.userInput = function(value, verbatim, ignored) {
+  window.chrome.setSuggestResult(value);
+};
+
+window.chrome.setDropdownDimensions = function(x, y, w, h) {
+  window.validHeight = (h > 0);
+};
+
+</script>
+</body>
+</html>