We don't want to tick animation controllers for non-active layers

We don't want to tick animation controllers for non-active layers

With this change, we will only tick animation controllers once one of their corresponding layers is active.

R=enne@chromium.org
NOTRY=true
BUG=None


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

[enne (OOO), 2012-12-20 18:03:03]

Maybe I'm misunderstanding something about this patch or the animation system,
but I think we still want to tick active animation controllers on the pending
tree, i.e. animations that were started on the main thread.

I think it's more that I was thinking that we wouldn't start animations that
need to start on the impl thread until they're on the active tree.

[Ian Vollick, 2012-12-20 18:14:17]

On 2012/12/20 18:03:03, enne wrote:
> Maybe I'm misunderstanding something about this patch or the animation system,
> but I think we still want to tick active animation controllers on the pending
> tree, i.e. animations that were started on the main thread.

Animations on the main thread always wait for their synchronized start time from
the impl thread. So although they'll be ticked, they will not advance until they
get the animation started notification sent back. We'll only generate this
notification if the animation controller is assoc'd with a layer in the active
tree. So it should should be safe to skip ticking pending-only animation
controllers (as long as their activation state wrt to the AnimationRegistrar
doesn't change -- we definitely want to keep calling
LayerAnimationController::animate even if we short circuit for a while until we
get an active layer).

> 
> I think it's more that I was thinking that we wouldn't start animations that
> need to start on the impl thread until they're on the active tree.

[enne (OOO), 2012-12-20 18:53:54]

Ok, thanks for the explanation.  This looks like a great way to only start
animations once they've been put on a tree that can be visible.

Please correct me if I'm wrong, but these controllers will still be considered
"active" by the registrar and LTHI will still get background ticks for
animation, even if they get ignored?

My only other concern is that when we swap the pending tree to the active tree,
we don't necessarily do another calcDrawProperties, if the pending tree has
already been updated.  If animations don't animate when they're on the pending
tree, then maybe it really does need an update?  On the other hand, it looks
like we call LTHI::animate just prior to drawing, so maybe this is ok.

https://codereview.chromium.org/11633044/diff/1/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/11633044/diff/1/cc/layer.h#newcode342
cc/layer.h:342: virtual bool IsActive() const OVERRIDE;
We have a lot of "active" floating around.  How do you feel about
"ShouldTickAnimations"?

[Ian Vollick, 2012-12-20 19:11:03]

On 2012/12/20 18:53:54, enne wrote:
> Ok, thanks for the explanation.  This looks like a great way to only start
> animations once they've been put on a tree that can be visible.
> 
> Please correct me if I'm wrong, but these controllers will still be considered
> "active" by the registrar and LTHI will still get background ticks for
> animation, even if they get ignored?
Yes, exactly. They'll still be in the registrar's 'active' list and will still
get bg ticks.
> 
> My only other concern is that when we swap the pending tree to the active
tree,
> we don't necessarily do another calcDrawProperties, if the pending tree has
> already been updated.  If animations don't animate when they're on the pending
> tree, then maybe it really does need an update?  On the other hand, it looks
> like we call LTHI::animate just prior to drawing, so maybe this is ok.
Yeah, LTHI::animate should dirty all the affected properties.
> 
> https://codereview.chromium.org/11633044/diff/1/cc/layer.h
> File cc/layer.h (right):
> 
> https://codereview.chromium.org/11633044/diff/1/cc/layer.h#newcode342
> cc/layer.h:342: virtual bool IsActive() const OVERRIDE;
> We have a lot of "active" floating around.  How do you feel about
> "ShouldTickAnimations"?

I'm not against this, but I'm hoping to get rid of a lot of he 'active's we have
floating around with https://codereview.chromium.org/11636051/. Do you think
'active' could be meaningful here if this other change were to land?

[enne (OOO), 2012-12-20 19:35:25]

On 2012/12/20 19:11:03, vollick wrote:
> On 2012/12/20 18:53:54, enne wrote:
> > My only other concern is that when we swap the pending tree to the active
> tree,
> > we don't necessarily do another calcDrawProperties, if the pending tree has
> > already been updated.  If animations don't animate when they're on the
pending
> > tree, then maybe it really does need an update?  On the other hand, it looks
> > like we call LTHI::animate just prior to drawing, so maybe this is ok.
> Yeah, LTHI::animate should dirty all the affected properties.

Hrm.  Actually, looking at thread proxy it looks like the order is animate ->
maybe activate trees -> calc draw properties.  It seems like animate should go
post-tree activation?

> > https://codereview.chromium.org/11633044/diff/1/cc/layer.h
> > File cc/layer.h (right):
> > 
> > https://codereview.chromium.org/11633044/diff/1/cc/layer.h#newcode342
> > cc/layer.h:342: virtual bool IsActive() const OVERRIDE;
> > We have a lot of "active" floating around.  How do you feel about
> > "ShouldTickAnimations"?
> 
> I'm not against this, but I'm hoping to get rid of a lot of he 'active's we
have
> floating around with https://codereview.chromium.org/11636051/. Do you think
> 'active' could be meaningful here if this other change were to land?

I think it's mostly that IsActive makes some sense in an observer interface, but
LayerImpl::IsActive suddenly seems ambiguous.

[nduca, 2012-12-20 20:02:46]

This whole sequencing thing makes me think Enne that we need to make activation
a real scheduler action... activate, then scheduledActionDraw/ForceDraw as usual

[enne (OOO), 2012-12-20 20:10:26]

The animate ordering issue isn't related to this patch, so we should just punt
on it and fix it later.  Maybe you could add a "TODO(nduca): make animation
happen after tree activation" in thread_proxy before LTHI::animate.

[enne (OOO), 2012-12-20 22:20:41]

At any rate, lgtm in general.  I don't have strong feelings about naming.

[commit-bot: I haz the power, 2013-01-03 02:00:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/11633044/10001

[commit-bot: I haz the power, 2013-01-07 14:30:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/11633044/10001

[commit-bot: I haz the power, 2013-01-07 14:43:40]

Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests,
unit_tests

[commit-bot: I haz the power, 2013-01-07 15:50:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/11633044/10001

[commit-bot: I haz the power, 2013-01-07 15:50:18]

Change committed as 175338