Bug 116193

Summary: ScriptedAnimationController::setThrottled should extend MinimumAnimationInterval
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: WebCore Misc.Assignee: Gavin Barraclough <barraclough>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, rego+ews, simon.fraser, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Fix
simon.fraser: review+
Fix GTK build darin: review+

Description Gavin Barraclough 2013-05-15 16:44:30 PDT
.
Comment 1 Gavin Barraclough 2013-05-15 16:52:22 PDT
Created attachment 201896 [details]
Fix
Comment 2 Gavin Barraclough 2013-05-15 17:17:25 PDT
Created attachment 201898 [details]
Fix GTK build
Comment 3 Darin Adler 2013-05-15 17:26:02 PDT
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.
Comment 4 Gavin Barraclough 2013-05-15 17:28:35 PDT
Fixed in r150159.
Comment 5 Gavin Barraclough 2013-05-15 17:42:48 PDT
Missed Darin's review comments – fixed in r150162.