[webkit-reviews] review denied: [Bug 67171] requestAnimationFrame doesn't throttle on Mac : [Attachment 106946] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 9 17:43:57 PDT 2011

Simon Fraser (smfr) <simon.fraser at apple.com> has denied  review:
Bug 67171: requestAnimationFrame doesn't throttle on Mac

Attachment 106946: Patch

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

> Source/WebCore/dom/ScriptedAnimationController.cpp:40
> +#define MaximumAnimationInterval 0.015

This is a minimum interval, not a maximum

> Source/WebCore/dom/ScriptedAnimationController.cpp:142
> +    double scheduleTime = MaximumAnimationInterval - (currentTime() -

scheduleTime is not an absolute time, it's an interval. The name should reflect
this. Call it 'delay' maybe.

Is currentTime() the right thing to use? I thought we had a "time for all the
current animations" value.

I find this hard to read. Is m_timeSinceLastAnimationFrame just there for
clamping? If so, what's MaximumAnimationInterval for?

m_timeSinceLastAnimationFrame is also poorly named. It's not an interval, it's
an absolute time.

> Source/WebCore/dom/ScriptedAnimationController.cpp:155
> +    m_timeSinceLastAnimationFrame = currentTime();

Is currentTime() the right thing to use again?

More information about the webkit-reviews mailing list