Handle ui::Layer's visibility using cc::Layer's m_isDrawable flag

Handle ui::Layer's visibility using cc::Layer's m_isDrawable flag

Currently, ui::Layers make themselves invisible by setting the opacity
of their corresponding cc::Layer to 0. This is problematic when we
want to animate opacity on the compositor thread but we want to still
allow visibility to concurrently be animated on the main thread. This
CL makes ui::Layers handle visibility changes by recursively setting/
unsetting the m_isDrawable flag of all cc::Layers in the subtree rooted
at the layer whose visibility changed. 

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

[piman, 2013-01-30 21:38:50]

I like the approach much better.

https://chromiumcodereview.appspot.com/12093067/diff/1/cc/layer.h
File cc/layer.h (right):

https://chromiumcodereview.appspot.com/12093067/diff/1/cc/layer.h#newcode202
cc/layer.h:202: void setVisible(bool);
Nit: we may want to name that as "setSubtreeVisible" or something that carries
the fact that this applies to this layer and its entire subtree, not just this
layer.

[enne (OOO), 2013-01-30 21:47:56]

I like the other approach way better and really would rather not add yet another
flag on layers.

Is adding a layer such overhead that you're advocating for not using the
primitives that already exist?

[piman, 2013-01-30 22:04:31]

On Wed, Jan 30, 2013 at 1:47 PM, <enne@chromium.org> wrote:

> I like the other approach way better and really would rather not add yet
> another
> flag on layers.
>
> Is adding a layer such overhead that you're advocating for not using the
> primitives that already exist?
>

Any time you change the tree structure, it causes texture layers (and most
likely, delegated renderer layers) to synchronize with the impl thread.
It's virtually equivalent to just removing from the tree.
Also doubling the number of layers doesn't seem very appealing.



>
https://chromiumcodereview.**appspot.com/12093067/<https://chromiumcodereview...
>

[enne (OOO), 2013-01-30 22:12:50]

Ok, so it's a performance optimization.  Are you planning to potentially animate
this property on everything?  I had suggested an extra layer that I was assuming
you could set up ahead of time (avoiding changing tree structure) and wouldn't
have to be done for every layer.

[piman, 2013-01-30 22:24:37]

On Wed, Jan 30, 2013 at 2:12 PM, <enne@chromium.org> wrote:

> Ok, so it's a performance optimization.  Are you planning to potentially
> animate
> this property on everything?  I had suggested an extra layer that I was
> assuming
> you could set up ahead of time (avoiding changing tree structure) and
> wouldn't
> have to be done for every layer.
>

