Call XSetInputFocus after XRaiseWindow when activating.

Call XSetInputFocus after XRaiseWindow when activating.

XRaiseWindow does not reliably set the focus. GTK code is doing the same
thing, see gdk/x11/gdkwindow-x11.c.

BUG=None

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

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

[mlamouri (slow - plz ping), 2014-01-14 00:29:07]

Note that this solves issues I have with my inactive window patch.

[Elliot Glaysher, 2014-01-14 00:38:51]

lgtm

[commit-bot: I haz the power, 2014-01-14 01:00:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/137533006/1

[commit-bot: I haz the power, 2014-01-14 18:53:03]

Change committed as 244779

[mlamouri (slow - plz ping), 2014-01-14 23:59:04]

Unfortunately, this was not passing views_unittests [I guess it was but somehow
flaky otherwise it would not have been commited?].

The problem is that XSetInputFocus() will fail if the window that is given the
keyboard focus is not viewable [1]. Given that we do not have access to the
DesktopWindowTreeHostX11 instance from X11DesktopHandler, I had to use
XGetWindowAttributes which exposes a map_state [2].

PTAL.

[1] http://tronche.com/gui/x/xlib/input/XSetInputFocus.html
[2] http://tronche.com/gui/x/xlib/window-information/XGetWindowAttributes.html

[Elliot Glaysher, 2014-01-15 00:03:35]

Redirecting to sadrul@ since I obviously got this wrong.

[sadrul, 2014-01-15 19:23:49]

https://codereview.chromium.org/137533006/diff/290001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right):

https://codereview.chromium.org/137533006/diff/290001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_handler.cc:103: if
(attributes.map_state == IsViewable)
Is this only ever called from DesktopWindowTreeHostX11::Activate()? Can we make
sure the window is mapped in there? i.e.:

  if (!window_mapped_)
    Show();
  ...

Would that fix the issue? If it does, then I think this would be better.

[mlamouri (slow - plz ping), 2014-01-15 23:15:18]

PTAL

https://codereview.chromium.org/137533006/diff/290001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right):

https://codereview.chromium.org/137533006/diff/290001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_handler.cc:103: if
(attributes.map_state == IsViewable)
On 2014/01/15 19:23:50, sadrul wrote:
> Is this only ever called from DesktopWindowTreeHostX11::Activate()? Can we
make
> sure the window is mapped in there? i.e.:
> 
>   if (!window_mapped_)
>     Show();
>   ...
> 
> Would that fix the issue? If it does, then I think this would be better.

The assertion goes away but the test fail with that.

However, we can't do that because it is assumed that ::Activate() should not
show the widget if the widget isn't shown yet.

[sadrul, 2014-01-15 23:18:53]

https://codereview.chromium.org/137533006/diff/290001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right):

https://codereview.chromium.org/137533006/diff/290001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_handler.cc:103: if
(attributes.map_state == IsViewable)
On 2014/01/15 23:15:18, Mounir Lamouri wrote:
> On 2014/01/15 19:23:50, sadrul wrote:
> > Is this only ever called from DesktopWindowTreeHostX11::Activate()? Can we
> make
> > sure the window is mapped in there? i.e.:
> > 
> >   if (!window_mapped_)
> >     Show();
> >   ...
> > 
> > Would that fix the issue? If it does, then I think this would be better.
> 
> The assertion goes away but the test fail with that.
> 
> However, we can't do that because it is assumed that ::Activate() should not
> show the widget if the widget isn't shown yet.

Shouldn't we return early from DesktopWindowTreeHostX11::Activate() if
!window_mapped_, in that case? What does XRaiseWindow do to a hidden window?

[mlamouri (slow - plz ping), 2014-01-15 23:27:49]

https://codereview.chromium.org/137533006/diff/290001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right):

https://codereview.chromium.org/137533006/diff/290001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_handler.cc:103: if
(attributes.map_state == IsViewable)
On 2014/01/15 23:18:53, sadrul wrote:
> On 2014/01/15 23:15:18, Mounir Lamouri wrote:
> > On 2014/01/15 19:23:50, sadrul wrote:
> > > Is this only ever called from DesktopWindowTreeHostX11::Activate()? Can we
> > make
> > > sure the window is mapped in there? i.e.:
> > > 
> > >   if (!window_mapped_)
> > >     Show();
> > >   ...
> > > 
> > > Would that fix the issue? If it does, then I think this would be better.
> > 
> > The assertion goes away but the test fail with that.
> > 
> > However, we can't do that because it is assumed that ::Activate() should not
> > show the widget if the widget isn't shown yet.
> 
> Shouldn't we return early from DesktopWindowTreeHostX11::Activate() if
> !window_mapped_, in that case? What does XRaiseWindow do to a hidden window?

I think that would be reasonable but it would break
DesktopScreenX11Test.GetWindowAtScreenPoint. I believe the reason is that
DesktopScreenX11::GetWindowAtScreenPoint() go trough the windows and check their
bounds. Activating the window must change its position in the list.

If you are fine with me disabling the test, I can put an early return in
::Activate().

[sadrul, 2014-01-15 23:31:34]

https://codereview.chromium.org/137533006/diff/290001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/x11_desktop_handler.cc (right):

https://codereview.chromium.org/137533006/diff/290001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/x11_desktop_handler.cc:103: if
(attributes.map_state == IsViewable)
On 2014/01/15 23:27:50, Mounir Lamouri wrote:
> On 2014/01/15 23:18:53, sadrul wrote:
> > On 2014/01/15 23:15:18, Mounir Lamouri wrote:
> > > On 2014/01/15 19:23:50, sadrul wrote:
> > > > Is this only ever called from DesktopWindowTreeHostX11::Activate()? Can
we
> > > make
> > > > sure the window is mapped in there? i.e.:
> > > > 
> > > >   if (!window_mapped_)
> > > >     Show();
> > > >   ...
> > > > 
> > > > Would that fix the issue? If it does, then I think this would be better.
> > > 
> > > The assertion goes away but the test fail with that.
> > > 
> > > However, we can't do that because it is assumed that ::Activate() should
not
> > > show the widget if the widget isn't shown yet.
> > 
> > Shouldn't we return early from DesktopWindowTreeHostX11::Activate() if
> > !window_mapped_, in that case? What does XRaiseWindow do to a hidden window?
> 
> I think that would be reasonable but it would break
> DesktopScreenX11Test.GetWindowAtScreenPoint. I believe the reason is that
> DesktopScreenX11::GetWindowAtScreenPoint() go trough the windows and check
their
> bounds. Activating the window must change its position in the list.
> 
> If you are fine with me disabling the test, I can put an early return in
> ::Activate().

The test can be fixed by making sure the widgets created are all Show()n before
trying to activate. The test does expect the windows to be visible, otherwise
trying to get the window at a specific point in the screen doesn't make sense.

[mlamouri (slow - plz ping), 2014-01-16 02:36:39]

sadrul@, I ended up finding why the test was failing for me: creating a window
too close to the top left border is generally not a good idea because there
might be a panel or a dock there. This problem was only showing up when :Show()
is called because otherwise the window doesn't get actually mapped by X. When
mapped, there is a ConfigureNotify event that changes its position.

I believe this patch is taking into account all your comments. PTAL.

[sadrul, 2014-01-16 02:42:33]

Nice! This LGTM

Thanks for digging into the issue.

[commit-bot: I haz the power, 2014-01-16 04:08:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/137533006/380001

[commit-bot: I haz the power, 2014-01-16 17:34:03]

Change committed as 245231