[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
bugzilla-daemon at webkit.org
Mon Aug 4 01:54:27 PDT 2008
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.
More information about the webkit-reviews
mailing list