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.
<rdar://problem/59687906>
We should probably move these data structures to hang off of ElementRareData. Create something like ElementAnimationRareData and then put it on ElementRareData.
I like Ryosuke's suggestion and will implement that.
Should also make KeyframeEffectStack hang off that instead of ElementRareData directly.
(In reply to Antoine Quint from comment #4) > Should also make KeyframeEffectStack hang off that instead of > ElementRareData directly. That would make most sense.
Created attachment 393217 [details] Patch
Created attachment 393219 [details] Patch
Created attachment 393231 [details] Patch
Created attachment 393233 [details] Patch
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.
Created attachment 393252 [details] Patch
(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.
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?
(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"`.
Committed r258316: <https://trac.webkit.org/changeset/258316>
(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.