Bug 26019

Summary: beginElement broken by setAttribute
Product: WebKit Reporter: jay <jay>
Component: SVGAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, dinesh.garg, eric, jchaffraix, webkit.review.bot, zimmermann
Priority: P2 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Bug Depends on: 26008    
Bug Blocks: 41761    
Attachments:
Description Flags
reduced testcase for this bug
none
Fixed bug 26019
eric: review-, eric: commit-queue-
Fixed bug 26019
zimmermann: review-, zimmermann: commit-queue-
Propose fix after review comments
cfleizach: review-, cfleizach: commit-queue-
Updated patch - addressing Chris' comments.
zimmermann: review-, zimmermann: commit-queue-
Updated patch - fixed the test per Nikolas comments none

Description jay 2009-05-26 03:39:46 PDT
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
Comment 1 jay 2009-05-26 03:40:32 PDT
Created attachment 30667 [details]
reduced testcase for this bug
Comment 2 jay 2009-05-26 03:43:56 PDT
do these two bugs have a common issue related to the implementation of beginElement and endElement?
Comment 3 Dinesh Garg 2010-09-10 17:05:35 PDT
Created attachment 67268 [details]
Fixed bug 26019

AnimationValid was reset but not ActiveState of SvgAnimationElement. So reset the ActiveState as well.
Comment 4 WebKit Review Bot 2010-09-10 17:07:28 PDT
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 5 Eric Seidel (no email) 2010-09-10 17:08:21 PDT
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.
Comment 6 Dinesh Garg 2010-09-10 17:18:12 PDT
Created attachment 67271 [details]
Fixed bug 26019

AnimationValid was reset but not ActiveState of SvgAnimationElement. So reset the ActiveState as well.
Comment 7 Nikolas Zimmermann 2010-09-19 07:05:59 PDT
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.
Comment 8 Dinesh Garg 2010-09-30 17:21:22 PDT
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 9 Adam Barth 2010-10-10 12:39:49 PDT
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 10 chris fleizach 2010-10-13 00:41:23 PDT
Comment on attachment 69401 [details]
Propose fix after review comments

As Adam said, this is not a significant contribution. Please remove copyright change.
Comment 11 Dinesh Garg 2010-10-13 05:30:11 PDT
(In reply to comment #10)
Sure. I will upload one without copyright soon.
Comment 12 Julien Chaffraix 2011-04-12 15:01:29 PDT
Created attachment 89279 [details]
Updated patch - addressing Chris' comments.
Comment 13 Nikolas Zimmermann 2011-04-19 00:47:56 PDT
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.
Comment 14 Julien Chaffraix 2011-04-19 14:43:22 PDT
> > 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.
Comment 15 Julien Chaffraix 2011-04-19 14:46:00 PDT
Created attachment 90258 [details]
Updated patch - fixed the test per Nikolas comments
Comment 16 Eric Seidel (no email) 2011-04-26 15:51:17 PDT
Comment on attachment 90258 [details]
Updated patch - fixed the test per Nikolas comments

Seems reasonable as far as I can tell.
Comment 17 WebKit Commit Bot 2011-04-26 18:26:15 PDT
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 18 WebKit Commit Bot 2011-04-26 18:28:20 PDT
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>
Comment 19 WebKit Commit Bot 2011-04-26 18:28:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 WebKit Review Bot 2011-04-26 21:17:42 PDT
http://trac.webkit.org/changeset/84999 might have broken SnowLeopard Intel Release (WebKit2 Tests)
Comment 21 Julien Chaffraix 2011-05-03 07:52:19 PDT
FYI, the original test case covered several bugs so there is a follow up bug: bug59897.