Clear absolute clip rects when transform changes

Clear absolute clip rects when transform changes

A layer's absolute clip rects are no longer valid when its
transform or the transform of an ancestor changes, since the
layer moves in absolute space. This CL clears the absolute
clip rects of a layer and its descendants when the layer's
transform changes.

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

[ajuma, 2014-06-17 15:20:25]

Without this change, the bounds used for overlap testing can be incorrect (since
they get clipped by a stale clip rect), which can cause us to fail to promote a
layer that does in fact overlap a composited layer. See
http://jsfiddle.net/XN8Jm/8/ for an example of content that disappears because
we fail to promote.

[Ian Vollick, 2014-06-17 15:57:28]

https://codereview.chromium.org/334373002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/334373002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:508:
m_clipper.clearClipRectsIncludingDescendants(AbsoluteClipRects);
Doesn't this mean that even if we call updateTransform, and the transform
doesn't change, we'll always clear our AbsoluteClipRects? Should we compare the
old and new transforms?

[ajuma, 2014-06-17 16:01:50]

https://codereview.chromium.org/334373002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/334373002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:508:
m_clipper.clearClipRectsIncludingDescendants(AbsoluteClipRects);
On 2014/06/17 15:57:28, Ian Vollick wrote:
> Doesn't this mean that even if we call updateTransform, and the transform
> doesn't change, we'll always clear our AbsoluteClipRects? Should we compare
the
> old and new transforms?

The caller of updateTransform (RenderLayer::styleChanged) already checks if the
old and new transforms are the same, and only calls updateTransform when they're
not.

[Ian Vollick, 2014-06-17 16:07:25]

lgtm

https://codereview.chromium.org/334373002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/334373002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:508:
m_clipper.clearClipRectsIncludingDescendants(AbsoluteClipRects);
On 2014/06/17 16:01:50, ajuma wrote:
> On 2014/06/17 15:57:28, Ian Vollick wrote:
> > Doesn't this mean that even if we call updateTransform, and the transform
> > doesn't change, we'll always clear our AbsoluteClipRects? Should we compare
> the
> > old and new transforms?
> 
> The caller of updateTransform (RenderLayer::styleChanged) already checks if
the
> old and new transforms are the same, and only calls updateTransform when
they're
> not.

Ah, thanks. In that case, maybe we could add an ASSERT here just to make sure
that remains true.

[ajuma, 2014-06-17 17:10:41]

https://codereview.chromium.org/334373002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/334373002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:508:
m_clipper.clearClipRectsIncludingDescendants(AbsoluteClipRects);
On 2014/06/17 16:07:25, Ian Vollick wrote:
> On 2014/06/17 16:01:50, ajuma wrote:
> > On 2014/06/17 15:57:28, Ian Vollick wrote:
> > > Doesn't this mean that even if we call updateTransform, and the transform
> > > doesn't change, we'll always clear our AbsoluteClipRects? Should we
compare
> > the
> > > old and new transforms?
> > 
> > The caller of updateTransform (RenderLayer::styleChanged) already checks if
> the
> > old and new transforms are the same, and only calls updateTransform when
> they're
> > not.
> 
> Ah, thanks. In that case, maybe we could add an ASSERT here just to make sure
> that remains true.

