Bug 99768 – We should limit the tile cache coverage when a page can't take advantage of fast tile scrolling anyway

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

Right now, we only limit the size of the tile cache if a page cannot scroll at all. We should limit it whenever a page can't take advantage of fast tile scrolling. Making this change seems to be a performance improvement on pages that are very painting-intensive such as Facebook.

[Beth Dakin]

Created attachment 169497 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 169497 [details]
Patch

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

I think you could go a little further and limit horizontal/vertical coverage when there is no scrollbar in that direction.

> Source/WebCore/page/FrameView.cpp:797
> +    if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) {
> +        if (hasViewportConstrainedObjects() && !scrollingCoordinator->supportsFixedPositionLayers())
> +            return true;
> +    }

Isn't this replicating logic about whether we can use threaded scrolling? Can't we just ask the ScrollingCoordinator whether it's going to use threaded scrolling?

[Beth Dakin]

(In reply to comment #2)
> (From update of attachment 169497 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169497&action=review
> 
> I think you could go a little further and limit horizontal/vertical coverage when there is no scrollbar in that direction.
> 
> > Source/WebCore/page/FrameView.cpp:797
> > +    if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) {
> > +        if (hasViewportConstrainedObjects() && !scrollingCoordinator->supportsFixedPositionLayers())
> > +            return true;
> > +    }
> 
> Isn't this replicating logic about whether we can use threaded scrolling? Can't we just ask the ScrollingCoordinator whether it's going to use threaded scrolling?

It is. That was my plan originally, actually, but I talked with Anders in person, and I got the impression that he liked the idea of keeping this all in FrameView. Maybe I misinterpreted him though. I will give him a chance to defend the idea or claim I am crazy since I only did it this way because I thought he liked it better :-)

[Anders Carlsson]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 169497 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=169497&action=review
> > 
> > I think you could go a little further and limit horizontal/vertical coverage when there is no scrollbar in that direction.
> > 
> > > Source/WebCore/page/FrameView.cpp:797
> > > +    if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) {
> > > +        if (hasViewportConstrainedObjects() && !scrollingCoordinator->supportsFixedPositionLayers())
> > > +            return true;
> > > +    }
> > 
> > Isn't this replicating logic about whether we can use threaded scrolling? Can't we just ask the ScrollingCoordinator whether it's going to use threaded scrolling?
> 
> It is. That was my plan originally, actually, but I talked with Anders in person, and I got the impression that he liked the idea of keeping this all in FrameView. Maybe I misinterpreted him though. I will give him a chance to defend the idea or claim I am crazy since I only did it this way because I thought he liked it better :-)

I'll do you one better and claim that _I_ am crazy. It's clearly better to keep the logic in ScrollingCoordinator.

[Tim Horton]

(In reply to comment #2)
> (From update of attachment 169497 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169497&action=review
> 
> I think you could go a little further and limit horizontal/vertical coverage when there is no scrollbar in that direction.
> 
> > Source/WebCore/page/FrameView.cpp:797
> > +    if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) {
> > +        if (hasViewportConstrainedObjects() && !scrollingCoordinator->supportsFixedPositionLayers())
> > +            return true;
> > +    }
> 
> Isn't this replicating logic about whether we can use threaded scrolling? Can't we just ask the ScrollingCoordinator whether it's going to use threaded scrolling?

Does seem odd, especially since there are more reasons that  shouldUpdateScrollLayerPositionOnMainThread could be true which aren't covered here...

[Beth Dakin]

Created attachment 169521 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 169521 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:282
> +MainThreadScrollingReasons ScrollingCoordinator::shouldUpdateScrollLayerPositionOnMainThread() const

Returning the MainThreadScrollingReasons from a 'should' method is a bit confusing; I think this would be clearer as two methods (possibly inline):

MainThreadScrollingReasons mainThreadScrollingReasons() const;
bool shouldUpdateScrollLayerPositionOnMainThread() const { return mainThreadScrollingReasons() != 0; }

[Beth Dakin]

Thanks, Simon! http://trac.webkit.org/changeset/131939