Bug 12437 - SVG Animations update baseVal instead of animVal
Summary: SVG Animations update baseVal instead of animVal
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: All All
: P2 Major
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on: 31897 63797 65167 79790
Blocks: 41761 67563 80750 80758 81212 81219 81448 81640 82144 82311 82316 82317 82326 82459 82844 83140
  Show dependency treegraph
 
Reported: 2007-01-27 04:02 PST by Eric Seidel (no email)
Modified: 2012-04-27 06:50 PDT (History)
13 users (show)

See Also:


Attachments
Draft patch (11.39 KB, patch)
2011-06-25 18:36 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.38 MB, application/zip)
2011-06-25 19:57 PDT, WebKit Review Bot
no flags Details
Next draft patch (134.61 KB, patch)
2012-03-03 06:32 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v1 (177.25 KB, patch)
2012-03-06 04:16 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (229.73 KB, patch)
2012-03-09 04:08 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v3 (231.02 KB, patch)
2012-03-10 01:47 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v4 (231.27 KB, patch)
2012-03-12 03:04 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Follow-up patch (5.62 KB, patch)
2012-03-13 09:02 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final patch v1 (38.87 KB, patch)
2012-04-04 09:10 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final patch v2 (39.05 KB, patch)
2012-04-05 03:06 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final patch v3 (Updated v2 against ToT) (38.97 KB, patch)
2012-04-25 01:33 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final patch v4 (Updated v3 against ToT) (39.81 KB, patch)
2012-04-26 02:47 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-01-27 04:02:40 PST
animations update baseVal instead of animVal

This is a fundamental problem with the current animation architecture.  All animation value access (in the animation classes) is funneled through:

String SVGAnimationElement::targetAttributeAnimatedValue() const
and
void SVGAnimationElement::setTargetAttributeAnimatedValue(const String& value)

But these don't actually grab from the animVal for the associated property, instead they grab from the baseVal (or something similar) and set to the actual renderer (or worse) the attribute baseVal.

I'm not certain how we would go about fixing this.  If we actually made these methods get/set from animVal, there would be problems with how those animVal changes would actually propagate to the renderers.  We might even need to have an "animated style" (similar to a pseudo style) which is only applied during animation.  I'm unclear as to how that all would work.

Charles Ying, suggested that we consider using a "shadow node" whenever any attribute on a node is animated.  Basically a clone of the node is made, all of the attributes on the clone, now acting as the "animated values".  The renderer would update from the cloned version instead of the base version while the animation was in progress.

I'm not sure if that's a full solution either.  Until we solve this bug, we won't be able to support anything other than fill='freeze' (as we have no way to reset back to the initial base value) nor will animVal DOM accessors work as expected.
Comment 1 Nikolas Zimmermann 2009-11-25 19:31:07 PST
Hi Eric,

I'm finally on the route to fix this aged bug :-)
Just to let anyone watching this bug know.

Cheers,
Niko
Comment 2 Nikolas Zimmermann 2011-06-25 18:36:28 PDT
Created attachment 98613 [details]
Draft patch

This adds full animVal support for SVGRectElements SVGAnimatedLength width property.
Turns out support animVal is _easy_ in the new system, compared to all approaches we ever examined!
/me is excited and tired, almost 4am.

The patch aims to show the route for animVal support, even this small change makes animVal-basics.html work flawlessly and as expected.
Comment 3 WebKit Review Bot 2011-06-25 19:56:57 PDT
Comment on attachment 98613 [details]
Draft patch

Attachment 98613 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8936918

New failing tests:
svg/animations/svglength-animation-px-to-pt.html
svg/animations/svglength-animation-px-to-in.html
svg/animations/svglength-animation-invalid-value-2.html
svg/animations/svglength-animation-invalid-value-3.html
svg/animations/svglength-animation-values.html
svg/animations/svglength-animation-number-to-number.html
inspector/styles/parse-utf8-bom.html
svg/animations/svglength-animation-LengthModeWidth.html
svg/animations/svglength-animation-invalid-value-1.html
svg/animations/svglength-animation-px-to-pc.html
svg/animations/svglength-animation-px-to-cm.html
svg/animations/svglength-animation-px-to-ems.html
svg/animations/svglength-animation-px-to-number.html
svg/animations/svglength-animation-px-to-px.html
svg/animations/svglength-animation-px-to-exs.html
svg/animations/svglength-animation-px-to-percentage.html
Comment 4 WebKit Review Bot 2011-06-25 19:57:02 PDT
Created attachment 98614 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 5 Dirk Schulze 2011-06-26 00:39:27 PDT
Needs a lot of testing of course. I just write some question that come into my mind. We should collect all questions and test them.

