Introduce ui_test_utils::FocusView and de-flake OmniboxViewTest.BeginningShownAfterBlur

Introduce ui_test_utils::FocusView and de-flake OmniboxViewTest.BeginningShownAfterBlur

Currently interactive UI tests typically call `ClickOnView` to transfer
focus. On Mac, the behaviour of a click changes when an application
loses focus, and re-activating the application in a test is not
reliable. The test application shouldn't be losing focus in
interactive_ui_tests, but tracing suggests this is ocurring anyway.

This CL introduces ui_test_utils::FocusView which is a more robust way
to transfer focus in a test.

OmniboxViewTest.BeginningShownAfterBlur is changed to use
ui_test_utils::FocusView and re-enabled.

BUG=406012
Committed: https://crrev.com/98d6f1e45c66a03b7b583eec3694c84ff7411fec
Cr-Commit-Position: refs/heads/master@{#293516}

[tapted, 2014-09-03 06:01:07]

Patchset #2 (id:20001) has been deleted

[tapted, 2014-09-03 07:29:29]

tapted@chromium.org changed reviewers:
+ sky@chromium.org

[tapted, 2014-09-03 07:29:29]

Hi sky, please take a look. There's a couple more notes on the bug at
http://crbug.com/406012#c7

[sky, 2014-09-03 15:49:38]

LGTM

[sky, 2014-09-03 15:50:10]

One other question (this shouldn't hold up landing). If ClickToFocus
is unreliable on mac should we remove it?

On Wed, Sep 3, 2014 at 12:29 AM,  <tapted@chromium.org> wrote:
> Reviewers: sky,
>
> Message:
> Hi sky, please take a look. There's a couple more notes on the bug at
> http://crbug.com/406012#c7
>
> Description:
> Introduce ui_test_utils::FocusView and de-flake
> OmniboxViewTest.BeginningShownAfterBlur
>
> Currently interactive UI tests typically call `ClickOnView` to transfer
> focus. On Mac, the behaviour of a click changes when an application
> loses focus, and re-activating the application in a test is not
> reliable. The test application shouldn't be losing focus in
> interactive_ui_tests, but tracing suggests this is ocurring anyway.
>
> This CL introduces ui_test_utils::FocusView which is a more robust way
> to transfer focus in a test.
>
> OmniboxViewTest.BeginningShownAfterBlur is changed to use
> ui_test_utils::FocusView and re-enabled.
>
> BUG=406012
>
> Please review this at https://codereview.chromium.org/534993002/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+25, -14 lines):
>   M chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
>   M chrome/test/base/interactive_test_utils.h
>   M chrome/test/base/interactive_test_utils_mac.mm
>   M chrome/test/base/interactive_test_utils_views.cc
>
>
> Index: chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
> diff --git a/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
> b/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
> index
>
10f517e44e87805e4d44c9b331c2fd5471d1de3f..77346c13911a570fbb82f44832d12bb75302891e
> 100644
> --- a/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
> +++ b/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
> @@ -1676,13 +1676,7 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest,
> EditSearchEngines) {
>    EXPECT_FALSE(omnibox_view->model()->popup_model()->IsOpen());
>  }
>
> -#if defined(OS_MACOSX)
> -// http://crbug.com/406012
> -#define MAYBE_BeginningShownAfterBlur DISABLED_BeginningShownAfterBlur
> -#else
> -#define MAYBE_BeginningShownAfterBlur BeginningShownAfterBlur
> -#endif
> -IN_PROC_BROWSER_TEST_F(OmniboxViewTest, MAYBE_BeginningShownAfterBlur) {
> +IN_PROC_BROWSER_TEST_F(OmniboxViewTest, BeginningShownAfterBlur) {
>    OmniboxView* omnibox_view = NULL;
>    ASSERT_NO_FATAL_FAILURE(GetOmniboxView(&omnibox_view));
>
> @@ -1690,18 +1684,18 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest,
> MAYBE_BeginningShownAfterBlur) {
>
> omnibox_view->SetWindowTextAndCaretPos(ASCIIToUTF16("data:text/plain,test"),
>        5U, false, false);
>    omnibox_view->OnAfterPossibleChange();
> -  ASSERT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
> +  EXPECT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
>    size_t start, end;
>    omnibox_view->GetSelectionBounds(&start, &end);
> -  ASSERT_EQ(5U, start);
> -  ASSERT_EQ(5U, end);
> +  EXPECT_EQ(5U, start);
> +  EXPECT_EQ(5U, end);
>
> -  ui_test_utils::ClickOnView(browser(), VIEW_ID_TAB_CONTAINER);
> -  ASSERT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
> +  ui_test_utils::FocusView(browser(), VIEW_ID_TAB_CONTAINER);
> +  EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX));
>
>    omnibox_view->GetSelectionBounds(&start, &end);
> -  ASSERT_EQ(0U, start);
> -  ASSERT_EQ(0U, end);
> +  EXPECT_EQ(0U, start);
> +  EXPECT_EQ(0U, end);
>  }
>
>  IN_PROC_BROWSER_TEST_F(OmniboxViewTest, CtrlArrowAfterArrowSuggestions) {
> Index: chrome/test/base/interactive_test_utils.h
> diff --git a/chrome/test/base/interactive_test_utils.h
> b/chrome/test/base/interactive_test_utils.h
> index
>
0779a4f862ee70467c3f352982af822ccdbbb355..e8c7a224b16fb61f8d4c12931634602e3f5f5573
> 100644
> --- a/chrome/test/base/interactive_test_utils.h
> +++ b/chrome/test/base/interactive_test_utils.h
> @@ -31,6 +31,9 @@ bool IsViewFocused(const Browser* browser, ViewID vid);
>  // Simulates a mouse click on a View in the browser.
>  void ClickOnView(const Browser* browser, ViewID vid);
>
> +// Makes focus shift to the given View without clicking it.
> +void FocusView(const Browser* browser, ViewID vid);
> +
>  // A collection of utilities that are used from interactive_ui_tests. These
> are
>  // separated from ui_test_utils.h to ensure that browser_tests don't use
> them,
>  // since they depend on focus which isn't possible for sharded test.
> Index: chrome/test/base/interactive_test_utils_mac.mm
> diff --git a/chrome/test/base/interactive_test_utils_mac.mm
> b/chrome/test/base/interactive_test_utils_mac.mm
> index
>
e2d5cdb5fa17b0210ee3a2fe54d8cd36e0522d80..488f5c6a984486d2a1ee2c21d373731dce65d1c5
> 100644
> --- a/chrome/test/base/interactive_test_utils_mac.mm
> +++ b/chrome/test/base/interactive_test_utils_mac.mm
> @@ -77,6 +77,13 @@ void ClickOnView(const Browser* browser, ViewID vid) {
>        base::MessageLoop::QuitClosure());
>    content::RunMessageLoop();
>  }
> +void FocusView(const Browser* browser, ViewID vid) {
> +   NSWindow* window = browser->window()->GetNativeWindow();
> +   DCHECK(window);
> +   NSView* view = view_id_util::GetView(window, vid);
> +   DCHECK(view);
> +   [window makeFirstResponder:view];
> + }
>
>  void HideNativeWindow(gfx::NativeWindow window) {
>    [window orderOut:nil];
> Index: chrome/test/base/interactive_test_utils_views.cc
> diff --git a/chrome/test/base/interactive_test_utils_views.cc
> b/chrome/test/base/interactive_test_utils_views.cc
> index
>
67bcf63360c9112431c9d13dee4eff76e2f3ac17..ac5047fa6fc3204231e7ce58185e7fa893490676
> 100644
> --- a/chrome/test/base/interactive_test_utils_views.cc
> +++ b/chrome/test/base/interactive_test_utils_views.cc
> @@ -43,6 +43,13 @@ void ClickOnView(const Browser* browser, ViewID vid) {
>    content::RunMessageLoop();
>  }
>
> +void FocusView(const Browser* browser, ViewID vid) {
> +  views::View* view =
> +      BrowserView::GetBrowserViewForBrowser(browser)->GetViewByID(vid);
> +  DCHECK(view);
> +  view->RequestFocus();
> +}
> +
>  #endif  // defined(OS_MACOSX)
>
>  void MoveMouseToCenterAndPress(views::View* view,
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[msw, 2014-09-03 18:21:23]

msw@chromium.org changed reviewers:
+ msw@chromium.org

[msw, 2014-09-03 18:21:24]

lgtm with a nit.

https://codereview.chromium.org/534993002/diff/50001/chrome/test/base/interac...
File chrome/test/base/interactive_test_utils_mac.mm (right):

https://codereview.chromium.org/534993002/diff/50001/chrome/test/base/interac...
chrome/test/base/interactive_test_utils_mac.mm:80: void FocusView(const Browser*
browser, ViewID vid) {
nit: blank line above.

[tapted, 2014-09-05 13:01:13]

On 2014/09/03 15:50:10, sky wrote:
> One other question (this shouldn't hold up landing). If ClickToFocus
> is unreliable on mac should we remove it?

Yeah - I think it's worth trying to phase out ClickOnView, at least for tests
that aren't actually testing mouse events. There should be more robust options.

E.g. if tests use it to `activate` a control (rather than shift focus) a more
robust option might be just to send the event to the view directly using
EventHandler::OnKeyEvent(..) or -[NSResponder interpretKeyEvents]

Drag and drop testing remains tricky, but I've refactored
ui::aura::EventGenerator into ui::test and it has a Mac port in the tree that
isn't really views-specific. The EventGenerator is robust in tests because it
simulates the event dispatch done by NSWindow to avoid these kinds of flakiness.

With that, some of the tests can probably move to browser_tests, and get
parallelized.

I'll keep an eye out for de-flaking opportunities when I bring up parts of the
UI in MacViews. A lot of tests in this file are already disabled for being flaky
(and not just flaky on Mac). So there's a lot of work here. If MacViews is
successful it might mean less to sort out in the end too :)

https://codereview.chromium.org/534993002/diff/50001/chrome/test/base/interac...
File chrome/test/base/interactive_test_utils_mac.mm (right):

https://codereview.chromium.org/534993002/diff/50001/chrome/test/base/interac...
chrome/test/base/interactive_test_utils_mac.mm:80: void FocusView(const Browser*
browser, ViewID vid) {
On 2014/09/03 18:21:24, msw wrote:
> nit: blank line above.

Done.

[tapted, 2014-09-05 13:02:11]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-09-05 13:02:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tapted@chromium.org/534993002/70001

[commit-bot: I haz the power, 2014-09-05 14:15:38]

Committed patchset #4 (id:70001) as 1e662a4c3bdee1178b8a0691772da60737e97f9f

[commit-bot: I haz the power, 2014-09-10 03:39:25]

Patchset 4 (id:??) landed as
https://crrev.com/98d6f1e45c66a03b7b583eec3694c84ff7411fec
Cr-Commit-Position: refs/heads/master@{#293516}