Changeset 239965 in webkit

Timestamp:

Jan 14, 2019 5:31:48 PM (5 years ago)

Author:

Simon Fraser

Message:

Animation and other code is too aggressive about invalidating layer composition
​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:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/legacy-animation-engine/compositing/animation/animation-compositing.html (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/animation/KeyframeEffect.cpp (modified) (1 diff)
• Source/WebCore/page/animation/AnimationBase.cpp (modified) (1 diff)
• Source/WebCore/page/animation/CSSAnimationController.cpp (modified) (5 diffs)
• Source/WebCore/page/animation/KeyframeAnimation.cpp (modified) (2 diffs)
• Source/WebCore/rendering/RenderImage.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderLayer.cpp (modified) (1 diff)
• Source/WebCore/rendering/svg/SVGResourcesCache.cpp (modified) (1 diff)
• Source/WebCore/style/StyleTreeResolver.cpp (modified) (1 diff)
• Source/WebCore/svg/SVGAnimateElementBase.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/legacy-animation-engine/compositing/animation/animation-compositing.html

@@ -40 +40 @@
         }
       }, false);
-      document.getElementById('box').className = 'spinning';
+      document.getElementById('box').classList.add('spinning');
     }
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/animation/KeyframeEffect.cpp

@@ -61 +61 @@
 {
     if (element)
-        element->invalidateStyleAndLayerComposition();
+        element->invalidateStyle();
 }
 

trunk/Source/WebCore/page/animation/AnimationBase.cpp

@@ -91 +91 @@
 
     ASSERT(element->document().pageCacheState() == Document::NotInPageCache);
-    element->invalidateStyleAndLayerComposition();
+    element->invalidateStyle();
 }
 

trunk/Source/WebCore/page/animation/CSSAnimationController.cpp

@@ -144 +144 @@
                 if (callSetChanged != CallSetChanged)
                     break;
+                
                 Element& element = *compositeAnimation.key;
                 ASSERT(element.document().pageCacheState() == Document::NotInPageCache);
-                element.invalidateStyleAndLayerComposition();
+                element.invalidateStyle();
                 calledSetChanged = true;
             }
@@ -226 +227 @@
 
     for (auto& change : m_elementChangesToDispatch)
-        change->invalidateStyleAndLayerComposition();
+        change->invalidateStyle();
 
     m_elementChangesToDispatch.clear();
@@ -413 +414 @@
     CompositeAnimation& compositeAnimation = ensureCompositeAnimation(element);
     if (compositeAnimation.pauseAnimationAtTime(name, t)) {
-        element.invalidateStyleAndLayerComposition();
+        element.invalidateStyle();
         startUpdateStyleIfNeededDispatcher();
         return true;
@@ -425 +426 @@
     CompositeAnimation& compositeAnimation = ensureCompositeAnimation(element);
     if (compositeAnimation.pauseTransitionAtTime(cssPropertyID(property), t)) {
-        element.invalidateStyleAndLayerComposition();
+        element.invalidateStyle();
         startUpdateStyleIfNeededDispatcher();
         return true;
@@ -608 +609 @@
         return;
     ASSERT(element.document().pageCacheState() == Document::NotInPageCache);
-    element.invalidateStyleAndLayerComposition();
+    element.invalidateStyle();
 }
 

trunk/Source/WebCore/page/animation/KeyframeAnimation.cpp

@@ -159 +159 @@
 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()) {
@@ -191 +193 @@
         return false;
     }
-
-    // FIXME: the code below never changes the state, so this function always returns false.
-    AnimationState oldState = state();
 
     // Run a cycle of animation.

trunk/Source/WebCore/rendering/RenderImage.cpp

@@ -285 +285 @@
             if (element()) {
                 m_needsToSetSizeForAltText = true;
-                element()->invalidateStyleAndLayerComposition();
+                element()->invalidateStyle();
             }
             return;

trunk/Source/WebCore/rendering/RenderLayer.cpp

@@ -6600 +6600 @@
 {
     // 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();
 }

trunk/Source/WebCore/rendering/svg/SVGResourcesCache.cpp

@@ -117 +117 @@
 
     if (renderer.element() && !renderer.element()->isSVGElement())
-        renderer.element()->invalidateStyleAndLayerComposition();
+        renderer.element()->invalidateStyle();
 }
 

trunk/Source/WebCore/style/StyleTreeResolver.cpp

@@ -307 +307 @@
 
         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)

trunk/Source/WebCore/svg/SVGAnimateElementBase.cpp

@@ -250 +250 @@
         return;
 
-    targetElement.invalidateStyleAndLayerComposition();
+    targetElement.invalidateStyle();
 }
 
@@ -257 +257 @@
     ASSERT(!targetElement.m_deletionHasBegun);
     targetElement.ensureAnimatedSMILStyleProperties().removeProperty(id);
-    targetElement.invalidateStyleAndLayerComposition();
+    targetElement.invalidateStyle();
 }