Attachment 358933 Details for Bug 193343 – Patch

[patch] Patch

bug-193343-20190111131841.patch (text/plain), 12.48 KB, created by Simon Fraser (smfr) on 2019-01-11 13:18:43 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2019-01-11 13:18:43 PST

Size: 12.48 KB

patch

obsolete

>Subversion Revision: 239658
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index b1494f8548de3e68fd06ed6d5ce921c6749cf737..1c26922682ac9d25c9a1a46fcae5f67ee678aae1 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,36 @@
>+2019-01-11  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 NOBODY (OOPS!).
>+
>+        No new tests (OOPS!).
>+
>+        * 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-04  Simon Fraser  <simon.fraser@apple.com>
> 
>         Factor legacy WK1 code for fixed and scrolling layers into their own helper class
>diff --git a/Source/WebCore/animation/KeyframeEffect.cpp b/Source/WebCore/animation/KeyframeEffect.cpp
>index 13dbcd0fd133979c92351cfb0460d98211a730c3..a54f65f063191193f4fd65dfc02739e8746b8b32 100644
>--- a/Source/WebCore/animation/KeyframeEffect.cpp
>+++ b/Source/WebCore/animation/KeyframeEffect.cpp
>@@ -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)
>diff --git a/Source/WebCore/page/animation/AnimationBase.cpp b/Source/WebCore/page/animation/AnimationBase.cpp
>index 3a74f96934869d33d6c952c12d668ef6306043af..ad5f154d8ec979083bd1455a351db2ee014a23f2 100644
>--- a/Source/WebCore/page/animation/AnimationBase.cpp
>+++ b/Source/WebCore/page/animation/AnimationBase.cpp
>@@ -90,7 +90,7 @@ void AnimationBase::setNeedsStyleRecalc(Element* element)
>         return;
> 
>     ASSERT(element->document().pageCacheState() == Document::NotInPageCache);
>-    element->invalidateStyleAndLayerComposition();
>+    element->invalidateStyle();
> }
> 
> double AnimationBase::duration() const
>diff --git a/Source/WebCore/page/animation/CSSAnimationController.cpp b/Source/WebCore/page/animation/CSSAnimationController.cpp
>index dc1ceab3ab2094a4a627f563df98757eb60104a3..32108b49a7065508718e70a30332b3dc363734a8 100644
>--- a/Source/WebCore/page/animation/CSSAnimationController.cpp
>+++ b/Source/WebCore/page/animation/CSSAnimationController.cpp
>@@ -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)
>diff --git a/Source/WebCore/page/animation/KeyframeAnimation.cpp b/Source/WebCore/page/animation/KeyframeAnimation.cpp
>index 0f9650882943c54940ed39979a16bf480ea26b8e..0ce80426db93fd61d46272bea86951b8afe70ddb 100644
>--- a/Source/WebCore/page/animation/KeyframeAnimation.cpp
>+++ b/Source/WebCore/page/animation/KeyframeAnimation.cpp
>@@ -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)
>diff --git a/Source/WebCore/rendering/RenderImage.cpp b/Source/WebCore/rendering/RenderImage.cpp
>index 85a101dc86c563b51c5e3150d3868561914baa91..76441ead18d486ef55819cd16c61d18a956fa541 100644
>--- a/Source/WebCore/rendering/RenderImage.cpp
>+++ b/Source/WebCore/rendering/RenderImage.cpp
>@@ -284,7 +284,7 @@ void RenderImage::imageChanged(WrappedImagePtr newImage, const IntRect* rect)
>             ASSERT(element());
>             if (element()) {
>                 m_needsToSetSizeForAltText = true;
>-                element()->invalidateStyleAndLayerComposition();
>+                element()->invalidateStyle();
>             }
>             return;
>         }
>diff --git a/Source/WebCore/rendering/RenderLayer.cpp b/Source/WebCore/rendering/RenderLayer.cpp
>index 0e2e0f16b4490034f3b9af7b4c3dbfd51034bc01..c39057d0b835cbb7d2ec0e2ba35edf4f3b20ec26 100644
>--- a/Source/WebCore/rendering/RenderLayer.cpp
>+++ b/Source/WebCore/rendering/RenderLayer.cpp
>@@ -6597,8 +6597,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();
> }
> 
>diff --git a/Source/WebCore/rendering/svg/SVGResourcesCache.cpp b/Source/WebCore/rendering/svg/SVGResourcesCache.cpp
>index 3a231c7e76a1bb18150430ec0005b318f858aa74..da24c07621000228563fcb583c04d05eec0212d4 100644
>--- a/Source/WebCore/rendering/svg/SVGResourcesCache.cpp
>+++ b/Source/WebCore/rendering/svg/SVGResourcesCache.cpp
>@@ -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)
>diff --git a/Source/WebCore/style/StyleTreeResolver.cpp b/Source/WebCore/style/StyleTreeResolver.cpp
>index c8af0970cfe28f612f7387c91a1ffd0074e2e007..7a9759369fb6601b8783829e37f24aa68daebd39 100644
>--- a/Source/WebCore/style/StyleTreeResolver.cpp
>+++ b/Source/WebCore/style/StyleTreeResolver.cpp
>@@ -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);
>diff --git a/Source/WebCore/svg/SVGAnimateElementBase.cpp b/Source/WebCore/svg/SVGAnimateElementBase.cpp
>index a4a24faf871504c1d73c3bf20ffd3edc6b6de84d..6fd1bc1097044cb78e9a7d0e0fc57e8c458c3a92 100644
>--- a/Source/WebCore/svg/SVGAnimateElementBase.cpp
>+++ b/Source/WebCore/svg/SVGAnimateElementBase.cpp
>@@ -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)
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 1ce9b3abfe75aca76d0c1be9a9caec47c2092082..81546fd5416c9e5a9f8782ba5380dc1d4204bf6d 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,12 @@
>+2019-01-11  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 NOBODY (OOPS!).
>+
>+        * legacy-animation-engine/compositing/animation/animation-compositing.html:
>+
> 2019-01-05  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r239607.
>diff --git a/LayoutTests/legacy-animation-engine/compositing/animation/animation-compositing.html b/LayoutTests/legacy-animation-engine/compositing/animation/animation-compositing.html
>index 2b533162778714f6fc94ab0d5c8aaf409b147498..9324b387b7dfa116c0772923ec094281ee113d25 100644
>--- a/LayoutTests/legacy-animation-engine/compositing/animation/animation-compositing.html
>+++ b/LayoutTests/legacy-animation-engine/compositing/animation/animation-compositing.html
>@@ -39,7 +39,7 @@
>             testRunner.notifyDone();
>         }
>       }, false);
>-      document.getElementById('box').className = 'spinning';
>+      document.getElementById('box').classList.add('spinning');
>     }
> 
>     window.addEventListener('load', doTest, false);

Flags:

graouts: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193343: 358859 | 358865 | 358867 | 358869 | 358870 | 358874 | 358933 | 358970 | 358972 | 358973