Bug 92011 – 50% fixed position coverage slow scroll heuristic is incorrect when invalidations aren't clipped

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adrienne Walker]

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?

[Adrienne Walker]

Created attachment 153871 [details]
Patch

[Adrienne Walker]

(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.

[Noam Rosenthal]

(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.

[Simon Fraser (smfr)]

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.

[Adrienne Walker]

(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.

[Simon Fraser (smfr)]

(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.

[Adrienne Walker]

(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.

[Adrienne Walker]

Created attachment 154175 [details]
Remove fixed object heuristic

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Benjamin Poulain]

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?

[Adrienne Walker]

(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.

[Adrienne Walker]

smfr, thorton: ping, re: questions in comment 7.

[Adrienne Walker]

(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?

[WebKit Review Bot]

Comment on attachment 154175 [details]
Remove fixed object heuristic

Clearing flags on attachment: 154175

Committed r124919: <http://trac.webkit.org/changeset/124919>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.