Ensure that the AURA focused window is set correctly when the window is activated.

Ensure that the AURA focused window is set correctly when the window is activated.

Currently this only works if the window is activated with the mouse. If we Alt Tab to the window then
the focus window is not restored in AURA. The view is correctly focused though.

Fixes as below:-
1. Added a new function HandleActivationChanged to the DesktopNativeWidgetAura class. This is invoked from
   the DesktopRootWindowHostWin::HandleActivationChanged and DesktopRootWindowHostX11::HandleNativeWidgetActivationChanged
   functions.

2. Most of the common code in those functions now lives in the DesktopNativeWidgetAura::HandleActivationChanged function.
   This includes deactivating the window in the aura activation client and now additionally activating the window when
   we are being activated.

Added an interactive ui test DesktopNativeWidgetAuraActivationAndFocusTest for Windows AURA.

BUG=278000
TEST=Interactive ui test DesktopNativeWidgetAuraActivationAndFocusTest
R=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221225

[dhw, 2013-09-01 00:17:25]

Please update BUG=278000

and test for keyboard shortcuts, arrow keys, and backspace

[sky, 2013-09-03 18:21:51]

https://codereview.chromium.org/23702017/diff/1/ui/views/widget/desktop_aura/...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/23702017/diff/1/ui/views/widget/desktop_aura/...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:248:
focus_view->GetWidget()->GetNativeView());
Shouldn't we activate something if we can't find a view? Otherwise nothing is
going to work, right?

https://codereview.chromium.org/23702017/diff/1/ui/views/widget/widget_unitte...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/23702017/diff/1/ui/views/widget/widget_unitte...
ui/views/widget/widget_unittest.cc:2387: #if defined(OS_WIN) &&
defined(USE_AURA)
Since this changes focus it needs to be an interactive ui test.

[ananta, 2013-09-03 19:03:31]

https://codereview.chromium.org/23702017/diff/1/ui/views/widget/desktop_aura/...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/23702017/diff/1/ui/views/widget/desktop_aura/...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:248:
focus_view->GetWidget()->GetNativeView());
On 2013/09/03 18:21:51, sky wrote:
> Shouldn't we activate something if we can't find a view? Otherwise nothing is
> going to work, right?

Added code to activate the root view, if we don't find a reasonable view to
activate.

https://codereview.chromium.org/23702017/diff/1/ui/views/widget/widget_unitte...
File ui/views/widget/widget_unittest.cc (right):

https://codereview.chromium.org/23702017/diff/1/ui/views/widget/widget_unitte...
ui/views/widget/widget_unittest.cc:2387: #if defined(OS_WIN) &&
defined(USE_AURA)
On 2013/09/03 18:21:51, sky wrote:
> Since this changes focus it needs to be an interactive ui test.

As per our discussion, focus is a thread specific state. The SetForegroundWindow
call below could mess up other tests though which may run simultaneously on the
machine. Replaced that with SetActiveWindow, which changes the active window
which is also a thread specific state.

[sky, 2013-09-03 20:02:27]

On Tue, Sep 3, 2013 at 12:03 PM,  <ananta@chromium.org> wrote:
>
>
https://codereview.chromium.org/23702017/diff/1/ui/views/widget/desktop_aura/...
> File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):
>
>
https://codereview.chromium.org/23702017/diff/1/ui/views/widget/desktop_aura/...
> ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:248:
> focus_view->GetWidget()->GetNativeView());
> On 2013/09/03 18:21:51, sky wrote:
>>
>> Shouldn't we activate something if we can't find a view? Otherwise
>
> nothing is
>>
>> going to work, right?
>
>
> Added code to activate the root view, if we don't find a reasonable view
> to activate.
>
>
>
https://codereview.chromium.org/23702017/diff/1/ui/views/widget/widget_unitte...
> File ui/views/widget/widget_unittest.cc (right):
>
>
https://codereview.chromium.org/23702017/diff/1/ui/views/widget/widget_unitte...
> ui/views/widget/widget_unittest.cc:2387: #if defined(OS_WIN) &&
> defined(USE_AURA)
> On 2013/09/03 18:21:51, sky wrote:
>>
>> Since this changes focus it needs to be an interactive ui test.
>
>
> As per our discussion, focus is a thread specific state. The
> SetForegroundWindow call below could mess up other tests though which
> may run simultaneously on the machine. Replaced that with
> SetActiveWindow, which changes the active window which is also a thread
> specific state.

Lets say this test is running with another test that is doing the
same, meaning invoking SetActiveWindow, might they conflict? What if
the test isn't in the foreground?

  -Scott
>
> https://codereview.chromium.org/23702017/

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

[ananta, 2013-09-03 20:04:24]

