[webkit-reviews] review denied: [Bug 19938] Improve AnimationController : [Attachment 22442] Patch update 2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jul 23 14:55:21 PDT 2008


Dave Hyatt <hyatt at apple.com> has denied Dean Jackson <dino at apple.com>'s request
for review:
Bug 19938: Improve AnimationController
https://bugs.webkit.org/show_bug.cgi?id=19938

Attachment 22442: Patch update 2
https://bugs.webkit.org/attachment.cgi?id=22442&action=edit

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
CSSComputedStyleDeclaration.cpp should not be in this patch.

Please get rid of the loadComplete stuff.  Animations need to just start
immediately when the now keyword is used.  It's completely wrong to wait until
the entire document has loaded (in an incremental world you just can't hold off
starting animations because some ad in an iframe never completes).  The
animation feature will just be completely bizarre and unpredictable in the real
world if a user is able to interact with the page for many seconds before
animations are allowed to run.

Is "#pragma mark -" really necessary?  We don't tend to put those in WebCore
code.  Really that just signals that the file needs to be split up, which we
could do post-landing.

This seems wrong:

+    HTMLElement* elementForEventDispatch()
+    {
+	 if (m_object->node() && m_object->node()->isHTMLElement())
+	     return static_cast<HTMLElement*>(m_object->node());
+	 return 0;
+    }

Why would I need an HTMLElement to do event dispatch?  Animation events should
be fired on all types of elements...

I notice that HTMLElements are used in several places.	This seems wrong and
this code needs to be changed to use Element instead.

If you really want Transition to be refcounted, then I would suggest moving to
a model where you have Vector<RefPtr<Transition> >* m_transitions in the
RenderStyle.  If someone outside of RenderStyle is holding on to a ref to a
transition, then I might also suggest having the RenderStyle transition "list
node" object hold the refcounted "transition" object as a member in each node
of the list.

The mixture though just makes everything look really strange, since RenderStyle
doesn't really pay attention to the refcounting (it clones m_next chains in the
assignment operator and copy constructor).

I also think Transition should be renamed to something more generic that is
suitable for both Animations and Transitions (but am unsure of what that name
should be).


More information about the webkit-reviews mailing list