Initial cut at layerless windows.

Initial cut at layerless windows.

This has the minimal support working. You can create layerless
windows add/remove them from a hierarchy, add children with layers and
everything is kept in sync (including bounds).

Painting has not been wired up yet.

The interesting bit of this change is that Layer and Window bounds are
no longer in sync. This is necessitated by Layer's having to be
parented to Layers. To avoid changing bounds() to dynamically figure
out the real bounds I made Window cache the bounds.

BUG=none
TEST=none
R=ben@chromium.org

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

[Ben Goodger (Google), 2013-11-22 05:49:23]

This looks really good. See question about layer() below:

https://codereview.chromium.org/82283002/diff/30001/ui/aura/aura.gyp
File ui/aura/aura.gyp (right):

https://codereview.chromium.org/82283002/diff/30001/ui/aura/aura.gyp#newcode103
ui/aura/aura.gyp:103: 'window_layer_type.h',
looks like you forgot to add this file.

https://codereview.chromium.org/82283002/diff/30001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/82283002/diff/30001/ui/aura/window.cc#newcode955
ui/aura/window.cc:955: // We should only remove the child's layer if the child
still owns that
use of the term "child" here is confusing.

// We should only remove this window's layer from its parent if this window
still owns the layer.

https://codereview.chromium.org/82283002/diff/30001/ui/aura/window.h
File ui/aura/window.h (right):

https://codereview.chromium.org/82283002/diff/30001/ui/aura/window.h#newcode521
ui/aura/window.h:521: bool is_layerless() const { return window_layer_type_ ==
WINDOW_LAYER_NONE; }
Isn't NULL-checking layer() sufficient?

[sky, 2013-11-22 20:20:11]

https://codereview.chromium.org/82283002/diff/30001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/82283002/diff/30001/ui/aura/window.cc#newcode955
ui/aura/window.cc:955: // We should only remove the child's layer if the child
still owns that
On 2013/11/22 05:49:24, Ben Goodger (Google) wrote:
> use of the term "child" here is confusing.
> 
> // We should only remove this window's layer from its parent if this window
> still owns the layer.

Ya, sorry, I copied this comment from 858 old where it was in terms of child.
Updated.

https://codereview.chromium.org/82283002/diff/30001/ui/aura/window.h
File ui/aura/window.h (right):

https://codereview.chromium.org/82283002/diff/30001/ui/aura/window.h#newcode521
ui/aura/window.h:521: bool is_layerless() const { return window_layer_type_ ==
WINDOW_LAYER_NONE; }
On 2013/11/22 05:49:24, Ben Goodger (Google) wrote:
> Isn't NULL-checking layer() sufficient?

