Remove RenderElement::isCSSAnimating boolean
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Sep 2017 15:31:41 +0000 (15:31 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Sep 2017 15:31:41 +0000 (15:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=176779

Reviewed by Andreas Kling.

This optimization can be replaced with a simple style test that doesn't require keeping
two sources of truth in sync.

* page/animation/CSSAnimationController.cpp:
(WebCore::CSSAnimationControllerPrivate::ensureCompositeAnimation):
(WebCore::CSSAnimationControllerPrivate::clear):

    Can't test here as style might have become non-animating and we don't clear animation when that happens.
    This is only called on renderer destruction so it is not an important optimization.

(WebCore::CSSAnimationControllerPrivate::isRunningAnimationOnRenderer const):
(WebCore::CSSAnimationControllerPrivate::isRunningAcceleratedAnimationOnRenderer const):
(WebCore::CSSAnimationControllerPrivate::getAnimatedStyleForRenderer):
(WebCore::CSSAnimationControllerPrivate::computeExtentOfAnimation const):
(WebCore::CSSAnimationController::cancelAnimations):
(WebCore::CSSAnimationController::getAnimatedStyleForRenderer):
(WebCore::CSSAnimationController::computeExtentOfAnimation const):
(WebCore::CSSAnimationController::isRunningAnimationOnRenderer const):
(WebCore::CSSAnimationController::isRunningAcceleratedAnimationOnRenderer const):

    Test if the style has any animations. This is roughly equivalent of the old test.
    (it is actually somewhat better as the boolean was never cleared on style changes)

* rendering/RenderElement.cpp:
(WebCore::RenderElement::RenderElement):
* rendering/RenderElement.h:
(WebCore::RenderElement::isCSSAnimating const): Deleted.
(WebCore::RenderElement::setIsCSSAnimating): Deleted.
* rendering/style/RenderStyle.h:
(WebCore::RenderStyle::hasAnimationsOrTransitions const):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@221916 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/page/animation/CSSAnimationController.cpp
Source/WebCore/rendering/RenderElement.cpp
Source/WebCore/rendering/RenderElement.h
Source/WebCore/rendering/style/RenderStyle.h
Source/WebCore/rendering/style/StyleRareNonInheritedData.h

index 29c125f..9544142 100644 (file)
@@ -1,3 +1,41 @@
+2017-09-12  Antti Koivisto  <antti@apple.com>
+
+        Remove RenderElement::isCSSAnimating boolean
+        https://bugs.webkit.org/show_bug.cgi?id=176779
+
+        Reviewed by Andreas Kling.
+
+        This optimization can be replaced with a simple style test that doesn't require keeping
+        two sources of truth in sync.
+
+        * page/animation/CSSAnimationController.cpp:
+        (WebCore::CSSAnimationControllerPrivate::ensureCompositeAnimation):
+        (WebCore::CSSAnimationControllerPrivate::clear):
+
+            Can't test here as style might have become non-animating and we don't clear animation when that happens.
+            This is only called on renderer destruction so it is not an important optimization.
+
+        (WebCore::CSSAnimationControllerPrivate::isRunningAnimationOnRenderer const):
+        (WebCore::CSSAnimationControllerPrivate::isRunningAcceleratedAnimationOnRenderer const):
+        (WebCore::CSSAnimationControllerPrivate::getAnimatedStyleForRenderer):
+        (WebCore::CSSAnimationControllerPrivate::computeExtentOfAnimation const):
+        (WebCore::CSSAnimationController::cancelAnimations):
+        (WebCore::CSSAnimationController::getAnimatedStyleForRenderer):
+        (WebCore::CSSAnimationController::computeExtentOfAnimation const):
+        (WebCore::CSSAnimationController::isRunningAnimationOnRenderer const):
+        (WebCore::CSSAnimationController::isRunningAcceleratedAnimationOnRenderer const):
+
+            Test if the style has any animations. This is roughly equivalent of the old test.
+            (it is actually somewhat better as the boolean was never cleared on style changes)
+
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::RenderElement):
+        * rendering/RenderElement.h:
+        (WebCore::RenderElement::isCSSAnimating const): Deleted.
+        (WebCore::RenderElement::setIsCSSAnimating): Deleted.
+        * rendering/style/RenderStyle.h:
+        (WebCore::RenderStyle::hasAnimationsOrTransitions const):
+
 2017-09-12  Ms2ger  <Ms2ger@igalia.com>
 
         Disallow passing null data to uniform1uiv() and friends.
index b11da5c..72a4d04 100644 (file)
@@ -85,11 +85,9 @@ CSSAnimationControllerPrivate::~CSSAnimationControllerPrivate()
 
 CompositeAnimation& CSSAnimationControllerPrivate::ensureCompositeAnimation(RenderElement& renderer)
 {
-    auto result = m_compositeAnimations.add(&renderer, nullptr);
-    if (result.isNewEntry) {
-        result.iterator->value = CompositeAnimation::create(*this);
-        renderer.setIsCSSAnimating(true);
-    }
+    auto result = m_compositeAnimations.ensure(&renderer, [&] {
+        return CompositeAnimation::create(*this);
+    });
 
     if (animationsAreSuspendedForDocument(&renderer.document()))
         result.iterator->value->suspendAnimations();
@@ -99,10 +97,11 @@ CompositeAnimation& CSSAnimationControllerPrivate::ensureCompositeAnimation(Rend
 
 bool CSSAnimationControllerPrivate::clear(RenderElement& renderer)
 {
-    LOG(Animations, "CSSAnimationControllerPrivate %p clear: %p", this, &renderer);
+    auto it = m_compositeAnimations.find(&renderer);
+    if (it == m_compositeAnimations.end())
+        return false;
 
-    ASSERT(renderer.isCSSAnimating());
-    ASSERT(m_compositeAnimations.contains(&renderer));
+    LOG(Animations, "CSSAnimationControllerPrivate %p clear: %p", this, &renderer);
 
     Element* element = renderer.element();
 
@@ -116,11 +115,14 @@ bool CSSAnimationControllerPrivate::clear(RenderElement& renderer)
     
     // Return false if we didn't do anything OR we are suspended (so we don't try to
     // do a invalidateStyleForSubtree() when suspended).
-    RefPtr<CompositeAnimation> animation = m_compositeAnimations.take(&renderer);
-    ASSERT(animation);
-    renderer.setIsCSSAnimating(false);
-    animation->clearRenderer();
-    return animation->isSuspended();
+    // FIXME: The code below does the opposite of what the comment above says regarding suspended state.
+    auto& animation = *it->value;
+    bool result = animation.isSuspended();
+    animation.clearRenderer();
+
+    m_compositeAnimations.remove(it);
+
+    return result;
 }
 
 std::optional<Seconds> CSSAnimationControllerPrivate::updateAnimations(SetChanged callSetChanged/* = DoNotCallSetChanged*/)
@@ -287,18 +289,18 @@ void CSSAnimationControllerPrivate::animationTimerFired()
 
 bool CSSAnimationControllerPrivate::isRunningAnimationOnRenderer(RenderElement& renderer, CSSPropertyID property, AnimationBase::RunningState runningState) const
 {
-    ASSERT(renderer.isCSSAnimating());
-    ASSERT(m_compositeAnimations.contains(&renderer));
-    const CompositeAnimation& animation = *m_compositeAnimations.get(&renderer);
-    return animation.isAnimatingProperty(property, false, runningState);
+    auto* animation = m_compositeAnimations.get(&renderer);
+    if (!animation)
+        return false;
+    return animation->isAnimatingProperty(property, false, runningState);
 }
 
 bool CSSAnimationControllerPrivate::isRunningAcceleratedAnimationOnRenderer(RenderElement& renderer, CSSPropertyID property, AnimationBase::RunningState runningState) const
 {
-    ASSERT(renderer.isCSSAnimating());
-    ASSERT(m_compositeAnimations.contains(&renderer));
-    const CompositeAnimation& animation = *m_compositeAnimations.get(&renderer);
-    return animation.isAnimatingProperty(property, true, runningState);
+    auto* animation = m_compositeAnimations.get(&renderer);
+    if (!animation)
+        return false;
+    return animation->isAnimatingProperty(property, true, runningState);
 }
 
 void CSSAnimationControllerPrivate::updateThrottlingState()
@@ -467,12 +469,13 @@ void CSSAnimationControllerPrivate::receivedStartTimeResponse(double time)
 
 std::unique_ptr<RenderStyle> CSSAnimationControllerPrivate::getAnimatedStyleForRenderer(RenderElement& renderer)
 {
+    auto* animation = m_compositeAnimations.get(&renderer);
+    if (!animation)
+        return RenderStyle::clonePtr(renderer.style());
+
     AnimationPrivateUpdateBlock animationUpdateBlock(*this);
 
-    ASSERT(renderer.isCSSAnimating());
-    ASSERT(m_compositeAnimations.contains(&renderer));
-    const CompositeAnimation& rendererAnimations = *m_compositeAnimations.get(&renderer);
-    std::unique_ptr<RenderStyle> animatingStyle = rendererAnimations.getAnimatedStyle();
+    std::unique_ptr<RenderStyle> animatingStyle = animation->getAnimatedStyle();
     if (!animatingStyle)
         animatingStyle = RenderStyle::clonePtr(renderer.style());
     
@@ -481,14 +484,14 @@ std::unique_ptr<RenderStyle> CSSAnimationControllerPrivate::getAnimatedStyleForR
 
 bool CSSAnimationControllerPrivate::computeExtentOfAnimation(RenderElement& renderer, LayoutRect& bounds) const
 {
-    ASSERT(renderer.isCSSAnimating());
-    ASSERT(m_compositeAnimations.contains(&renderer));
+    auto* animation = m_compositeAnimations.get(&renderer);
+    if (!animation)
+        return true;
 
-    const CompositeAnimation& rendererAnimations = *m_compositeAnimations.get(&renderer);
-    if (!rendererAnimations.isAnimatingProperty(CSSPropertyTransform, false, AnimationBase::Running | AnimationBase::Paused))
+    if (!animation->isAnimatingProperty(CSSPropertyTransform, false, AnimationBase::Running | AnimationBase::Paused))
         return true;
 
-    return rendererAnimations.computeExtentOfTransformAnimation(bounds);
+    return animation->computeExtentOfTransformAnimation(bounds);
 }
 
 unsigned CSSAnimationControllerPrivate::numberOfActiveAnimations(Document* document) const
@@ -630,9 +633,6 @@ CSSAnimationController::~CSSAnimationController()
 
 void CSSAnimationController::cancelAnimations(RenderElement& renderer)
 {
-    if (!renderer.isCSSAnimating())
-        return;
-
     if (!m_data->clear(renderer))
         return;
 
@@ -680,14 +680,15 @@ bool CSSAnimationController::updateAnimations(RenderElement& renderer, const Ren
 
 std::unique_ptr<RenderStyle> CSSAnimationController::getAnimatedStyleForRenderer(RenderElement& renderer)
 {
-    if (!renderer.isCSSAnimating())
+    if (!renderer.style().hasAnimationsOrTransitions())
         return RenderStyle::clonePtr(renderer.style());
+
     return m_data->getAnimatedStyleForRenderer(renderer);
 }
 
 bool CSSAnimationController::computeExtentOfAnimation(RenderElement& renderer, LayoutRect& bounds) const
 {
-    if (!renderer.isCSSAnimating())
+    if (!renderer.style().hasAnimationsOrTransitions())
         return true;
 
     return m_data->computeExtentOfAnimation(renderer, bounds);
@@ -721,12 +722,16 @@ bool CSSAnimationController::pauseTransitionAtTime(RenderElement* renderer, cons
 
 bool CSSAnimationController::isRunningAnimationOnRenderer(RenderElement& renderer, CSSPropertyID property, AnimationBase::RunningState runningState) const
 {
-    return renderer.isCSSAnimating() && m_data->isRunningAnimationOnRenderer(renderer, property, runningState);
+    if (!renderer.style().hasAnimationsOrTransitions())
+        return false;
+    return m_data->isRunningAnimationOnRenderer(renderer, property, runningState);
 }
 
 bool CSSAnimationController::isRunningAcceleratedAnimationOnRenderer(RenderElement& renderer, CSSPropertyID property, AnimationBase::RunningState runningState) const
 {
-    return renderer.isCSSAnimating() && m_data->isRunningAcceleratedAnimationOnRenderer(renderer, property, runningState);
+    if (!renderer.style().hasAnimationsOrTransitions())
+        return false;
+    return m_data->isRunningAcceleratedAnimationOnRenderer(renderer, property, runningState);
 }
 
 bool CSSAnimationController::isSuspended() const
index ec76fce..1bec90e 100644 (file)
@@ -107,7 +107,6 @@ inline RenderElement::RenderElement(ContainerNode& elementOrDocument, RenderStyl
     , m_renderBoxNeedsLazyRepaint(false)
     , m_hasPausedImageAnimations(false)
     , m_hasCounterNodeMap(false)
-    , m_isCSSAnimating(false)
     , m_hasContinuation(false)
     , m_hasValidCachedFirstLineStyle(false)
     , m_renderBlockHasMarginBeforeQuirk(false)
index 4f8226b..8a7bf49 100644 (file)
@@ -203,9 +203,6 @@ public:
     bool hasCounterNodeMap() const { return m_hasCounterNodeMap; }
     void setHasCounterNodeMap(bool f) { m_hasCounterNodeMap = f; }
 
-    bool isCSSAnimating() const { return m_isCSSAnimating; }
-    void setIsCSSAnimating(bool b) { m_isCSSAnimating = b; }
-    
     const RenderElement* enclosingRendererWithTextDecoration(TextDecoration, bool firstLine) const;
     void drawLineForBoxSide(GraphicsContext&, const FloatRect&, BoxSide, Color, EBorderStyle, float adjacentWidth1, float adjacentWidth2, bool antialias = false) const;
 
@@ -338,7 +335,6 @@ private:
     unsigned m_renderBoxNeedsLazyRepaint : 1;
     unsigned m_hasPausedImageAnimations : 1;
     unsigned m_hasCounterNodeMap : 1;
-    unsigned m_isCSSAnimating : 1;
     unsigned m_hasContinuation : 1;
     mutable unsigned m_hasValidCachedFirstLineStyle : 1;
 
index a247d39..1c14905 100644 (file)
@@ -662,7 +662,7 @@ public:
     AnimationList* animations() { return m_rareNonInheritedData->animations.get(); }
     AnimationList* transitions() { return m_rareNonInheritedData->transitions.get(); }
     
-    bool hasAnimationsOrTransitions() const { return m_rareNonInheritedData->hasAnimationsOrTransitions(); }
+    bool hasAnimationsOrTransitions() const { return hasAnimations() || hasTransitions(); }
 
     AnimationList& ensureAnimations();
     AnimationList& ensureTransitions();
index 37dc502..2693a84 100644 (file)
@@ -92,8 +92,6 @@ public:
 
     bool hasOpacity() const { return opacity < 1; }
 
-    bool hasAnimationsOrTransitions() const { return animations || transitions; }
-
     float opacity;
 
     float aspectRatioDenominator;