[Webkit-unassigned] [Bug 65645] Scroll animator changes to nail the framerate

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=65645


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #102835|review-                     |review+
               Flag|                            |




--- Comment #11 from James Robinson <jamesr at chromium.org>  2011-08-04 17:43:08 PST ---
(From update of attachment 102835)
View in context: https://bugs.webkit.org/attachment.cgi?id=102835&action=review

In that case I think this looks fine, although I left some comments for you to consider before landing.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:346
>      double currentTime = WTF::currentTime();

could you use WTF::monotonicallyIncreasingTime() instead for this logic?  That's what WebCore::Timer is based off of.  The main differences between it and WTF::currentTime() are:
- monotonicallyIncreasingTime() does not go backwards when the system clock is adjusted (which is rare in practice, but does happen due to NTP and such)
- monotonicallyIncreasingTime() does not go crazy on windows when the clocks drift (which is not so rare in practice)
- monotonicallyIncreasingTime() can be a lot cheaper to call on some systems, primarily windows

this might not be an appropriate change for this patch, but something to think about for the future.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:347
> +    PerAxisData* data = (timer == &m_horizontalData.m_animationTimer) ? &m_horizontalData : &m_verticalData;

this stuff is slightly confusing - could you perhaps use a temporary bool to keep track of which scrollbar we're updating here, and maybe use const references to the relevant PerAxisData instead of a pointer (since it seems we don't mutate it at all).

Do we never use the same timer for both scrollbars?

> Source/WebCore/platform/ScrollAnimatorNone.cpp:350
> +        double nextTimerInterval = max(kMinimumTimerInterval, deltaToNextFrame - WTF::currentTime() + currentTime);

this seems to be trying to account for the time delta between the top of this function and hitting this line, which is a little confusing - do you expect the operations in animateScroll() to be expensive enough to have to worry about this delta?  querying the clock is not necessarily cheap on all platforms

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.