[Webkit-unassigned] [Bug 19028] Implement CSS Animation (with keyframes, events, interfaces, etc)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=19028


eric at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #21181|review?(hyatt at apple.com)    |review-
               Flag|                            |




------- Comment #21 from eric at webkit.org  2008-08-04 01:54 PDT -------
(From update of attachment 21181)
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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.