Bug 208079 – AnimationTimeline should not have multiple HashMaps with raw Element* keys

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Simon Fraser (smfr)]

We need to find a different solution to the 6 HashMaps that AnimationTimeline has. We have to consult each of them every time we remove an element.

[Radar WebKit Bug Importer]

<rdar://problem/59687906>

[Ryosuke Niwa]

We should probably move these data structures to hang off of ElementRareData. Create something like ElementAnimationRareData and then put it on ElementRareData.

[Antoine Quint]

I like Ryosuke's suggestion and will implement that.

[Antoine Quint]

Should also make KeyframeEffectStack hang off that instead of ElementRareData directly.

[Ryosuke Niwa]

(In reply to Antoine Quint from comment #4)
> Should also make KeyframeEffectStack hang off that instead of
> ElementRareData directly.

That would make most sense.

[Antoine Quint]

Created attachment 393217 [details]
Patch

[Antoine Quint]

Created attachment 393219 [details]
Patch

[Antoine Quint]

Created attachment 393231 [details]
Patch

[Antoine Quint]

Created attachment 393233 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 393233 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393233&action=review

> Source/WebCore/dom/NodeRareData.h:262
> +        Animations = 1 << 14,

I think you missed the #if DUMP_NODE_STATISTICS code that uses this enum value.

[Antoine Quint]

Created attachment 393252 [details]
Patch

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 393233 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393233&action=review
> 
> > Source/WebCore/dom/NodeRareData.h:262
> > +        Animations = 1 << 14,
> 
> I think you missed the #if DUMP_NODE_STATISTICS code that uses this enum
> value.

I did! I added the required code in the newest patch.

[Ryosuke Niwa]

Comment on attachment 393252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393252&action=review

> Source/WebCore/animation/AnimationTimeline.cpp:105
> +    if (is<CSSTransition>(animation) && downcast<CSSTransition>(animation).owningElement())
> +        element.transitions().add(&animation);

Should we check that the owning element is same as element here??

> Source/WebCore/animation/AnimationTimeline.cpp:107
> +    else if (is<CSSAnimation>(animation) && downcast<CSSAnimation>(animation).owningElement())
> +        element.cssAnimations().add(&animation);

Ditto.

> Source/WebCore/animation/ElementAnimationRareData.h:38
> +    WTF_MAKE_FAST_ALLOCATED;

Make this not copyable?

> Source/WebCore/dom/Element.h:496
> +    AnimationCollection& webAnimations();

Should we also forward declare AnimationCollection & CSSAnimationCollection?

[Antoine Quint]

(In reply to Ryosuke Niwa from comment #13)
> Comment on attachment 393252 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393252&action=review
> 
> > Source/WebCore/animation/AnimationTimeline.cpp:105
> > +    if (is<CSSTransition>(animation) && downcast<CSSTransition>(animation).owningElement())
> > +        element.transitions().add(&animation);
> 
> Should we check that the owning element is same as element here??

These should always be the same thing, so I'll add an ASSERT().

> > Source/WebCore/animation/AnimationTimeline.cpp:107
> > +    else if (is<CSSAnimation>(animation) && downcast<CSSAnimation>(animation).owningElement())
> > +        element.cssAnimations().add(&animation);
> 
> Ditto.

Will add an ASSERT().

> > Source/WebCore/animation/ElementAnimationRareData.h:38
> > +    WTF_MAKE_FAST_ALLOCATED;
> 
> Make this not copyable?

Will do.

> > Source/WebCore/dom/Element.h:496
> > +    AnimationCollection& webAnimations();
> 
> Should we also forward declare AnimationCollection & CSSAnimationCollection?

No, these come from `#include "WebAnimationTypes.h"`.

[Antoine Quint]

Committed r258316: <https://trac.webkit.org/changeset/258316>

[Antoine Quint]

(In reply to Ryosuke Niwa from comment #13)
> Comment on attachment 393252 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393252&action=review
> 
> > Source/WebCore/animation/AnimationTimeline.cpp:105
> > +    if (is<CSSTransition>(animation) && downcast<CSSTransition>(animation).owningElement())
> > +        element.transitions().add(&animation);
> 
> Should we check that the owning element is same as element here??

In the end that was the right thing to do and I added the equality check in the commit.