[webkit-reviews] review denied: [Bug 19028] Implement CSS Animation (with keyframes, events, interfaces, etc) : [Attachment 21181] Patch for animation/transition events and other dom interfaces

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dean Jackson <dino at apple.com>'s
request for review:
Bug 19028: Implement CSS Animation (with keyframes, events, interfaces, etc)
https://bugs.webkit.org/show_bug.cgi?id=19028

Attachment 21181: Patch for animation/transition events and other dom
interfaces
https://bugs.webkit.org/attachment.cgi?id=21181&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
In general this patch looks fine.

Tabs vs. spaces here:
39     VoidCallback \
 340	WebKitAnimationEvent \
 341	WebKitCSSKeyframeRule \
 342	WebKitCSSKeyframesRule \
 343	WebKitCSSTransformValue \
340344	   WheelEvent \

Single line ifs don't get { } (yes, I know the rest of that function has them
wrong already:
+    } else if (attr->name() == onwebkittransitionendAttr) {
+	 setHTMLEventListener(webkitTransitionEndEvent, attr);	      
     }

Can't we use CamelCase for these?
+onwebkitanimationend
+onwebkitanimationiteration
+onwebkitanimationstart
+onwebkittransitionend

Seems it would make them more readable.  Yes, i know the other event attributes
are all lower-case.  I'm not sure they should be though.  I guess the case
matters for XHTML though, so I guess they probably have to be all lower case...


This seems wrong:
+    if (eventType == "WebKitAnimationEvent")
+	 return new WebKitAnimationEvent;    
+    if (eventType == "WebKitTransitionEvent")
+	 return new WebKitTransitionEvent;  

I guess Events haven't been converted to use the new Foo::create() syntax yet? 
They still start with a RefCount of 0 instead of 1?

These could probably go in the header just fine:
+bool Event::isWebKitTransitionEvent() const
+{
+    return false;
+}   


Again, no create() syntax?

+    RefPtr<WebKitAnimationEvent> animEvent = new
WebKitAnimationEvent(eventType, animationName, elapsedTime);


I would think we would want to use the newer PassRefPtr<WebKitAnimationEvent>
create(); syntax for all new classes.  If the parent class hasn't yet been
converted to start with a RefCount of 1, this one could easily ASSERT that new
WebKitAnimationEvent() comes back with a RefCount of O (to make things crash if
someone changes the base class and forgets to change this one) and then still
return it from create().  When someone fixes Event to start with a ref count of
1, then they'll have to change the "new" in your create() method to be
adoptRef(new Foo) instead.

I'm surprised this ASSERT isn't in dispatchEvent!?
+    ASSERT(!eventDispatchForbidden());

Why should each caller need to ASSERT that?

This does not need to call the constructor for the String, but it *does* need
to init the double to some sane value:
+    WebKitAnimationEvent::WebKitAnimationEvent()
+    : m_animationName()
+    {

And again:
+    WebKitTransitionEvent::WebKitTransitionEvent()
+    : m_propertyName()
+    {

Seems these two classes are just copy/paste of one another... maybe they should
just be the same class?

This is not needed in the idl files:
+    // Introduced in DOM Level ?:


Otherwise this looks fine.  Maybe you still want Hyatt to look at it for
content.

r- though for the various small nits above.