Bug 49437

Created attachment 73731 [details] Patch

Comment on attachment 73731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73731&action=review Hm, I'm slightly worried that we have no new tests about this. What happens when you change 'target' dynamically via JS, while the animation is running? I guess m_targetElement won't be recached from reading the code... I think you should try that and compare to Opera. > WebCore/svg/SVGAnimateMotionElement.cpp:55 > -bool SVGAnimateMotionElement::hasValidTarget() const > +bool SVGAnimateMotionElement::hasValidTarget() Why is it a non-const version? > WebCore/svg/SVGAnimateTransformElement.cpp:106 > + SVGElement* targetElement = this->targetElement(); I'd rather use const_cast<SVGAnimateTransformElement*>(this)->targetElement() here!

btw, r- as patch doesn't apply...

(In reply to comment #2) > (From update of attachment 73731 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73731&action=review > > Hm, I'm slightly worried that we have no new tests about this. Yes, asking Sean if we might upload his test. I tried to create a test my self, but couldn't reproduce such a perf issue like on his test. > What happens when you change 'target' dynamically via JS, while the animation is running? I guess m_targetElement won't be recached from reading the code... > Bug 12065 has a test for it and I plan to upload as a patch on this bug. > I think you should try that and compare to Opera. > > > WebCore/svg/SVGAnimateMotionElement.cpp:55 > > -bool SVGAnimateMotionElement::hasValidTarget() const > > +bool SVGAnimateMotionElement::hasValidTarget() targetElement() is not const anymore, since I save the reference to the target in m_targetElement, this caused these functions to be non-const. > > Why is it a non-const version? > > > WebCore/svg/SVGAnimateTransformElement.cpp:106 > > + SVGElement* targetElement = this->targetElement(); > > I'd rather use const_cast<SVGAnimateTransformElement*>(this)->targetElement() here! Well, I'm not a friend of const_cast and we would need it nearly everywhere, where we call targetElement(), like in all hasValidTarget functions. The patch did not apply because of devregion=english changes in XCode :-(

(In reply to comment #2) > (From update of attachment 73731 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73731&action=review > > Hm, I'm slightly worried that we have no new tests about this. > What happens when you change 'target' dynamically via JS, while the animation is running? I guess m_targetElement won't be recached from reading the code... Oh sorry, misunderstood the comment. You were not talking about removing the target, but _changing_ the target. Indeed this needs to be tested. Not necessarily part of this patch. And I could imagine situations that really cause wrong results. I'll write some test cases and open a new bug for these scenarios.

(In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 73731 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=73731&action=review > > > > Hm, I'm slightly worried that we have no new tests about this. > Yes, asking Sean if we might upload his test. I tried to create a test my self, but couldn't reproduce such a perf issue like on his test. Well, this would be useless, as the SVG animation won't run.... > targetElement() is not const anymore, since I save the reference to the target in m_targetElement, this caused these functions to be non-const. You mean hasValidTarget() is not const anymore? I'd rather revert the constness changes for hasValidTarget and targetElement and make the m_targetElement mutable? > The patch did not apply because of devregion=english changes in XCode :-( Ah, ok.

(In reply to comment #5) > (In reply to comment #2) > > (From update of attachment 73731 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=73731&action=review > > > > Hm, I'm slightly worried that we have no new tests about this. > > What happens when you change 'target' dynamically via JS, while the animation is running? I guess m_targetElement won't be recached from reading the code... > > Oh sorry, misunderstood the comment. You were not talking about removing the target, but _changing_ the target. Indeed this needs to be tested. Not necessarily part of this patch. And I could imagine situations that really cause wrong results. > I'll write some test cases and open a new bug for these scenarios. I think it's part of this patch, as before your patch changing the target, would be immediately cause a new targetElement() to be returned,, which is not the case anymore. (I'm sure that would reveal other bugs, but still..)

(In reply to comment #7) > I think it's part of this patch, as before your patch changing the target, would be immediately cause a new targetElement() to be returned,, which is not the case anymore. > (I'm sure that would reveal other bugs, but still..) Hm, you're right, there might be some scenarios: * simple example, the animated value changed by JS (IIRC the spec wants us to proceed the animation and set the baseVal afterwards, take a look at it) * the animation is a children of the target and we change the hierarchy in the DOM. I would expect that the animation stops or, that the old parent still gets animated till the animation stoped. * the IRI of the target changed, or more worst, another element gets the id of our current target. This is a bit more complicated and will not work with this patch. Not sure how we could handle it at all. Maybe we use the Resource handler to give back changes to the animation API. hmm..

(In reply to comment #8) > (In reply to comment #7) > > I think it's part of this patch, as before your patch changing the target, would be immediately cause a new targetElement() to be returned,, which is not the case anymore. > > (I'm sure that would reveal other bugs, but still..) > > Hm, you're right, there might be some scenarios: > * simple example, the animated value changed by JS (IIRC the spec wants us to proceed the animation and set the baseVal afterwards, take a look at it) Yes, but that's another issue, hightly related to the fact that we're animating baseVal at the moment.... no way to fix that, w/o adding "proper" animVal support. > * the animation is a children of the target and we change the hierarchy in the DOM. I would expect that the animation stops or, that the old parent still gets animated till the animation stoped. Very true, didn't think about that one. > * the IRI of the target changed, or more worst, another element gets the id of our current target. This is a bit more complicated and will not work with this patch. Not sure how we could handle it at all. Maybe we use the Resource handler to give back changes to the animation API. hmm.. Hm, indeed really tricky. With your patch, the target element could change the ID, and the animation wouldnt't be affected (it would just go on), so it would be even better than the current behaviour (maybe just write a test for that..) If an element <x> is animated, and it's id changes, it has to notify all SVGAnimate*Elements about this change (triggered from eg. SVGElement::svgAttributeChanged, when 'id' changes), similar to what we do when the id changes, that SVGResourcesCache gets updated... but that's really out of scope for this patch.

Created attachment 73755 [details] Test if changing the DOM affects animation (expected behavior) Ok, a short test for the three scenarios I mentioned above. You'll see three animations á 10s. 1. baseVal gets changed by XML-bindings after 5s to 0, shouldn't affect the animation (works in Safari) 2. two rects with id's b1 and b2, animation is applied to b1. Change ids b2 to b1 and b1 to b2 after 5 seconds, 2nd rect should get animated; first rect should be reseted to x=0. (old Safari changes the animation to the second rect, but doesn't reset first rect) 3. Move animation from first rect to second rect after 5s. first rect should get reset to x=0 second rect should proceed where the first rect stopped (looks the same like 2.). (old Safari changes the animation to the second rect, but doesn't reset first rect like for 2.) With the patch above, the first rect of 1. and the first rect of 2. get animated the whole 10 seconds. The second rects don't get touched at all. FF and Opera behave like the expected results.

Fix landed with http://trac.webkit.org/changeset/79569.