Bug 82083 – Too many ScrollAnimators are allocated on pages with frames

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Beth Dakin]

Currently, ScrollableArea::scrollAnimator() will create a ScrollAnimator to return if m_scrollAnimator is null. Because of this, we end up creating a ScrollAnimator for nearly every frame on webpages. For example, we allocate 22 ScrollAnimators on  http://www.disboards.com/showthread.php?t=2512881 even though only the main document needs one.

<rdar://problem/11070121>
Patch forthcoming.

[Beth Dakin]

Created attachment 133547 [details]
Patch

[Geoffrey Garen]

Comment on attachment 133547 [details]
Patch

nepotism=me

[Beth Dakin]

Thanks Geoff! Committed http://trac.webkit.org/changeset/111896

[Simon Fraser (smfr)]

Comment on attachment 133547 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133547&action=review

> Source/WebCore/platform/ScrollableArea.h:99
> +    ScrollAnimator* getExistingScrollAnimator() const { return m_scrollAnimator.get(); }

We usually avoid 'get' in accessors like this.

[Beth Dakin]

(In reply to comment #4)
> (From update of attachment 133547 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133547&action=review
> 
> > Source/WebCore/platform/ScrollableArea.h:99
> > +    ScrollAnimator* getExistingScrollAnimator() const { return m_scrollAnimator.get(); }
> 
> We usually avoid 'get' in accessors like this.

Would you prefer existingScrollAnmator()?

[Beth Dakin]

Changed the name to existingScrollAnimator() : http://trac.webkit.org/changeset/111901

[Nikolas Zimmermann]

(In reply to comment #6)
> Changed the name to existingScrollAnimator() : http://trac.webkit.org/changeset/111901

Hm, I'm still confused by your terminology.
I would have suggested to rename scrollAnimator() to ensureScrollAnimator(), and existingScrollAnimator() to scrollAnimator() - that's how we do it in numerous other places in WebCore.

scrollAnimator() sounds just like a simple accessor, anyone not reading the comment in the header, won't know it creates something. Would you want to prepare another patch? :-)