[webkit-reviews] review granted: [Bug 54151] Implement an API to play/pause/scrub animations : [Attachment 85255] Step 1 Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 54151: Implement an API to play/pause/scrub animations
https://bugs.webkit.org/show_bug.cgi?id=54151

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=85255&action=review

Generally looks good!

> LayoutTests/animations/animation-api-1.html:65
> +var a_anims = a.webkitGetAnimations();

a_anims is fugly.

> Source/WebCore/dom/Element.h:348
> +    PassRefPtr<WebKitAnimationList> webkitGetAnimations();

Method should be const. You don't need the prefix to propagate into the
function names that are not exposed through bindings (but I guess this one
does).

> Source/WebCore/page/animation/AnimationBase.cpp:811
> +    // Remove ourselves from the wait lists if we still have a renderer.

I think you should flip the comment around. When does it not have a renderer?

> Source/WebCore/page/animation/AnimationBase.h:192
> +    Animation* animation() { return m_animation.get(); }

const. Return const Animation*?

> Source/WebCore/page/animation/AnimationController.cpp:47
> +// FIXME: Why isn't this set to 60fps or something?

File a bug?

> Source/WebCore/rendering/RenderObject.h:754
> +    PassRefPtr<WebKitAnimationList> animations();

Not sure why you need to add this method to RenderObject. Why not just get the
list via AnimationController?