Bug 116193 - ScriptedAnimationController::setThrottled should extend MinimumAnimationInterval
Summary: ScriptedAnimationController::setThrottled should extend MinimumAnimationInterval
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
Depends on:
Reported: 2013-05-15 16:44 PDT by Gavin Barraclough
Modified: 2013-05-15 17:42 PDT (History)
5 users (show)

See Also:

Fix (3.53 KB, patch)
2013-05-15 16:52 PDT, Gavin Barraclough
simon.fraser: review+
Details | Formatted Diff | Diff
Fix GTK build (3.69 KB, patch)
2013-05-15 17:17 PDT, Gavin Barraclough
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
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
> +    double animationInterval = m_throttled ? MinimumThrottledAnimationInterval : MinimumAnimationInterval;
> +#else
> +    double animationInterval = MinimumAnimationInterval;
> +#endif

I’d write it like this:

    double animationInterval = MinimumAnimationInterval;
    if (m_isThrottled)
        animationInterval = MinimumThrottledAnimationInterval;

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.