Adding an ASSERT is a good idea, but we don't have access to the old style here,
just the new one. We also can't compare m_transform before and after the
applyTransform below since the transform (expressed as TransformOperations)
could change without changing the resulting matrix (also, having to copy the old
m_transform just for the sake of an ASSERT doesn't seem great).

[Ian Vollick, 2014-06-17 17:18:42]

On 2014/06/17 17:10:41, ajuma wrote:
>
https://codereview.chromium.org/334373002/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/334373002/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderLayer.cpp:508:
> m_clipper.clearClipRectsIncludingDescendants(AbsoluteClipRects);
> On 2014/06/17 16:07:25, Ian Vollick wrote:
> > On 2014/06/17 16:01:50, ajuma wrote:
> > > On 2014/06/17 15:57:28, Ian Vollick wrote:
> > > > Doesn't this mean that even if we call updateTransform, and the
transform
> > > > doesn't change, we'll always clear our AbsoluteClipRects? Should we
> compare
> > > the
> > > > old and new transforms?
> > > 
> > > The caller of updateTransform (RenderLayer::styleChanged) already checks
if
> > the
> > > old and new transforms are the same, and only calls updateTransform when
> > they're
> > > not.
> > 
> > Ah, thanks. In that case, maybe we could add an ASSERT here just to make
sure
> > that remains true.
> 
> Adding an ASSERT is a good idea, but we don't have access to the old style
here,
> just the new one. We also can't compare m_transform before and after the
> applyTransform below since the transform (expressed as TransformOperations)
> could change without changing the resulting matrix (also, having to copy the
old
> m_transform just for the sake of an ASSERT doesn't seem great).

Hmm. How about this? Moving the if (!oldStyle || transform is really different)
check into updateTransform (so that it's easier to see), and pass in the render
style unconditionally.

[ajuma, 2014-06-17 23:21:39]

PTAL.

Turns out there was one more caller of updateTransform --
RenderBox::updateLayerTransform, that gets called after layout (since layout can
change the size of the box, which affects transform-origin). But we really don't
need to go through all of updateTransform after layout, since layout can't
change whether we have a transform or whether we have a 3d transform, and clip
rects already get cleared during layout; we just need to update the
transformation matrix if we have one. So I've split the updating of m_transform
into a separate method (updateTransformationMatrix), which gets called by
updateTransform and by RenderBox::updateLayerTransform, and I've renamed
RenderBox::updateLayerTransform to updateLayerTransformAfterLayout.

https://codereview.chromium.org/334373002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/334373002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:527: if (had3DTransform !=
has3DTransform())
We don't need to also check this in the post-layout update (when
updateTransformationMatrix is called by
RenderBox::updateLayerTransformAfterLayout), since the z-coordinate of
transform-origin is always expressed as a length rather than as a percent of box
size, so won't be changed by layout.

https://codereview.chromium.org/334373002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (left):

https://codereview.chromium.org/334373002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2160:
toRenderBox(this)->updateLayerTransform();
This should be unnecessary given the above call to styleDidChange, since
RenderLayerModelObject::styleDidChange already calls RenderLayer::styleChanged,
which calls RenderLayer::updateTransform.

[Ian Vollick, 2014-06-18 16:26:57]

On 2014/06/17 at 23:21:39, ajuma wrote:
> PTAL.
> 
> Turns out there was one more caller of updateTransform --
RenderBox::updateLayerTransform, that gets called after layout (since layout can
change the size of the box, which affects transform-origin). But we really don't
need to go through all of updateTransform after layout, since layout can't
change whether we have a transform or whether we have a 3d transform, and clip
rects already get cleared during layout; we just need to update the
transformation matrix if we have one. So I've split the updating of m_transform
into a separate method (updateTransformationMatrix), which gets called by
updateTransform and by RenderBox::updateLayerTransform, and I've renamed
RenderBox::updateLayerTransform to updateLayerTransformAfterLayout.
> 
>
https://codereview.chromium.org/334373002/diff/20001/Source/core/rendering/Re...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/334373002/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayer.cpp:527: if (had3DTransform !=
has3DTransform())
> We don't need to also check this in the post-layout update (when
updateTransformationMatrix is called by
RenderBox::updateLayerTransformAfterLayout), since the z-coordinate of
transform-origin is always expressed as a length rather than as a percent of box
size, so won't be changed by layout.
> 
>
https://codereview.chromium.org/334373002/diff/20001/Source/core/rendering/Re...
> File Source/core/rendering/RenderObject.cpp (left):
> 
>
https://codereview.chromium.org/334373002/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderObject.cpp:2160:
toRenderBox(this)->updateLayerTransform();
> This should be unnecessary given the above call to styleDidChange, since
RenderLayerModelObject::styleDidChange already calls RenderLayer::styleChanged,
which calls RenderLayer::updateTransform.

slgtm. It's great that you were able to streamline the post-layout updates!

[ajuma, 2014-06-18 16:40:57]

The CQ bit was checked by ajuma@chromium.org

[commit-bot: I haz the power, 2014-06-18 16:41:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajuma@chromium.org/334373002/20001

[commit-bot: I haz the power, 2014-06-18 16:45:31]

Change committed as 176432