Bug 80372 – RenderLayerCompositor doesn't properly clip graphics layer sizes

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

[Adrienne Walker]

Copying from previous discussion from https://bugs.webkit.org/show_bug.cgi?id=50192.

On http://goo.gl/eYGqX with Chrome, using accelerated canvas as a compositing trigger, some layer sizes end up being too large.  See https://bugs.webkit.org/attachment.cgi?id=130232.

jamesr says in https://bugs.webkit.org/show_bug.cgi?id=50192#c29:

The <div> being composited has children that extend down that far, but they're clipped by a (software-rendered) ancestor.  When I load the page this part of the DOM looks like:
<div id="views-control" style="z-index: 0"> // this is composited
  <div>
    <div>
      <div class="mv-scroller" style="overflow-y: hidden; height: 26px"> // this clips
        <div>Traffic</div>
        <div>Transit</div>
        <div>Photos</div>
...


Only the Traffic div is visible because of mv-scroller, but the other <div>s still occupy layout space and grow this layer's bounds.

[Adrienne Walker]

Created attachment 130895 [details]
Patch

[Adrienne Walker]

(In reply to comment #1)
> Created an attachment (id=130895) [details]
> Patch

I'm not super happy with this patch because of the extra work walking up layer trees, but it's a first cut at getting the job done.

The other approach I considered was to switch calculateCompositedBounds to iterate through RenderLayer children directly rather than through the z-order and normal flow lists.  There's some hairiness with which layers get included in those lists and which don't, but it might end up being cleaner.

smfr: What do you think?

[Simon Fraser (smfr)]

I wonder if we can make use of cached clip rects somehow, or, if we need to, cache a new clip rect for use here.

I don't like how clippingLayerClosestToAncestor() uses recursion when it could iterate.

[Adrienne Walker]

Created attachment 130911 [details]
Remove recursion

[Adrienne Walker]

(In reply to comment #3)
> I wonder if we can make use of cached clip rects somehow, or, if we need to, cache a new clip rect for use here.

I may just be misunderstanding clip rects, but the cached clip rects root didn't seem to necessarily be the composited layer passed originally to calculateCompositedBounds.

> I don't like how clippingLayerClosestToAncestor() uses recursion when it could iterate.

Fixed.

[Adrienne Walker]

(In reply to comment #5)
> (In reply to comment #3)
> > I wonder if we can make use of cached clip rects somehow, or, if we need to, cache a new clip rect for use here.
> 
> I may just be misunderstanding clip rects, but the cached clip rects root didn't seem to necessarily be the composited layer passed originally to calculateCompositedBounds.

smfr: If you want to use cached clip rects, do you have any suggestions on how best to go about that?

Is it a possibility to collapse clip rects down the layer tree hierarchy and store directly on child render layers when we build the z-order and normal flow lists? Or, do those lists not get dirtied at the right time?

[Simon Fraser (smfr)]

I think the " the cached clip rects root" should be the ancestor compositing layer, but I'd have to reload all the clip rects logic into my brain to know for sure.

[Adrienne Walker]

(In reply to comment #7)
> I think the " the cached clip rects root" should be the ancestor compositing layer, but I'd have to reload all the clip rects logic into my brain to know for sure.

As far as I can tell, sometimes layer->clippingRoot() can be an ancestor of that layer's ancestor compositing layer.

So, I can use the cached clip rects, but they are not in the right space.  Transforming them from a container layer to a local layer would require using localToAbsolute and absoluteToLocal (to respect transforms), and therefore a double walk of the render object hierarchy.

That seems more expensive than my original solution of walking up fewer render layers.

[Adrienne Walker]

(In reply to comment #8)
> (In reply to comment #7)
> > I think the " the cached clip rects root" should be the ancestor compositing layer, but I'd have to reload all the clip rects logic into my brain to know for sure.
> 
> As far as I can tell, sometimes layer->clippingRoot() can be an ancestor of that layer's ancestor compositing layer.

Er, ignore this.  I misread some code.  Let me take another look and see if I can get clip rects to work.

[Adrienne Walker]

Created attachment 132168 [details]
Use cached clip rects

[Adrienne Walker]

(In reply to comment #10)
> Created an attachment (id=132168) [details]
> Use cached clip rects

smfr: Do you have any time to look at this?

[Simon Fraser (smfr)]

Comment on attachment 132168 [details]
Use cached clip rects

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:511
> -
>  // The bounds of the GraphicsLayer created for a compositing layer is the union of the bounds of all the descendant

I want that blank line :)

[Adrienne Walker]

(In reply to comment #12)
> (From update of attachment 132168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132168&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:511
> > -
> >  // The bounds of the GraphicsLayer created for a compositing layer is the union of the bounds of all the descendant
> 
> I want that blank line :)

No problem, you can have it.  I've got plenty of extras.

[Adrienne Walker]

Committed r111456: <http://trac.webkit.org/changeset/111456>