views: Some more layer change, and add/update some tests for layers.

views: Some more layer change, and add/update some tests for layers.

Hide the RootView's layer when a Widget is hidden. Call OnNativeWidgetVisibilityChanged on NativeWidgetGtk's hide/show callbacks.

The tests:
. Make sure Layer bounds change with a View even if it's not visible.
. Make sure Layer visibility is updated with a View's visibility.
. Make sure Layers get unparented properly when a View is removed from the parent view.

BUG=none
TEST=views_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=104846

[sadrul, 2011-10-08 23:21:08]

@oshima: Please review the change in native_control_gtk.cc. I tested
FindInPageControllerTest.AcceleratorRestoring locally, and it passes with this
change.

@ben/@sky: Please review ALL the changes.

http://codereview.chromium.org/8202026/diff/6001/views/controls/native_contro...
File views/controls/native_control_gtk.cc (right):

http://codereview.chromium.org/8202026/diff/6001/views/controls/native_contro...
views/controls/native_control_gtk.cc:176: return;
This change is to make sure chromium-os:20545 remains fixed (more in
http://codereview.chromium.org/7978015)

[Ben Goodger (Google), 2011-10-09 20:03:51]

Does this code get called properly from the other platforms' NativeWidgets too?

http://codereview.chromium.org/8202026/diff/6001/views/widget/widget.cc
File views/widget/widget.cc (right):

http://codereview.chromium.org/8202026/diff/6001/views/widget/widget.cc#newco...
views/widget/widget.cc:878: if (compositor && root->layer())
if (GetCompositor() && root->layer())

[sadrul, 2011-10-10 00:02:17]

NativeWidgetWin and NativeWidgetView do call OnNativeWidgetVisibilityChanged.
However, NativeWidgetAura does not. Perhaps aura::WindowDelegate should have
OnVisibilityChanged to be called from aura::Window::SetVisible?

http://codereview.chromium.org/8202026/diff/6001/views/widget/widget.cc
File views/widget/widget.cc (right):

http://codereview.chromium.org/8202026/diff/6001/views/widget/widget.cc#newco...
views/widget/widget.cc:878: if (compositor && root->layer())
On 2011/10/09 20:03:51, Ben Goodger (Google) wrote:
> if (GetCompositor() && root->layer())

Done.

[Ben Goodger (Google), 2011-10-10 01:23:07]

On Sun, Oct 9, 2011 at 5:02 PM, <sadrul@chromium.org> wrote:

> NativeWidgetWin and NativeWidgetView do call OnNativeWidgetVisibilityChange
> **d.
> However, NativeWidgetAura does not. Perhaps aura::WindowDelegate should
> have
> OnVisibilityChanged to be called from aura::Window::SetVisible?


Can you at least add a TODO for this?

LGTM otherwise.

-Ben

[sadrul, 2011-10-10 05:49:52]

On 2011/10/10 01:23:07, Ben Goodger (Google) wrote:
> On Sun, Oct 9, 2011 at 5:02 PM, <mailto:sadrul@chromium.org> wrote:
> 
> > NativeWidgetWin and NativeWidgetView do call OnNativeWidgetVisibilityChange
> > **d.
> > However, NativeWidgetAura does not. Perhaps aura::WindowDelegate should
> > have
> > OnVisibilityChanged to be called from aura::Window::SetVisible?
> 
> 
> Can you at least add a TODO for this?

I have just made the change; so now all kinds of NativeWidget triggers
OnNativeWidgetVisibilityChanged.

> LGTM otherwise.
> 
> -Ben

[Ben Goodger (Google), 2011-10-10 06:23:10]

Cool thanks.

On Sun, Oct 9, 2011 at 10:49 PM, <sadrul@chromium.org> wrote:

> On 2011/10/10 01:23:07, Ben Goodger (Google) wrote:
>
>  On Sun, Oct 9, 2011 at 5:02 PM, <mailto:sadrul@chromium.org> wrote:
>>
>
>  > NativeWidgetWin and NativeWidgetView do call
>> OnNativeWidgetVisibilityChange
>> > **d.
>>
>> > However, NativeWidgetAura does not. Perhaps aura::WindowDelegate should
>> > have
>> > OnVisibilityChanged to be called from aura::Window::SetVisible?
>>
>
>
>  Can you at least add a TODO for this?
>>
>
> I have just made the change; so now all kinds of NativeWidget triggers
> OnNativeWidgetVisibilityChange**d.
>
>  LGTM otherwise.
>>
>
>  -Ben
>>
>
>
>
>
http://codereview.chromium.**org/8202026/<http://codereview.chromium.org/8202...
>

[oshima, 2011-10-10 08:10:09]

http://codereview.chromium.org/8202026/diff/9002/views/controls/native_contro...
File views/controls/native_control_gtk.cc (right):

http://codereview.chromium.org/8202026/diff/9002/views/controls/native_contro...
views/controls/native_control_gtk.cc:176: return;
I first thought this isn't correct (because you need to hide native widget when
parent view is invisible), but looking at code more closely, the code has
changed recently and Detach no longer *detach* the widget. Honestly I don't
understand why this works. Could be that we just happen to not have such use
case because there is far less native view used in the browser, but I may be
wrong. Let me talk to dave first and will get back to you.