Does it write values back to baseVal on fill="freeze"
What happens if the target element switches during the animation? Does it cause problems with animVal?
SVGLength is used for CSS properties as well, we need a fallback for CSS.
I guess setting baseVal during animation works out of the box. What about additive="sum"? Do we recognize changes to baseVal during the animation?
Comment 6 Nikolas Zimmermann 2011-06-26 02:57:17 PDT
(In reply to comment #5)
> Needs a lot of testing of course. I just write some question that come into my mind. We should collect all questions and test them.
Where's the excitement? :-) I was very happy to see how easy it is in principle to expose animVal, now.
I agree it's a great idea to collect things to discuss/test here _first_:
 
> Does it write values back to baseVal on fill="freeze"
* The baseVal dict that's maintained in SVGSMILElement, doesn't correspond to XML DOM changes.
If I use setAttribute("width", "5000") while the animation is running, and fill is NOT freeze, then when the baseVal is restored, it should be "5000". That won't work atm.

> What happens if the target element switches during the animation? Does it cause problems with animVal?
Yes, good catch. We need to call updateAnimVal(0) in case the target changes, on the old target, and reassign the animVal to the new target.

> SVGLength is used for CSS properties as well, we need a fallback for CSS.
Sure, I don't plan to work on fixing the CSS stuff as well. What we need for CSS is a new CSSStyleSheet that contains the animated CSS property values, which is added to the existing stylesheets on top -- but that's another bug, I'm concentrating on XML DOM and avoiding setAttribute calls while animating.

> I guess setting baseVal during animation works out of the box. 
Yes the SVG DOM stuff will work out-of-the box, it's safe to script baseVal while the property is animated. It won't influence the visible result on screen.
I'm just realising that in resetToBaseValue, all we have to do is call upateAnimVal(0), we don't actually need to "reset the base value" anymore, as we didn't actually change it!

>What about additive="sum"? Do we recognize changes to baseVal during the animation?
Yes, that should already work.
Comment 7 Dirk Schulze 2011-06-26 04:21:56 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Needs a lot of testing of course. I just write some question that come into my mind. We should collect all questions and test them.
> Where's the excitement? :-) I was very happy to see how easy it is in principle to expose animVal, now.

Oh sorry. We discussed it 4 months ago and I already implemented it for SVGLength values (but of course without the animator concept). So I'm not surprised that it works. ;)

I just stopped because of different problems with multiple animations applied to the same attribute and animations applied to lists (SVGLengthList). They caused problems which I was not able to fix at that time. I expect that you'll have to change some more parts to get the lists working as well.

But of course. It's great to have it one day! I'd definitely more than happy it if you continue working on that. :D
Comment 8 Nikolas Zimmermann 2012-02-28 08:13:03 PST
Will upload a new patch, once 79784/79790 are in. These were all prep steps for this bug.
Comment 9 Nikolas Zimmermann 2012-03-03 06:32:39 PST
Created attachment 130003 [details]
Next draft patch

Not meant for review, uploading what I have so Dirk can have a look where this is going.
Comment 10 Nikolas Zimmermann 2012-03-06 04:16:55 PST
Created attachment 130352 [details]
Patch v1
Comment 11 Zoltan Herczeg 2012-03-06 05:45:25 PST
From technical point of  view this patch looks good.
Comment 12 Nikolas Zimmermann 2012-03-06 05:59:24 PST
(In reply to comment #5)
An answer to an older comment from Dirk:

> Needs a lot of testing of course. I just write some question that come into my mind. We should collect all questions and test them.

> 
> Does it write values back to baseVal on fill="freeze"
Yes, that's covered by new tests in my patch.

> What happens if the target element switches during the animation? Does it cause problems with animVal?
Funny! Just checked that we don't support target element changes anymore at all. (even w/o my patch).
SVGAnimationElement doesn't handle dynamic xlink:href changes at all. It used to work before we started caching the m_targetElement (that's a year ago at least). Shall I fix that as well in this patch, or in another?

> SVGLength is used for CSS properties as well, we need a fallback for CSS.
That's also fixed.

> I guess setting baseVal during animation works out of the box. What about additive="sum"? Do we recognize changes to baseVal during the animation?
Yes, covered by tests in the new patch.
Comment 13 Stephen Chenney 2012-03-06 11:10:55 PST
(In reply to comment #12)
> > What happens if the target element switches during the animation? Does it cause problems with animVal?
> Funny! Just checked that we don't support target element changes anymore at all. (even w/o my patch).
> SVGAnimationElement doesn't handle dynamic xlink:href changes at all. It used to work before we started caching the m_targetElement (that's a year ago at least). Shall I fix that as well in this patch, or in another?

I was recently fixing bugs where the animation target changed. We have tests for that and we do support it. When the target is changed, it resets the m_targetElement value to 0, and then the next call to targetElement() fills it in. Do you mean we don't support _all_ forms of target change?

Otherwise I looked at this and it does not seem to break anything I've recently fixed. In any case, we have tests covering the nasty cases now.
Comment 14 Nikolas Zimmermann 2012-03-07 02:00:51 PST
(In reply to comment #13)
=> I was recently fixing bugs where the animation target changed. We have tests for that and we do support it. When the target is changed, it resets the m_targetElement value to 0, and then the next call to targetElement() fills it in. Do you mean we don't support _all_ forms of target change?
Right, what we don't support is the simplest thing: animate.setAttribute(xlinkNS, "xlink:href", "#newTarget"). The only callers of resetTargetElement() which sets m_TargetElement to 0 is SVGDocumentExtensions::removeAllAnimationFromTarget(). CAlling this function will invalidate the target element stored in the SVGSMILElement.
We do that properly:
- if the target element gets removed to the document
- if the target elements id changes

Just the setAttribute() changes are missing. SVGAnimateElement doesn't use SVGURIReference, so animateElement.href doesn't exist, so SVG DOM changes can't happen. Only setAttribute() changes of the xlink:href attribute aren't considered. We need to call resetTargetElement at some point, to reevaluate the target.

An even worse problem is that SVGAnimateElement::attributeChanged invalidates the animation if _any_ property changes, even animate.setAttribute("foo", "bar") causes this. The SVGSMILElement/SVGAnimationElement also override attributeChanged, which they shouldn't do - svgAttributeChanged should be used for consistency, even if it's only used for XML dom changes, as href isn't exposed to the SVG DOM.

We should deploy the isSupportedAttribute to scheme, to only ever invalidate the animation, if an animation attribute actually changed. I'm going to consider this for a future patch, it's out of scope for this patch.

> Otherwise I looked at this and it does not seem to break anything I've recently fixed. In any case, we have tests covering the nasty cases now.
Yes glad we have them :-)
Comment 15 Nikolas Zimmermann 2012-03-07 03:18:32 PST
(In reply to comment #14)
> We should deploy the isSupportedAttribute to scheme, to only ever invalidate the animation, if an animation attribute actually changed. I'm going to consider this for a future patch, it's out of scope for this patch.

Hrm, this is actually mandatory for my patch here. Otherwise we'll see crashes, as the animVal isn't properly cleared on the old target element, if it changes. I'll include a new testcase + the needed fixes.
Comment 16 Nikolas Zimmermann 2012-03-09 04:02:37 PST
Patch v2 has even more tests, and fixes removing animation elements while the animation is running. I had to include this in the patch, because we'd crash with the new animVal support, if that's not properly done. Written the most exhaustive ChangeLog ever, as this is an important patch.
Comment 17 Nikolas Zimmermann 2012-03-09 04:08:05 PST
Created attachment 131025 [details]
Patch v2
Comment 18 Zoltan Herczeg 2012-03-09 05:33:42 PST
Comment on attachment 131025 [details]
Patch v2

Uh, this is a huge changelog :)

View in context: https://bugs.webkit.org/attachment.cgi?id=131025&action=review

> Source/WebCore/ChangeLog:109
> +            Remove support for removing properties from the override style sheet. I've added this optimization to early, we should reevaluate

to -> too

> Source/WebCore/ChangeLog:122
> +            Added new helper struct, doing element->SetInstancesUpdatedBlock(true) on construction and setInstancesUpdatesBlocked(false) on

Set -> set

> Source/WebCore/ChangeLog:154
> +            

Unnecessary spaces.

> Source/WebCore/ChangeLog:206
> +            If so, then its safe to convert to void*, pass it to the SVGPropertyTearOff::updateAnimVal, which then converts it back

its-> it's. And then is unnecessary

> Source/WebCore/ChangeLog:227
> +            rect->xAnimated()->animVal()->propertyReference() (which returns the same SVGLength& that the SVGAnimatedElement

& refers to a reference?
Comment 19 Nikolas Zimmermann 2012-03-09 06:18:22 PST
(In reply to comment #18)
Will fix the typos.

> > Source/WebCore/ChangeLog:227
> > +            rect->xAnimated()->animVal()->propertyReference() (which returns the same SVGLength& that the SVGAnimatedElement 
> & refers to a reference?
Yes this returns *m_value.
Comment 20 Nikolas Zimmermann 2012-03-10 01:47:14 PST
Created attachment 131165 [details]
Patch v3

Fixed typos, splitted up updateAnimVal in animationStarted/animationEnded, as this is much better terminology.
Comment 21 Dirk Schulze 2012-03-10 15:10:35 PST
Comment on attachment 131165 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=131165&action=review

> Source/WebCore/ChangeLog:162
> +        (WebCore::SVGSMILElement::isSupportedAttribute): Add function like all SVG*Elements have identifying their supported attributes.

This sentence doesn't make sense for me. I'll look at the function during the further review.

> Source/WebCore/ChangeLog:163
> +        (WebCore::SVGSMILElement::svgAttributeChanged):

That sounds strange. You wrote sty. about that before, still. svgAttributeChanged is for the synchronization of DOM and SVG DOM. But SMIL elements are not settable by SVG DOM. So you are at least violating the original sense of the function, no?

> Source/WebCore/ChangeLog:175
> +            We now properly call endedActiveInterval() if the m_activeState is currently Active, so that animations are shut-down

Now we call..

> Source/WebCore/svg/SVGAnimationElement.cpp:211
> +void SVGAnimationElement::svgAttributeChanged(const QualifiedName& attrName)
> +{
> +    if (!isSupportedAttribute(attrName)) {
> +        SVGSMILElement::svgAttributeChanged(attrName);
> +        return;
> +    }
> +
> +    animationAttributeChanged();

still don't understand that. SMIL elements are not animate able. You can read this in SMIL Animaitions 3.0 and even in SVG Animations. All attributes are not animatable.


I have to stop the review here and return on this step later....
Comment 22 Nikolas Zimmermann 2012-03-11 00:08:56 PST
(In reply to comment #21)
> (From update of attachment 131165 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131165&action=review
> 
> > Source/WebCore/ChangeLog:162
> > +        (WebCore::SVGSMILElement::isSupportedAttribute): Add function like all SVG*Elements have identifying their supported attributes.
> 
> This sentence doesn't make sense for me. I'll look at the function during the further review.
"Add an isSupportedAttribute() function to SVGSMILElement, covering all attributes like begin/end/dur/etc.. that animation elements support - just like all other SVG*Element classes already have".

We basically forgot about changing SVGSMILElement to this pattern.

> 
> > Source/WebCore/ChangeLog:163
> > +        (WebCore::SVGSMILElement::svgAttributeChanged):
> 
> That sounds strange. You wrote sty. about that before, still. svgAttributeChanged is for the synchronization of DOM and SVG DOM. But SMIL elements are not settable by SVG DOM. So you are at least violating the original sense of the function, no?
Absolutely. It's SVGAnimationElement which has SVGTests/SVGExternalResourcesRequired interfaces.
Currently SVGSMILElement is the only exception which impelemented attributeChanged - I've changed that for consistency.

Note that svgAttributeChanged is not about the synchronization about DOM <-> SVG DOM at all. It's just a unified path for rect.x.baseVal.value = 100; and rect.setAttribute("x", "100") to notify the SVGRectElement that its x attribute changed, either via DOM or SVG DOM (or now via animations).
 
> > Source/WebCore/ChangeLog:175
> > +            We now properly call endedActiveInterval() if the m_activeState is currently Active, so that animations are shut-down
> 
> Now we call..
Ok.

> 
> > Source/WebCore/svg/SVGAnimationElement.cpp:211
> > +void SVGAnimationElement::svgAttributeChanged(const QualifiedName& attrName)
> > +{
> > +    if (!isSupportedAttribute(attrName)) {
> > +        SVGSMILElement::svgAttributeChanged(attrName);
> > +        return;
> > +    }
> > +
> > +    animationAttributeChanged();
> 
> still don't understand that. SMIL elements are not animate able. You can read this in SMIL Animaitions 3.0 and even in SVG Animations. All attributes are not animatable.
Ah you're mixing something up. SVGAnimationElement::svgAttributeChanged() needs to determine if one if it's _own_ properties changed (keySplines/keyTimes/etc..) if yes, we have to call animationAttributeChanged(), so that SVGAnimateElement & co can reset their active state to Inactive, so that the next timer call sets up the animation again with the new values.

"animationAttributeChanged()" is not related to an attribute change of the _target_ of the element. I think you mixed that up here.

> I have to stop the review here and return on this step later....
Comment 23 Zoltan Herczeg 2012-03-11 01:21:22 PST
Comment on attachment 131165 [details]
Patch v3

In general, this patch looks good, and I am willing to give an r+ it tomorrow if noone objects and the minor issues below are discussed. I didn't read the changelog again, I suspect it was not changed except the typos.

View in context: https://bugs.webkit.org/attachment.cgi?id=131165&action=review

SVGGenericAnimatedType

> Source/WebCore/svg/SVGAnimatedType.cpp:430
> +bool SVGAnimatedType::supportsAnimVal(AnimatedPropertyType type)

This looks like a simple enough function to be inline.

> Source/WebCore/svg/SVGAnimatedType.cpp:459
> +void SVGAnimatedType::setValue(SVGGenericAnimatedType type)

I cannot find the definition of SVGGenericAnimatedType but it looks like an object, not an enum, which I expect from the name. And SVGAnimatedType always used with a * which indicates it is a pointer. Could we make this something more object like?

class SVGGenericAnimatedType;
And use SVGGenericAnimatedType* everywhere.

> Source/WebCore/svg/SVGAnimatedType.h:61
> +    SVGGenericAnimatedType variant() const { return m_data.variant; }

Perhaps there is a convention this case, but I think a value or variantValue would sounds better to me. Would be more consistent with the setValue below.

> Source/WebCore/svg/properties/SVGAnimatedProperty.h:52
> +    virtual void animationStarted(SVGAnimatedType*) { ASSERT_NOT_REACHED(); }
> +    virtual void animationEnded() { ASSERT_NOT_REACHED(); }
> +    virtual void animationValueChanged() { ASSERT_NOT_REACHED(); }
> +    virtual SVGGenericAnimatedType currentBaseValue(AnimatedPropertyType) const { ASSERT_NOT_REACHED(); return 0; }

Should these become pure virtual eventually? If so, we should leave a comment here.

> Source/WebCore/svg/properties/SVGAnimatedPropertyTearOff.h:67
> +        ASSERT(animatedPropertyType() == expectedPropertyType);
> +        // If these types don't match, always return 0 here in release builds, to avoid trashing memory.
> +        if (expectedPropertyType != animatedPropertyType())
> +            return 0;

This looks dangerous... The assert says it must never happen. How can it still be? Please describe a situation, and a future solution possibility at least.

> Source/WebCore/svg/properties/SVGPropertyInfo.h:73
> +typedef void* SVGGenericAnimatedType;

Ah, here is the definition. I suggest the class thing above.
Comment 24 Nikolas Zimmermann 2012-03-11 01:36:33 PST
(In reply to comment #23)
> > Source/WebCore/svg/SVGAnimatedType.cpp:430
> > +bool SVGAnimatedType::supportsAnimVal(AnimatedPropertyType type)
> This looks like a simple enough function to be inline.
Mark as inline you mean?
 
> > Source/WebCore/svg/SVGAnimatedType.cpp:459
> > +void SVGAnimatedType::setValue(SVGGenericAnimatedType type)
> 
> I cannot find the definition of SVGGenericAnimatedType but it looks like an object, not an enum, which I expect from the name. And SVGAnimatedType always used with a * which indicates it is a pointer. Could we make this something more object like?
> 
> class SVGGenericAnimatedType;
> And use SVGGenericAnimatedType* everywhere.
Ok good idea, better than a typedef void*.
 
> > Source/WebCore/svg/SVGAnimatedType.h:61
> > +    SVGGenericAnimatedType variant() const { return m_data.variant; }
> 
> Perhaps there is a convention this case, but I think a value or variantValue would sounds better to me. Would be more consistent with the setValue below.
Ok, how about setVariantValue, and variantValue?
 
> > Source/WebCore/svg/properties/SVGAnimatedProperty.h:52
> > +    virtual void animationStarted(SVGAnimatedType*) { ASSERT_NOT_REACHED(); }
> > +    virtual void animationEnded() { ASSERT_NOT_REACHED(); }
> > +    virtual void animationValueChanged() { ASSERT_NOT_REACHED(); }
> > +    virtual SVGGenericAnimatedType currentBaseValue(AnimatedPropertyType) const { ASSERT_NOT_REACHED(); return 0; }
> 
> Should these become pure virtual eventually? If so, we should leave a comment here.
Nope they should stay as-is, and make sure we only ever use the animVal framework for those SVGAnimated* types that support it. It's only an aid.
 
> > Source/WebCore/svg/properties/SVGAnimatedPropertyTearOff.h:67
> > +        ASSERT(animatedPropertyType() == expectedPropertyType);
> This looks dangerous... The assert says it must never happen. How can it still be? Please describe a situation, and a future solution possibility at least.
You're right, i'll move this into an ASSERT_UNUSED(expectedPropertyType, animatedPropertyType() == expectedPropertyType) again. The code guarantees this idiom at present, and we try hard to enforce it. (animatedEnded() is guaranteed to be called on the SVGAnimatedProperty, every time the animatedPropertyType() changes, and we enforce this using an ASSERT(!m_isAnimating) in the destructor of SVGAnimatedPropertyTearOff).
Also we have plenty of test cases found by fuzzing, fixed by schenney, that cover updating the animated property type in general (we had problems before, but now assertions in SVGAnimateElement guarantee that the expected property type (which the SVGAnimatedType holds) and the property type of the SVGAnimatedProperty* match).

To summarize:
<rect x="10"...> <animate attributeName="x"...>
the <animate> runs stores a OwnPtr<SVGAnimatedType> m_animatedType, baking a SVGLength object, as 'x' is an AnimatedLength. The SVGAnimatedProperty<SVGLength> (SVGAnimatedLength) object for the SVGRectElement x property, is also associated with an AnimatedLength type.

The assertion above just checks that if we expect 'x' to be a length, that it's really a length and not a length list. By using this assertion we can guarantee in debug build, that the attribute name to -> attribute type property sets are correct (see SVGStyledElement).
 
> > Source/WebCore/svg/properties/SVGPropertyInfo.h:73
> > +typedef void* SVGGenericAnimatedType;
> Ah, here is the definition. I suggest the class thing above.
As discussed on IRC, I can use "class Foo; and pass around Foo*" everywhere, without ever needing to define Foo. That's what I want!
Comment 25 Dirk Schulze 2012-03-11 08:28:08 PDT
Comment on attachment 131165 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=131165&action=review

>>> Source/WebCore/ChangeLog:162
>>> +        (WebCore::SVGSMILElement::isSupportedAttribute): Add function like all SVG*Elements have identifying their supported attributes.
>> 
>> This sentence doesn't make sense for me. I'll look at the function during the further review.
> 
> "Add an isSupportedAttribute() function to SVGSMILElement, covering all attributes like begin/end/dur/etc.. that animation elements support - just like all other SVG*Element classes already have".
> 
> We basically forgot about changing SVGSMILElement to this pattern.

Hm, according to SVG there shouldn't be a SMILElement at all. That is sth that we introduced for the organization of animations. Should't SVGAnimationElement have all attributes already? SMILElement should not introduce new attributes?

>>> Source/WebCore/svg/SVGAnimationElement.cpp:211
>>> +    animationAttributeChanged();
>> 
>> still don't understand that. SMIL elements are not animate able. You can read this in SMIL Animaitions 3.0 and even in SVG Animations. All attributes are not animatable.
>> 
>> 
>> I have to stop the review here and return on this step later....
> 
> Ah you're mixing something up. SVGAnimationElement::svgAttributeChanged() needs to determine if one if it's _own_ properties changed (keySplines/keyTimes/etc..) if yes, we have to call animationAttributeChanged(), so that SVGAnimateElement & co can reset their active state to Inactive, so that the next timer call sets up the animation again with the new values.
> 
> "animationAttributeChanged()" is not related to an attribute change of the _target_ of the element. I think you mixed that up here.

No, I know that it just affects the animation element. That is what confuses me. These elements can not set by SVG DOM (animateElement.begin.baseVal or so), nor can you animate them. After all they are still SVGElements, so you can rename them. Nevertheless, others might think that these elements have attributes that are animated (.. have SVG DOM), what  shouldn't be the case, even after inherence from SVGTest, SVGElement and SVGExternalResourcesRequired.

>> Source/WebCore/svg/properties/SVGAnimatedProperty.h:52
>> +    virtual SVGGenericAnimatedType currentBaseValue(AnimatedPropertyType) const { ASSERT_NOT_REACHED(); return 0; }
> 
> Should these become pure virtual eventually? If so, we should leave a comment here.

Also, it is the wrong style. Multilines should be multilines in the header. RenderObject would move this function to CPP.

> LayoutTests/ChangeLog:304
> +        * svg/repaint/repainting-after-animation-element-removal.svg: Added.

I still think that we should try to write ref tests for them. For your example it is quite easy:

<html>
<style>
body {
   background-color: grey;
}
div {
   position: absolute;
   width: 100px;
   height: 100px;
}
</style>
<body]>
<div style="background-color: green"></div>
<div style="background-color: white"></div>
</body>
</html>
Comment 26 Dirk Schulze 2012-03-11 08:29:46 PDT
(In reply to comment #24)

> <html>
> <style>
> body {
>    background-color: grey;
> }
> div {
>    position: absolute;
>    width: 100px;
>    height: 100px;
> }
> </style>
> <body]>
> <div style="background-color: green"></div>
> <div style="background-color: white"></div>
> </body>
> </html>

The div boxes must be inlined of course (or floated)
Comment 27 Nikolas Zimmermann 2012-03-11 09:24:15 PDT
(In reply to comment #25)
> Hm, according to SVG there shouldn't be a SMILElement at all. That is sth that we introduced for the organization of animations. Should't SVGAnimationElement have all attributes already? SMILElement should not introduce new attributes?
It doesn't introduce new attributes. You're aware that begin/end/dur is handled in SVGSMILElement, not in SVGAnimationElement? That's why it handles these attributes instead of SVGAnimationElement, that's al.


> No, I know that it just affects the animation element. That is what confuses me. These elements can not set by SVG DOM (animateElement.begin.baseVal or so), nor can you animate them. After all they are still SVGElements, so you can rename them. Nevertheless, others might think that these elements have attributes that are animated (.. have SVG DOM), what  shouldn't be the case, even after inherence from SVGTest, SVGElement and SVGExternalResourcesRequired.
Ah okay. But remember svgAttributeChanged() is not only used for SVG DOM changes, so it's still appropriate to implement that in SVGSMILElement. I could also special case attributeChanged in SVGAnimationElement, but this way its more consistent.

> >> Source/WebCore/svg/properties/SVGAnimatedProperty.h:52
> >> +    virtual SVGGenericAnimatedType currentBaseValue(AnimatedPropertyType) const { ASSERT_NOT_REACHED(); return 0; }
> > 
> > Should these become pure virtual eventually? If so, we should leave a comment here.
> 
> Also, it is the wrong style. Multilines should be multilines in the header. RenderObject would move this function to CPP.
I can do that. There is no cpp file here (this is a template type!) so I can use multilines if you prefer.

> 
> > LayoutTests/ChangeLog:304
> > +        * svg/repaint/repainting-after-animation-element-removal.svg: Added.
> 
> I still think that we should try to write ref tests for them. For your example it is quite easy:
Suppose attaching the actual animation to the target fails somewhen in future in trunk, we wouldn't notice this at all, if we'd use a reftest for this.

This specific test animates a property with fill="freeze", and removes the animation while running, to see that it resets back to initial state. Using a repaint test here makes most sense IMHO.

(If I don't use scripting I use reftests in future for all tests)
Comment 28 Nikolas Zimmermann 2012-03-12 03:04:03 PDT
Created attachment 131305 [details]
Patch v4

Fixed both Dirks & Zoltans suggestions.
Comment 29 Dirk Schulze 2012-03-12 09:27:09 PDT
(In reply to comment #27)
> (In reply to comment #25)
> > Hm, according to SVG there shouldn't be a SMILElement at all. That is sth that we introduced for the organization of animations. Should't SVGAnimationElement have all attributes already? SMILElement should not introduce new attributes?
> It doesn't introduce new attributes. You're aware that begin/end/dur is handled in SVGSMILElement, not in SVGAnimationElement? That's why it handles these attributes instead of SVGAnimationElement, that's al.
> 
> 
> > No, I know that it just affects the animation element. That is what confuses me. These elements can not set by SVG DOM (animateElement.begin.baseVal or so), nor can you animate them. After all they are still SVGElements, so you can rename them. Nevertheless, others might think that these elements have attributes that are animated (.. have SVG DOM), what  shouldn't be the case, even after inherence from SVGTest, SVGElement and SVGExternalResourcesRequired.
> Ah okay. But remember svgAttributeChanged() is not only used for SVG DOM changes, so it's still appropriate to implement that in SVGSMILElement. I could also special case attributeChanged in SVGAnimationElement, but this way its more consistent.
> 
> > >> Source/WebCore/svg/properties/SVGAnimatedProperty.h:52
> > >> +    virtual SVGGenericAnimatedType currentBaseValue(AnimatedPropertyType) const { ASSERT_NOT_REACHED(); return 0; }
> > > 
> > > Should these become pure virtual eventually? If so, we should leave a comment here.
> > 
> > Also, it is the wrong style. Multilines should be multilines in the header. RenderObject would move this function to CPP.
> I can do that. There is no cpp file here (this is a template type!) so I can use multilines if you prefer.
> 
> > 
> > > LayoutTests/ChangeLog:304
> > > +        * svg/repaint/repainting-after-animation-element-removal.svg: Added.
> > 
> > I still think that we should try to write ref tests for them. For your example it is quite easy:
> Suppose attaching the actual animation to the target fails somewhen in future in trunk, we wouldn't notice this at all, if we'd use a reftest for this.
> 
> This specific test animates a property with fill="freeze", and removes the animation while running, to see that it resets back to initial state. Using a repaint test here makes most sense IMHO.
> 
> (If I don't use scripting I use reftests in future for all tests)

Ok, svgattr is fine with me

You misunderstood me with ref tests. You should use repaint logic. That is fine. But use a ref test instead of pixel test. That would not harm you on repaint.js. You can do both!
Comment 30 Nikolas Zimmermann 2012-03-12 10:47:46 PDT
(In reply to comment #29)
> You misunderstood me with ref tests. You should use repaint logic. That is fine. But use a ref test instead of pixel test. That would not harm you on repaint.js. You can do both!
How is that supposed to work? Shall I emulate the gray rect which is painted over the web view?
This proposal doesn't make sense to me, either you want to use reftests or repaint.js tests, not both.
Comment 31 Dirk Schulze 2012-03-12 12:57:28 PDT
Comment on attachment 131305 [details]
Patch v4

Joined forces on reviewing this patch :D Assuming that Zoltan would give r+ on this patch (as I understand Niko), I give my r+ as well.
Comment 32 Dirk Schulze 2012-03-12 13:03:17 PDT
We discussed ref tests vs. repaint tests on IRC.

Like Niko said, it is hard to use ref tests in _combination_ with repaint tests, since the testing harness is implemented in different ways on different platforms. So the combination is no option here.

The repaint test is still useful, since we _explicitly_ want to test the repainting to be sure that we really started the animation in the first place. This seems to be nearly impossible with ref tests.
Comment 33 Nikolas Zimmermann 2012-03-13 01:28:01 PDT
Committed r110545: <http://trac.webkit.org/changeset/110545>
Comment 34 Nikolas Zimmermann 2012-03-13 01:42:42 PDT
Keeping this bug open as master bug, as all other primitives except SVGLength are left TODO.
Comment 35 Eric Seidel (no email) 2012-03-13 01:45:55 PDT
Wow, what an ancient bug.  :)  Thanks for the fix.
Comment 36 Hajime Morrita 2012-03-13 06:49:48 PDT
(In reply to comment #35)
> Wow, what an ancient bug.  :)  Thanks for the fix.

Bad news is that we have some crashes on chromium-mac, probably due to this change ;-/
https://gist.github.com/2028708
Maybe innocent though.
Comment 37 Nikolas Zimmermann 2012-03-13 09:02:27 PDT
Created attachment 131631 [details]
Follow-up patch
Comment 38 Nikolas Zimmermann 2012-03-13 09:16:23 PDT
Let's try if the follow-up patch resolves the problem for V8 as well. I couldn't reproduce it with JSC.
Comment 39 Dirk Schulze 2012-03-13 09:56:43 PDT
Comment on attachment 131631 [details]
Follow-up patch

LGTM. Even so I am surprised that it just occurs on Mac chromium.
Comment 40 Stephen Chenney 2012-03-13 10:02:07 PDT
It also occurs on chromium linux. So it is really V8 specific, apparently.
Comment 41 Dirk Schulze 2012-03-13 10:09:52 PDT
(In reply to comment #40)
> It also occurs on chromium linux. So it is really V8 specific, apparently.

Oh, is the build bot a release vernon? Otherwise I wonder why we don't have the assertion problem on the EWS bots.
Comment 42 Nikolas Zimmermann 2012-03-13 12:28:20 PDT
Comment on attachment 131631 [details]
Follow-up patch

Follow-up patch is in, I hope that fixes the CR assertions.
I ran nrwt --tolerance 0 -p svg --gc-between-tests w/o problems.
Comment 43 Nikolas Zimmermann 2012-03-13 12:28:50 PDT
(In reply to comment #42)
> (From update of attachment 131631 [details])
> Follow-up patch is in, I hope that fixes the CR assertions.
> I ran nrwt --tolerance 0 -p svg --gc-between-tests w/o problems.

Committed r110591: <http://trac.webkit.org/changeset/110591>
Comment 44 Eric Seidel (no email) 2012-03-27 12:51:48 PDT
Comment on attachment 131305 [details]
Patch v4

Cleared Dirk Schulze's review+ from obsolete attachment 131305 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 45 Eric Seidel (no email) 2012-03-27 12:51:55 PDT
Comment on attachment 131631 [details]
Follow-up patch

Cleared Dirk Schulze's review+ from obsolete attachment 131631 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 46 Nikolas Zimmermann 2012-03-28 07:10:48 PDT
Once bug 82459 lands, animVal support can be considered done for all SVG DOM primitives.

Before we can close this bug "SVG Animations update baseVal instead of animVal" two last issues have to be resolved with SVGPoly*Element and SVGPathElement.

SVGPoly*Element has "readonly attribute SVGPointList points" and "readonly attribute SVGPointList animatedPoints", which act as baseVal / animVal. Same for SVGPathElements "readonly attribute SVGPathSegList pathSegList / animatedPathSegList".
We currently still update points and pathSegList instead of animatedPoints / animatedPathSegList. Its very easy to fix in the new animVal framework - I'll get to it soon.
Comment 47 Nikolas Zimmermann 2012-04-04 04:13:18 PDT
(In reply to comment #46)
> Once bug 82459 lands, animVal support can be considered done for all SVG DOM primitives.
Done. AnimatedPoints also finished and landed in trunk, now AnimatedPath is the only missing thing: bug 83140 has a patch for this.
Comment 48 Nikolas Zimmermann 2012-04-04 09:10:03 PDT
Created attachment 135605 [details]
Final patch v1
Comment 49 Nikolas Zimmermann 2012-04-05 01:10:08 PDT
(In reply to comment #48)
> Created an attachment (id=135605) [details]
> Patch

I discussed parts of this patch with Antti yesterday, he's worried about the SMILStyleChange introduction. I'll decouple this from the patch, and try another approach.
Comment 50 Nikolas Zimmermann 2012-04-05 03:06:43 PDT
Created attachment 135792 [details]
Final patch v2
Comment 51 Nikolas Zimmermann 2012-04-25 01:33:15 PDT
Created attachment 138758 [details]
Final patch v3 (Updated v2 against ToT)
Comment 52 Antti Koivisto 2012-04-25 01:45:09 PDT
Comment on attachment 135792 [details]
Final patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=135792&action=review

Looks ok to me but Dirk better take a look too.

> Source/WebCore/svg/SVGAnimateElement.cpp:68
> +    value = CSSComputedStyleDeclaration::create(element)->getPropertyValue(id);

This is an unfortunate use of CSSOM API type but I guess we don't have internal way to do it yet.
Comment 53 Nikolas Zimmermann 2012-04-26 00:39:29 PDT
Dirk was worried about the order when CSS Animations/Transitions/SMIL CSS Style changes are applied.
This patch does NOT change the existing behavior, it stays as-is (including the layering bugs we currently have when css anims/transitions and SMIL animations run at the same time). The main benefit of this patch is to remove the caching of CSS properties, as that fully breaks additive="sum" for CSS properties. The cache in SMILTimeContainer is never updated, and my simple goal is to remove the cache, and to keep the behaviour for retrieving CSS "base values" as is, just without the cache.

All layering bugs should be addressed separated.
Comment 54 Nikolas Zimmermann 2012-04-26 02:47:32 PDT
Created attachment 138960 [details]
Final patch v4 (Updated v3 against ToT)
Comment 55 Dirk Schulze 2012-04-26 21:54:54 PDT
Comment on attachment 138960 [details]
Final patch v4 (Updated v3 against ToT)

View in context: https://bugs.webkit.org/attachment.cgi?id=138960&action=review

The patch and the tests look good to me. I would suggest that you use a different term for computedOverrideStyle. This term seems to be based on SMIL, but is questionable nowadays where we have more animations than just SMIL on properties. I give you r=me but with the renaming or a response why you stay on this naming before landing.

> Source/WebCore/svg/SVGElementRareData.h:110
> +    RenderStyle* overrideComputedStyle(SVGElement* element)
> +    {
> +        ASSERT(element);
> +        if (!m_useOverrideComputedStyle)
> +            return 0;
> +        if (!m_needsOverrideComputedStyleUpdate && m_overrideComputedStyle)
> +            return m_overrideComputedStyle.get();
> +
> +        m_needsOverrideComputedStyleUpdate = false;
> +        if (!element->document()) {
> +            m_overrideComputedStyle.clear();
> +            return 0;
> +        }
> +
> +        m_overrideComputedStyle = element->document()->styleResolver()->styleForElement(element, element->parentNode() ? element->parentNode()->computedStyle() : 0, AllowStyleSharing, MatchAllRulesExcludingSMIL);
> +        return m_overrideComputedStyle.get();
> +    }

with your clarification it makes more sense what you do here. At least I believe that. override style and computed style terms are still confusing and shouldn't be used here, since they include animations. We discussed it on IRC and by mail and you don't want to include these. So why not use the new term "intrinsic style"? This seems to be introduced in CSS Animations: http://www.w3.org/TR/css3-animations/#animations. There is a bug report on W3C bugzilla to nail the definition down, but during writing this review the server was down. I think you should consider using "intrinsicStyle".
Comment 56 Nikolas Zimmermann 2012-04-27 06:43:47 PDT
(In reply to comment #55)
> (From update of attachment 138960 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138960&action=review
> 
> The patch and the tests look good to me. I would suggest that you use a different term for computedOverrideStyle. This term seems to be based on SMIL, but is questionable nowadays where we have more animations than just SMIL on properties. I give you r=me but with the renaming or a response why you stay on this naming before landing.

Ah, I now see why you disliked the name before. It's not meant as SMIL term, but as WebKit term. We're using computeCSSPropertyValue() to figure out the "base value" for a CSS property, which in turn uses "CSSComputedStyleDeclaration::create(element)->getPropertyValue(id);". The computed style would include all effects of CSS Animations/Transitions and previous SMIL animations. This is not what we want, so we have to override the "computed style" (which would normally return the renderers current style()) to instead operator on the "intrinsic style" (without CSS animations/transitions/SMIL animations applied).
That's why I'm using this term, as we override the computed style by the "intrinsic style" temporarily, to figure out our base value. Hope this is reasonable.

Note: I've filed bug 85059 to look at the effect of CSS Transitions + SMIL animations running at the same time.
Comment 57 Nikolas Zimmermann 2012-04-27 06:50:14 PDT
Committed r115423: <http://trac.webkit.org/changeset/115423>