Fix prepopulate value for Find and add browser tests for it.

Notable changes:

FindBarController now sets the prepopulate text during Show for the Find dialog, to fix regression 40121. It also checks not just the current find string (find_text()) but the previous one also (previous_find_text()) in MaybeSetPrepopulateText() because the current find string is blanked out when closing the Find window. And TabContents now makes sure previous_find_text() is never overwritten with an empty find_text value (to prevent the last search from being cleared if you open Find, close it without searching and open it again).

This changelist also adds automated tests for the codepaths above, which results in adding a GetFindText method to the FindBarTesting interface, to allow the tests to check what string is being shown to the user when the Find box opens.

BUG=40121
TEST=FindInPageControllerTest.Prepopulate* (three tests)

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

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

13 files changed, 224 insertions, 28 deletions

diff --git a/chrome/browser/cocoa/find_bar_bridge.h b/chrome/browser/cocoa/find_bar_bridge.h
index 855886a..39c146e 100644
--- a/chrome/browser/cocoa/find_bar_bridge.h
+++ b/chrome/browser/cocoa/find_bar_bridge.h
@@ -30,7 +30,7 @@ class FindBarCocoaController;
 // BrowserWindowCocoa::AddFindBar() in order to add its FindBarView to
 // the cocoa views hierarchy.
 //
-// Memory ownership is relatively straightfoward.  The FindBarBridge
+// Memory ownership is relatively straightforward.  The FindBarBridge
 // object is owned by the Browser.  FindBarCocoaController is retained
 // by bother FindBarBridge and BrowserWindowController, since both use it.
 
@@ -75,6 +75,7 @@ class FindBarBridge : public FindBar,
   // Methods from FindBarTesting.
   virtual bool GetFindBarWindowInfo(gfx::Point* position,
                                     bool* fully_visible);
+  virtual string16 GetFindText();
 
   // Used to disable find bar animations when testing.
   static bool disable_animations_during_testing_;
diff --git a/chrome/browser/cocoa/find_bar_bridge.mm b/chrome/browser/cocoa/find_bar_bridge.mm
index 7b46fd2..299a17b 100644
--- a/chrome/browser/cocoa/find_bar_bridge.mm
+++ b/chrome/browser/cocoa/find_bar_bridge.mm
@@ -83,3 +83,12 @@ bool FindBarBridge::GetFindBarWindowInfo(gfx::Point* position,
 
   return window_visible;
 }
+
+string16 FindBarBridge::GetFindText() {
+  // This function is currently only used in Windows and Linux specific browser
+  // tests (testing prepopulate values that Mac's don't rely on), but if we add
+  // more tests that are non-platform specific, we need to flesh out this
+  // function.
+  NOTIMPLEMENTED();
+  return string16();
+}
diff --git a/chrome/browser/find_bar.h b/chrome/browser/find_bar.h
index 7797387..e95e8a1 100644
--- a/chrome/browser/find_bar.h
+++ b/chrome/browser/find_bar.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
@@ -87,6 +87,9 @@ class FindBarTesting {
   // shown (return value of false), the out params will be {(0, 0), false}.
   virtual bool GetFindBarWindowInfo(gfx::Point* position,
                                     bool* fully_visible) = 0;
+
+  // Gets the search string currently visible in the Find box.
+  virtual string16 GetFindText() = 0;
 };
 
 #endif  // CHROME_BROWSER_FIND_BAR_H_
