Issue 13461032: Fix Windows InstantExtendedTest focus issues with OmniboxViewViews.

Issue 13461032: Fix Windows InstantExtendedTest focus issues with OmniboxViewViews. (Closed)

Created:
7 years, 8 months ago by msw

Modified:
7 years, 8 months ago

Reviewers:
sreeram, Jered

CC:
chromium-reviews, melevin, dhollowa+watch_chromium.org, dougw+watch_chromium.org, sreeram, gideonwald, dominich, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Ben Goodger (Google), sail

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Fix Windows InstantExtendedTest focus issues with OmniboxViewViews. Four InstantExtendedTest interactive_ui_tests fail with Views Textfields. interactive_ui_tests.exe --gtest_filter=InstantExtendedTest.* --enable-views-textfield OmniboxFocusLoadsInstant, OmniboxTextUponFocusedCommittedSERP NavigationSuggestionIsDiscardedUponSearchSuggestion, OmniboxHasFocusOnNewTab Fix these tests by explicitly calling On[Will]KillFocus for the omnibox. Add InstantExtendedTest::BlurOmniboxAndFocusContents helper function. BUG=131660, 226189 TEST=InstantExtendedTest passes with --enable-views-textfield. R=jered@chromium.org,sreeram@chromium.org Abandoned in favor of sail's http://crrev.com/192645

Patch Set 1 #

Patch Set 2 : Add BlurOmniboxAndFocusContents convenience function. #

Patch Set 3 : Fix another focus failure that surfaced on Mac. #

Total comments: 6

Patch Set 4 : Restore BringBrowserWindowToFront calls; fix comment typo. #

Created: 7 years, 8 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+22 lines, -17 lines)			Patch
	M	chrome/browser/ui/search/instant_extended_browsertest.cc	View	1 2 3	6 chunks	+22 lines, -17 lines	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

msw

Hey Jered, Sreeeram, and Scott (for OWNERS), please take a look; thanks!

7 years, 8 months ago (2013-04-05 15:06:07 UTC) #1

Jered

https://codereview.chromium.org/13461032/diff/7001/chrome/browser/ui/search/instant_extended_browsertest.cc File chrome/browser/ui/search/instant_extended_browsertest.cc (left): https://codereview.chromium.org/13461032/diff/7001/chrome/browser/ui/search/instant_extended_browsertest.cc#oldcode175 chrome/browser/ui/search/instant_extended_browsertest.cc:175: EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser())); The new code does not do this. Is ...

7 years, 8 months ago (2013-04-05 16:27:07 UTC) #3

msw

Comments addressed; please take another look, thanks! https://codereview.chromium.org/13461032/diff/7001/chrome/browser/ui/search/instant_extended_browsertest.cc File chrome/browser/ui/search/instant_extended_browsertest.cc (left): https://codereview.chromium.org/13461032/diff/7001/chrome/browser/ui/search/instant_extended_browsertest.cc#oldcode175 chrome/browser/ui/search/instant_extended_browsertest.cc:175: EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser())); On ...

7 years, 8 months ago (2013-04-05 16:39:02 UTC) #4

Jered

lgtm On 2013/04/05 16:39:02, msw wrote: > Comments addressed; please take another look, thanks! > ...

7 years, 8 months ago (2013-04-05 16:53:29 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/13461032/16001

7 years, 8 months ago (2013-04-05 16:54:34 UTC) #6

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/13461032/16001

7 years, 8 months ago (2013-04-05 16:55:23 UTC) #7

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/msw@chromium.org/13461032/16001

7 years, 8 months ago (2013-04-05 22:40:26 UTC) #8

commit-bot: I haz the power

Failed to apply patch for chrome/browser/ui/search/instant_extended_browsertest.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...

7 years, 8 months ago (2013-04-06 01:46:14 UTC) #9

Failed to apply patch for
chrome/browser/ui/search/instant_extended_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/search/instant_extended_browsertest.cc
  Hunk #4 succeeded at 365 with fuzz 2 (offset -9 lines).
  Hunk #5 FAILED at 417.
  Hunk #6 succeeded at 634 (offset -16 lines).
  1 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/ui/search/instant_extended_browsertest.cc.rej

Patch:       chrome/browser/ui/search/instant_extended_browsertest.cc
Index: chrome/browser/ui/search/instant_extended_browsertest.cc
diff --git a/chrome/browser/ui/search/instant_extended_browsertest.cc
b/chrome/browser/ui/search/instant_extended_browsertest.cc
index
981ce6a0f8bb5ae7dca867f80b7098ae1311a202..da6da5681bdb32371c757759ee05fdb0bb901cc0
100644
--- a/chrome/browser/ui/search/instant_extended_browsertest.cc
+++ b/chrome/browser/ui/search/instant_extended_browsertest.cc
@@ -118,6 +118,16 @@ class InstantExtendedTest : public InProcessBrowserTest,
                         &on_change_calls_);
   }
 
