The heuristic in FrameView::scrollContentsFastPath that tries to estimate the coverage of fixed position elements on the screen is incorrect if !FrameView::clipsRepaints(). This is because you could have a very large off-screen fixed position element that covers more than the viewport size. Some thoughts: (1) Maybe fixed position elements should be explicitly clipped to the viewport or to some ancestor clipping layer of their container. (2) For Chromium, this heuristic is not useful at all when the view's layer is composited. We don't shift scroll composited layers, so the slow scroll "invalidate everything" path is always going to be slower. Maybe we could just skip this heuristic for composited layers?
Created attachment 153871 [details] Patch
(In reply to comment #1) > Created an attachment (id=153871) [details] > Patch smfr, thorton, noamr: Putting this patch up for discussion. I'm not sure how the fast/slow scrolling works for other ports, but the FrameView slow scroll path doesn't make any sense for Chromium as there's no shift scrolling, so the extra invalidations only make things worse.
(In reply to comment #2) > (In reply to comment #1) > > Created an attachment (id=153871) [details] [details] > > Patch > > smfr, thorton, noamr: Putting this patch up for discussion. I'm not sure how the fast/slow scrolling works for other ports, but the FrameView slow scroll path doesn't make any sense for Chromium as there's no shift scrolling, so the extra invalidations only make things worse. Pretty similar for the Qt port, at least for WebKit2. Scrolling is done in the UI process and we're forcing fixed position layers to be composited. The only time we need slow scrolling is for non-layer fixed objects like backgrounds, in which case this patch should work fine.
Comment on attachment 153871 [details] Patch I think our similar-but-different scrolling models are conflicting here. On Mac Mountain Lion we use a tile cache behind the root's layer, so contentsInCompositedLayer() is true. However, we don't have support for compositing fixed elements, so we have to repaint fixed things on scrolling. Once we put fixed things into composited layers, they'll be excluded from the regionToUpdate. But the ! isCompositedContentLayer check doesn't appropriately reflect the "content is painted into a layer which moves when you scroll and doesn't need to repaint". That seems like a behavior that the ScrollingCoordinator should export, like frameRepaintsOnScroll(FrameView*) or something.
(In reply to comment #4) > (From update of attachment 153871 [details]) > I think our similar-but-different scrolling models are conflicting here. > > On Mac Mountain Lion we use a tile cache behind the root's layer, so contentsInCompositedLayer() is true. However, we don't have support for compositing fixed elements, so we have to repaint fixed things on scrolling. Once we put fixed things into composited layers, they'll be excluded from the regionToUpdate. But the ! isCompositedContentLayer check doesn't appropriately reflect the "content is painted into a layer which moves when you scroll and doesn't need to repaint". That seems like a behavior that the ScrollingCoordinator should export, like frameRepaintsOnScroll(FrameView*) or something. Maybe I'm misunderstanding something here. This patch changes the logic so that the scrollContentsFastPath is always used (and not aborted by this heuristic) when the contents are in a composited layer. scrollContentsFastPath always does invalidations for all fixed position objects that are not composited. The difference is that scrollContentsSlowPath just invalidates the entire frame view contents rather than each fixed position object individually. In both cases, repainting for fixed position object occurs. It's just that the slowContentsSlowPath does more invalidations than are necessary if this heuristic is hit. It doesn't give any other code a chance to do something more smart than doing all that extra painting.
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 153871 [details] [details]) > > I think our similar-but-different scrolling models are conflicting here. > > > > On Mac Mountain Lion we use a tile cache behind the root's layer, so contentsInCompositedLayer() is true. However, we don't have support for compositing fixed elements, so we have to repaint fixed things on scrolling. Once we put fixed things into composited layers, they'll be excluded from the regionToUpdate. But the ! isCompositedContentLayer check doesn't appropriately reflect the "content is painted into a layer which moves when you scroll and doesn't need to repaint". That seems like a behavior that the ScrollingCoordinator should export, like frameRepaintsOnScroll(FrameView*) or something. > > Maybe I'm misunderstanding something here. > > This patch changes the logic so that the scrollContentsFastPath is always used (and not aborted by this heuristic) when the contents are in a composited layer. I'm not sure why being in a composited layer affects the behavior at all.
(In reply to comment #6) > I'm not sure why being in a composited layer affects the behavior at all. Now that I think about it, I think Chromium's non-composited paint aggregator should automatically handle "too many" invalidations already. So, in both cases it'd be better for us to skip the heuristic and defer to our own invalidation handling code to make the choice about what to do. So then the questions are: (1) Is this heuristic useful for other ports for both composited and non-composited mode? (2) If it is useful, then is ScrollingCoordinator the best place to put this switch? I'm not quite sure what this function should be called, though.
Created attachment 154175 [details] Remove fixed object heuristic
Comment on attachment 154175 [details] Remove fixed object heuristic Attachment 154175 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13347035 New failing tests: fast/loader/loadInProgress.html fast/inline/positionedLifetime.html fast/loader/unload-form-post-about-blank.html
Created attachment 154192 [details] Archive of layout-test-results from gce-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
I may be missing something, but if you composite all the fixed elements and the elements above, why do you need to go in that path at all? Shouldn't you take a shortcut and just update the newly exposed area?
(In reply to comment #11) > I may be missing something, but if you composite all the fixed elements and the elements above, why do you need to go in that path at all? > > Shouldn't you take a shortcut and just update the newly exposed area? Chromium doesn't automatically composite all fixed position elements (yet), so regionToUpdate still includes them.
smfr, thorton: ping, re: questions in comment 7.
(In reply to comment #13) > smfr, thorton: ping, re: questions in comment 7. Sorry to re-ping on this bug, but it's been a week since I posted a three line patch and nobody but noamr has been forthcoming about whether or why this heuristic is useful for their port. Is there somebody else I should CC who works on Safari?
Comment on attachment 154175 [details] Remove fixed object heuristic Clearing flags on attachment: 154175 Committed r124919: <http://trac.webkit.org/changeset/124919>
All reviewed patches have been landed. Closing bug.