diff --git a/chrome/browser/find_bar_controller.cc b/chrome/browser/find_bar_controller.cc
index b56b929..643e777 100644
--- a/chrome/browser/find_bar_controller.cc
+++ b/chrome/browser/find_bar_controller.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -28,6 +28,8 @@ void FindBarController::Show() {
   // Only show the animation if we're not already showing a find bar for the
   // selected TabContents.
   if (!tab_contents_->find_ui_active()) {
+    MaybeSetPrepopulateText();
+
     tab_contents_->set_find_ui_active(true);
     find_bar_->Show(true);
   }
@@ -74,25 +76,7 @@ void FindBarController::ChangeTabContents(TabContents* contents) {
   registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED,
                  Source<NavigationController>(&tab_contents_->controller()));
 
-#if !defined(OS_MACOSX)
-  // Find out what we should show in the find text box. Usually, this will be
-  // the last search in this tab, but if no search has been issued in this tab
-  // we use the last search string (from any tab).
-  string16 find_string = tab_contents_->find_text();
-  if (find_string.empty())
-    find_string = tab_contents_->find_prepopulate_text();
-
-  // Update the find bar with existing results and search text, regardless of
-  // whether or not the find bar is visible, so that if it's subsequently
-  // shown it is showing the right state for this tab. We update the find text
-  // _first_ since the FindBarView checks its emptiness to see if it should
-  // clear the result count display when there's nothing in the box.
-  find_bar_->SetFindText(find_string);
-#else
-  // Having a per-tab find_string is not compatible with OS X's find pasteboard,
-  // so we always have the same find text in all find bars. This is done through
-  // the find pasteboard mechanism, so don't set the text here.
-#endif
+  MaybeSetPrepopulateText();
 
   if (tab_contents_->find_ui_active()) {
     // A tab with a visible find bar just got selected and we need to show the
@@ -209,3 +193,27 @@ void FindBarController::UpdateFindBarForCurrentResult() {
 
   find_bar_->UpdateUIForFindResult(find_result, tab_contents_->find_text());
 }
+
+void FindBarController::MaybeSetPrepopulateText() {
+#if !defined(OS_MACOSX)
+  // Find out what we should show in the find text box. Usually, this will be
+  // the last search in this tab, but if no search has been issued in this tab
+  // we use the last search string (from any tab).
+  string16 find_string = tab_contents_->find_text();
+  if (find_string.empty())
+    find_string = tab_contents_->previous_find_text();
+  if (find_string.empty())
+    find_string = tab_contents_->find_prepopulate_text();
+
+  // Update the find bar with existing results and search text, regardless of
+  // whether or not the find bar is visible, so that if it's subsequently
+  // shown it is showing the right state for this tab. We update the find text
+  // _first_ since the FindBarView checks its emptiness to see if it should
+  // clear the result count display when there's nothing in the box.
+  find_bar_->SetFindText(find_string);
+#else
+  // Having a per-tab find_string is not compatible with OS X's find pasteboard,
+  // so we always have the same find text in all find bars. This is done through
+  // the find pasteboard mechanism, so don't set the text here.
+#endif
+}
diff --git a/chrome/browser/find_bar_controller.h b/chrome/browser/find_bar_controller.h
index 8edbc79..7e450af 100644
--- a/chrome/browser/find_bar_controller.h
+++ b/chrome/browser/find_bar_controller.h
@@ -60,6 +60,13 @@ class FindBarController : public NotificationObserver {
   // de-flickering in addition to just calling the update function.
   void UpdateFindBarForCurrentResult();
 
+  // For Windows and Linux this function sets the prepopulate text for the
+  // Find text box. The propopulate value is the last value the user searched
+  // for in the current tab, or (if blank) the last value searched for in any
+  // tab. Mac has a global value for search, so this function does nothing on
+  // Mac.
+  void MaybeSetPrepopulateText();
+
   NotificationRegistrar registrar_;
 
   scoped_ptr<FindBar> find_bar_;
diff --git a/chrome/browser/find_bar_host_browsertest.cc b/chrome/browser/find_bar_host_browsertest.cc
index c6ff0d7..d168e7e 100644
--- a/chrome/browser/find_bar_host_browsertest.cc
+++ b/chrome/browser/find_bar_host_browsertest.cc
@@ -60,7 +60,6 @@ class FindInPageControllerTest : public InProcessBrowserTest {
 #elif defined(OS_MACOSX)
     FindBarBridge::disable_animations_during_testing_ = true;
 #endif
-
   }
 
  protected:
@@ -69,6 +68,20 @@ class FindInPageControllerTest : public InProcessBrowserTest {
         browser()->GetFindBarController()->find_bar()->GetFindBarTesting();
     return find_bar->GetFindBarWindowInfo(position, fully_visible);
   }
+
+  string16 GetFindBarText() {
+    FindBarTesting* find_bar =
+        browser()->GetFindBarController()->find_bar()->GetFindBarTesting();
+    return find_bar->GetFindText();
+  }
+
+  void EnsureFindBoxOpen() {
+    browser()->ShowFindBar();
+    gfx::Point position;
+    bool fully_visible = false;
+    EXPECT_TRUE(GetFindBarWindowInfo(&position, &fully_visible));
+    EXPECT_TRUE(fully_visible);
+  }
 };
 
 // Platform independent FindInPage that takes |const wchar_t*|
@@ -810,6 +823,142 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, PreferPreviousSearch) {
   EXPECT_EQ(tab1->find_text(), WideToUTF16(L"Default"));
 }
 
+// This tests that whenever you close and reopen the Find bar, it should show
+// the last search entered in that tab. http://crbug.com/40121.
+IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, PrepopulateSameTab) {
+#if defined(OS_MACOSX)
+  // FindInPage on Mac doesn't use prepopulated values. Search there is global.
+  return;
+#endif
+
+  HTTPTestServer* server = StartHTTPServer();
+
+  // First we navigate to any page.
+  GURL url = server->TestServerPageW(kSimple);
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // Find the word "page".
+  int ordinal = 0;
+  TabContents* tab1 = browser()->GetSelectedTabContents();
+  EXPECT_EQ(1, FindInPageWchar(tab1, L"page", kFwd, kIgnoreCase, &ordinal));
+
+  // Open the Find box.
+  EnsureFindBoxOpen();
+
+  EXPECT_EQ(ASCIIToUTF16("page"), GetFindBarText());
+
+  // Close the Find box.
+  browser()->GetFindBarController()->EndFindSession(
+      FindBarController::kKeepSelection);
+
+  // Open the Find box again.
+  EnsureFindBoxOpen();
+
+  // After the Find box has been reopened, it should have been prepopulated with
+  // the word "page" again.
+  EXPECT_EQ(ASCIIToUTF16("page"), GetFindBarText());
+}
+
+// This tests that whenever you open Find in a new tab it should prepopulate
+// with a previous search term (in any tab), if a search has not been issued in
+// this tab before.
+IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, PrepopulateInNewTab) {
+#if defined(OS_MACOSX)
+  // FindInPage on Mac doesn't use prepopulated values. Search there is global.
+  return;
+#endif
+
+  HTTPTestServer* server = StartHTTPServer();
+
+  // First we navigate to any page.
+  GURL url = server->TestServerPageW(kSimple);
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // Find the word "page".
+  int ordinal = 0;
+  TabContents* tab1 = browser()->GetSelectedTabContents();
+  EXPECT_EQ(1, FindInPageWchar(tab1, L"page", kFwd, kIgnoreCase, &ordinal));
+
+  // Now create a second tab and load the same page.
+  browser()->AddTabWithURL(url, GURL(), PageTransition::TYPED, true, -1,
+                           false, NULL);
+  browser()->SelectTabContentsAt(1, false);
+  TabContents* tab2 = browser()->GetSelectedTabContents();
+  EXPECT_NE(tab1, tab2);
+
+  // Open the Find box.
+  EnsureFindBoxOpen();
+
+  // The new tab should have "page" prepopulated, since that was the last search
+  // in the first tab.
+  EXPECT_EQ(ASCIIToUTF16("page"), GetFindBarText());
+}
+
+// This makes sure that we can search for A in tabA, then for B in tabB and
+// when we come back to tabA we should still see A (because that was the last
+// search in that tab).
+IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, PrepopulatePreserveLast) {
+#if defined(OS_MACOSX)
+  // FindInPage on Mac doesn't use prepopulated values. Search there is global.
+  return;
+#endif
+
+  HTTPTestServer* server = StartHTTPServer();
+
+  // First we navigate to any page.
+  GURL url = server->TestServerPageW(kSimple);
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // Find the word "page".
+  int ordinal = 0;
+  TabContents* tab1 = browser()->GetSelectedTabContents();
+  EXPECT_EQ(1, FindInPageWchar(tab1, L"page", kFwd, kIgnoreCase, &ordinal));
+
+  // Open the Find box.
+  EnsureFindBoxOpen();
+
+  EXPECT_EQ(ASCIIToUTF16("page"), GetFindBarText());
+
+  // Close the Find box.
+  browser()->GetFindBarController()->EndFindSession(
+      FindBarController::kKeepSelection);
+
+  // Now create a second tab and load the same page.
+  browser()->AddTabWithURL(url, GURL(), PageTransition::TYPED, true, -1,
+                           false, NULL);
+  browser()->SelectTabContentsAt(1, false);
+  TabContents* tab2 = browser()->GetSelectedTabContents();
+  EXPECT_NE(tab1, tab2);
+
+  // Find "text".
+  FindInPageWchar(tab2, L"text", kFwd, kIgnoreCase, &ordinal);
+
+  // Go back to the first tab and make sure we have NOT switched the prepopulate
+  // text to "text".
+  browser()->SelectTabContentsAt(0, false);
+
+  // Open the Find box.
+  EnsureFindBoxOpen();
+
+  // After the Find box has been reopened, it should have been prepopulated with
+  // the word "page" again, since that was the last search in that tab.
+  EXPECT_EQ(ASCIIToUTF16("page"), GetFindBarText());
+
+  // Close the Find box.
+  browser()->GetFindBarController()->EndFindSession(
+      FindBarController::kKeepSelection);
+
+  // Re-open the Find box.
+  // This is a special case: previous search in TabContents used to get cleared
+  // if you opened and closed the FindBox, which would cause the global
+  // prepopulate value to show instead of last search in this tab.
+  EnsureFindBoxOpen();
+
+  // After the Find box has been reopened, it should have been prepopulated with
+  // the word "page" again, since that was the last search in that tab.
+  EXPECT_EQ(ASCIIToUTF16("page"), GetFindBarText());
+}
+
 // This makes sure that dismissing the find bar with kActivateSelection works.
 IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, ActivateLinkNavigatesPage) {
   HTTPTestServer* server = StartHTTPServer();
diff --git a/chrome/browser/gtk/find_bar_gtk.cc b/chrome/browser/gtk/find_bar_gtk.cc
index 39b6511..e3bc7ff 100644
--- a/chrome/browser/gtk/find_bar_gtk.cc
+++ b/chrome/browser/gtk/find_bar_gtk.cc
@@ -529,6 +529,11 @@ bool FindBarGtk::GetFindBarWindowInfo(gfx::Point* position,
   return true;
 }
 
+string16 FindBarGtk::GetFindText() {
+  std::string contents(gtk_entry_get_text(GTK_ENTRY(text_entry_)));
+  return UTF8ToUTF16(contents);
+}
+
 void FindBarGtk::FindEntryTextInContents(bool forward_search) {
   TabContents* tab_contents = find_bar_controller_->tab_contents();
   if (!tab_contents)
diff --git a/chrome/browser/gtk/find_bar_gtk.h b/chrome/browser/gtk/find_bar_gtk.h
index c36ca50..3de3dfd 100644
--- a/chrome/browser/gtk/find_bar_gtk.h
+++ b/chrome/browser/gtk/find_bar_gtk.h
@@ -64,6 +64,7 @@ class FindBarGtk : public FindBar,
   // Methods from FindBarTesting.
   virtual bool GetFindBarWindowInfo(gfx::Point* position,
                                     bool* fully_visible);
+  virtual string16 GetFindText();
 
   // Overridden from NotificationObserver:
   virtual void Observe(NotificationType type,
diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc
index cdd5d1f6..8f49bb1 100644
--- a/chrome/browser/tab_contents/tab_contents.cc
+++ b/chrome/browser/tab_contents/tab_contents.cc
@@ -1205,7 +1205,8 @@ void TabContents::StopFinding(
   // by the user, but the UI has not been dismissed.
   if (selection_action != FindBarController::kClearSelection)
     find_ui_active_ = false;
-  previous_find_text_ = find_text_;
+  if (!find_text_.empty())
+    previous_find_text_ = find_text_;
   find_text_.clear();
   find_op_aborted_ = true;
   last_search_result_ = FindNotificationDetails();
diff --git a/chrome/browser/views/find_bar_host.cc b/chrome/browser/views/find_bar_host.cc
index b957ad2..cd4658f 100644
--- a/chrome/browser/views/find_bar_host.cc
+++ b/chrome/browser/views/find_bar_host.cc
@@ -136,6 +136,10 @@ bool FindBarHost::GetFindBarWindowInfo(gfx::Point* position,
   return true;
 }
 
+string16 FindBarHost::GetFindText() {
+  return find_bar_view()->GetFindText();
+}
+
 gfx::Rect FindBarHost::GetDialogPosition(gfx::Rect avoid_overlapping_rect) {
   // Find the area we have to work with (after accounting for scrollbars, etc).
   gfx::Rect widget_bounds;
diff --git a/chrome/browser/views/find_bar_host.h b/chrome/browser/views/find_bar_host.h
index 1e2f92a..d0a946e 100644
--- a/chrome/browser/views/find_bar_host.h
+++ b/chrome/browser/views/find_bar_host.h
@@ -75,6 +75,7 @@ class FindBarHost : public DropdownBarHost,
   // FindBarTesting implementation:
   virtual bool GetFindBarWindowInfo(gfx::Point* position,
                                     bool* fully_visible);
+  virtual string16 GetFindText();
 
   // Overridden from DropdownBarHost:
   // Returns the rectangle representing where to position the find bar. It uses
diff --git a/chrome/browser/views/find_bar_view.cc b/chrome/browser/views/find_bar_view.cc
index a4040e7..fa25bdc 100644
--- a/chrome/browser/views/find_bar_view.cc
+++ b/chrome/browser/views/find_bar_view.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -170,6 +170,10 @@ void FindBarView::SetFindText(const string16& find_text) {
   find_text_->SetText(find_text);
 }
 
+string16 FindBarView::GetFindText() const {
+  return find_text_->text();
+}
+
 void FindBarView::UpdateForResult(const FindNotificationDetails& result,
                                   const string16& find_text) {
   bool have_valid_range =
diff --git a/chrome/browser/views/find_bar_view.h b/chrome/browser/views/find_bar_view.h
index 43f2bc3..74f23d0 100644
--- a/chrome/browser/views/find_bar_view.h
+++ b/chrome/browser/views/find_bar_view.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -42,7 +42,8 @@ class FindBarView : public DropdownBarView,
   explicit FindBarView(FindBarHost* host);
   virtual ~FindBarView();
 
-  // Sets the text displayed in the text box.
+  // Gets/sets the text displayed in the text box.
+  string16 GetFindText() const;
   void SetFindText(const string16& find_text);
 
   // Updates the label inside the Find text box that shows the ordinal of the
@@ -57,7 +58,9 @@ class FindBarView : public DropdownBarView,
   virtual void Paint(gfx::Canvas* canvas);
   virtual void Layout();
   virtual gfx::Size GetPreferredSize();
-  virtual void ViewHierarchyChanged(bool is_add, views::View* parent, views::View* child);
+  virtual void ViewHierarchyChanged(bool is_add,
+                                    views::View* parent,
+                                    views::View* child);
 
   // Overridden from views::ButtonListener:
   virtual void ButtonPressed(views::Button* sender, const views::Event& event);