[webkit-reviews] review denied: [Bug 26019] beginElement broken by setAttribute : [Attachment 89279] Updated patch - addressing Chris' comments.

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 26019: beginElement broken by setAttribute
https://bugs.webkit.org/show_bug.cgi?id=26019

Attachment 89279: Updated patch - addressing Chris' comments.
https://bugs.webkit.org/attachment.cgi?id=89279&action=review

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