beginElement broken by setAttribute

[jay]

open attachment
click on green button
  circle moves across screen
click on red button
  circle reset see bug 26008
click on green button
  circle restarts across screen in opera, nothing in safari

[jay]

Created attachment 30667 [details]
reduced testcase for this bug

[jay]

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

[Dinesh Garg]

Created attachment 67268 [details]
Fixed bug 26019

AnimationValid was reset but not ActiveState of SvgAnimationElement. So reset the ActiveState as well.

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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.

[Dinesh Garg]

Created attachment 67271 [details]
Fixed bug 26019

AnimationValid was reset but not ActiveState of SvgAnimationElement. So reset the ActiveState as well.

[Nikolas Zimmermann]

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 @@
> [phone]  Dinesh K Garg  <[e-mail]>
> +
> +        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 @@
> [phone]  Dinesh K Garg  <[e-mail]>
> +
> +        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  <[e-mail]>
>  
>          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.

[Dinesh Garg]

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.

[Adam Barth]

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.

[chris fleizach]

Comment on attachment 69401 [details]
Propose fix after review comments

As Adam said, this is not a significant contribution. Please remove copyright change.

[Dinesh Garg]

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

[Julien Chaffraix]

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

[Nikolas Zimmermann]

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.

[Julien Chaffraix]

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

[Julien Chaffraix]

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

[Eric Seidel (no email)]

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

Seems reasonable as far as I can tell.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 90258 [details]:

http/tests/xmlhttprequest/cross-origin-authorization.html bug 52398 (author: [e-mail])
http/tests/xmlhttprequest/failed-auth.html bug 51835 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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

[Julien Chaffraix]

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