[webkit-reviews] review denied: [Bug 51218] Implement mozilla's requestAnimationFrame API : [Attachment 78468] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied James Robinson
<jamesr at chromium.org>'s request for review:
Bug 51218: Implement mozilla's requestAnimationFrame API
https://bugs.webkit.org/show_bug.cgi?id=51218

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78468&action=review

would be good to add tests for changing the display property (hiding/showing an
element) from the animation callback of another element.

> Source/WebCore/dom/Document.cpp:4959
> +	   m_requestAnimationFrameCallbacks = new
Vector<RefPtr<RequestAnimationFrameCallback> >();

nit: a typedef might be nice?
typedef Vector<RefPtr<RequestAnimationFrameCallback> >
RequestAnimationFrameCallbackList;

> Source/WebCore/dom/Document.cpp:4990
> +    // visibility so this code has to iterate carefully.

maybe just add a FIXME or something about the fact that we don't do this
visibility test yet.

> Source/WebCore/dom/Document.cpp:5008
> +		   callback->m_enabled = false;

m_enabled might be better if named m_firedOrCancelled (inverting the sense).

> Source/WebCore/dom/Document.cpp:5023
> +	       m_requestAnimationFrameCallbacks->insert(callbacksAdded++,
callbacks[i]);

any callback that is no longer enabled needs to be removed from this list.

maybe you even want to just loop over m_requestAnimationFrameCallbacks??

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:636
> +    webkit_support::PostDelayedTask(invokeScheduleComposite,
static_cast<void*>(this), 0);

isn't this cast unnecessary since 'this' is a pointer type?