Bug 12437

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

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 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

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

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?

(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.

(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

Will upload a new patch, once 79784/79790 are in. These were all prep steps for this bug.

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.

Created attachment 130352 [details] Patch v1

From technical point of view this patch looks good.

(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.

(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.

(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 :-)

(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.

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.

Created attachment 131025 [details] Patch v2

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?

(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.

Created attachment 131165 [details] Patch v3 Fixed typos, splitted up updateAnimVal in animationStarted/animationEnded, as this is much better terminology.

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....

(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 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.

(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 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>

(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)

(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)

Created attachment 131305 [details] Patch v4 Fixed both Dirks & Zoltans suggestions.

(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!

(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 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.

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.

Committed r110545: <http://trac.webkit.org/changeset/110545>

Keeping this bug open as master bug, as all other primitives except SVGLength are left TODO.

Wow, what an ancient bug. :) Thanks for the fix.

(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.

Created attachment 131631 [details] Follow-up patch

Let's try if the follow-up patch resolves the problem for V8 as well. I couldn't reproduce it with JSC.

Comment on attachment 131631 [details] Follow-up patch LGTM. Even so I am surprised that it just occurs on Mac chromium.

It also occurs on chromium linux. So it is really V8 specific, apparently.

(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 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.

(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 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 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.

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.

(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.

Created attachment 135605 [details] Final patch v1

(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.

Created attachment 135792 [details] Final patch v2

Created attachment 138758 [details] Final patch v3 (Updated v2 against ToT)

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.

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.

Created attachment 138960 [details] Final patch v4 (Updated v3 against ToT)

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".

(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.

Committed r115423: <http://trac.webkit.org/changeset/115423>