Allow inactive windows to be created with Linux Aura.

Allow inactive windows to be created with Linux Aura.

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

[mlamouri (slow - plz ping), 2013-12-10 18:04:43]

I am not sure if the method I am using is the right one but I was not able to
find anything else. Let me know if you are aware of something nicer.

[Elliot Glaysher, 2013-12-10 20:21:55]

+sadrul, piman

Adding the X-perts.

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

https://codereview.chromium.org/100623008/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:355:
XSetWMHints(xdisplay_, xwindow_, &wm_hints);
While I'm not sure if the use of .input is correct here...

https://codereview.chromium.org/100623008/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:372:
XSetWMHints(xdisplay_, xwindow_, &wm_hints);
I also suspect that we should be setting this all the time. Look at the function
update_wm_hints() in gdk/x11/gdkwindow-x11.c in the gtk source tree.

[mlamouri (slow - plz ping), 2013-12-11 14:58:47]

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

https://codereview.chromium.org/100623008/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:372:
XSetWMHints(xdisplay_, xwindow_, &wm_hints);
On 2013/12/10 20:21:55, Elliot Glaysher wrote:
> I also suspect that we should be setting this all the time. Look at the
function
> update_wm_hints() in gdk/x11/gdkwindow-x11.c in the gtk source tree.

I guess setting this on the previous call all the time would be better so the
hints are used by the WM before mapping the window. Maybe add a WindowHint about
the window being in a normal state too.

https://codereview.chromium.org/100623008/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:754:
ShowWindowWithState(ui::SHOW_STATE_NORMAL);
I realise that ::Show() is actually expecting to not activate the window. See
Widget::Show(). I will fix that.

[mlamouri (slow - plz ping), 2013-12-13 12:11:44]

ping sadrul@, piman@ and erg@. The Chrome Apps feature has landed and it will
not work on Linux Aura as long as the bug is not fixed. Could you let me know if
that approach is right. If not, which one should I take?

[Elliot Glaysher, 2013-12-13 18:48:20]

(Note that I'm not commenting because I don't know much about hints. Hence
adding the domain experts here.)

[piman, 2013-12-13 21:06:23]

On Fri, Dec 13, 2013 at 10:48 AM, <erg@chromium.org> wrote:

> (Note that I'm not commenting because I don't know much about hints. Hence
> adding the domain experts here.)
>

I really don't know either...


>
> https://codereview.chromium.org/100623008/
>

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

[sadrul, 2013-12-13 21:37:20]

I think this looks reasonable in general. I don't know about hints a lot myself.
I will take a closer look at this and gtk later tonight.

[sadrul, 2013-12-13 21:37:20]

I think this looks reasonable in general. I don't know about hints a lot myself.
I will take a closer look at this and gtk later tonight.

[Elliot Glaysher, 2013-12-13 23:32:52]

Further thoughts about this patch:

- Creating windows which become inactive immediately still seems like a weird
thing to do.

- After reading  I suspect that it would be better to lay out this patch as a
private helper function, like DesktopRootWindowHostX11::UpdateWMHints(). This
copies the structure of gdkwindow-x11.cc:update_wm_hints(), which would let us
organically grow setting the number of wm hints we handle.

- I'm unsure about the second call to XSetWMHints(); GTK+ does nothing
analogous. All calls set StateHint. I'd urge you to go read the gdk sources
here; a working X11 toolkit is better documentation then the actual
documentation. :-/

- Since you're using hints, have you tested this under multiple different window
managers? I suspect that it won't work in some places (which is probably fine;
this seems like a weird request, so I'd expect at least some window managers to
ignore it).

[benwells, 2013-12-15 05:10:56]

On 2013/12/13 23:32:52, Elliot Glaysher wrote:
> Further thoughts about this patch:
> 
> - Creating windows which become inactive immediately still seems like a weird
> thing to do.
We're doing this to support things like chat windows which can pop up
unexpectedly. We don't want them taking the focus from whatever you're doing
(e.g. typing in a password field).

