[webkit-reviews] review requested: [Bug 26019] beginElement broken by setAttribute : [Attachment 69401] Propose fix after review comments

[bugzilla-daemon at webkit.org]

Dinesh Garg <dinesh.garg at gmail.com> has asked  for review:
Bug 26019: beginElement broken by setAttribute
https://bugs.webkit.org/show_bug.cgi?id=26019

Attachment 69401: Propose fix after review comments
https://bugs.webkit.org/attachment.cgi?id=69401&action=review

------- Additional Comments from Dinesh Garg <dinesh.garg at gmail.com>
(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.