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.
Created attachment 169497 [details] Patch
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?
(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 :-)
(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.
(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...
Created attachment 169521 [details] Patch
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; }
Thanks, Simon! http://trac.webkit.org/changeset/131939