[webkit-reviews] review denied: [Bug 26019] beginElement broken by setAttribute : [Attachment 67271] Fixed bug 26019

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dinesh Garg
<dinesh.garg at gmail.com>'s request for review:
Bug 26019: beginElement broken by setAttribute
https://bugs.webkit.org/show_bug.cgi?id=26019

Attachment 67271: Fixed bug 26019
https://bugs.webkit.org/attachment.cgi?id=67271&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
> 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 at 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 at 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 at 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.