[Webkit-unassigned] [Bug 26019] beginElement broken by setAttribute

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26019


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #67271|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #7 from Nikolas Zimmermann <zimmermann at kde.org>  2010-09-19 07:05:59 PST ---
(From update of attachment 67271)
> 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.