At the ui/compositor level, it's not known ahead of time. I don't know at
the UI level if that info is available today, and it seems particularly
constraining for UI folks to have to specify that (AKA they'll just set the
"maybe we'll animate opacity on this layer" flag everywhere, "just in
case"). It doesn't seem like the sort of complexity that should leak to the
user.


>
https://chromiumcodereview.**appspot.com/12093067/<https://chromiumcodereview...
>

[enne (OOO), 2013-01-30 22:56:22]

Thanks for the extra explanation.  I wasn't aware that the use case was
"generally use this on any ui::Layer" rather than "here's one thing we want to
animate".  I still am really reluctant to add more flags to cc::Layer, because
it's a dumping ground for every effect.

If you want to animate opacity in two different ways, can you have an animation
curve that combines two other curves by multiplying them together?  Or adding
some visibility flag on your float animation curve adapter?

Alternatively, can you use the existing m_isDrawable flag and just set/unset it
recursively?

[piman, 2013-01-30 23:02:00]

On Wed, Jan 30, 2013 at 2:56 PM, <enne@chromium.org> wrote:

> Thanks for the extra explanation.  I wasn't aware that the use case was
> "generally use this on any ui::Layer" rather than "here's one thing we
> want to
> animate".  I still am really reluctant to add more flags to cc::Layer,
> because
> it's a dumping ground for every effect.
>
> If you want to animate opacity in two different ways, can you have an
> animation
> curve that combines two other curves by multiplying them together?  Or
> adding
> some visibility flag on your float animation curve adapter?
>
> Alternatively, can you use the existing m_isDrawable flag and just
> set/unset it
> recursively?
>

I'm ok with either option.


>
>
https://chromiumcodereview.**appspot.com/12093067/<https://chromiumcodereview...
>

[ajuma, 2013-01-31 16:25:18]

On 2013/01/30 22:56:22, enne wrote:
> Alternatively, can you use the existing m_isDrawable flag and just set/unset
it
> recursively?

I've updated the patch to use this approach. PTAL.

[piman, 2013-01-31 23:21:59]

The approach looks good. A couple of things.

https://chromiumcodereview.appspot.com/12093067/diff/11001/ui/compositor/laye...
File ui/compositor/layer.cc (right):

https://chromiumcodereview.appspot.com/12093067/diff/11001/ui/compositor/laye...
ui/compositor/layer.cc:137: child->OnParentIsDrawnChanged(IsDrawn(), true);
If we are going to propagate the information down, could we then store it and
make IsDrawn() non recursive?

https://chromiumcodereview.appspot.com/12093067/diff/11001/ui/compositor/laye...
ui/compositor/layer.cc:678: children_[i]->OnParentIsDrawnChanged(visible_,
false);
Can we share that code with OnParentIsDrawnChanged? In particular the logic for
what we pass to setIsDrawable would benefit from being specified only once.

[ajuma, 2013-02-01 16:04:06]

https://chromiumcodereview.appspot.com/12093067/diff/11001/ui/compositor/laye...
File ui/compositor/layer.cc (right):

https://chromiumcodereview.appspot.com/12093067/diff/11001/ui/compositor/laye...
ui/compositor/layer.cc:137: child->OnParentIsDrawnChanged(IsDrawn(), true);
On 2013/01/31 23:21:59, piman wrote:
> If we are going to propagate the information down, could we then store it and
> make IsDrawn() non recursive?

Done.

https://chromiumcodereview.appspot.com/12093067/diff/11001/ui/compositor/laye...
ui/compositor/layer.cc:678: children_[i]->OnParentIsDrawnChanged(visible_,
false);
On 2013/01/31 23:21:59, piman wrote:
> Can we share that code with OnParentIsDrawnChanged? In particular the logic
for
> what we pass to setIsDrawable would benefit from being specified only once.

Done. The logic is now specified only in UpdateIsDrawn.

[piman, 2013-02-01 18:06:55]

LGTM + nits. Thanks for the work, this looks great now.

https://chromiumcodereview.appspot.com/12093067/diff/10004/ui/compositor/laye...
File ui/compositor/layer.cc (right):

https://chromiumcodereview.appspot.com/12093067/diff/10004/ui/compositor/laye...
ui/compositor/layer.cc:659: bool schedule_draw = (opacity != opacity_ &&
IsDrawn());
nit: I'm pretty sure you can remove that logic now. Setting the opacity should
schedule a draw.

https://chromiumcodereview.appspot.com/12093067/diff/10004/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://chromiumcodereview.appspot.com/12093067/diff/10004/ui/compositor/laye...
ui/compositor/layer.h:363: bool is_drawn_;
nit: can you just add comment that explains that this is the computed value
gathered from the ancestors?

[ajuma, 2013-02-01 18:40:23]

https://chromiumcodereview.appspot.com/12093067/diff/10004/ui/compositor/laye...
File ui/compositor/layer.cc (right):

https://chromiumcodereview.appspot.com/12093067/diff/10004/ui/compositor/laye...
ui/compositor/layer.cc:659: bool schedule_draw = (opacity != opacity_ &&
IsDrawn());
On 2013/02/01 18:06:55, piman wrote:
> nit: I'm pretty sure you can remove that logic now. Setting the opacity should
> schedule a draw.

Done.

https://chromiumcodereview.appspot.com/12093067/diff/10004/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://chromiumcodereview.appspot.com/12093067/diff/10004/ui/compositor/laye...
ui/compositor/layer.h:363: bool is_drawn_;
On 2013/02/01 18:06:55, piman wrote:
> nit: can you just add comment that explains that this is the computed value
> gathered from the ancestors?

Done.

[commit-bot: I haz the power, 2013-02-01 18:46:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/12093067/3007

[commit-bot: I haz the power, 2013-02-02 15:52:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/12093067/3007

[commit-bot: I haz the power, 2013-02-02 19:06:22]

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

[commit-bot: I haz the power, 2013-02-02 19:10:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/12093067/3007

[commit-bot: I haz the power, 2013-02-02 22:45:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/12093067/3007

[commit-bot: I haz the power, 2013-02-03 02:23:56]

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

[commit-bot: I haz the power, 2013-02-03 15:14:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajuma@chromium.org/12093067/3007

[commit-bot: I haz the power, 2013-02-03 15:33:13]

Change committed as 180310