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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@69043 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 58 insertions, 16 deletions

diff --git a/chrome/browser/instant/instant_browsertest.cc b/chrome/browser/instant/instant_browsertest.cc
index 0ef8410..f362158 100644
--- a/chrome/browser/instant/instant_browsertest.cc
+++ b/chrome/browser/instant/instant_browsertest.cc
@@ -83,8 +83,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(
@@ -94,7 +94,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(
@@ -244,6 +244,29 @@ IN_PROC_BROWSER_TEST_F(InstantTest,
   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 c2e8ab6..28a22de3 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 9b44f76..328299f 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);