> 
> - After reading  I suspect that it would be better to lay out this patch as a
> private helper function, like DesktopRootWindowHostX11::UpdateWMHints(). This
> copies the structure of gdkwindow-x11.cc:update_wm_hints(), which would let us
> organically grow setting the number of wm hints we handle.
> 
> - I'm unsure about the second call to XSetWMHints(); GTK+ does nothing
> analogous. All calls set StateHint. I'd urge you to go read the gdk sources
> here; a working X11 toolkit is better documentation then the actual
> documentation. :-/
> 
> - Since you're using hints, have you tested this under multiple different
window
> managers? I suspect that it won't work in some places (which is probably fine;
> this seems like a weird request, so I'd expect at least some window managers
to
> ignore it).

[sadrul, 2013-12-16 03:40:09]

https://codereview.chromium.org/100623008/diff/10001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right):

https://codereview.chromium.org/100623008/diff/10001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:374: if (show_state
!= ui::SHOW_STATE_INACTIVE)
Is this change sufficient to not give focus to the new window? i.e. if you don't
call SetInitialFocus(), does the new window still get focus?

If you want to raise a window without giving it focus, can you call
XRaiseWindow?

https://codereview.chromium.org/100623008/diff/10001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:753:
ShowWindowWithState(ui::SHOW_STATE_INACTIVE);
This is not right.

[mlamouri (slow - plz ping), 2013-12-16 15:26:05]

> - After reading  I suspect that it would be better to lay out this patch as a
> private helper function, like DesktopRootWindowHostX11::UpdateWMHints(). This
> copies the structure of gdkwindow-x11.cc:update_wm_hints(), which would let us
> organically grow setting the number of wm hints we handle.

> - I'm unsure about the second call to XSetWMHints(); GTK+ does nothing
> analogous. All calls set StateHint. I'd urge you to go read the gdk sources
> here; a working X11 toolkit is better documentation then the actual
> documentation. :-/

Without the second call, the window is not focusable. This is why I am not sure
that this solution is right because my understanding is that |wm_hints.input =
false;| should only be used if the window should never get the user input. I am
using that to prevent the WM to give it focus at creation. It is working but I
would not be surprised to see side effects.

> - Since you're using hints, have you tested this under multiple different
window
> managers? I suspect that it won't work in some places (which is probably fine;
> this seems like a weird request, so I'd expect at least some window managers
to
> ignore it).

If some WM refuses to create unfocused windows, I think it is fair.

https://codereview.chromium.org/100623008/diff/10001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right):

https://codereview.chromium.org/100623008/diff/10001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:374: if (show_state
!= ui::SHOW_STATE_INACTIVE)
On 2013/12/16 03:40:10, sadrul wrote:
> Is this change sufficient to not give focus to the new window? i.e. if you
don't
> call SetInitialFocus(), does the new window still get focus?
> 
> If you want to raise a window without giving it focus, can you call
> XRaiseWindow?

We still need to call XMapWindow, otherwise XRaiseWindow does not work. We could
do XMapRaised() but it seems to be exactly like XMapWindow. It might be because
of Unity. I have not tried with another WM.

https://codereview.chromium.org/100623008/diff/10001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:753:
ShowWindowWithState(ui::SHOW_STATE_INACTIVE);
On 2013/12/16 03:40:10, sadrul wrote:
> This is not right.

This is actually what Widget::Show() is expected to do.

[mlamouri (slow - plz ping), 2014-01-09 14:37:47]

This is an updated patch with ::Show() creating an activated window. What should
we do with this patch? Should we take it as-is and maybe improve things later if
needed or does someone has a clue of what should be the right way?

[mlamouri (slow - plz ping), 2014-01-12 23:35:08]

Brief summary of the issue:
We want to be able to create inactive windows in Linux Aura. This is needed
generally speaking (Widget::ShowInactive, ui::SHOW_STATE_INACTIVE need to work
as expected) but it is also needed for Chrome Apps because we want to push a new
feature that allow creating inactive Windows. Typically chat applications want
to create a new window without activating it when a new chat starts. Activating
the window meaning moving the focus which is highly disruptive for the user
workflow.

