[Web Animations] ElementAnimationRareData is created too frequently
Created attachment 394248 [details] Patch
Created attachment 394252 [details] Patch
Comment on attachment 394252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394252&action=review > Source/WebCore/animation/AnimationTimeline.cpp:226 > + if (const auto* transitions = element.transitions()) { No particular reason to spell out const here explicitly. > Source/WebCore/dom/Element.cpp:3833 > +const PropertyToTransitionMap* Element::completedTransitionsByProperty() const > +{ > + if (auto* animationData = animationRareData()) > + return &animationRareData()->completedTransitionsByProperty(); > + return nullptr; > +} > + > +const PropertyToTransitionMap* Element::runningTransitionsByProperty() const > +{ > + if (auto* animationData = animationRareData()) > + return &animationRareData()->runningTransitionsByProperty(); > + return nullptr; > +} Instead of exposing the maps, perhaps you could just have RefPtr<CSSTransition> Element::completedTransitionForProperty(CSSProperty) Seems like that's the only thing these are used for and it would simplify the call sites.
Comment on attachment 394252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394252&action=review >> Source/WebCore/dom/Element.cpp:3833 >> +} > > Instead of exposing the maps, perhaps you could just have > > RefPtr<CSSTransition> Element::completedTransitionForProperty(CSSProperty) > > Seems like that's the only thing these are used for and it would simplify the call sites. Or even just bool Element::hasCompletedTransitionForProperty(CSSProperty) ?
Committed r258834: <https://trac.webkit.org/changeset/258834>
<rdar://problem/60771170>
New warnings: [1031/1697] Building CXX object Source/WebCore/CMakeFiles...es/WebCore/unified-sources/UnifiedSource-be65d27a-7.cpp.o In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-be65d27a-7.cpp:8: /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Element.cpp: In member function ‘const AnimationCollection* WebCore::Element::webAnimations() const’: /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Element.cpp:3802:15: warning: unused variable ‘animationData’ [-Wunused-variable] 3802 | if (auto* animationData = animationRareData()) | ^~~~~~~~~~~~~ /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Element.cpp: In member function ‘const AnimationCollection* WebCore::Element::cssAnimations() const’: /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Element.cpp:3809:15: warning: unused variable ‘animationData’ [-Wunused-variable] 3809 | if (auto* animationData = animationRareData()) | ^~~~~~~~~~~~~ /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Element.cpp: In member function ‘const AnimationCollection* WebCore::Element::transitions() const’: /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Element.cpp:3816:15: warning: unused variable ‘animationData’ [-Wunused-variable] 3816 | if (auto* animationData = animationRareData()) | ^~~~~~~~~~~~~ /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Element.cpp: In member function ‘bool WebCore::Element::hasCompletedTransitionsForProperty(WebCore::CSSPropertyID) const’: /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Element.cpp:3823:15: warning: unused variable ‘animationData’ [-Wunused-variable] 3823 | if (auto* animationData = animationRareData()) | ^~~~~~~~~~~~~ /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Element.cpp: In member function ‘bool WebCore::Element::hasRunningTransitionsForProperty(WebCore::CSSPropertyID) const’: /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Element.cpp:3830:15: warning: unused variable ‘animationData’ [-Wunused-variable] 3830 | if (auto* animationData = animationRareData()) | ^~~~~~~~~~~~~ /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Element.cpp: In member function ‘bool WebCore::Element::hasRunningTransitions() const’: /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/Element.cpp:3837:15: warning: unused variable ‘animationData’ [-Wunused-variable] 3837 | if (auto* animationData = animationRareData()) | ^~~~~~~~~~~~~ Solution is to just remove the unnecessary variable declaration... OK?
Planning to commit this: diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp index b64ebb1239b1..d4d7b143e082 100644 --- a/Source/WebCore/dom/Element.cpp +++ b/Source/WebCore/dom/Element.cpp @@ -3799,42 +3799,42 @@ OptionSet<AnimationImpact> Element::applyKeyframeEffects(RenderStyle& targetStyl const AnimationCollection* Element::webAnimations() const { - if (auto* animationData = animationRareData()) + if (animationRareData()) return &animationRareData()->webAnimations(); return nullptr; } const AnimationCollection* Element::cssAnimations() const { - if (auto* animationData = animationRareData()) + if (animationRareData()) return &animationRareData()->cssAnimations(); return nullptr; } const AnimationCollection* Element::transitions() const { - if (auto* animationData = animationRareData()) + if (animationRareData()) return &animationRareData()->transitions(); return nullptr; } bool Element::hasCompletedTransitionsForProperty(CSSPropertyID property) const { - if (auto* animationData = animationRareData()) + if (animationRareData()) return animationRareData()->completedTransitionsByProperty().contains(property); return false; } bool Element::hasRunningTransitionsForProperty(CSSPropertyID property) const { - if (auto* animationData = animationRareData()) + if (animationRareData()) return animationRareData()->runningTransitionsByProperty().contains(property); return false; } bool Element::hasRunningTransitions() const { - if (auto* animationData = animationRareData()) + if (animationRareData()) return !animationRareData()->runningTransitionsByProperty().isEmpty(); return false; }
Bug #209438