[webkit-reviews] review denied: [Bug 74830] Avoid instantiating ScrollAnimators when possible. : [Attachment 119794] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Andreas Kling <kling at webkit.org>'s
request for review:
Bug 74830: Avoid instantiating ScrollAnimators when possible.
https://bugs.webkit.org/show_bug.cgi?id=74830

Attachment 119794: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=119794&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=119794&action=review


> Source/WebCore/platform/ScrollableArea.cpp:133
> +    if (!m_scrollAnimator && !offset.x() && !offset.y())
> +	   return;

This is a great idea. But I don’t think this it is technically correct; the
offset might already be something other than 0,0 if it was already scrolled by
a call to setScrollOffset rather than a call to scrollToOffsetWithoutAnimation.
It would be great to have this check in ScrollableArea, but at the moment it
seems impractical to do it correctly given what’s abstract in the class and
what’s concrete.

Instead, I suggest putting a check into RenderLayer::scrollToOffset:

    IntPoint newScrollOffset(x, y);
    if (newScrollOffset != scrollOffset())
	scrollToOffsetWithoutAnimation(newScrollOffset);

I also noticed two other things:

1) In RenderLayer there are two calls that use explicit ScrollableArea prefixes
where they need not: This one and the call to
ScrollableArea::setConstrainsScrollingToContentEdge.

2) There is a unpleasant mix of float, int, and LayoutUnit in RenderLayer and
ScrollableArea. It’s bizarre that scrollToOffsetWithoutAnimation takes a
FloatPoint, setScrollOffset takes an IntPoint, and scrollToOffset takes a
LayoutUnit. There may be some reason why, but I am not clear what it is.