Bug 26019

Created attachment 30667 [details] reduced testcase for this bug

do these two bugs have a common issue related to the implementation of beginElement and endElement?

Created attachment 67268 [details] Fixed bug 26019 AnimationValid was reset but not ActiveState of SvgAnimationElement. So reset the ActiveState as well.

Attachment 67268 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 67268 [details] Fixed bug 26019 This is not a text plain patch file. Please upload a new patch using webkit-patch upload.

Created attachment 67271 [details] Fixed bug 26019 AnimationValid was reset but not ActiveState of SvgAnimationElement. So reset the ActiveState as well.

Comment on attachment 67271 [details] Fixed bug 26019 > diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog > index bfaa4fa..d25d800 100644 > --- a/LayoutTests/ChangeLog > +++ b/LayoutTests/ChangeLog > @@ -1,3 +1,14 @@ > +2010-09-10 Dinesh K Garg <dineshg@codeaurora.org> > + > + Reviewed by NOBODY (OOPS!). > + > + New test case for BugId - 26019: You can omit this line. > --- /dev/null > +++ b/LayoutTests/svg/animations/animate-beginElementAt.svg > @@ -0,0 +1,56 @@ > +<?xml version="1.0" encoding="utf-8" standalone="no"?> > +<svg > + xmlns:xlink="http://www.w3.org/1999/xlink" > +width='100%' height='100%' xmlns='http://www.w3.org/2000/svg' > > + <title>Animated Values Test</title> > + > +<circle cx='0' cy='50' r='20' style='fill:orange; ' id='testCircle'> > + <animate attributeName='cx' attributeType='XML' begin='indefinite' end='indefinite' dur='10s' repeatCount='indefinite' from="10" to="200" id='dTripper' fill="freeze" /> > +</circle> > + > +<text y="130" x="20" id="expectedResult">This test verifies the animation behaviour of beginElement in SVG. BugId-26019</text> > + > +<text y="150" x="20" id="console"/> > + > +<script><![CDATA[ > + > + var animatedElement; > + animatedElement = document.getElementById("dTripper"); > + animatedElement.setAttribute("to", 420); > + animatedElement.beginElement(); > + setTimeout(beginElement,100); I don't understand, why you are doing setAttribute("to".., then calling beginElement, and then setTimeout(beginElement, 100) which does the same? Please explain why this is needed. Also you shouldn't use 100ms as timeout, try setTImeout(beginElement, 0) if you want it async. > + > + function stopCircle(evt) > + { > + animatedElement.endElement(); > + } > + > + function beginElement() > + { > + animatedElement.setAttribute("to", 420); > + animatedElement.beginElement(); > + setTimeout(dumpResult,100); Same here, just use setTimeout(dumpResult, 0) > +++ b/WebCore/ChangeLog > @@ -1,3 +1,19 @@ > +2010-09-10 Dinesh K Garg <dineshg@codeaurora.org> > + > + Reviewed by NOBODY (OOPS!). > + > + beginElement broken by setAttribute > + https://bugs.webkit.org/show_bug.cgi?id=26019 Add a newline here. > + Initialized animation element state in beginElementAt The why is missing here, please provide a more detailed ChangeLog, another sentence describing why it is needed etc. > + > + Tests: svg/animations/animate-beginElementAt.svg > + > + * svg/SVGAnimationElement.cpp: > + (WebCore::SVGAnimationElement::beginElementAt): > + (WebCore::SVGAnimationElement::updateAnimation): > + * svg/animation/SVGSMILElement.h: > + (WebCore::SVGSMILElement::setActiveState): > + > 2010-09-10 Ryosuke Niwa <rniwa@webkit.org> > > Reviewed by Antonio Gomes. > diff --git a/WebCore/svg/SVGAnimationElement.cpp b/WebCore/svg/SVGAnimationElement.cpp > index b5eaafc..270fbbd 100644 > --- a/WebCore/svg/SVGAnimationElement.cpp > +++ b/WebCore/svg/SVGAnimationElement.cpp > @@ -159,6 +159,8 @@ void SVGAnimationElement::attributeChanged(Attribute* attr, bool preserveDecls) > { > // Assumptions may not hold after an attribute change. > m_animationValid = false; > + // Reset the animation to InActive state as well. > + setInactive(); > SVGSMILElement::attributeChanged(attr, preserveDecls); > } > > @@ -556,7 +558,7 @@ void SVGAnimationElement::updateAnimation(float percent, unsigned repeat, SVGSMI > { > if (!m_animationValid) > return; > - > + This change is unrelated, please leave it out. > diff --git a/WebCore/svg/animation/SVGSMILElement.h b/WebCore/svg/animation/SVGSMILElement.h > index ed8bbf8..73a8f9d 100644 > --- a/WebCore/svg/animation/SVGSMILElement.h > +++ b/WebCore/svg/animation/SVGSMILElement.h > @@ -88,7 +88,7 @@ namespace WebCore { > bool isContributing(SMILTime elapsed) const; > bool isInactive() const; > bool isFrozen() const; > - > + Ditto. Not needed. Other than that, looks nice. Please upload a new version.

Created attachment 69401 [details] Propose fix after review comments (In reply to comment #7) > > + New test case for BugId - 26019: > You can omit this line. Done > > + var animatedElement; > > + animatedElement = document.getElementById("dTripper"); > > + animatedElement.setAttribute("to", 420); > > + animatedElement.beginElement(); > > + setTimeout(beginElement,100); > I don't understand, why you are doing setAttribute("to".., then calling beginElement, and then setTimeout(beginElement, 100) which does the same? Please explain why this is needed. I am removing setAttribute from here. setAttribute() breaks beginElementAt() when beginElementAt() has been called. i.e. beginElementAt() - setAttribute() - beginElementAt() is broken but not setAttribute() - beginElementAt() > Also you shouldn't use 100ms as timeout, try setTImeout(beginElement, 0) if you want it async. I want some timegap after beginElementAt to be sure that Animation has started. > > + function beginElement() > > + { > > + animatedElement.setAttribute("to", 420); > > + animatedElement.beginElement(); > > + setTimeout(dumpResult,100); > Same here, just use setTimeout(dumpResult, 0) I want circle to move so that I can compare circle positions i.e. current and last one. Value 0 wont help me. if bug fix is working circle will move next time beginElementAt() is called and then dumpresult shall compare current and positions and dump result accordingly. > > + beginElement broken by setAttribute > > + https://bugs.webkit.org/show_bug.cgi?id=26019 > Add a newline here. Done > > + Initialized animation element state in beginElementAt > The why is missing here, please provide a more detailed ChangeLog, another sentence describing why it is needed etc. Done > > @@ -556,7 +558,7 @@ void SVGAnimationElement::updateAnimation(float percent, unsigned repeat, SVGSMI > > - > > + > This change is unrelated, please leave it out. Done > > diff --git a/WebCore/svg/animation/SVGSMILElement.h b/WebCore/svg/animation/SVGSMILElement.h > > @@ -88,7 +88,7 @@ namespace WebCore { > > bool isContributing(SMILTime elapsed) const; > > bool isInactive() const; > > bool isFrozen() const; > > - > > + > Ditto. Not needed. Done > Other than that, looks nice. Please upload a new version.

Comment on attachment 69401 [details] Propose fix after review comments Generally, contributors don't add themselves to the copyright line when adding one line to a file.

Comment on attachment 69401 [details] Propose fix after review comments As Adam said, this is not a significant contribution. Please remove copyright change.

(In reply to comment #10) Sure. I will upload one without copyright soon.

Created attachment 89279 [details] Updated patch - addressing Chris' comments.

Comment on attachment 89279 [details] Updated patch - addressing Chris' comments. View in context: https://bugs.webkit.org/attachment.cgi?id=89279&action=review The code change is great, the test just needs a simple tweak, then it's ready... > LayoutTests/svg/animations/animate-beginElementAt.svg:3 > + xmlns:xlink="http://www.w3.org/1999/xlink" xlink namespace is not needed here. Just make it: <svg xmlns="..."> > LayoutTests/svg/animations/animate-beginElementAt.svg:5 > + <title>Animated Values Test</title> Not needed. > LayoutTests/svg/animations/animate-beginElementAt.svg:7 > +<circle cx='0' cy='50' r='20' style='fill:orange; ' id='testCircle'> you could use fill="orange", no need to use the inline style attribute. The id attribute is not needed as well. > LayoutTests/svg/animations/animate-beginElementAt.svg:21 > + setTimeout(beginElement,100); s/,100/, 0/ > LayoutTests/svg/animations/animate-beginElementAt.svg:32 > + setTimeout(dumpResult,100); Ditto. > LayoutTests/svg/animations/animate-beginElementAt.svg:38 > + if(document.getElementById("testCircle").getAttribute("cx") < 11 ) Formatting: if (document.... < 11) > LayoutTests/svg/animations/animate-beginElementAt.svg:42 > + stopCircle(); Do you really need another method here? Just remove it IMHO.

> > LayoutTests/svg/animations/animate-beginElementAt.svg:7 > > +<circle cx='0' cy='50' r='20' style='fill:orange; ' id='testCircle'> > > you could use fill="orange", no need to use the inline style attribute. The id attribute is not needed as well. The id attribute is needed actually as we need to check the circle's cx position. > > LayoutTests/svg/animations/animate-beginElementAt.svg:21 > > + setTimeout(beginElement,100); > > s/,100/, 0/ OK, reworked the test to remove the 100ms delays. The rest is taken care of in the next iteration. Thanks for the comments.

Created attachment 90258 [details] Updated patch - fixed the test per Nikolas comments

Comment on attachment 90258 [details] Updated patch - fixed the test per Nikolas comments Seems reasonable as far as I can tell.

The commit-queue encountered the following flaky tests while processing attachment 90258 [details]: http/tests/xmlhttprequest/cross-origin-authorization.html bug 52398 (author: ap@webkit.org) http/tests/xmlhttprequest/failed-auth.html bug 51835 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 90258 [details] Updated patch - fixed the test per Nikolas comments Clearing flags on attachment: 90258 Committed r84999: <http://trac.webkit.org/changeset/84999>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/84999 might have broken SnowLeopard Intel Release (WebKit2 Tests)

FYI, the original test case covered several bugs so there is a follow up bug: bug59897.