Web Animations: Introduce ActiveAnimations and AnimationStack

Web Animations: Introduce ActiveAnimations and AnimationStack

The list of active animations has been moved into a new class,
ActiveAnimations and is now grouped by timeline, element pairs in
AnimationStack. The name 'Stack' relates to the stack of animations
which is formed for each element+property pair when animations are
composited together.

For now there is one AnimationStack formed per Timeline, in future if
inherited timelines are introduced it's likely the AnimationStack will
be inherited from any root timelines. I expect that there will only be
two root timelines, the animation timeline (the 'document timeline'),
and a special timeline for CSS Transitions (due to a different
application level in the style cascade).

A base class, Timeline, will soon be factored out of DocumentTimeline
and a new timeline for CSS Transitions will be introduced.

BUG=232273
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154733

[dstockwell, 2013-07-16 10:16:44]

shans for web animation specifics, esprehn for the move out of ElementRareData,
OWNERS and general sanity.

[shans, 2013-07-17 06:25:28]

LGTM

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
File Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
Source/core/animation/Animation.cpp:75: m_activeInAnimationStack = false;
reuse willDetach() here? Or add a detach() function?

[esprehn, 2013-07-17 06:48:01]

This looks okay, but you're adding two hash lookups and a hashtable where there
used to be none.

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
File Source/core/animation/AnimationStack.cpp (right):

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
Source/core/animation/AnimationStack.cpp:52: ASSERT_NOT_REACHED();
This should be a release assert then.

RELEASE_ASSERT(result != m_elementAnimations.end());

and get rid of this if/else

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
File Source/core/animation/AnimationStack.h (right):

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
Source/core/animation/AnimationStack.h:44: 
Extra nl

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
Source/core/animation/AnimationStack.h:46: AnimationStack() { };
Leave off default constructor

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
Source/core/animation/AnimationStack.h:50: void remove(const Element* element) {
m_elementAnimations.remove(element); }
removeAll ? Also why is this one inline but ::add isn't?

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Docu...
File Source/core/animation/DocumentTimeline.h (right):

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Docu...
Source/core/animation/DocumentTimeline.h:60: AnimationStack m_active;
m_animationStack, m_active is not very descriptive

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Time...
File Source/core/animation/TimedItem.h (right):

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Time...
Source/core/animation/TimedItem.h:77: virtual void willDetach() { };
= 0; or you plan to have ones that do nothing?

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Time...
Source/core/animation/TimedItem.h:85: ASSERT(m_player == player);
This is really weird, why store it in here if you just plan to clear it on
detach? I'd rather detach just didn't take arguments.

https://codereview.chromium.org/19266007/diff/9001/Source/core/css/resolver/S...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/19266007/diff/9001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:1059: if
(!animationStack.hasActiveAnimations(target))
This requires two hash lookups now for every element. Once for has() and again
for get() below.

[dstockwell, 2013-07-17 13:10:19]

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
File Source/core/animation/Animation.cpp (right):

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
Source/core/animation/Animation.cpp:75: m_activeInAnimationStack = false;
On 2013/07/17 06:25:28, shans wrote:
> reuse willDetach() here? Or add a detach() function?

Done.

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
File Source/core/animation/AnimationStack.cpp (right):

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
Source/core/animation/AnimationStack.cpp:52: ASSERT_NOT_REACHED();
On 2013/07/17 06:48:01, esprehn wrote:
> This should be a release assert then.
> 
> RELEASE_ASSERT(result != m_elementAnimations.end());
> 
> and get rid of this if/else

Done.

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
File Source/core/animation/AnimationStack.h (right):

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
Source/core/animation/AnimationStack.h:44: 
On 2013/07/17 06:48:01, esprehn wrote:
> Extra nl

Done.

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
Source/core/animation/AnimationStack.h:46: AnimationStack() { };
On 2013/07/17 06:48:01, esprehn wrote:
> Leave off default constructor

Done.

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Anim...
Source/core/animation/AnimationStack.h:50: void remove(const Element* element) {
m_elementAnimations.remove(element); }
On 2013/07/17 06:48:01, esprehn wrote:
> removeAll ?

Done.

> Also why is this one inline but ::add isn't?

Only because ::add is marginally non-trivial, and will likely become more
complicated.

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Docu...
File Source/core/animation/DocumentTimeline.h (right):

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Docu...
Source/core/animation/DocumentTimeline.h:60: AnimationStack m_active;
On 2013/07/17 06:48:01, esprehn wrote:
> m_animationStack, m_active is not very descriptive

Done.

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Time...
File Source/core/animation/TimedItem.h (right):

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Time...
Source/core/animation/TimedItem.h:77: virtual void willDetach() { };
On 2013/07/17 06:48:01, esprehn wrote:
> = 0; or you plan to have ones that do nothing?

Done.

