visibility bug fix for https://crbug.com/246844

visibility bug fix for 246844 on Linux machines

BUG=246844
It fixes the the bug on Linux machines by propagating the window minimization
information to the content window after receiving X event from the platform, so
the render side can update its visibility properly.

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

[Zhen Wang, 2014-06-19 15:53:02]

Hi Elliot, Peter and Tony,

I made some changes to fix the visibility bug (246844)) on Linux machines. The
changes are related to handling X event from the platform. It propagates the
browser minimization information to the content window, so the render side can
update its visibility properly.

Best
-Zhen

[Elliot Glaysher, 2014-06-19 18:11:39]

So, I don't see desktop_window_tree_host_win.cc doing anything analogous to
this. How does this work on Windows?

[Elliot Glaysher, 2014-06-19 20:09:57]

https://codereview.chromium.org/340783004/diff/60001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/340783004/diff/60001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1193: // minimized
or shown from minimized state.
If you go this route, please also mention that on Windows, this works by calling
OnHostResized() with an empty size. In particular,
HWNDMessageHandler::GetClientAreaBounds() returns an empty size when the window
is minimized.

If you instead just insert "if (IsMinimized()) return gfx::Rect();" into
DWTHX11::GetClientAreaBounds() like DWTHWin does, does visibility work?

(pkotwicz@ mentioned that DRWHWin::IsVisible() also returns true when minimized,
so the X11 equivalent here should be fine...)

https://codereview.chromium.org/340783004/diff/60001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1194: is_minimized
= IsMinimized();
Move the declaration of bool is_minimized here, since this is the first place
it's used.

[Zhen Wang, 2014-06-19 22:40:48]

Hi Elliot,

I have updated the patch with your suggestions. I will also reply to your
comments with our discussion, so others will be able to see.

Best
-Zhen

[Zhen Wang, 2014-06-19 22:45:02]

I just put in the notes. I should have combined this email with the previous
one. Stilling getting used to the code review system. ;-)

Best
-Zhen

https://codereview.chromium.org/340783004/diff/60001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/340783004/diff/60001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1193: // minimized
or shown from minimized state.
This is just a note. I have discussed with Elliot and tried the Linux version of
returning empty size. It does not work. So for now, we will just use this way to
update visibility accordingly.

On 2014/06/19 20:09:56, Elliot Glaysher wrote:
> If you go this route, please also mention that on Windows, this works by
calling
> OnHostResized() with an empty size. In particular,
> HWNDMessageHandler::GetClientAreaBounds() returns an empty size when the
window
> is minimized.
> 
> If you instead just insert "if (IsMinimized()) return gfx::Rect();" into
> DWTHX11::GetClientAreaBounds() like DWTHWin does, does visibility work?
> 
> (pkotwicz@ mentioned that DRWHWin::IsVisible() also returns true when
minimized,
> so the X11 equivalent here should be fine...)

[Elliot Glaysher, 2014-06-19 22:47:10]

lgtm++

https://codereview.chromium.org/340783004/diff/80001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc
(right):

https://codereview.chromium.org/340783004/diff/80001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11_unittest.cc:391:
ui::X11AtomCache atom_cache(display, kAtomsToCache);
I believe you can safely share this between these blocks; the atom cache should
be immutable.

[pkotwicz, 2014-06-19 22:50:36]

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

https://codereview.chromium.org/340783004/diff/80001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1202: }
I think a structure like this would be slightly nicer?

if (is_minimized != was_minimized) {
  if (is_minimized)
    content_window_->Hide();
  else
    content_window_->Show();
}

[Zhen Wang, 2014-06-19 23:02:24]

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

https://codereview.chromium.org/340783004/diff/80001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1202: }
Indeed. Updated.

On 2014/06/19 22:50:36, pkotwicz wrote:
> I think a structure like this would be slightly nicer?
> 
> if (is_minimized != was_minimized) {
>   if (is_minimized)
>     content_window_->Hide();
>   else
>     content_window_->Show();
> }

[pkotwicz, 2014-06-20 15:27:38]

LGTM

[Zhen Wang, 2014-06-20 15:48:30]

The CQ bit was checked by zhenw@chromium.org

[commit-bot: I haz the power, 2014-06-20 15:49:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zhenw@chromium.org/340783004/100001

[commit-bot: I haz the power, 2014-06-20 17:39:45]

Change committed as 278754