I was a bit worried about a comment in the destructor (that I'm removing)
mentioning a test doesn't invoke Init, thereby allowing for a NULL layer_. But,
it seems all tests pass, so I nuked window_layer_type_.

[sky, 2013-11-22 23:37:13]

One comment about this. There is a slight wrinkle with this patch. In particular
if someone invokes SuppressPaint() and changes the bounds of the window/layer,
then the window and layer become out of sync. This happens because Window keeps
Window::bounds_ in sync with the bounds of the Layer by Layer notifying the
window by way of the window being the LayerDelegate. SuppressPaint sets the
LayerDelegate to NULL, meaning Window is no longer told about bounds changes.

I don't think is really a problem though as in general we only invoke
SupressPaint prior to shut down, at which point it shouldn't matter.
Additionally there isn't really an UnsuppressPaint(), further reinforcing
SuppressPaint is a one shot type operation.

[Ben Goodger (Google), 2013-12-02 22:12:52]

lgtm, but see note below:

https://codereview.chromium.org/82283002/diff/140001/ui/aura/window.h
File ui/aura/window.h (right):

https://codereview.chromium.org/82283002/diff/140001/ui/aura/window.h#newcode521
ui/aura/window.h:521: bool is_layerless() const { return layer_ == NULL; }
do we really want this? vs. !!layer()?

if we do, I'd make this method be has_layer() and write the checks as
!has_layer()

[sky, 2013-12-02 22:32:58]

I nuked is_layerless() and went with !layer_

[commit-bot: I haz the power, 2013-12-02 22:33:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/82283002/390001

[commit-bot: I haz the power, 2013-12-03 00:46:38]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-03 01:24:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/82283002/390001

[commit-bot: I haz the power, 2013-12-03 04:17:48]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[sky, 2013-12-03 16:29:41]

Committed patchset #5 manually as r238408 (presubmit successful).

[oshima, 2014-01-10 04:08:52]

https://codereview.chromium.org/82283002/diff/390001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/82283002/diff/390001/ui/aura/window.cc#newcod...
ui/aura/window.cc:305: return false;
Scott, I just noticed that Window::IsVisible returns true even if it's detached
from Window tree (that is, GetRootWindow() == NULL) Is this expected?

[sky, 2014-01-10 16:36:19]

https://codereview.chromium.org/82283002/diff/390001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/82283002/diff/390001/ui/aura/window.cc#newcod...
ui/aura/window.cc:305: return false;
On 2014/01/10 04:08:53, oshima wrote:
> Scott, I just noticed that Window::IsVisible returns true even if it's
detached
> from Window tree (that is, GetRootWindow() == NULL) Is this expected?

I don't think this patch changed that behavior. Meaning before and after this
patch I believe you would see that behavior.

Is the behavior right? It matches the docs and I would prefer to keep it simple
like it is now. Where is this behavior causing problems?

[oshima, 2014-01-10 20:41:14]

On 2014/01/10 16:36:19, sky wrote:
> https://codereview.chromium.org/82283002/diff/390001/ui/aura/window.cc
> File ui/aura/window.cc (right):
> 
>
https://codereview.chromium.org/82283002/diff/390001/ui/aura/window.cc#newcod...
> ui/aura/window.cc:305: return false;
> On 2014/01/10 04:08:53, oshima wrote:
> > Scott, I just noticed that Window::IsVisible returns true even if it's
> detached
> > from Window tree (that is, GetRootWindow() == NULL) Is this expected?
> 
> I don't think this patch changed that behavior. Meaning before and after this
> patch I believe you would see that behavior.
> 
> Is the behavior right? It matches the docs and I would prefer to keep it
simple
> like it is now. Where is this behavior causing problems?

I found the place that expects GetRootWindow() != NULL when IsVisible() == true,
and I wanted to know which one (that code, or IsVisible()) to fix. Sounds like I
should fix that code.

Thanks.

[sky, 2014-01-10 21:17:06]

Ok, sounds good.

On Fri, Jan 10, 2014 at 12:41 PM,  <oshima@chromium.org> wrote:
> On 2014/01/10 16:36:19, sky wrote:
>>
>> https://codereview.chromium.org/82283002/diff/390001/ui/aura/window.cc
>> File ui/aura/window.cc (right):
>
>
>
>
https://codereview.chromium.org/82283002/diff/390001/ui/aura/window.cc#newcod...
>>
>> ui/aura/window.cc:305: return false;
>> On 2014/01/10 04:08:53, oshima wrote:
>> > Scott, I just noticed that Window::IsVisible returns true even if it's
>> detached
>> > from Window tree (that is, GetRootWindow() == NULL) Is this expected?
>
>
>> I don't think this patch changed that behavior. Meaning before and after
>> this
>> patch I believe you would see that behavior.
>
>
>> Is the behavior right? It matches the docs and I would prefer to keep it
>
> simple
>>
>> like it is now. Where is this behavior causing problems?
>
>
> I found the place that expects GetRootWindow() != NULL when IsVisible() ==
> true,
> and I wanted to know which one (that code, or IsVisible()) to fix. Sounds
> like I
> should fix that code.
>
> Thanks.
>
> https://codereview.chromium.org/82283002/

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