https://codereview.chromium.org/19266007/diff/9001/Source/core/animation/Time...
Source/core/animation/TimedItem.h:85: ASSERT(m_player == player);
On 2013/07/17 06:48:01, esprehn wrote:
> This is really weird, why store it in here if you just plan to clear it on
> detach? I'd rather detach just didn't take arguments.

Done.

https://codereview.chromium.org/19266007/diff/9001/Source/core/css/resolver/S...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/19266007/diff/9001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:1059: if
(!animationStack.hasActiveAnimations(target))
On 2013/07/17 06:48:01, esprehn wrote:
> This requires two hash lookups now for every element. Once for has() and again
> for get() below.

I think we should still proceed with this for now. We're guarded by a runtime
flag and the AnimationStack API should give flexibility to change where/how this
data ends up being stored. There should be numerous ways we can improve on the
current situation if necessary.

Shane also has some other thoughts on how to slim down ElementRareData.

[esprehn, 2013-07-17 13:51:58]

On 2013/07/17 13:10:19, dstockwell wrote:
> ...
> 
>
https://codereview.chromium.org/19266007/diff/9001/Source/core/css/resolver/S...
> Source/core/css/resolver/StyleResolver.cpp:1059: if
> (!animationStack.hasActiveAnimations(target))
> On 2013/07/17 06:48:01, esprehn wrote:
> > This requires two hash lookups now for every element. Once for has() and
again
> > for get() below.
> 
> I think we should still proceed with this for now. We're guarded by a runtime
> flag and the AnimationStack API should give flexibility to change where/how
this
> data ends up being stored. There should be numerous ways we can improve on the
> current situation if necessary.
> 
> Shane also has some other thoughts on how to slim down ElementRareData.

Hmm, I'm a bit confused here. Why did you stop using ElementRareData and switch
to this side hash table?

[shans, 2013-07-17 21:59:55]

On 2013/07/17 13:51:58, esprehn wrote:
> On 2013/07/17 13:10:19, dstockwell wrote:
> > ...
> > 
> >
>
https://codereview.chromium.org/19266007/diff/9001/Source/core/css/resolver/S...
> > Source/core/css/resolver/StyleResolver.cpp:1059: if
> > (!animationStack.hasActiveAnimations(target))
> > On 2013/07/17 06:48:01, esprehn wrote:
> > > This requires two hash lookups now for every element. Once for has() and
> again
> > > for get() below.
> > 
> > I think we should still proceed with this for now. We're guarded by a
runtime
> > flag and the AnimationStack API should give flexibility to change where/how
> this
> > data ends up being stored. There should be numerous ways we can improve on
the
> > current situation if necessary.
> > 
> > Shane also has some other thoughts on how to slim down ElementRareData.
> 
> Hmm, I'm a bit confused here. Why did you stop using ElementRareData and
switch
> to this side hash table?

esprehn and I discussed this on chat. He's a lot more concerned with the
performance cost of hashing than the memory cost of ERD.

We decided to keep the Animation data in ERD, but introduce a new aggregator
(ElementRareAnimationData?) to keep the impact on ERD size to a minimum.

[dstockwell, 2013-07-18 03:40:42]

On 2013/07/17 21:59:55, shans wrote:
> On 2013/07/17 13:51:58, esprehn wrote:
> > On 2013/07/17 13:10:19, dstockwell wrote:
> > > ...
> > > 
> > >
> >
>
https://codereview.chromium.org/19266007/diff/9001/Source/core/css/resolver/S...
> > > Source/core/css/resolver/StyleResolver.cpp:1059: if
> > > (!animationStack.hasActiveAnimations(target))
> > > On 2013/07/17 06:48:01, esprehn wrote:
> > > > This requires two hash lookups now for every element. Once for has() and
> > again
> > > > for get() below.
> > > 
> > > I think we should still proceed with this for now. We're guarded by a
> runtime
> > > flag and the AnimationStack API should give flexibility to change
where/how
> > this
> > > data ends up being stored. There should be numerous ways we can improve on
> the
> > > current situation if necessary.
> > > 
> > > Shane also has some other thoughts on how to slim down ElementRareData.
> > 
> > Hmm, I'm a bit confused here. Why did you stop using ElementRareData and
> switch
> > to this side hash table?
> 
> esprehn and I discussed this on chat. He's a lot more concerned with the
> performance cost of hashing than the memory cost of ERD.
> 
> We decided to keep the Animation data in ERD, but introduce a new aggregator
> (ElementRareAnimationData?) to keep the impact on ERD size to a minimum.

PTAL

Moved back to ElementRareData, but called the new class ActiveAnimations.

[dstockwell, 2013-07-23 01:13:27]