Brief summary of the patch:
The patch is telling the WM that the window is not focusable before creating it.
As soon as the window is mapped, we will tell the WM that it is now focusable.
It is generally speaking working but not very reliably. When I use it I have
sometimes odd behaviour like the window losing focus after being focused.

Do you guys know a better way to show inactive windows than the one I am using
in this patch? Otherwise, do you know how the odd behaviour I saw can be fixed?

[Matt Giuca, 2014-01-13 00:18:57]

It seems like a reasonable approach. Can you elaborate on what you mean by "it
isn't working very reliably"?

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right):

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:349: // will ignore
toplevel XMoveWindow commands.
Optional nit (I realize you didn't write this): top-level
Also, could you put an (eg, ...) and name a window manager, so this can be
tested?

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:358: wm_hints.input
= show_state != ui::SHOW_STATE_INACTIVE;
// If SHOW_STATE_INACTIVE, tell the window manager that the window is not
focusable. This will make the window inactive upon creation.

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:365: // burn if
passed |xwindow_| before the window is mapped, and XMapWindow is
Optional nit (I realize you didn't write this): Get rid of "and burn". And if
they do not actually crash, please describe what they actually do.

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:371: // confuse the
WM at that point.
What does "confuse" mean? Which hint are you referring to? The input=false hint?
Surely that isn't a matter of confusion: if you leave that as false, it won't
work at all.

Or the initial_state hint? I don't see why you remove the initial_state hint if
SHOW_STATE_INACTIVE is true, but otherwise you keep it? Is this a mistake?

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:375: wm_hints.input
= true;
// Tell the window manager that the window is now focusable.

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:379: if (show_state
!= ui::SHOW_STATE_INACTIVE)
How about "else"?

[mlamouri (slow - plz ping), 2014-01-13 00:50:43]

On 2014/01/13 00:18:57, Matt Giuca wrote:
> It seems like a reasonable approach. Can you elaborate on what you mean by "it
> isn't working very reliably"?

Sure. There are two issues:
- sometimes the window believe it is not focused while it actually is (might not
be related to the change;
- if the window is started inactive, gets activated by script then you move the
mouse inside it then move the mouse outside it, the window will get inactive
again.

You can try those by using the test app here:
https://code.google.com/p/chromium/issues/detail?id=316410#c11

[mlamouri (slow - plz ping), 2014-01-13 02:04:58]

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right):

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:349: // will ignore
toplevel XMoveWindow commands.
On 2014/01/13 00:18:57, Matt Giuca wrote:
> Optional nit (I realize you didn't write this): top-level
> Also, could you put an (eg, ...) and name a window manager, so this can be
> tested?

If I knew which WM would ignore the toplevel XMoveWindow, I would gladly :)

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:358: wm_hints.input
= show_state != ui::SHOW_STATE_INACTIVE;
On 2014/01/13 00:18:57, Matt Giuca wrote:
> // If SHOW_STATE_INACTIVE, tell the window manager that the window is not
> focusable. This will make the window inactive upon creation.

Done.

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:365: // burn if
passed |xwindow_| before the window is mapped, and XMapWindow is
On 2014/01/13 00:18:57, Matt Giuca wrote:
> Optional nit (I realize you didn't write this): Get rid of "and burn". And if
> they do not actually crash, please describe what they actually do.

I removed "and burn". I guess those other API calls assume that xwindow is
mapped.

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:371: // confuse the
WM at that point.
On 2014/01/13 00:18:57, Matt Giuca wrote:
> What does "confuse" mean? Which hint are you referring to? The input=false
hint?
> Surely that isn't a matter of confusion: if you leave that as false, it won't
> work at all.
> 
> Or the initial_state hint? I don't see why you remove the initial_state hint
if
> SHOW_STATE_INACTIVE is true, but otherwise you keep it? Is this a mistake?

Updated the comment. There is no need to set |initial_state| given that it does
not change.

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:375: wm_hints.input
= true;
On 2014/01/13 00:18:57, Matt Giuca wrote:
> // Tell the window manager that the window is now focusable.

Done.

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:379: if (show_state
!= ui::SHOW_STATE_INACTIVE)
On 2014/01/13 00:18:57, Matt Giuca wrote:
> How about "else"?

That has changed. It is being taken care of in cl 109433005.

[Matt Giuca, 2014-01-13 03:33:20]

Can you upload the new patch set?

[mlamouri (slow - plz ping), 2014-01-14 02:44:30]

On top of cl 137533006, this patch is working fine.

Could you PTAL?

[Matt Giuca, 2014-01-14 05:43:27]

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right):

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:358: wm_hints.input
= show_state != ui::SHOW_STATE_INACTIVE;
I don't see any such comment?

https://codereview.chromium.org/100623008/diff/30001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:379: if (show_state
!= ui::SHOW_STATE_INACTIVE)
As I wrote on that CL, is this one going first or second? If this one is going
first, then you should fix this now, and then do what you like in the next CL.
If this one is going second, you should base it off the other one so we can
understand what the final result will look like.

https://codereview.chromium.org/100623008/diff/120001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right):

https://codereview.chromium.org/100623008/diff/120001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:338: if
(!window_mapped_) {
Why did you revert this change? (Is it because you need the bits at the end?) If
you need the bits at the end, can you move the body of this if statement to a
separate function? The Chrome code style is to return early where possible to
avoid nesting, and I think it would help readability to not have this giant
section indented.

[mlamouri (slow - plz ping), 2014-01-15 02:50:04]

Matt, I apply all your comments. PTAL.

(Summary: added the comments that disappeared, remove the SetInitialFocus
condition and created a method for MapWindow().)

[Matt Giuca, 2014-01-15 03:46:56]

https://codereview.chromium.org/100623008/diff/170001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right):

https://codereview.chromium.org/100623008/diff/170001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:338: if
(!window_mapped_) {
Nit: No curlies.

https://codereview.chromium.org/100623008/diff/170001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:347:
native_widget_delegate_->AsWidget()->SetInitialFocus();
I thought you did not want this if SHOW_STATE_INACTIVE?

[mlamouri (slow - plz ping), 2014-01-15 04:03:03]

https://codereview.chromium.org/100623008/diff/170001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right):

https://codereview.chromium.org/100623008/diff/170001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:347:
native_widget_delegate_->AsWidget()->SetInitialFocus();
On 2014/01/15 03:46:56, Matt Giuca wrote:
> I thought you did not want this if SHOW_STATE_INACTIVE?

Let's have this dealt with in the other cl.

[Matt Giuca, 2014-01-15 04:11:24]

https://codereview.chromium.org/100623008/diff/170001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right):

https://codereview.chromium.org/100623008/diff/170001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:347:
native_widget_delegate_->AsWidget()->SetInitialFocus();
> Let's have this dealt with in the other cl.

Why? Isn't it not going to work unless you DO NOT set the focus here? Maybe I
misunderstand the point of this line?

[mlamouri (slow - plz ping), 2014-01-15 04:16:37]

https://codereview.chromium.org/100623008/diff/170001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right):

https://codereview.chromium.org/100623008/diff/170001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:347:
native_widget_delegate_->AsWidget()->SetInitialFocus();
On 2014/01/15 04:11:24, Matt Giuca wrote:
> > Let's have this dealt with in the other cl.
> 
> Why? Isn't it not going to work unless you DO NOT set the focus here? Maybe I
> misunderstand the point of this line?

SetInitialFocus() will not give the input focus to the window directly. It might
if we end up going via the RequestFocus() path and that patch triggers a call to
::Activate(). This happens with some backends. Solving this is the purpose of
other CLs. It is a common problem on Windows, Chrome OS and Linux Aura.

[Matt Giuca, 2014-01-15 04:21:58]

lgtm

https://codereview.chromium.org/100623008/diff/170001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right):

https://codereview.chromium.org/100623008/diff/170001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:347:
native_widget_delegate_->AsWidget()->SetInitialFocus();
OK.

[Elliot Glaysher, 2014-01-15 19:02:14]

lgtm

[commit-bot: I haz the power, 2014-01-16 04:26:21]

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

[commit-bot: I haz the power, 2014-01-16 20:26:42]

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

[commit-bot: I haz the power, 2014-01-16 21:45:25]

Change committed as 245338