They won't conflict unless that test is executing on the same thread as this
one.
Active window is a thread specific state and won't mess up by another thread
calling SetActiveWindow.

On 2013/09/03 20:02:27, sky wrote:
> On Tue, Sep 3, 2013 at 12:03 PM,  <mailto:ananta@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/23702017/diff/1/ui/views/widget/desktop_aura/...
> > File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):
> >
> >
>
https://codereview.chromium.org/23702017/diff/1/ui/views/widget/desktop_aura/...
> > ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:248:
> > focus_view->GetWidget()->GetNativeView());
> > On 2013/09/03 18:21:51, sky wrote:
> >>
> >> Shouldn't we activate something if we can't find a view? Otherwise
> >
> > nothing is
> >>
> >> going to work, right?
> >
> >
> > Added code to activate the root view, if we don't find a reasonable view
> > to activate.
> >
> >
> >
>
https://codereview.chromium.org/23702017/diff/1/ui/views/widget/widget_unitte...
> > File ui/views/widget/widget_unittest.cc (right):
> >
> >
>
https://codereview.chromium.org/23702017/diff/1/ui/views/widget/widget_unitte...
> > ui/views/widget/widget_unittest.cc:2387: #if defined(OS_WIN) &&
> > defined(USE_AURA)
> > On 2013/09/03 18:21:51, sky wrote:
> >>
> >> Since this changes focus it needs to be an interactive ui test.
> >
> >
> > As per our discussion, focus is a thread specific state. The
> > SetForegroundWindow call below could mess up other tests though which
> > may run simultaneously on the machine. Replaced that with
> > SetActiveWindow, which changes the active window which is also a thread
> > specific state.
> 
> Lets say this test is running with another test that is doing the
> same, meaning invoking SetActiveWindow, might they conflict? What if
> the test isn't in the foreground?
> 
>   -Scott
> >
> > https://codereview.chromium.org/23702017/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2013-09-03 20:50:00]

I'm probably missing something, but won't setactivewindow do nothing if chrome
isn't in the foreground?

[ananta, 2013-09-03 22:06:08]

On 2013/09/03 20:50:00, sky wrote:
> I'm probably missing something, but won't setactivewindow do nothing if chrome
> isn't in the foreground?

The documentation is misleading on that sadly. It generates a
WM_SETFOCUS/WM_KILLFOCUS combo even when it
is in the background. WM_ACTIVATE is sent intermittently.

In any case, your point about the test failing in the background is correct
because we call HandleActivationChanged
in the WM_NCACTIVATE handler which is not invoked if the thread is in the
background.

We should additionally call HandleActivationChanged from
DesktopRootWindowHostWin::HandleNativeFocus and
DesktopRootWindowHostWin::HandleNativeFocus with true and false respectively,
which works well.

PTAL

[ananta, 2013-09-03 23:39:19]

On 2013/09/03 22:06:08, ananta wrote:
> On 2013/09/03 20:50:00, sky wrote:
> > I'm probably missing something, but won't setactivewindow do nothing if
chrome
> > isn't in the foreground?
> 
> The documentation is misleading on that sadly. It generates a
> WM_SETFOCUS/WM_KILLFOCUS combo even when it
> is in the background. WM_ACTIVATE is sent intermittently.
> 
> In any case, your point about the test failing in the background is correct
> because we call HandleActivationChanged
> in the WM_NCACTIVATE handler which is not invoked if the thread is in the
> background.
> 
> We should additionally call HandleActivationChanged from
> DesktopRootWindowHostWin::HandleNativeFocus and
> DesktopRootWindowHostWin::HandleNativeFocus with true and false respectively,
> which works well.
> 
> PTAL

As per our discussion the DesktopNativeWidgetAuraActivationAndFocusTest is now
an interactive ui test
which lives in ui\views\widget\interactive_widget_tests.cc

PTAL

[sky, 2013-09-03 23:40:34]

LGTM

https://codereview.chromium.org/23702017/diff/46001/ui/views/widget/interacti...
File ui/views/widget/interactive_widget_tests.cc (right):

https://codereview.chromium.org/23702017/diff/46001/ui/views/widget/interacti...
ui/views/widget/interactive_widget_tests.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
Name this file widget_interactive_uitest.

[ananta, 2013-09-03 23:47:46]

https://codereview.chromium.org/23702017/diff/46001/ui/views/widget/interacti...
File ui/views/widget/interactive_widget_tests.cc (right):

https://codereview.chromium.org/23702017/diff/46001/ui/views/widget/interacti...
ui/views/widget/interactive_widget_tests.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/09/03 23:40:34, sky wrote:
> Name this file widget_interactive_uitest.

Done.

[ananta, 2013-09-04 17:48:07]

Committed patchset #7 manually as r221225 (presubmit successful).