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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied  review:
Bug 67171: requestAnimationFrame doesn't throttle on Mac
https://bugs.webkit.org/show_bug.cgi?id=67171

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

------- 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() -
m_timeSinceLastAnimationFrame);

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?