On 2013/07/18 03:40:42, dstockwell wrote:
> On 2013/07/17 21:59:55, shans wrote:
> > On 2013/07/17 13:51:58, esprehn wrote:
> > > On 2013/07/17 13:10:19, dstockwell wrote:
> > > > ...
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/19266007/diff/9001/Source/core/css/resolver/S...
> > > > Source/core/css/resolver/StyleResolver.cpp:1059: if
> > > > (!animationStack.hasActiveAnimations(target))
> > > > On 2013/07/17 06:48:01, esprehn wrote:
> > > > > This requires two hash lookups now for every element. Once for has()
and
> > > again
> > > > > for get() below.
> > > > 
> > > > I think we should still proceed with this for now. We're guarded by a
> > runtime
> > > > flag and the AnimationStack API should give flexibility to change
> where/how
> > > this
> > > > data ends up being stored. There should be numerous ways we can improve
on
> > the
> > > > current situation if necessary.
> > > > 
> > > > Shane also has some other thoughts on how to slim down ElementRareData.
> > > 
> > > Hmm, I'm a bit confused here. Why did you stop using ElementRareData and
> > switch
> > > to this side hash table?
> > 
> > esprehn and I discussed this on chat. He's a lot more concerned with the
> > performance cost of hashing than the memory cost of ERD.
> > 
> > We decided to keep the Animation data in ERD, but introduce a new aggregator
> > (ElementRareAnimationData?) to keep the impact on ERD size to a minimum.
> 
> PTAL
> 
> Moved back to ElementRareData, but called the new class ActiveAnimations.

Ping

[esprehn, 2013-07-23 01:20:37]

LGTM. Can you remove that extra method that doesn't touch any instance vars
though?

https://codereview.chromium.org/19266007/diff/37001/Source/core/animation/Doc...
File Source/core/animation/DocumentTimeline.h (right):

https://codereview.chromium.org/19266007/diff/37001/Source/core/animation/Doc...
Source/core/animation/DocumentTimeline.h:59: return
element->activeAnimations()->defaultStack();
if (ActiveAnimations* animations = element->activeAnimations())
  return animations->defaultStack();
return 0;

That pattern is good because if you make activeAnimations more expensive in the
future (ex. hash maps) we don't call it all over.

https://codereview.chromium.org/19266007/diff/37001/Source/core/animation/Doc...
Source/core/animation/DocumentTimeline.h:61: AnimationStack*
ensureAnimationStack(Element* element) { return
element->ensureActiveAnimations()->defaultStack(); }
Remove this method, just call this directly or put an inline method in your .cpp
file. This certainly doesn't need to be an instance method. :)

[dstockwell, 2013-07-23 01:33:00]

https://codereview.chromium.org/19266007/diff/37001/Source/core/animation/Doc...
File Source/core/animation/DocumentTimeline.h (right):

https://codereview.chromium.org/19266007/diff/37001/Source/core/animation/Doc...
Source/core/animation/DocumentTimeline.h:59: return
element->activeAnimations()->defaultStack();
On 2013/07/23 01:20:38, esprehn wrote:
> if (ActiveAnimations* animations = element->activeAnimations())
>   return animations->defaultStack();
> return 0;
> 
> That pattern is good because if you make activeAnimations more expensive in
the
> future (ex. hash maps) we don't call it all over.

Done.

https://codereview.chromium.org/19266007/diff/37001/Source/core/animation/Doc...
Source/core/animation/DocumentTimeline.h:61: AnimationStack*
ensureAnimationStack(Element* element) { return
element->ensureActiveAnimations()->defaultStack(); }
On 2013/07/23 01:20:38, esprehn wrote:
> Remove this method, just call this directly or put an inline method in your
.cpp
> file. This certainly doesn't need to be an instance method. :)

Removed.

The plan was for this to become virtual once there were multiple timelines bound
to different stacks inside ActiveAnimations. But I'll add it back at that point
if necessary.

[commit-bot: I haz the power, 2013-07-23 01:48:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/19266007/52001

[commit-bot: I haz the power, 2013-07-23 04:52:47]

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

[commit-bot: I haz the power, 2013-07-23 05:11:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/19266007/22002

[commit-bot: I haz the power, 2013-07-23 06:54:15]

Change committed as 154733

[cbiesinger, 2013-07-23 22:07:22]

On 2013/07/23 06:54:15, I haz the power (commit-bot) wrote:
> Change committed as 154733

So this patch almost certainly made TimedItem.Sanity very very flaky on Mac dbg:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

If you could fix that, that'd be great... or mark it as flaky.

[dstockwell, 2013-07-24 00:27:10]

On 2013/07/23 22:07:22, cbiesinger wrote:
> On 2013/07/23 06:54:15, I haz the power (commit-bot) wrote:
> > Change committed as 154733
> 
> So this patch almost certainly made TimedItem.Sanity very very flaky on Mac
dbg:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...
> 
> If you could fix that, that'd be great... or mark it as flaky.

Fixing now.