[sky, 2011-10-10 15:47:44]

Since Ben reviewed this one, don't wait for me.

  -Scott

On Mon, Oct 10, 2011 at 1:10 AM,  <oshima@chromium.org> wrote:
>
>
http://codereview.chromium.org/8202026/diff/9002/views/controls/native_contro...
> File views/controls/native_control_gtk.cc (right):
>
>
http://codereview.chromium.org/8202026/diff/9002/views/controls/native_contro...
> views/controls/native_control_gtk.cc:176: return;
> I first thought this isn't correct (because you need to hide native
> widget when parent view is invisible), but looking at code more closely,
> the code has changed recently and Detach no longer *detach* the widget.
> Honestly I don't understand why this works. Could be that we just happen
> to not have such use case because there is far less native view used in
> the browser, but I may be wrong. Let me talk to dave first and will get
> back to you.
>
> http://codereview.chromium.org/8202026/
>

[oshima, 2011-10-10 20:48:43]

http://codereview.chromium.org/8202026/diff/11006/views/controls/native_contr...
File views/controls/native_control_gtk.cc (right):

http://codereview.chromium.org/8202026/diff/11006/views/controls/native_contr...
views/controls/native_control_gtk.cc:176: return;
I discussed this with dave and we concluded that the correct way to fix this is
simply remove 1st if block. Since we no longer detach it's not necessary, and
Layout can show/hide native widget based on the visibility.

[sadrul, 2011-10-10 22:29:58]

http://codereview.chromium.org/8202026/diff/11006/views/controls/native_contr...
File views/controls/native_control_gtk.cc (right):

http://codereview.chromium.org/8202026/diff/11006/views/controls/native_contr...
views/controls/native_control_gtk.cc:176: return;
On 2011/10/10 20:48:43, oshima wrote:
> I discussed this with dave and we concluded that the correct way to fix this
is
> simply remove 1st if block. Since we no longer detach it's not necessary, and
> Layout can show/hide native widget based on the visibility.

Done. trybots seem happy with the change.

[Ben Goodger (Google), 2011-10-10 22:38:31]

Add a test for OnWindowVisibilityChanged?

[sadrul, 2011-10-11 00:05:33]

On 2011/10/10 22:38:31, Ben Goodger (Google) wrote:
> Add a test for OnWindowVisibilityChanged?

Updated the existing Visibility test.

[oshima, 2011-10-11 00:52:19]

cool. thank you for the change. one question.

http://codereview.chromium.org/8202026/diff/12010/ui/aura/window.cc
File ui/aura/window.cc (right):

http://codereview.chromium.org/8202026/diff/12010/ui/aura/window.cc#newcode373
ui/aura/window.cc:373: delegate_->OnWindowVisibilityChanged(is_visible);
how child layers get notifified visibility change?

[sadrul, 2011-10-11 01:03:18]

http://codereview.chromium.org/8202026/diff/12010/ui/aura/window.cc
File ui/aura/window.cc (right):

http://codereview.chromium.org/8202026/diff/12010/ui/aura/window.cc#newcode373
ui/aura/window.cc:373: delegate_->OnWindowVisibilityChanged(is_visible);
On 2011/10/11 00:52:19, oshima wrote:
> how child layers get notifified visibility change?

Child windows don't get notified of the visibility change. This is the same at
least on GTK (when a toplevel window is hidden, a child widget's "hide" callback
does not trigger).

[oshima, 2011-10-11 03:18:03]

native control bits LGTM

On 2011/10/11 01:03:18, sadrul wrote:
> http://codereview.chromium.org/8202026/diff/12010/ui/aura/window.cc
> File ui/aura/window.cc (right):
> 
> http://codereview.chromium.org/8202026/diff/12010/ui/aura/window.cc#newcode373
> ui/aura/window.cc:373: delegate_->OnWindowVisibilityChanged(is_visible);
> On 2011/10/11 00:52:19, oshima wrote:
> > how child layers get notifified visibility change?
> 
> Child windows don't get notified of the visibility change. This is the same at
> least on GTK (when a toplevel window is hidden, a child widget's "hide"
callback
> does not trigger).

My understanding is that this is equivalent to visibility notify event, and I
believe you get visibility notify on child if parent's visibility changes.
That's being said, I'm not yet familiar with layer code and this analogy may be
wrong. I'll leave this to you and ben.

[Ben Goodger (Google), 2011-10-11 03:19:55]

lgtm