Summary: | Avoid instantiating ScrollAnimators when possible. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||
Component: | Layout and Rendering | Assignee: | Andreas Kling <kling> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bdakin, tonikitoo | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Andreas Kling
2011-12-18 18:16:11 PST
Created attachment 119794 [details]
Proposed patch
Comment on attachment 119794 [details] Proposed patch 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. Created attachment 119872 [details]
Proposed patch v2
Comment on attachment 119872 [details]
Proposed patch v2
Looks good!
Committed r103245: <http://trac.webkit.org/changeset/103245> |