Fix Z-order for a bubble when it appears for inactive window.

Fix Z-order for a bubble when it appears for an inactive window.

The bubble should be immediately above its anchor. On Windows, however, it appeared above other Chrome windows.

BUG=486730
Committed: https://crrev.com/a0a864b4582b637425675c7bee8437f841c97c86
Cr-Commit-Position: refs/heads/master@{#333464}

[vasilii, 2015-06-02 13:32:59]

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

[vasilii, 2015-06-02 15:10:26]

Hi Scott,

please review.

[sky, 2015-06-02 16:32:57]

https://codereview.chromium.org/1162883003/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1162883003/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:417: }
Is it not possible to implement this on X?

https://codereview.chromium.org/1162883003/diff/20001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1162883003/diff/20001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:537: // Otherwise, make hwnd() a top
window.
Why does this need to change? Also, are we sure other_hwnd is always non-null?

[vasilii, 2015-06-03 13:47:39]

https://codereview.chromium.org/1162883003/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1162883003/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:417: }
On 2015/06/02 16:32:57, sky wrote:
> Is it not possible to implement this on X?

The bubble isn't an X window.
I think it's doable with XRestackWindows. It can be called for siblings only.
Chrome windows aren't siblings. I see on my Goobuntu that a Chrome window has 2
parents before the root. Probably added by a window manager.

https://codereview.chromium.org/1162883003/diff/20001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1162883003/diff/20001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:537: // Otherwise, make hwnd() a top
window.
On 2015/06/02 16:32:57, sky wrote:
> Why does this need to change? Also, are we sure other_hwnd is always non-null?

The previous implementation should be called StackBelow().
We are not sure. If other_hwnd == 0 then it's a bug in the caller because it
doesn't make sense. The only caller DesktopWindowTreeHostWin::StackAbove()
checks this condition.

[vasilii, 2015-06-05 12:04:49]

Patchset #3 (id:40001) has been deleted

[vasilii, 2015-06-05 12:05:34]

I implemented DesktopWindowTreeHostX11::StackAbove.

[sky, 2015-06-05 15:27:41]

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

[sky, 2015-06-05 15:27:41]

+erg for x11 side.

https://codereview.chromium.org/1162883003/diff/60001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1162883003/diff/60001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:536: // Find a window above |other_hwnd|.
If it exists, place hwnd() behind it.
This comment just describes what the code is doing. Document why it is like that
instead of the more straightforward old code.

https://codereview.chromium.org/1162883003/diff/60001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:538: HWND next_window =
GetNextWindow(other_hwnd, GW_HWNDPREV);
DCHECK(other_hwnd)

[vasilii, 2015-06-05 15:37:26]

https://codereview.chromium.org/1162883003/diff/60001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/1162883003/diff/60001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:536: // Find a window above |other_hwnd|.
If it exists, place hwnd() behind it.
On 2015/06/05 15:27:41, sky wrote:
> This comment just describes what the code is doing. Document why it is like
that
> instead of the more straightforward old code.

Done.

https://codereview.chromium.org/1162883003/diff/60001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:538: HWND next_window =
GetNextWindow(other_hwnd, GW_HWNDPREV);
On 2015/06/05 15:27:41, sky wrote:
> DCHECK(other_hwnd)

Done.

[Elliot Glaysher, 2015-06-05 22:03:00]

erg@chromium.org changed reviewers:
+ sadrul@chromium.org

[Elliot Glaysher, 2015-06-05 22:03:01]

+sadrul

Do you know if there's a cleaner way to do the restacking here in the x11 code?

https://codereview.chromium.org/1162883003/diff/80001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1162883003/diff/80001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:141: &root_win,
&parent_win, &child_windows, &num_child_windows))
Doing a whole query tree here seems a bit unfortunate.

[vasilii, 2015-06-08 09:14:37]

I'm fine with leaving X11 implementation empty.

https://codereview.chromium.org/1162883003/diff/80001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/1162883003/diff/80001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:141: &root_win,
&parent_win, &child_windows, &num_child_windows))
On 2015/06/05 22:03:01, Elliot Glaysher wrote:
> Doing a whole query tree here seems a bit unfortunate.

I don't see another function to get a parent.

[sky, 2015-06-08 15:17:16]

LGTM for windows side.

[Elliot Glaysher, 2015-06-08 22:54:36]

lgtm. we can always pull out the x11 side if there's problems; this seems
important enough to get the win stuff in.

[vasilii, 2015-06-09 08:09:28]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2015-06-09 08:09:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162883003/80001

[commit-bot: I haz the power, 2015-06-09 08:58:05]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-06-09 08:58:59]

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