74830 2011-12-18 18:16:11 -0800 Avoid instantiating ScrollAnimators when possible. 2011-12-19 10:11:32 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 kling kling bdakin tonikitoo oldest_to_newest 523607 0 kling 2011-12-18 18:16:11 -0800 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. 523614 1 119794 kling 2011-12-18 18:25:59 -0800 Created attachment 119794 Proposed patch 523621 2 119794 darin 2011-12-18 18:41:59 -0800 Comment on attachment 119794 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. 523944 3 119872 kling 2011-12-19 09:38:31 -0800 Created attachment 119872 Proposed patch v2 523948 4 119872 bdakin 2011-12-19 09:44:50 -0800 Comment on attachment 119872 Proposed patch v2 Looks good! 523966 5 kling 2011-12-19 10:11:32 -0800 Committed r103245: <http://trac.webkit.org/changeset/103245> 119794 2011-12-18 18:25:59 -0800 2011-12-19 09:38:31 -0800 Proposed patch bug-74830.diff text/plain 1621 kling ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBlOTJmZTkzLi44NjBmMjkxIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjEg QEAKIDIwMTEtMTItMTggIEFuZHJlYXMgS2xpbmcgIDxrbGluZ0B3ZWJraXQub3JnPgogCisgICAg ICAgIEF2b2lkIGluc3RhbnRpYXRpbmcgU2Nyb2xsQW5pbWF0b3JzIHdoZW4gcG9zc2libGUuCisg ICAgICAgIDxodHRwOi8vd2Via2l0Lm9yZy9iLzc0ODMwPgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIERvbid0IGluc3RhbnRpYXRlIGEgU2Nyb2xsQW5p bWF0b3IgaW4gU2Nyb2xsYWJsZUFyZWEgaWYgaXQncyBvbmx5IHRvIHNjcm9sbAorICAgICAgICBp bW1lZGlhdGVseSB0byAoMCwgMCkuCisKKyAgICAgICAgVGhpcyByZWR1Y2VzIG1lbW9yeSBjb25z dW1wdGlvbiBieSA0MDAga0Igd2hlbiB2aWV3aW5nIHRoZSBmdWxsIEhUTUw1IHNwZWMsCisgICAg ICAgIHdoZXJlIHRoZSBjYWxsIG9yaWdpbmF0ZXMgaW4gUmVuZGVyTGF5ZXI6OnVwZGF0ZVNjcm9s bEluZm9BZnRlckxheW91dCgpLgorCisgICAgICAgICogcGxhdGZvcm0vU2Nyb2xsYWJsZUFyZWEu Y3BwOgorICAgICAgICAoV2ViQ29yZTo6U2Nyb2xsYWJsZUFyZWE6OnNjcm9sbFRvT2Zmc2V0V2l0 aG91dEFuaW1hdGlvbik6CisKKzIwMTEtMTItMTggIEFuZHJlYXMgS2xpbmcgIDxrbGluZ0B3ZWJr aXQub3JnPgorCiAgICAgICAgIEhUTUxBbGxDb2xsZWN0aW9uOiBHZXQgcmlkIG9mIHN0YXRlZnVs IG5hbWVkSXRlbSB0cmF2ZXJzYWwuCiAgICAgICAgIDxodHRwOi8vd2Via2l0Lm9yZy9iLzc0ODAz PgogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9TY3JvbGxhYmxlQXJlYS5j cHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9TY3JvbGxhYmxlQXJlYS5jcHAKaW5kZXggOWVl NzhmMi4uYTY0YmVlNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vU2Nyb2xs YWJsZUFyZWEuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1Njcm9sbGFibGVBcmVh LmNwcApAQCAtMTI3LDYgKzEyNywxMCBAQCBib29sIFNjcm9sbGFibGVBcmVhOjpzY3JvbGwoU2Ny b2xsRGlyZWN0aW9uIGRpcmVjdGlvbiwgU2Nyb2xsR3JhbnVsYXJpdHkgZ3JhbnVsYQogCiB2b2lk IFNjcm9sbGFibGVBcmVhOjpzY3JvbGxUb09mZnNldFdpdGhvdXRBbmltYXRpb24oY29uc3QgRmxv YXRQb2ludCYgb2Zmc2V0KQogeworICAgIC8vIElnbm9yZSBzY3JvbGxzIHRvICgwLCAwKSBpZiB3 ZSBkb24ndCBoYXZlIGEgU2Nyb2xsQW5pbWF0b3IuCisgICAgLy8gVGhlIGNhbGwgd291bGQgYWNj b21wbGlzaCBub3RoaW5nIGFuZCBsZWF2ZSB1cyB3aXRoIG9uZSBtb3JlIGFuaW1hdG9yLgorICAg IGlmICghbV9zY3JvbGxBbmltYXRvciAmJiAhb2Zmc2V0LngoKSAmJiAhb2Zmc2V0LnkoKSkKKyAg ICAgICAgcmV0dXJuOwogICAgIHNjcm9sbEFuaW1hdG9yKCktPnNjcm9sbFRvT2Zmc2V0V2l0aG91 dEFuaW1hdGlvbihvZmZzZXQpOwogfQogCg== 119872 2011-12-19 09:38:31 -0800 2011-12-19 09:44:50 -0800 Proposed patch v2 bug-74830-v2.diff text/plain 1778 kling ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBkMTFmNzczLi5mY2FhYzYzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjEg QEAKKzIwMTEtMTItMTkgIEFuZHJlYXMgS2xpbmcgIDxrbGluZ0B3ZWJraXQub3JnPgorCisgICAg ICAgIEF2b2lkIGluc3RhbnRpYXRpbmcgU2Nyb2xsQW5pbWF0b3JzIHdoZW4gcG9zc2libGUuCisg ICAgICAgIDxodHRwOi8vd2Via2l0Lm9yZy9iLzc0ODMwPgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEhhdmUgUmVuZGVyTGF5ZXI6OnNjcm9sbFRvT2Zm c2V0KCkgY2hlY2sgaWYgd2UncmUgc2Nyb2xsaW5nIHRvIHRoZSBhbHJlYWR5CisgICAgICAgIGN1 cnJlbnQgb2Zmc2V0LiBJbiB0aGF0IGNhc2UsIGRvbid0IGNhbGwgZG93biB0byBzY3JvbGxUb09m ZnNldFdpdGhvdXRBbmltYXRpb24oKSwKKyAgICAgICAgYXZvaWRpbmcgdGhlIGluc3RhbnRpYXRp b24gb2YgYSBTY3JvbGxBbmltYXRvci4KKworICAgICAgICBUaGlzIHJlZHVjZXMgbWVtb3J5IGNv bnN1bXB0aW9uIGJ5IDQwMCBrQiAob24gMzItYml0KSB3aGVuIHZpZXdpbmcgdGhlIGZ1bGwgSFRN TDUKKyAgICAgICAgc3BlYyBvbiA8aHR0cDovL3doYXR3Zy5vcmcvYz4sIHNpbmNlIHdlIHdlcmUg Y3JlYXRpbmcgYSBTY3JvbGxBbmltYXRvciBmb3IgZXZlcnkKKyAgICAgICAgc2luZ2xlIFJlbmRl ckxheWVyLgorCisgICAgICAgICogcmVuZGVyaW5nL1JlbmRlckxheWVyLmNwcDoKKyAgICAgICAg KFdlYkNvcmU6OlJlbmRlckxheWVyOjpzY3JvbGxUb09mZnNldCk6CisKIDIwMTEtMTItMTkgIEtl bm5ldGggUm9oZGUgQ2hyaXN0aWFuc2VuICA8a2VubmV0aEB3ZWJraXQub3JnPgogCiAgICAgICAg IE1ha2UgdGhlIEVkaXRvcjo6c2V0SWdub3JlQ29tcG9zaXRpb25TZWxlY3Rpb25DaGFuZ2UgcHVi bGljIGFzIGl0IGlzIG5lZWRlZCBieSBRdApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVu ZGVyaW5nL1JlbmRlckxheWVyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJM YXllci5jcHAKaW5kZXggYzkyMWU4YS4uOTkxYzhiMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNv cmUvcmVuZGVyaW5nL1JlbmRlckxheWVyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJp bmcvUmVuZGVyTGF5ZXIuY3BwCkBAIC0xNDEyLDggKzE0MTIsMTAgQEAgdm9pZCBSZW5kZXJMYXll cjo6c2Nyb2xsVG9PZmZzZXQoTGF5b3V0VW5pdCB4LCBMYXlvdXRVbml0IHksIFNjcm9sbE9mZnNl dENsYW1waW4KICAgICAgICAgeCA9IG1pbihtYXg8TGF5b3V0VW5pdD4oeCwgMCksIG1heFgpOwog ICAgICAgICB5ID0gbWluKG1heDxMYXlvdXRVbml0Pih5LCAwKSwgbWF4WSk7CiAgICAgfQotICAg IAotICAgIFNjcm9sbGFibGVBcmVhOjpzY3JvbGxUb09mZnNldFdpdGhvdXRBbmltYXRpb24oTGF5 b3V0UG9pbnQoeCwgeSkpOworCisgICAgTGF5b3V0UG9pbnQgbmV3U2Nyb2xsT2Zmc2V0KHgsIHkp OworICAgIGlmIChuZXdTY3JvbGxPZmZzZXQgIT0gTGF5b3V0UG9pbnQoc2Nyb2xsWE9mZnNldCgp LCBzY3JvbGxZT2Zmc2V0KCkpKQorICAgICAgICBzY3JvbGxUb09mZnNldFdpdGhvdXRBbmltYXRp b24obmV3U2Nyb2xsT2Zmc2V0KTsKIH0KIAogdm9pZCBSZW5kZXJMYXllcjo6c2Nyb2xsVG8oaW50 IHgsIGludCB5KQo=