[webkit-reviews] review denied: [Bug 12073] Implement setCurrentTime() and pauseAnimations() on SVGSVGElement : [Attachment 116137] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Joel Webber
<jgw at chromium.org>'s request for review:
Bug 12073: Implement setCurrentTime() and pauseAnimations() on SVGSVGElement
https://bugs.webkit.org/show_bug.cgi?id=12073

Attachment 116137: Patch
https://bugs.webkit.org/attachment.cgi?id=116137&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116137&action=review


I fully agree with Erics review comments. I have some more issues with testing:


> LayoutTests/svg/animations/script-tests/animate-setcurrenttime.js:31
> +animate1.setAttribute("to", "200");
> +animate1.setAttribute("fill", "freeze");

We need lots more tests, as you can guess :-)
* Accumulating animations need to be tested
* Several animations running in parallel
* Nested time containers, test modifying timeline in a inner <svg> element.
  <svg>some content <svg width="50" height="50"> rect-with-anim-in-inner-svg...
</svg> </svg>
  Test moving time in outer <svg>, should it affect the inner <svg> timeline?
* Use pure SVG/HTML5 self-contained tests, not using script-tests/ framework as
well
...
I'm sure I could come up with more, once I have some more time to think about
that.
Do you also test your testcases in eg. Opera? Would be nice to have them pass
in Opera fully as well.