Bug 209415 – [Web Animations] ElementAnimationRareData is created too frequently

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

[Antoine Quint]

[Web Animations] ElementAnimationRareData is created too frequently

[Antoine Quint]

Created attachment 394248 [details]
Patch

[Antoine Quint]

Created attachment 394252 [details]
Patch

[Antti Koivisto]

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.

[Antti Koivisto]

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)

?

[Antoine Quint]

Committed r258834: <https://trac.webkit.org/changeset/258834>

[Radar WebKit Bug Importer]

<rdar://problem/60771170>

[Michael Catanzaro]

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?

[Michael Catanzaro]

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;
}

[Michael Catanzaro]

Bug #209438