[webkit-reviews] review granted: [Bug 65645] Scroll animator changes to nail the framerate : [Attachment 102835] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted  review:
Bug 65645: Scroll animator changes to nail the framerate
https://bugs.webkit.org/show_bug.cgi?id=65645

Attachment 102835: Patch
https://bugs.webkit.org/attachment.cgi?id=102835&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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