Bug 116193 – ScriptedAnimationController::setThrottled should extend MinimumAnimationInterval

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Gavin Barraclough]

.

[Gavin Barraclough]

Created attachment 201896 [details]
Fix

[Gavin Barraclough]

Created attachment 201898 [details]
Fix GTK build

[Darin Adler]

Comment on attachment 201898 [details]
Fix GTK build

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.

[Gavin Barraclough]

Fixed in r150159.

[Gavin Barraclough]

Missed Darin's review comments – fixed in r150162.