+  // A convenience function to explicitly blur the omnibox and focus
|contents|.
+  // TODO(msw|jered|sreeram): Fix test focus issues; see
http://crbug.com/226189
+  void BlurOmniboxAndFocusContents(content::WebContents* contents) {
+    omnibox()->model()->OnWillKillFocus(contents->GetView()->GetNativeView());
+    omnibox()->model()->OnKillFocus();
+    contents->GetView()->Focus();
+    EXPECT_FALSE(omnibox()->model()->has_focus());
+    EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(),
VIEW_ID_TAB_CONTAINER));
+  }
+
   int on_most_visited_change_calls_;
   int most_visited_items_count_;
   int first_most_visited_item_id_;
@@ -171,12 +181,10 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
ExtendedModeIsOn) {
 IN_PROC_BROWSER_TEST_F(InstantExtendedTest, OmniboxFocusLoadsInstant) {
   ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
 
-  // Explicitly unfocus the omnibox.
+  // Blur the omnibox and focus the active web contents.
   EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
-  ui_test_utils::ClickOnView(browser(), VIEW_ID_TAB_CONTAINER);
-
-  EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_TAB_CONTAINER));
-  EXPECT_FALSE(omnibox()->model()->has_focus());
+  BlurOmniboxAndFocusContents(
+      browser()->tab_strip_model()->GetActiveWebContents());
 
   // Delete any existing overlay.
   instant()->overlay_.reset();
@@ -202,9 +210,8 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
OmniboxFocusLoadsInstant) {
   EXPECT_EQ(overlay, instant()->GetOverlayContents());
 
   // Unfocusing and refocusing the omnibox should also preserve the overlay.
-  ui_test_utils::ClickOnView(browser(), VIEW_ID_TAB_CONTAINER);
-  EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_TAB_CONTAINER));
-
+  BlurOmniboxAndFocusContents(
+    browser()->tab_strip_model()->GetActiveWebContents());
   FocusOmnibox();
   EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_TAB_CONTAINER));
   EXPECT_EQ(overlay, instant()->GetOverlayContents());
@@ -367,15 +374,13 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
   browser()->window()->GetLocationBar()->AcceptInput();
   observer.Wait();
 
-  // With a committed results page, do a search by unfocusing the omnibox and
-  // focusing the contents.
+  // With a committed results page, do a search by blurring the omnibox and
+  // focusing the contents. Call handleOnChange before focusing the contents.
   SetOmniboxText("hello");
-  // Calling handleOnChange manually to make sure it is called before the
-  // Focus() call below.
   EXPECT_TRUE(content::ExecuteScript(instant()->instant_tab()->contents(),
                                      "suggestion = 'hello kitty';"
                                      "handleOnChange();"));
-  instant()->instant_tab()->contents()->GetView()->Focus();
+  BlurOmniboxAndFocusContents(instant()->instant_tab()->contents());
 
   // Search term extraction should kick in with the autocompleted text.
   EXPECT_EQ(ASCIIToUTF16("hello kitty"), omnibox()->GetText());
@@ -412,7 +417,7 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
   EXPECT_TRUE(ExecuteScript("suggestion = 'exams are great';"
                             "behavior = 2;"
                             "handleOnChange();"));
-  instant()->overlay()->contents()->GetView()->Focus();
+  BlurOmniboxAndFocusContents(instant()->overlay()->contents());
   EXPECT_EQ(ASCIIToUTF16("exams are great"), omnibox()->GetText());
 
   // TODO(jered): Remove this after fixing OnBlur().
@@ -645,10 +650,10 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
OmniboxHasFocusOnNewTab) {
   ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
   FocusOmniboxAndWaitForInstantExtendedSupport();
 
-  // Explicitly unfocus the omnibox.
+  // Explicitly blur the omnibox.
   EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
-  ui_test_utils::ClickOnView(browser(), VIEW_ID_TAB_CONTAINER);
-  EXPECT_FALSE(omnibox()->model()->has_focus());
+  BlurOmniboxAndFocusContents(
+      browser()->tab_strip_model()->GetActiveWebContents());
 
   // Open new tab. Preloaded NTP contents should have been used.
   ui_test_utils::NavigateToURLWithDisposition(

Abandoning in favor of sail's http://crrev.com/192645

Expand Messages | Collapse Messages