Adding a test to catch regressions where the Find box moves when you open and close a tab. Test is disabled until we fix the bug.

Also made the error message for when tests fails because crash_service isn't running a bit explicit (unrelated to the rest of this change). BUG=1343052 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@1488 0039d316-1c4b-4281-b951-d872f2087c98
2008-08-28 16:31:57 +00:00
commit 8d36e0622b
@@ -751,6 +751,10 @@ void AutomationProvider::OnMessageReceived(const IPC::Message& message) {
                        OnMessageFromExternalHost)
    IPC_MESSAGE_HANDLER(AutomationMsg_FindRequest,
                        HandleFindRequest)
+    IPC_MESSAGE_HANDLER(AutomationMsg_FindWindowVisibilityRequest,
+                        GetFindWindowVisibility)
+    IPC_MESSAGE_HANDLER(AutomationMsg_FindWindowLocationRequest,
+                        HandleFindWindowLocationRequest)
  IPC_END_MESSAGE_MAP()
 }

@@ -1553,25 +1557,20 @@ void AutomationProvider::ExecuteJavascript(const IPC::Message& message,
                                           const std::wstring& frame_xpath,
                                           const std::wstring& script) {
  bool succeeded = false;
-  if (tab_tracker_->ContainsHandle(handle)) {
-    NavigationController* tab = tab_tracker_->GetResource(handle);
-    TabContents* tab_contents = tab->active_contents();
-    if (tab_contents && tab_contents->type() == TAB_CONTENTS_WEB) {
-      WebContents* web_contents = tab_contents->AsWebContents();
+  WebContents* web_contents = GetWebContentsForHandle(handle, NULL);
+  if (web_contents) {
+    // Set the routing id of this message with the controller.
+    // This routing id needs to be remembered for the reverse
+    // communication while sending back the response of
+    // this javascript execution.
+    std::wstring url;
+    SStringPrintf(&url,
+      L"javascript:void(window.domAutomationController.setAutomationId(%d));",
+      message.routing_id());

-      // Set the routing id of this message with the controller.
-      // This routing id needs to be remembered for the reverse
-      // communication while sending back the response of
-      // this javascript execution.
-      std::wstring url;
-      SStringPrintf(&url,
-        L"javascript:void(window.domAutomationController.setAutomationId(%d));",
-        message.routing_id());
-
-      web_contents->ExecuteJavascriptInWebFrame(frame_xpath, url);
-      web_contents->ExecuteJavascriptInWebFrame(frame_xpath, script);
-      succeeded = true;
-    }
+    web_contents->ExecuteJavascriptInWebFrame(frame_xpath, url);
+    web_contents->ExecuteJavascriptInWebFrame(frame_xpath, script);
+    succeeded = true;
  }

  if (!succeeded) {
@@ -1582,14 +1581,10 @@ void AutomationProvider::ExecuteJavascript(const IPC::Message& message,
 void AutomationProvider::GetShelfVisibility(const IPC::Message& message,
                                            int handle) {
  bool visible = false;
-  if (tab_tracker_->ContainsHandle(handle)) {
-    NavigationController* tab = tab_tracker_->GetResource(handle);
-    TabContents* tab_contents = tab->active_contents();
-    if (tab_contents && tab_contents->type() == TAB_CONTENTS_WEB) {
-      WebContents* web_contents = tab_contents->AsWebContents();
-      visible = web_contents->IsDownloadShelfVisible();
-    }
-  }
+
+  WebContents* web_contents = GetWebContentsForHandle(handle, NULL);
+  if (web_contents)
+    visible = web_contents->IsDownloadShelfVisible();

  Send(new AutomationMsg_ShelfVisibilityResponse(message.routing_id(),
                                                 visible));
@@ -1695,27 +1690,40 @@ void AutomationProvider::HandleFindRequest(const IPC::Message& message,

 void AutomationProvider::HandleOpenFindInPageRequest(
    const IPC::Message& message, int handle) {
-  if (tab_tracker_->ContainsHandle(handle)) {
-    NavigationController* tab = tab_tracker_->GetResource(handle);
+  NavigationController* tab = NULL;
+  WebContents* web_contents = GetWebContentsForHandle(handle, &tab);
+  if (web_contents) {
    Browser* browser = Browser::GetBrowserForController(tab, NULL);
-    if (tab->active_contents()->AsWebContents()) {
-      WebContents* web_contents = tab->active_contents()->AsWebContents();
-      web_contents->OpenFindInPageWindow(*browser);
-    }
+    web_contents->OpenFindInPageWindow(*browser);
  }
 }

+void AutomationProvider::GetFindWindowVisibility(const IPC::Message& message,
+                                                 int handle) {
+  bool visible = false;
+  WebContents* web_contents = GetWebContentsForHandle(handle, NULL);
+  if (web_contents)
+    visible = web_contents->IsFindWindowFullyVisible();
+
+  Send(new AutomationMsg_FindWindowVisibilityResponse(message.routing_id(),
+                                                      visible));
+}
+
+void AutomationProvider::HandleFindWindowLocationRequest(
+    const IPC::Message& message, int handle) {
+  int x = -1, y = -1;
+  WebContents* web_contents = GetWebContentsForHandle(handle, NULL);
+  if (web_contents)
+    web_contents->GetFindInPageWindowLocation(&x, &y);
+
+  Send(new AutomationMsg_FindWindowLocationResponse(message.routing_id(),
+                                                    x, y));
+}
+
 void AutomationProvider::HandleInspectElementRequest(
    const IPC::Message& message, int handle, int x, int y) {
-  if (!tab_tracker_->ContainsHandle(handle)) {
-    Send(new AutomationMsg_InspectElementResponse(message.routing_id(), -1));
-    return;
-  }
-
-  NavigationController* nav = tab_tracker_->GetResource(handle);
-  TabContents* tab_contents = nav->active_contents();
-  if (tab_contents->type() == TAB_CONTENTS_WEB) {
-    WebContents* web_contents = tab_contents->AsWebContents();
+  WebContents* web_contents = GetWebContentsForHandle(handle, NULL);
+  if (web_contents) {
    web_contents->InspectElementAt(x, y);
    inspect_element_routing_id_ = message.routing_id();
  } else {
@@ -1863,16 +1871,12 @@ void AutomationProvider::ShowInterstitialPage(const IPC::Message& message,

 void AutomationProvider::HideInterstitialPage(const IPC::Message& message,
                                              int tab_handle) {
-  if (tab_tracker_->ContainsHandle(tab_handle)) {
-    NavigationController* controller = tab_tracker_->GetResource(tab_handle);
-    TabContents* tab_contents = controller->active_contents();
-    if (tab_contents->type() == TAB_CONTENTS_WEB) {
-      WebContents* web_contents = tab_contents->AsWebContents();
-      web_contents->HideInterstitialPage(false, false);
-      Send(new AutomationMsg_HideInterstitialPageResponse(message.routing_id(),
-                                                          true));
-      return;
-    }
+  WebContents* web_contents = GetWebContentsForHandle(tab_handle, NULL);
+  if (web_contents) {
+    web_contents->HideInterstitialPage(false, false);
+    Send(new AutomationMsg_HideInterstitialPageResponse(message.routing_id(),
+                                                        true));
+    return;
  }
  Send(new AutomationMsg_HideInterstitialPageResponse(message.routing_id(),
                                                      false));
@@ -2080,16 +2084,14 @@ void AutomationProvider::IsPageMenuCommandEnabled(const IPC::Message& message,
 }

 void AutomationProvider::PrintNow(const IPC::Message& message, int tab_handle) {
-  if (tab_tracker_->ContainsHandle(tab_handle)) {
-    NavigationController* tab = tab_tracker_->GetResource(tab_handle);
+  NavigationController* tab = NULL;
+  WebContents* web_contents = GetWebContentsForHandle(tab_handle, &tab);
+  if (web_contents) {
    FindAndActivateTab(tab);
-    WebContents* const web_contents = tab->active_contents()->AsWebContents();
-    if (web_contents) {
-      notification_observer_list_.AddObserver(
-          new DocumentPrintedNotificationObserver(this, message.routing_id()));
-      if (web_contents->PrintNow())
-        return;
-    }
+    notification_observer_list_.AddObserver(
+        new DocumentPrintedNotificationObserver(this, message.routing_id()));
+    if (web_contents->PrintNow())
+      return;
  }
  Send(new AutomationMsg_PrintNowResponse(message.routing_id(), false));
 }
@@ -2218,6 +2220,21 @@ void AutomationProvider::OnMessageFromExternalHost(
  }
 }

+WebContents* AutomationProvider::GetWebContentsForHandle(
+    int handle, NavigationController** tab) {
+  WebContents* web_contents = NULL;
+  if (tab_tracker_->ContainsHandle(handle)) {
+    NavigationController* nav_controller = tab_tracker_->GetResource(handle);
+    TabContents* tab_contents = nav_controller->active_contents();
+    if (tab_contents && tab_contents->type() == TAB_CONTENTS_WEB) {
+      web_contents = tab_contents->AsWebContents();
+      if (tab)
+        *tab = nav_controller;
+    }
+  }
+  return web_contents;
+}
+
 TestingAutomationProvider::TestingAutomationProvider(Profile* profile)
    : AutomationProvider(profile) {
  BrowserList::AddObserver(this);
@@ -2260,4 +2277,3 @@ void TestingAutomationProvider::Observe(NotificationType type,
 void TestingAutomationProvider::OnRemoveProvider() {
  AutomationProviderList::GetInstance()->RemoveProvider(this);
 }
-
@@ -202,6 +202,12 @@ class AutomationProvider : public base::RefCounted<AutomationProvider>,
  void HandleOpenFindInPageRequest(const IPC::Message& message,
                                   int handle);

+  // Get the visibility state of the Find window.
+  void GetFindWindowVisibility(const IPC::Message& message, int handle);
+
+  // Responds to requests to find the location of the Find window.
+  void HandleFindWindowLocationRequest(const IPC::Message& message, int handle);
+
  // Responds to InspectElement request
  void HandleInspectElementRequest(const IPC::Message& message,
                                   int handle,
@@ -295,6 +301,11 @@ class AutomationProvider : public base::RefCounted<AutomationProvider>,
  void OnMessageFromExternalHost(int handle, const std::string& target,
                                 const std::string& message);

+  // Convert a tab handle into a WebContents. If |tab| is specified a pointer
+  // to the tab is returned. Returns NULL in case of failure or if the tab is
+  // not of the WebContents type.
+  WebContents* GetWebContentsForHandle(int handle, NavigationController** tab);
+
  // Callback for history redirect queries.
  virtual void OnRedirectQueryComplete(
      HistoryService::Handle request_handle,
@@ -201,6 +201,10 @@ void FindInPageController::Show() {
  view_->OnShow();
 }

+bool FindInPageController::IsAnimating() {
+  return animation_->IsAnimating();
+}
+
 void FindInPageController::EndFindSession() {
  if (IsVisible()) {
    show_on_tab_selection_ = false;
@@ -86,6 +86,9 @@ class FindInPageController : public RenderViewHostDelegate::FindInPage,
  // Moves the window according to the new window size.
  void RespondToResize(const gfx::Size& new_size);

+  // Whether we are animating the position of the Find window.
+  bool IsAnimating();
+
  // Changes the parent window for the FindInPage controller. If |new_parent| is
  // already the parent of this window then no action is taken. |new_parent| can
  // not be NULL.
@@ -2,8 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

+#include "chrome/app/chrome_dll_resource.h"
 #include "chrome/browser/find_in_page_controller.h"
+#include "chrome/test/automation/browser_proxy.h"
 #include "chrome/test/automation/tab_proxy.h"
+#include "chrome/test/automation/window_proxy.h"
 #include "chrome/test/ui/ui_test.h"
 #include "net/url_request/url_request_unittest.h"

@@ -116,3 +119,63 @@ TEST_F(FindInPageControllerTest, FindEnoughMatches_Issue1341577) {
  // properly after a timeout, it will find 5 matches, not just 1.
  EXPECT_EQ(5, tab->FindInPage(L"008.xml", FWD, IGNORE_CASE, false));
 }
+
+// The find window should not change its location just because we open and close
+// a new tab.
+TEST_F(FindInPageControllerTest, DISABLED_FindMovesOnTabClose_Issue1343052) {
+  TestServer server(L"chrome/test/data");
+
+  GURL url = server.TestServerPageW(kFramePage);
+  scoped_ptr<TabProxy> tabA(GetActiveTab());
+  ASSERT_TRUE(tabA->NavigateToURL(url));
+
+  scoped_ptr<WindowProxy> window(automation()->GetActiveWindow());
+  EXPECT_TRUE(window.get() != NULL);
+
+  scoped_ptr<BrowserProxy> browser(
+    automation()->GetBrowserForWindow(window.get()));
+
+  // Toggle the bookmark bar state.
+  browser->ApplyAccelerator(IDC_SHOW_BOOKMARKS_BAR);
+
+  // Open the Find window and wait for it to animate.
+  EXPECT_TRUE(tabA->OpenFindInPage());
+  EXPECT_TRUE(WaitForFindWindowFullyVisible(tabA.get()));
+
+  // Find its location.
+  int x = -1, y = -1;
+  EXPECT_TRUE(tabA->GetFindWindowLocation(&x, &y));
+
+  // Open another tab (tab B).
+  EXPECT_TRUE(browser->AppendTab(url));
+  scoped_ptr<TabProxy> tabB(GetActiveTab());
+
+  // Close tab B.
+  EXPECT_TRUE(tabB->Close(true));
+
+  // See if the Find window has moved.
+  int new_x = -1, new_y = -1;
+  EXPECT_TRUE(tabA->GetFindWindowLocation(&new_x, &new_y));
+
+  EXPECT_EQ(x, new_x);
+  EXPECT_EQ(y, new_y);
+
+  // Now reset the bookmarks bar state and try the same again.
+  browser->ApplyAccelerator(IDC_SHOW_BOOKMARKS_BAR);
+
+  // Bookmark bar has moved, reset our coordinates.
+  EXPECT_TRUE(tabA->GetFindWindowLocation(&x, &y));
+
+  // Open another tab (tab C).
+  EXPECT_TRUE(browser->AppendTab(url));
+  scoped_ptr<TabProxy> tabC(GetActiveTab());
+
+  // Close it.
+  EXPECT_TRUE(tabC->Close(true));
+
+  // See if the Find window has moved.
+  EXPECT_TRUE(tabA->GetFindWindowLocation(&new_x, &new_y));
+
+  EXPECT_EQ(x, new_x);
+  EXPECT_EQ(y, new_y);
+}
@@ -686,6 +686,26 @@ bool WebContents::AdvanceFindSelection(bool forward_direction) {
  return true;
 }

+bool WebContents::IsFindWindowFullyVisible() {
+  return find_in_page_controller_->IsVisible() &&
+         !find_in_page_controller_->IsAnimating();
+}
+
+bool WebContents::GetFindInPageWindowLocation(int* x, int* y) {
+  DCHECK(x && y);
+  HWND find_wnd = find_in_page_controller_->GetHWND();
+  CRect window_rect;
+  if (IsFindWindowFullyVisible() &&
+      ::IsWindow(find_wnd) &&
+      ::GetWindowRect(find_wnd, &window_rect)) {
+    *x = window_rect.TopLeft().x;
+    *y = window_rect.TopLeft().y;
+    return true;     
+  }
+
+  return false;
+}
+
 void WebContents::AlterTextSize(text_zoom::TextSize size) {
  render_view_host()->AlterTextSize(size);
  // TODO(creis): should this be propagated to other and future RVHs?
@@ -2247,7 +2267,7 @@ void WebContents::UpdateMaxPageIDIfNecessary(SiteInstance* site_instance,
 }

 void WebContents::BeforeUnloadFiredFromRenderManager(
-    bool proceed, 
+    bool proceed,
    bool* proceed_to_fire_unload) {
  delegate()->BeforeUnloadFired(this, proceed, proceed_to_fire_unload);
 }
@@ -87,6 +87,8 @@ class WebContents : public TabContents,
  virtual void OpenFindInPageWindow(const Browser& browser);
  virtual void ReparentFindWindow(HWND new_parent);
  virtual bool AdvanceFindSelection(bool forward_direction);
+  virtual bool IsFindWindowFullyVisible();
+  virtual bool GetFindInPageWindowLocation(int* x, int* y);

  // Text zoom
  virtual void AlterTextSize(text_zoom::TextSize size);
@@ -634,7 +636,7 @@ class WebContents : public TabContents,
  // RenderViewHostManager::Delegate pass-throughs -----------------------------

  virtual void BeforeUnloadFiredFromRenderManager(
-      bool proceed, 
+      bool proceed,
      bool* proceed_to_fire_unload);
  virtual void DidStartLoadingFromRenderManager(
      RenderViewHost* render_view_host, int32 page_id) {
@@ -654,7 +656,7 @@ class WebContents : public TabContents,
  }

  // ---------------------------------------------------------------------------
-  
+
  // Enumerate and 'un-parent' any plugin windows that are children
  // of this web contents.
  void DetachPluginWindows();
@@ -739,5 +739,18 @@ IPC_BEGIN_MESSAGES(Automation, 0)
                      int, /* tab_handle */
                      FindInPageRequest /* request */)

+  // Is the Find window fully visible (and not animating) for the specified
+  // tab?
+  IPC_MESSAGE_ROUTED1(AutomationMsg_FindWindowVisibilityRequest,
+                      int /* tab_handle */)
+  IPC_MESSAGE_ROUTED1(AutomationMsg_FindWindowVisibilityResponse,
+                      bool /* is_visible */)
+
+  // Where is the Find window located. |x| and |y| will be -1, -1 on failure.
+  IPC_MESSAGE_ROUTED1(AutomationMsg_FindWindowLocationRequest,
+                      int /* tab_handle */)
+  IPC_MESSAGE_ROUTED2(AutomationMsg_FindWindowLocationResponse,
+                      int, /* x */
+                      int  /* y */)
 IPC_END_MESSAGES(Automation)

@@ -76,6 +76,48 @@ bool TabProxy::OpenFindInPage() {
  // This message expects no response.
 }

+bool TabProxy::IsFindWindowFullyVisible(bool* is_visible) {
+  if (!is_valid())
+    return false;
+
+  if (!is_visible) {
+    NOTREACHED();
+    return false;
+  }
+
+  IPC::Message* response = NULL;
+  bool succeeded = sender_->SendAndWaitForResponse(
+      new AutomationMsg_FindWindowVisibilityRequest(0, handle_),
+      &response,
+      AutomationMsg_FindWindowVisibilityResponse::ID);
+  if (!succeeded)
+    return false;
+
+  void* iter = NULL;
+  response->ReadBool(&iter, is_visible);
+  delete response;
+  return true;
+}
+
+bool TabProxy::GetFindWindowLocation(int* x, int* y) {
+  if (!is_valid() || !x || !y)
+    return false;
+
+  IPC::Message* response = NULL;
+  bool succeeded = sender_->SendAndWaitForResponse(
+      new AutomationMsg_FindWindowLocationRequest(0, handle_),
+      &response,
+      AutomationMsg_FindWindowLocationResponse::ID);
+  if (!succeeded)
+    return false;
+
+  void* iter = NULL;
+  response->ReadInt(&iter, x);
+  response->ReadInt(&iter, y);
+  delete response;
+  return true;
+}
+
 int TabProxy::FindInPage(const std::wstring& search_string,
                         FindInPageDirection forward,
                         FindInPageCase match_case,
@@ -167,6 +167,14 @@ class TabProxy : public AutomationResourceProxy {
  // you don't need to call this function, just use FindInPage(...) directly.
  bool OpenFindInPage();

+  // Returns whether the Find window is fully visible If animating, |is_visible|
+  // will be false. Returns false on failure.
+  bool IsFindWindowFullyVisible(bool* is_visible);
+
+  // Get the x, y coordinates for the Find window. If animating, |x| and |y|
+  // will be -1, -1. Returns false on failure.
+  bool GetFindWindowLocation(int* x, int* y);
+
  // Starts a search within the current tab. The parameter |search_string|
  // specifies what string to search for, |forward| specifies whether to search
  // in forward direction, and |match_case| specifies case sensitivity
@@ -114,7 +114,9 @@ void UITest::TearDown() {
  std::wstring error_msg =
      L"Encountered an unexpected crash in the program during this test.";
  if (expected_crashes_ > 0 && actual_crashes == 0)
-    error_msg += L"  Have you started crash_service.exe?";
+    error_msg += L"  NOTE: This test is expected to fail if crash_service.exe "
+                 L"is not running. Start it manually before running this "
+                 L"test (see the build output directory).";
  EXPECT_EQ(expected_crashes_, actual_crashes) << error_msg;
 }

@@ -359,6 +361,21 @@ bool UITest::WaitForDownloadShelfVisible(TabProxy* tab) {
  return false;
 }

+bool UITest::WaitForFindWindowFullyVisible(TabProxy* tab) {
+  const int kCycles = 20;
+  for (int i = 0; i < kCycles; i++) {
+    bool visible = false;
+    if (!tab->IsFindWindowFullyVisible(&visible))
+      return false;  // Some error.
+    if (visible)
+      return true;  // Find window is visible.
+
+    // Give it a chance to catch up.
+    Sleep(kWaitForActionMaxMsec / kCycles);
+  }
+  return false;
+}
+
 GURL UITest::GetActiveTabURL() {
  scoped_ptr<TabProxy> tab_proxy(GetActiveTab());
  if (!tab_proxy.get())
@@ -416,7 +433,7 @@ DictionaryValue* UITest::GetLocalState() {
 }

 DictionaryValue* UITest::GetDefaultProfilePreferences() {
-   std::wstring path;
+  std::wstring path;
  PathService::Get(chrome::DIR_USER_DATA, &path);
  file_util::AppendToPath(&path, chrome::kNotSignedInProfile);
  file_util::AppendToPath(&path, chrome::kPreferencesFilename);
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

-#ifndef CHROME_TEST_UI_UI_TEST_H__
-#define CHROME_TEST_UI_UI_TEST_H__
+#ifndef CHROME_TEST_UI_UI_TEST_H_
+#define CHROME_TEST_UI_UI_TEST_H_

 // This file provides a common base for running UI unit tests, which operate
 // the entire browser application in a separate process for holistic
@@ -122,6 +122,11 @@ class UITest : public testing::Test {
  // as possible.
  bool WaitForDownloadShelfVisible(TabProxy* tab);

+  // Waits until the Find window has become fully visible (and stopped
+  // animating) in the specified tab. This function can time out (return false)
+  // if the window doesn't appear within a specific time.
+  bool WaitForFindWindowFullyVisible(TabProxy* tab);
+
  // Closes the specified browser.  Returns true if the browser was closed.
  // This call is blocking.  |application_closed| is set to true if this was
  // the last browser window (and therefore as a result of it closing the
@@ -345,5 +350,4 @@ std::ostream& operator<<(std::ostream& out, const ::scoped_ptr<T>& ptr) {
 }
 #endif  // UNIT_TEST

-#endif  // CHROME_TEST_UI_UI_TEST_H__
-
+#endif  // CHROME_TEST_UI_UI_TEST_H_