Bug 74830

Summary: Avoid instantiating ScrollAnimators when possible.
Product: WebKit Reporter: Andreas Kling <kling>
Component: Layout and RenderingAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, tonikitoo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch
darin: review-
Proposed patch v2 bdakin: review+

Andreas Kling
Reported 2011-12-18 18:16:11 PST
On the HTML5 spec page ( http://whatwg.org/c ), we instantiate 1683 ScrollAnimator objects, but only a single one is used. Bytes Used Count Symbol Name 420.75 KB 1.0% 1683 WebCore::ScrollAnimator::create(WebCore::ScrollableArea*) 420.75 KB 1.0% 1683 WebCore::ScrollableArea::scrollAnimator() const 420.25 KB 1.0% 1681 WebCore::ScrollableArea::scrollToOffsetWithoutAnimation(WebCore::FloatPoint const&) 420.25 KB 1.0% 1681 WebCore::RenderLayer::scrollToOffset(int, int, WebCore::RenderLayer::ScrollOffsetClamping) 420.25 KB 1.0% 1681 WebCore::RenderLayer::updateScrollInfoAfterLayout() 420.25 KB 1.0% 1681 WebCore::RenderBlock::updateScrollInfoAfterLayout() 420.25 KB 1.0% 1681 WebCore::RenderBlock::layoutBlock(bool, int, WebCore::RenderBlock::BlockLayoutPass) 420.25 KB 1.0% 1681 WebCore::RenderBlock::layout() 256 Bytes 0.0% 1 WebCore::ScrollableArea::didAddVerticalScrollbar(WebCore::Scrollbar*) 256 Bytes 0.0% 1 WebCore::FrameView::contentsResized() We should find a way to avoid creating the RenderLayer ones unless they're actually needed.
Attachments
Proposed patch (1.58 KB, patch)
2011-12-18 18:25 PST, Andreas Kling 		darin: review-
DetailsFormatted DiffDiff
Proposed patch v2 (1.74 KB, patch)
2011-12-19 09:38 PST, Andreas Kling 		bdakin: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Andreas Kling
Comment 1 2011-12-18 18:25:59 PST
Created attachment 119794 [details] Proposed patch
Darin Adler
Comment 2 2011-12-18 18:41:59 PST
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.
Andreas Kling
Comment 3 2011-12-19 09:38:31 PST
Created attachment 119872 [details] Proposed patch v2
Beth Dakin
Comment 4 2011-12-19 09:44:50 PST
Comment on attachment 119872 [details] Proposed patch v2 Looks good!
Andreas Kling
Comment 5 2011-12-19 10:11:32 PST
Committed r103245: <http://trac.webkit.org/changeset/103245>
Note You need to log in before you can comment on or make changes to this bug.