[webkit-reviews] review granted: [Bug 116193] ScriptedAnimationController::setThrottled should extend MinimumAnimationInterval : [Attachment 201898] Fix GTK build

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 116193: ScriptedAnimationController::setThrottled should extend
MinimumAnimationInterval
https://bugs.webkit.org/show_bug.cgi?id=116193

Attachment 201898: Fix GTK build
https://bugs.webkit.org/attachment.cgi?id=201898&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=201898&action=review


> Source/WebCore/dom/ScriptedAnimationController.cpp:60
> +    , m_throttled(false)

Typically we try to name boolean data members and such so they finish a phrase
“controller <xxx>”. Thus we’d call this m_isThrottled.

> Source/WebCore/dom/ScriptedAnimationController.cpp:91
> +    m_throttled = isThrottled;
> +    if (m_animationTimer.isActive()) {

This seems not quite right to me. If we were not throttled before and are still
not throttled, it seems strange to stop and restart the timer. Maybe harmless,
but strange nonetheless. We should have an early return when m_throttled is
already == isThrottled.

> Source/WebCore/dom/ScriptedAnimationController.cpp:197
> +#if USE(REQUEST_ANIMATION_FRAME_TIMER) &&
USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
> +    double animationInterval = m_throttled ?
MinimumThrottledAnimationInterval : MinimumAnimationInterval;
> +#else
> +    double animationInterval = MinimumAnimationInterval;
> +#endif

I’d write it like this:

    double animationInterval = MinimumAnimationInterval;
#if USE(REQUEST_ANIMATION_FRAME_TIMER) &&
USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
    if (m_isThrottled)
	animationInterval = MinimumThrottledAnimationInterval;
#endif

I think easier to read than the repeated code and #else in the code you posted.


> Source/WebCore/dom/ScriptedAnimationController.h:93
>      bool m_useTimer;

Seems like it should be named m_isUsingTimer or something like that.