Return an infinite rect when calling RenderLayerClipper::parentClipRects on the root context layer

Return an infinite rect when calling RenderLayerClipper::parentClipRects on the root context layer

When calling RenderLayerClipper::backgroundClipRect on the
RenderLayerClipper object corresponding to the RenderLayer
which is equal to clipRectsContext.rootLayer, it first calls
parentClipRects on the clipper of that RenderLayer's parent.
In turn, this starts a recursive walk up the RenderLayer
tree in updateClipRects. This is incorrect, since a
RenderLayerClipper object is unable to clip above rootLayer
in the render tree. In particular, the recursion condition
in updateClipRects
depends on the recursion starting at a layer which is below
or equal to rootLayer.

BUG=336563
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170234

[Ian Vollick, 2014-03-27 17:30:06]

On 2014/03/25 23:17:07, chrishtr wrote:

I'm a bit worried about how this will affect code that's depending on the
backgroundClipRect for layer sizes. In particular, in
CLM::updateGraphicsLayerGeometry we have:

        ClipRectsContext clipRectsContext(compAncestor, CompositingClipRects,
IgnoreOverlayScrollbarSize, IgnoreOverflowClip);
        IntRect parentClipRect =
pixelSnappedIntRect(m_owningLayer.clipper().backgroundClipRect(clipRectsContext).rect());

IIUC, this will lead to infinite parent clip rects, breaking the
m_ancestorClippingLayer. Perhaps the code I've cited needs to be updated?

[Ian Vollick, 2014-03-27 17:30:28]

https://codereview.chromium.org/211683003/diff/110001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerClipper.cpp (right):

https://codereview.chromium.org/211683003/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderLayerClipper.cpp:365: }
nit: this could probably be moved above the ASSERT.

[chrishtr, 2014-03-27 22:41:28]

In the case you cited, the clipping context is explicitly w.r.t. the ancestor
composited layer. In that case, rootLayer is not the same as the current layer,
so parentClipRects will return a non-infinite rect.

The same goes for the other call sites of backgroundClipRect as far as I can
tell. e.g. one uses a pagination root, and two use the view root.

https://codereview.chromium.org/211683003/diff/110001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerClipper.cpp (right):

https://codereview.chromium.org/211683003/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderLayerClipper.cpp:365: }
On 2014/03/27 17:30:28, Ian Vollick wrote:
> nit: this could probably be moved above the ASSERT.

Done.

[Ian Vollick, 2014-03-27 23:06:21]

On 2014/03/27 22:41:28, chrishtr wrote:
> In the case you cited, the clipping context is explicitly w.r.t. the ancestor
> composited layer. In that case, rootLayer is not the same as the current
layer,
> so parentClipRects will return a non-infinite rect.
> 
> The same goes for the other call sites of backgroundClipRect as far as I can
> tell. e.g. one uses a pagination root, and two use the view root.
> 
>
https://codereview.chromium.org/211683003/diff/110001/Source/core/rendering/R...
> File Source/core/rendering/RenderLayerClipper.cpp (right):
> 
>
https://codereview.chromium.org/211683003/diff/110001/Source/core/rendering/R...
> Source/core/rendering/RenderLayerClipper.cpp:365: }
> On 2014/03/27 17:30:28, Ian Vollick wrote:
> > nit: this could probably be moved above the ASSERT.
> 
> Done.

lgtm. Thanks for the explanations!

[hartmanng, 2014-03-27 23:34:59]

lgtm2

[chrishtr, 2014-03-27 23:46:20]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-03-27 23:46:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrishtr@chromium.org/211683003/150001

[commit-bot: I haz the power, 2014-03-27 23:51:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 23:51:19]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[chrishtr, 2014-03-27 23:53:47]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-03-27 23:54:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrishtr@chromium.org/211683003/150001

[commit-bot: I haz the power, 2014-03-28 01:03:09]

Change committed as 170234