[webkit-reviews] review denied: [Bug 63727] animate-dom-02-f.html test from SVG1.1SE testsuite fails : [Attachment 99345] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Rob Buis
<rwlbuis at gmail.com>'s request for review:
Bug 63727: animate-dom-02-f.html test from SVG1.1SE testsuite fails
https://bugs.webkit.org/show_bug.cgi?id=63727

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

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

This seems like a good start, but you should at least add some more test --
that's tricky stuff! :-) r- as I had some comments and it doesn't build on gtk
at present.

> Source/WebCore/svg/animation/SMILTimeContainer.cpp:268
> +	       RefPtr<TimeEvent> event =
TimeEvent::create(eventNames().beginEvent,
animation->document()->defaultView(), 0);
> +	       animation->dispatchEvent(event.release());

Why the refcounting round trip? just use
animation->dispatchEvent(TimeEvent::create(...)

> Source/WebCore/svg/animation/SMILTimeContainer.cpp:298
> +	       RefPtr<TimeEvent> event =
TimeEvent::create(eventNames().endEvent, animation->document()->defaultView(),
0);
> +	       animation->dispatchEvent(event.release());

Ditto.

> Source/WebCore/svg/animation/TimeEvent.h:47
> +protected:

Why protected, you mean private.

> Source/WebCore/svg/animation/TimeEvent.idl:30
> +	   readonly attribute DOMWindow 	   view;
> +	   readonly attribute long		   detail;
> +	   
> +	   [OldStyleObjC] void initUIEvent(in DOMString type, 
> +					   in DOMWindow view, 
> +					   in long detail);

No need to wrap lines, nor to line up the argument names.