Animation and other code is too aggressive about invalidating layer composition
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Jan 2019 01:31:48 +0000 (01:31 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Jan 2019 01:31:48 +0000 (01:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=193343

Reviewed by Antoine Quint.

Source/WebCore:

We used to have the concept of a "SyntheticStyleChange", which was used to trigger
style updates for animation, and also to get compositing updated.

That morphed into a call to Element::invalidateStyleAndLayerComposition(), which causes
a style update to result in a "RecompositeLayer" diff, which in turn triggers compositing work,
and dirties DOM touch event regions (which can be expensive to update).

However, not all the callers of Element::invalidateStyleAndLayerComposition() need to trigger
compositing, and doing so from animations caused excessive touch event regions on yahoo.com,
which has several visibility:hidden elements with background-position animation.

So fix callers of invalidateStyleAndLayerComposition() which don't care about compositing to instead
call just invalidateStyle().

Also fix KeyframeAnimation::animate to correctly return true when animation state changes—it failed to
do so, because fireAnimationEventsIfNeeded() can run the state machine and change state.

* animation/KeyframeEffect.cpp:
(WebCore::invalidateElement):
* page/animation/AnimationBase.cpp:
(WebCore::AnimationBase::setNeedsStyleRecalc):
* page/animation/CSSAnimationController.cpp:
(WebCore::CSSAnimationControllerPrivate::updateAnimations):
(WebCore::CSSAnimationControllerPrivate::fireEventsAndUpdateStyle):
(WebCore::CSSAnimationControllerPrivate::pauseAnimationAtTime):
(WebCore::CSSAnimationControllerPrivate::pauseTransitionAtTime):
(WebCore::CSSAnimationController::cancelAnimations):
* page/animation/KeyframeAnimation.cpp:
(WebCore::KeyframeAnimation::animate):
* rendering/RenderImage.cpp:
(WebCore::RenderImage::imageChanged):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::calculateClipRects const):
* rendering/svg/SVGResourcesCache.cpp:
(WebCore::SVGResourcesCache::clientStyleChanged):
* style/StyleTreeResolver.cpp:
(WebCore::Style::TreeResolver::createAnimatedElementUpdate):
* svg/SVGAnimateElementBase.cpp:
(WebCore::applyCSSPropertyToTarget):
(WebCore::removeCSSPropertyFromTarget):

LayoutTests:

This test was clobbering the 'box' class on the animating element and therefore making it disappear.

* legacy-animation-engine/compositing/animation/animation-compositing.html:

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/legacy-animation-engine/compositing/animation/animation-compositing.html
Source/WebCore/ChangeLog
Source/WebCore/animation/KeyframeEffect.cpp
Source/WebCore/page/animation/AnimationBase.cpp
Source/WebCore/page/animation/CSSAnimationController.cpp
Source/WebCore/page/animation/KeyframeAnimation.cpp
Source/WebCore/rendering/RenderImage.cpp
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/svg/SVGResourcesCache.cpp
Source/WebCore/style/StyleTreeResolver.cpp
Source/WebCore/svg/SVGAnimateElementBase.cpp

index b622ccb..98578ae 100644 (file)
@@ -1,3 +1,14 @@
+2019-01-14  Simon Fraser  <simon.fraser@apple.com>
+
+        Animation and other code is too aggressive about invalidating layer composition
+        https://bugs.webkit.org/show_bug.cgi?id=193343
+
+        Reviewed by Antoine Quint.
+        
+        This test was clobbering the 'box' class on the animating element and therefore making it disappear.
+
+        * legacy-animation-engine/compositing/animation/animation-compositing.html:
+
 2019-01-14  Charles Vazac  <cvazac@akamai.com>
 
         Import current Resource-Timing WPTs
index 2b53316..9324b38 100644 (file)
@@ -39,7 +39,7 @@
             testRunner.notifyDone();
         }
       }, false);
-      document.getElementById('box').className = 'spinning';
+      document.getElementById('box').classList.add('spinning');
     }
 
     window.addEventListener('load', doTest, false);
