Attachment 371199 Details for Bug 197372 – Patch

[patch] Patch

bug-197372-20190603114258.patch (text/plain), 12.44 KB, created by Said Abou-Hallawa on 2019-06-03 11:42:58 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Said Abou-Hallawa

Created: 2019-06-03 11:42:58 PDT

Size: 12.44 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 246040)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,37 @@
>+2019-06-03  Said Abou-Hallawa  <sabouhallawa@apple.com>
>+
>+        Assertion fires when animating the 'class' attribute of an SVG element
>+        https://bugs.webkit.org/show_bug.cgi?id=197372
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        All instances of SVG animated properties have to share a single animVal
>+        such that once its value is progressed, all the instances will see the
>+        change. This was not happening for SVGAnimatedPrimitiveProperty. To do
>+        that we need to:
>+
>+        -- Override the virtual methods instanceStartAnimation() and
>+           instanceStopAnimation(). 
>+        -- Change the type of m_animVal from Optional to std::shared_ptr<>. So
>+           the master property will create it and all the instances will hold
>+           the same master pointer.
>+        -- Add a special case for the 'class' attribute to invalidate the style
>+           of the target element when its animVal changes.
>+
>+        Tests: svg/animations/animated-bool-externalResourcesRequired-instances.svg
>+               svg/animations/animated-string-class-instances.svg
>+               svg/animations/animated-string-href.svg
>+
>+        * svg/properties/SVGAnimatedPrimitiveProperty.h:
>+        (WebCore::SVGAnimatedPrimitiveProperty::setAnimVal):
>+        * svg/properties/SVGAnimatedPropertyAnimator.h:
>+        * svg/properties/SVGAttributeAnimator.cpp:
>+        (WebCore::SVGAttributeAnimator::isAnimatedStyleClassAniamtor const):
>+        (WebCore::SVGAttributeAnimator::invalidateStyle):
>+        (WebCore::SVGAttributeAnimator::applyAnimatedStylePropertyChange):
>+        (WebCore::SVGAttributeAnimator::removeAnimatedStyleProperty):
>+        * svg/properties/SVGAttributeAnimator.h:
>+
> 2019-06-03  Don Olmstead  <don.olmstead@sony.com>
> 
>         [CMake] Add WebKit::JavaScriptCore target
>Index: Source/WebCore/svg/properties/SVGAnimatedPrimitiveProperty.h
>===================================================================
>--- Source/WebCore/svg/properties/SVGAnimatedPrimitiveProperty.h	(revision 246038)
>+++ Source/WebCore/svg/properties/SVGAnimatedPrimitiveProperty.h	(working copy)
>@@ -60,8 +60,8 @@ public:
>     // Used by SVGAttributeAnimator::progress.
>     void setAnimVal(const PropertyType& animVal)
>     {
>-        ASSERT(isAnimating());
>-        m_animVal = animVal;
>+        ASSERT(isAnimating() && m_animVal);
>+        *m_animVal = animVal;
>     }
> 
>     const PropertyType& animVal() const
>@@ -82,7 +82,7 @@ public:
>     // Used to apply the SVGAttributeAnimator change to the target element.
>     String animValAsString() const override
>     {
>-        ASSERT(isAnimating() && !!m_animVal);
>+        ASSERT(isAnimating() && m_animVal);
>         return SVGPropertyTraits<PropertyType>::toString(*m_animVal);
>     }
> 
>@@ -109,7 +109,7 @@ public:
>     {
>         if (isAnimating())
>             return;
>-        m_animVal = m_baseVal;
>+        m_animVal = std::make_shared<PropertyType>(m_baseVal);
>         SVGAnimatedProperty::startAnimation();
>     }
> 
>@@ -117,10 +117,27 @@ public:
>     {
>         if (!isAnimating())
>             return;
>-        m_animVal = WTF::nullopt;
>+        m_animVal = nullptr;
>         SVGAnimatedProperty::stopAnimation();
>     }
> 
>+    // Controlling the instance animation.
>+    void instanceStartAnimation(SVGAnimatedProperty& animated) override
>+    {
>+        if (isAnimating())
>+            return;
>+        m_animVal = static_cast<SVGAnimatedPrimitiveProperty&>(animated).m_animVal;
>+        SVGAnimatedProperty::instanceStartAnimation(animated);
>+    }
>+
>+    void instanceStopAnimation() override
>+    {
>+        if (!isAnimating())
>+            return;
>+        m_animVal = nullptr;
>+        SVGAnimatedProperty::instanceStopAnimation();
>+    }
>+
> protected:
>     SVGAnimatedPrimitiveProperty(SVGElement* contextElement)
>         : SVGAnimatedProperty(contextElement)
>@@ -135,7 +152,7 @@ protected:
>     }
> 
>     PropertyType m_baseVal;
>-    mutable Optional<PropertyType> m_animVal;
>+    mutable std::shared_ptr<PropertyType> m_animVal;
>     SVGPropertyState m_state { SVGPropertyState::Clean };
> };
> 
>Index: Source/WebCore/svg/properties/SVGAnimatedPropertyAnimator.h
>===================================================================
>--- Source/WebCore/svg/properties/SVGAnimatedPropertyAnimator.h	(revision 246038)
>+++ Source/WebCore/svg/properties/SVGAnimatedPropertyAnimator.h	(working copy)
>@@ -77,6 +77,8 @@ public:
>     {
>         if (isAnimatedStylePropertyAniamtor(targetElement))
>             applyAnimatedStylePropertyChange(targetElement, m_animated->animValAsString());
>+        else if (isAnimatedStyleClassAniamtor())
>+            invalidateStyle(targetElement);
>         applyAnimatedPropertyChange(targetElement);
>     }
> 
>@@ -92,6 +94,8 @@ public:
>         applyAnimatedPropertyChange(targetElement);
>         if (isAnimatedStylePropertyAniamtor(targetElement))
>             removeAnimatedStyleProperty(targetElement);
>+        else if (isAnimatedStyleClassAniamtor())
>+            invalidateStyle(targetElement);
>     }
> 
>     Optional<float> calculateDistance(SVGElement* targetElement, const String& from, const String& to) const override
>Index: Source/WebCore/svg/properties/SVGAttributeAnimator.cpp
>===================================================================
>--- Source/WebCore/svg/properties/SVGAttributeAnimator.cpp	(revision 246038)
>+++ Source/WebCore/svg/properties/SVGAttributeAnimator.cpp	(working copy)
>@@ -28,19 +28,32 @@
> 
> #include "CSSComputedStyleDeclaration.h"
> #include "CSSPropertyParser.h"
>+#include "HTMLNames.h"
> #include "SVGElement.h"
> 
> namespace WebCore {
> 
>+bool SVGAttributeAnimator::isAnimatedStyleClassAniamtor() const
>+{
>+    return m_attributeName.matches(HTMLNames::classAttr);
>+}
>+
> bool SVGAttributeAnimator::isAnimatedStylePropertyAniamtor(const SVGElement* targetElement) const
> {
>     return targetElement->isAnimatedStyleAttribute(m_attributeName);
> }
> 
>+void SVGAttributeAnimator::invalidateStyle(SVGElement* targetElement)
>+{
>+    SVGElement::InstanceInvalidationGuard guard(*targetElement);
>+    targetElement->invalidateSVGPresentationAttributeStyle();
>+}
>+
> void SVGAttributeAnimator::applyAnimatedStylePropertyChange(SVGElement* element, CSSPropertyID id, const String& value)
> {
>     ASSERT(element);
>     ASSERT(!element->m_deletionHasBegun);
>+    ASSERT(id != CSSPropertyInvalid);
>     
>     if (!element->ensureAnimatedSMILStyleProperties().setProperty(id, value, false))
>         return;
>@@ -70,6 +83,7 @@ void SVGAttributeAnimator::removeAnimate
> {
>     ASSERT(element);
>     ASSERT(!element->m_deletionHasBegun);
>+    ASSERT(id != CSSPropertyInvalid);
> 
>     element->ensureAnimatedSMILStyleProperties().removeProperty(id);
>     element->invalidateStyle();
>Index: Source/WebCore/svg/properties/SVGAttributeAnimator.h
>===================================================================
>--- Source/WebCore/svg/properties/SVGAttributeAnimator.h	(revision 246038)
>+++ Source/WebCore/svg/properties/SVGAttributeAnimator.h	(working copy)
>@@ -74,8 +74,10 @@ public:
>     virtual Optional<float> calculateDistance(SVGElement*, const String&, const String&) const { return { }; }
> 
> protected:
>+    bool isAnimatedStyleClassAniamtor() const;
>     bool isAnimatedStylePropertyAniamtor(const SVGElement*) const;
> 
>+    static void invalidateStyle(SVGElement*);
>     static void applyAnimatedStylePropertyChange(SVGElement*, CSSPropertyID, const String& value);
>     static void removeAnimatedStyleProperty(SVGElement*, CSSPropertyID);
>     static void applyAnimatedPropertyChange(SVGElement*, const QualifiedName&);
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 246038)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,17 @@
>+2019-06-03  Said Abou-Hallawa  <sabouhallawa@apple.com>
>+
>+        Assertion fires when animating the 'class' attribute of an SVG element
>+        https://bugs.webkit.org/show_bug.cgi?id=197372
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * svg/animations/animated-bool-externalResourcesRequired-instances-expected.svg: Added.
>+        * svg/animations/animated-bool-externalResourcesRequired-instances.svg: Added.
>+        * svg/animations/animated-string-class-instances-expected.svg: Added.
>+        * svg/animations/animated-string-class-instances.svg: Added.
>+        * svg/animations/animated-string-href-expected.svg: Added.
>+        * svg/animations/animated-string-href.svg: Added.
>+
> 2019-06-03  Devin Rousso  <drousso@apple.com>
> 
>         Flaky Test: inspector/canvas/recording.html
>Index: LayoutTests/svg/animations/animated-bool-externalResourcesRequired-instances-expected.svg
>===================================================================
>--- LayoutTests/svg/animations/animated-bool-externalResourcesRequired-instances-expected.svg	(nonexistent)
>+++ LayoutTests/svg/animations/animated-bool-externalResourcesRequired-instances-expected.svg	(working copy)
>@@ -0,0 +1,3 @@
>+<svg xmlns="http://www.w3.org/2000/svg">
>+    <rect width="100" height="100" fill="green"/>
>+</svg>
>Index: LayoutTests/svg/animations/animated-bool-externalResourcesRequired-instances.svg
>===================================================================
>--- LayoutTests/svg/animations/animated-bool-externalResourcesRequired-instances.svg	(nonexistent)
>+++ LayoutTests/svg/animations/animated-bool-externalResourcesRequired-instances.svg	(working copy)
>@@ -0,0 +1,9 @@
>+<svg xmlns="http://www.w3.org/2000/svg">
>+    <desc>Test that the instance of the 'externalResourcesRequired' attribute is animated correctly.</desc>
>+    <def>
>+        <rect id="rect" width="100" height="100" fill="green">
>+            <set attributeName="externalResourcesRequired" to="true"/>
>+        </rect>
>+    </def>
>+    <use href="#rect"/>
>+</svg>
>Index: LayoutTests/svg/animations/animated-string-class-instances-expected.svg
>===================================================================
>--- LayoutTests/svg/animations/animated-string-class-instances-expected.svg	(nonexistent)
>+++ LayoutTests/svg/animations/animated-string-class-instances-expected.svg	(working copy)
>@@ -0,0 +1,3 @@
>+<svg xmlns="http://www.w3.org/2000/svg">
>+    <rect width="100" height="100" fill="green"/>
>+</svg>
>Index: LayoutTests/svg/animations/animated-string-class-instances.svg
>===================================================================
>--- LayoutTests/svg/animations/animated-string-class-instances.svg	(nonexistent)
>+++ LayoutTests/svg/animations/animated-string-class-instances.svg	(working copy)
>@@ -0,0 +1,17 @@
>+<svg xmlns="http://www.w3.org/2000/svg">
>+    <desc>Test that the instance of the 'class' attribute is animated and its renderer is invalidated correctly.</desc>
>+    <style>
>+        .red-fill {
>+            fill: red;
>+        }
>+        .green-fill {
>+            fill: green;
>+        }
>+    </style>
>+    <def>
>+        <rect id="rect" width="100" height="100" class="red-fill">
>+            <set id="green" attributeName="class" to="green-fill" fill="freeze"/>
>+        </rect>
>+    </def>
>+    <use href="#rect"/>
>+</svg>
>Index: LayoutTests/svg/animations/animated-string-href-expected.svg
>===================================================================
>--- LayoutTests/svg/animations/animated-string-href-expected.svg	(nonexistent)
>+++ LayoutTests/svg/animations/animated-string-href-expected.svg	(working copy)
>@@ -0,0 +1,3 @@
>+<svg xmlns="http://www.w3.org/2000/svg">
>+    <rect width="100" height="100" fill="green"/>
>+</svg>
>Index: LayoutTests/svg/animations/animated-string-href.svg
>===================================================================
>--- LayoutTests/svg/animations/animated-string-href.svg	(nonexistent)
>+++ LayoutTests/svg/animations/animated-string-href.svg	(working copy)
>@@ -0,0 +1,17 @@
>+<svg xmlns="http://www.w3.org/2000/svg">
>+    <desc>Test that the 'href' attribute is animated correctly.</desc>
>+    <defs>
>+        <linearGradient id="red-fill">
>+            <stop offset="0%"  stop-color="red" />
>+            <stop offset="100%" stop-color="red" />
>+        </linearGradient>
>+        <linearGradient id="green-fill">
>+            <stop offset="0%"  stop-color="green" />
>+            <stop offset="100%" stop-color="green" />
>+        </linearGradient>
>+        <linearGradient id="gradient" href="#red-fill">
>+            <set attributeName="href" to="#green-fill" fill="freeze"/>
>+        </linearGradient>
>+    </defs>
>+    <rect id="rect" width="100" height="100" fill="url('#gradient')"/>
>+</svg>

Actions: View | Formatted Diff | Diff

Attachments on bug 197372: 368462 | 368463 | 371199 | 373269 | 373365 | 373371 | 373373