index 0212644..89c71bc 100644 (file)
@@ -1,3 +1,51 @@
+2019-01-14  Simon Fraser  <simon.fraser@apple.com>
+
+        Animation and other code is too aggressive about invalidating layer composition
+        https://bugs.webkit.org/show_bug.cgi?id=193343
+
+        Reviewed by Antoine Quint.
+        
+        We used to have the concept of a "SyntheticStyleChange", which was used to trigger
+        style updates for animation, and also to get compositing updated.
+        
+        That morphed into a call to Element::invalidateStyleAndLayerComposition(), which causes
+        a style update to result in a "RecompositeLayer" diff, which in turn triggers compositing work,
+        and dirties DOM touch event regions (which can be expensive to update).
+        
+        However, not all the callers of Element::invalidateStyleAndLayerComposition() need to trigger
+        compositing, and doing so from animations caused excessive touch event regions on yahoo.com,
+        which has several visibility:hidden elements with background-position animation.
+        
+        So fix callers of invalidateStyleAndLayerComposition() which don't care about compositing to instead
+        call just invalidateStyle().
+        
+        Also fix KeyframeAnimation::animate to correctly return true when animation state changes—it failed to
+        do so, because fireAnimationEventsIfNeeded() can run the state machine and change state.
+
+        * animation/KeyframeEffect.cpp:
+        (WebCore::invalidateElement):
+        * page/animation/AnimationBase.cpp:
+        (WebCore::AnimationBase::setNeedsStyleRecalc):
+        * page/animation/CSSAnimationController.cpp:
+        (WebCore::CSSAnimationControllerPrivate::updateAnimations):
+        (WebCore::CSSAnimationControllerPrivate::fireEventsAndUpdateStyle):
+        (WebCore::CSSAnimationControllerPrivate::pauseAnimationAtTime):
+        (WebCore::CSSAnimationControllerPrivate::pauseTransitionAtTime):
+        (WebCore::CSSAnimationController::cancelAnimations):
+        * page/animation/KeyframeAnimation.cpp:
+        (WebCore::KeyframeAnimation::animate):
+        * rendering/RenderImage.cpp:
+        (WebCore::RenderImage::imageChanged):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::calculateClipRects const):
+        * rendering/svg/SVGResourcesCache.cpp:
+        (WebCore::SVGResourcesCache::clientStyleChanged):
+        * style/StyleTreeResolver.cpp:
+        (WebCore::Style::TreeResolver::createAnimatedElementUpdate):
+        * svg/SVGAnimateElementBase.cpp:
+        (WebCore::applyCSSPropertyToTarget):
+        (WebCore::removeCSSPropertyFromTarget):
+
 2019-01-14  Sihui Liu  <sihui_liu@apple.com>
 
         IndexedDB: When deleting databases, some open databases might be missed
index 90c6ad3..4302919 100644 (file)
@@ -60,7 +60,7 @@ using namespace JSC;
 static inline void invalidateElement(Element* element)
 {
     if (element)
-        element->invalidateStyleAndLayerComposition();
+        element->invalidateStyle();
 }
 
 static inline String CSSPropertyIDToIDLAttributeName(CSSPropertyID cssPropertyId)
index 3a74f96..ad5f154 100644 (file)
@@ -90,7 +90,7 @@ void AnimationBase::setNeedsStyleRecalc(Element* element)
         return;
 
     ASSERT(element->document().pageCacheState() == Document::NotInPageCache);
-    element->invalidateStyleAndLayerComposition();
+    element->invalidateStyle();
 }
 
 double AnimationBase::duration() const
index dc1ceab..32108b4 100644 (file)
@@ -143,9 +143,10 @@ Optional<Seconds> CSSAnimationControllerPrivate::updateAnimations(SetChanged cal
             if (timeToNextService && timeToNextService.value() == 0_s) {
                 if (callSetChanged != CallSetChanged)
                     break;
+                
                 Element& element = *compositeAnimation.key;
                 ASSERT(element.document().pageCacheState() == Document::NotInPageCache);
-                element.invalidateStyleAndLayerComposition();
+                element.invalidateStyle();
                 calledSetChanged = true;
             }
         }
@@ -225,7 +226,7 @@ void CSSAnimationControllerPrivate::fireEventsAndUpdateStyle()
     }
 
     for (auto& change : m_elementChangesToDispatch)
-        change->invalidateStyleAndLayerComposition();
+        change->invalidateStyle();
 
     m_elementChangesToDispatch.clear();
 
@@ -412,7 +413,7 @@ bool CSSAnimationControllerPrivate::pauseAnimationAtTime(Element& element, const
 {
     CompositeAnimation& compositeAnimation = ensureCompositeAnimation(element);
     if (compositeAnimation.pauseAnimationAtTime(name, t)) {
-        element.invalidateStyleAndLayerComposition();
+        element.invalidateStyle();
         startUpdateStyleIfNeededDispatcher();
         return true;
     }
@@ -424,7 +425,7 @@ bool CSSAnimationControllerPrivate::pauseTransitionAtTime(Element& element, cons
 {
     CompositeAnimation& compositeAnimation = ensureCompositeAnimation(element);
     if (compositeAnimation.pauseTransitionAtTime(cssPropertyID(property), t)) {
-        element.invalidateStyleAndLayerComposition();
+        element.invalidateStyle();
         startUpdateStyleIfNeededDispatcher();
         return true;
     }
@@ -607,7 +608,7 @@ void CSSAnimationController::cancelAnimations(Element& element)
     if (element.document().renderTreeBeingDestroyed())
         return;
     ASSERT(element.document().pageCacheState() == Document::NotInPageCache);
-    element.invalidateStyleAndLayerComposition();
+    element.invalidateStyle();
 }
 
 AnimationUpdate CSSAnimationController::updateAnimations(Element& element, const RenderStyle& newStyle, const RenderStyle* oldStyle)
index 0f96508..0ce8042 100644 (file)
@@ -158,9 +158,11 @@ void KeyframeAnimation::fetchIntervalEndpointsForProperty(CSSPropertyID property
 
 bool KeyframeAnimation::animate(CompositeAnimation& compositeAnimation, const RenderStyle& targetStyle, std::unique_ptr<RenderStyle>& animatedStyle, bool& didBlendStyle)
 {
-    // Fire the start timeout if needed
+    AnimationState oldState = state();
+
+    // Update state and fire the start timeout if needed (FIXME: this function needs a better name).
     fireAnimationEventsIfNeeded();
-    
+
     // If we have not yet started, we will not have a valid start time, so just start the animation if needed.
     if (isNew()) {
         if (m_animation->playState() == AnimationPlayState::Playing && !compositeAnimation.isSuspended())
@@ -191,9 +193,6 @@ bool KeyframeAnimation::animate(CompositeAnimation& compositeAnimation, const Re
         return false;
     }
 
-    // FIXME: the code below never changes the state, so this function always returns false.
-    AnimationState oldState = state();
-
     // Run a cycle of animation.
     // We know we will need a new render style, so make one if needed.
     if (!animatedStyle)
index 85a101d..76441ea 100644 (file)
@@ -284,7 +284,7 @@ void RenderImage::imageChanged(WrappedImagePtr newImage, const IntRect* rect)
             ASSERT(element());
             if (element()) {
                 m_needsToSetSizeForAltText = true;
-                element()->invalidateStyleAndLayerComposition();
+                element()->invalidateStyle();
             }
             return;
         }
index 8ad503e..410f2d3 100644 (file)
@@ -6599,8 +6599,10 @@ void RenderLayer::updateFilterPaintingStrategy()
 void RenderLayer::filterNeedsRepaint()
 {
     // We use the enclosing element so that we recalculate style for the ancestor of an anonymous object.
-    if (Element* element = enclosingElement())
+    if (Element* element = enclosingElement()) {
+        // FIXME: This really shouldn't have to invalidate layer composition, but tests like css3/filters/effect-reference-delete.html fail if that doesn't happen.
         element->invalidateStyleAndLayerComposition();
+    }
     renderer().repaint();
 }
 
index 3a231c7..da24c07 100644 (file)
@@ -116,7 +116,7 @@ void SVGResourcesCache::clientStyleChanged(RenderElement& renderer, StyleDiffere
     RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer, false);
 
     if (renderer.element() && !renderer.element()->isSVGElement())
-        renderer.element()->invalidateStyleAndLayerComposition();
+        renderer.element()->invalidateStyle();
 }
 
 void SVGResourcesCache::clientWasAddedToTree(RenderObject& renderer)
index c8af097..7a97593 100644 (file)
@@ -306,7 +306,7 @@ ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderSt
         auto& animationController = m_document.frame()->animation();
 
         auto animationUpdate = animationController.updateAnimations(element, *newStyle, oldStyle);
-        shouldRecompositeLayer = animationUpdate.stateChanged;
+        shouldRecompositeLayer = animationUpdate.stateChanged; // FIXME: constrain this to just property animations triggering acceleration.
 
         if (animationUpdate.style)
             newStyle = WTFMove(animationUpdate.style);
index a4a24fa..6fd1bc1 100644 (file)
@@ -249,14 +249,14 @@ static inline void applyCSSPropertyToTarget(SVGElement& targetElement, CSSProper
     if (!targetElement.ensureAnimatedSMILStyleProperties().setProperty(id, value, false))
         return;
 
-    targetElement.invalidateStyleAndLayerComposition();
+    targetElement.invalidateStyle();
 }
 
 static inline void removeCSSPropertyFromTarget(SVGElement& targetElement, CSSPropertyID id)
 {
     ASSERT(!targetElement.m_deletionHasBegun);
     targetElement.ensureAnimatedSMILStyleProperties().removeProperty(id);
-    targetElement.invalidateStyleAndLayerComposition();
+    targetElement.invalidateStyle();
 }
 
 static inline void applyCSSPropertyToTargetAndInstances(SVGElement& targetElement, const QualifiedName& attributeName, const String& valueAsString)