Fix border radius on composited children.

Fix border radius on composited children.

When a child of an overflow: hidden object is composited (and the ancestor is not) we were failing to apply the border-radius clip. This patch fixes the problem by adding a new masking layer for this situation, and modifying the layers used for painting to set the clip appropriately.

New tests are added to verify the behavior for border styles, and existing layout tests cover the bug (and fail before this patch). Unit testing is used for verifying update behavior.

There are remaining issues tracked in TODOs with corresponding bugs.

BUG=157218
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/0cfffd46dfaa1ee93d3d667f82b2fb3721078b34
Cr-Commit-Position: refs/heads/master@{#437948}

[Stephen Chennney, 2016-09-20 18:03:32]

Description was changed from

==========
Fix border radius on composited children.

WIP. Doesn't work yet.

BUG=157218
==========

to

==========
Fix border radius on composited children.

WIP. Doesn't work yet.

BUG=157218
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Stephen Chennney, 2016-09-20 18:06:44]

This is a super hacky patch that works on the test cases but will definitely not
work in practice. I'm posting it so that folks working on solving similar
problems can see where changes need to be made.

1. Need to handle changing properties that cause the ancestor clipping mask
layer to be needed/not-needed
2. Need to find and apply all necessary clips. So far just does one.
3. Need to unhack the way the border-radius object is found

To see it work, look at the new result for
fast/clip/overflow-border-radius-composited.html

[chrishtr, 2016-10-21 23:15:07]

chrishtr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2016-10-21 23:15:08]

https://codereview.chromium.org/2194273002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/LayerClipRecorder.cpp (right):

https://codereview.chromium.org/2194273002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/LayerClipRecorder.cpp:75: //
TODO(schenney): Not right, but hack to get something that applies a radius then
figure out the correct way to do it.
This is to get the right positioning relative to the new graphics layer?

[chrishtr, 2016-10-21 23:16:20]

https://codereview.chromium.org/2194273002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/2194273002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:217: if
(layoutObject.styleRef().hasBorderRadius() || addAncestorClips)
What does this one achieve? It seems to force setting radius to true, which
can't be true for all masks?

[Stephen Chennney, 2016-10-24 20:15:27]

Just responding to chrishtr's comments now that I am back looking at this.

https://codereview.chromium.org/2194273002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/LayerClipRecorder.cpp (right):

https://codereview.chromium.org/2194273002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/LayerClipRecorder.cpp:75: //
TODO(schenney): Not right, but hack to get something that applies a radius then
figure out the correct way to do it.
On 2016/10/21 23:15:08, chrishtr - OOO wrote:
> This is to get the right positioning relative to the new graphics layer?

Yes, the clipping container is in one layer (the parent's in the simple case)
and the mask is in another (the child's) and they are not at the same origin.
But it seems like there should be a better way to find their relative transform,
hence the TODO.

https://codereview.chromium.org/2194273002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/2194273002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:217: if
(layoutObject.styleRef().hasBorderRadius() || addAncestorClips)
On 2016/10/21 23:16:20, chrishtr - OOO wrote:
> What does this one achieve? It seems to force setting radius to true, which
> can't be true for all masks?

This is the hackish part of the whole thing. It's relying on the fact that the
only way addAncestorClips can be true is if, sometime back in
CompositedLayerMapping, we discovered a border radius clip and added a layer and
added a paint phase etc etc.

It's one of the things I want to improve.

[Stephen Chennney, 2016-11-08 21:42:52]

New patch moving closer to a final solution. I added lots of commentary.

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/borders/border-radius-with-composited-child.html
(right):

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/borders/border-radius-with-composited-child.html:5:
<div style="transform:translateZ(0); width:300px; height:300px;
margin-left:-50px; margin-top:-50px; background:green;"></div>
This is to make the test more obvious when wrong.

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:488:
return;
flackr@ told me we need this, and I agree.

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:566:
// clip rects applied to the ancestor clip?
This comment should be removed. I know why we have to do it this way.

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1086:
->position());  // TODO(schenney) Don't need this?
Looks like we do not need this, but more testing is required.

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
(right):

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:483:
bool& owningLayerIsMasked);
Not sure if it's a good idea to set two things with the same method, but it
avoids computing all the clipping container info twice.

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:1579: offsetFromRoot,
paintingAncestorClippingMask);
Here we can create a clipper (PaintLayerClipper) with the clipping container's
layer, but I'm still working on getting offsets and sizes right. Doing so would
avoid the extra argument.

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.h:846: bool
paintingAncestorClippingMask = false,
I'm trying to get rid of this extra arg. It's hackish.

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:325: // even if it
falls outside of the border box.
Wrong base? I'm not sure I changed this.

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:364: if
(addAncestorClips || shouldClipOverflow(context)) {
Trying to get rid of this by having the layoutObject be the clipping container.

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/2194273002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:934: //
TODO(schenney): This is the wrong object for painting AncestorClippingMask.
No it's not. It's the wrong object for all the clipping code, not the painting
code. Remove this comment.

[chrishtr, 2016-11-08 23:52:22]

I wonder if a different approach would work better. What if
m_ancestorClippingMaskLayer was
sized and painted the same as m_childClippingMaskLayer of the CLM that is the
ancestor in the
DOM and clips this object, but is not its stacking ancestor?

Then it remains to simply position this layer correctly, which should be
possible by
doing the same thing as what sets up m_childClippingMaskLayer. ?

[Stephen Chennney, 2016-11-14 18:16:01]

On 2016/11/08 23:52:22, chrishtr wrote:
> I wonder if a different approach would work better. What if
> m_ancestorClippingMaskLayer was
> sized and painted the same as m_childClippingMaskLayer of the CLM that is the
> ancestor in the
> DOM and clips this object, but is not its stacking ancestor?
> 
> Then it remains to simply position this layer correctly, which should be
> possible by
> doing the same thing as what sets up m_childClippingMaskLayer. ?

Let's see if I understand this.

We could make the m_ancestorClippingMaskLayer added by this patch a layer
hanging off the layer that defines the border radius in the first place, and
paint into it exactly as we paint the existing m_childClippingMaskLayer for that
same object. So far so good, I think.

But then when compositing we need it to be a mask layer for the child's layer so
that it masks the child's content. Hence we need it to be a sub-layer of one
GraphicsLayer for paint purposes, and another GraphicsLayer for compositing
purposes.

That's no doubt possible, but I do worry a bit about object lifetime issues. I'd
have to try it to see if those concerns are well-founded.

Given the state of this patch, the fastest way forward now is to get this method
working to see what is really involved (we not talking much more time there).
Then we can try the other method too and see which one ends up being cleaner.

[chrishtr, 2016-11-14 18:51:10]

On 2016/11/14 at 18:16:01, schenney wrote:
> On 2016/11/08 23:52:22, chrishtr wrote:
> > I wonder if a different approach would work better. What if
> > m_ancestorClippingMaskLayer was
> > sized and painted the same as m_childClippingMaskLayer of the CLM that is
the
> > ancestor in the
> > DOM and clips this object, but is not its stacking ancestor?
> > 
> > Then it remains to simply position this layer correctly, which should be
> > possible by
> > doing the same thing as what sets up m_childClippingMaskLayer. ?
> 
> Let's see if I understand this.
> 
> We could make the m_ancestorClippingMaskLayer added by this patch a layer
hanging off the layer that defines the border radius in the first place, and
paint into it exactly as we paint the existing m_childClippingMaskLayer for that
same object. So far so good, I think.

I wasn't suggesting that. I was suggesting that the parenting of
m_ancestorClippingMaskLayer remain exactly the same, but it be sized and painted
into in exactly the same way as the mask layer that would
be created for the thing with border radius. It should just work to paint that,
because, it works already for stacking children of the border-radius-clip
element.


> 
> But then when compositing we need it to be a mask layer for the child's layer
so that it masks the child's content. Hence we need it to be a sub-layer of one
GraphicsLayer for paint purposes, and another GraphicsLayer for compositing
purposes.
> 
> That's no doubt possible, but I do worry a bit about object lifetime issues.
I'd have to try it to see if those concerns are well-founded.
> 
> Given the state of this patch, the fastest way forward now is to get this
method working to see what is really involved (we not talking much more time
there). Then we can try the other method too and see which one ends up being
cleaner.

[Stephen Chennney, 2016-11-15 14:31:55]

In this version the only issue is over-clipping on the top-left in the test
cases. That is either a layer offset issue or it may also be due to specifying
some clipping rect that is too small. Investigation continues.

[Stephen Chennney, 2016-11-16 17:28:46]

This latest patch works on the test cases I have, so that's good.

But it may still be improved by choosing different offsets and bounds for the
ancestor masking layer, to avoid some of the coordinate transforms needed to
make it work.

Regardless of which path end up being best, there is also some code cleanup,
putting things in helpers for instance. And it's deeply odd to me that the two
places where we do clipping have different coordinate transform requirements (I
think).

[chrishtr, 2016-11-16 23:56:32]

https://codereview.chromium.org/2194273002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/2194273002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:323: // CSS clip
(different than clipping due to overflow) can clip to any box,
The code you are inserting here is SPv2-specific. Why are you solving that in
this patch?

[Stephen Chennney, 2016-11-17 18:05:48]

https://codereview.chromium.org/2194273002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/2194273002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:323: // CSS clip
(different than clipping due to overflow) can clip to any box,
On 2016/11/16 23:56:32, chrishtr wrote:
> The code you are inserting here is SPv2-specific. Why are you solving that in
> this patch?

I shouldn't be; I think I was being over-zealous. I need to remove it.

[chrishtr, 2016-11-17 18:09:58]

Ok. Is the patch ready for review?

[Stephen Chennney, 2016-11-17 19:24:44]

On 2016/11/17 18:09:58, chrishtr wrote:
> Ok. Is the patch ready for review?

Nope. Have a list of test cases to try first, and I want to try your idea of
positioning the layer to avoid so many translations.

[Stephen Chennney, 2016-11-29 00:36:31]

Description was changed from

==========
Fix border radius on composited children.

WIP. Doesn't work yet.

BUG=157218
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix border radius on composited children.

BUG=157218
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Stephen Chennney, 2016-11-30 19:12:43]

One more improvement to look at before review. Need to try to simplify the
coordinate system changes.

[Stephen Chennney, 2016-12-01 20:58:36]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 20:59:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stephen Chennney, 2016-12-01 21:03:24]

Description was changed from

==========
Fix border radius on composited children.

BUG=157218
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix border radius on composited children.

When a child of an overflow: hidden object is composited (and the ancestor is
not) we were failing to apply the border-radius clip. This patch fixes the
problem by adding a new masking layer for this situation, and modifying the
objects used for painting to set the clip appropriately.

New tests are added to verify the behavior for border styles, and existing
layout tests cover the bug (and fail now). Unit testing is used for verifying
update behavior.

BUG=157218
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Stephen Chennney, 2016-12-01 21:08:06]

The CQ bit was unchecked by schenney@chromium.org

[Stephen Chennney, 2016-12-01 21:10:32]

This latest version is ready for review. I need new test results but otherwise
am happy with the current state.

Cases we're not testing, and could: multi-col, scrolling cases

[chrishtr, 2016-12-02 19:21:48]

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:501:
if (clippingContainer->enclosingLayer()->hasRootScrollerAsDescendant())
This looks identical to 495-496.

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1727:
} else if (m_ancestorClippingLayer && m_ancestorClippingMaskLayer) {
Is it strictly necessary to check for m_ancestorClippingLayer here?

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.h:836: 
Nit: spurious.

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:351:
foregroundRect.intersect(offsetOverflowClipRect);
Is this just increasing clarity?

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:431: // TODO(trchen):
We haven't decided how to handle visual fragmentation with
This comment belongs down by line 446.

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:442:
paintLayerForFragmentsAndClip->convertToLayerCoords(
You can't map from an ancestor to a descendant PaintLayer with this method.
If that is desired, switch the two positions and negate.

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:645: 
Super nit: spurious.

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:910:
paintLayerForMask->convertToLayerCoords(layer.root(),
This duplicates line 442 I think?

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:952: context,
*(toLayoutBoxModelObject(m_paintLayer.clippingContainer())),
Might be easier to read if you make a local variable for the clipping object:

LayoutObject* clippingObject = paintFlags &
PaintLayerPaintingAncestorClippingMaskPhase ? m_paintLayer.clippingContainer()
: m_paintLayer.layoutObject()

[Stephen Chennney, 2016-12-06 22:04:59]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 22:05:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stephen Chennney, 2016-12-06 22:07:35]

The code in PaintLayerPainter and the clipper code is up for review to get early
feedback.

There are spurious changes in GraphicsLayer that should be in a different patch,
and I have not addressed the minor issues from the previous review, so it's not
ready to land.

Try jobs will fail, but I want to see what happens due to the changes to
showGraphicsLayers.

[commit-bot: I haz the power, 2016-12-06 22:51:40]

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

[commit-bot: I haz the power, 2016-12-06 22:51:41]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[chrishtr, 2016-12-07 19:16:24]

Code looks good now.

What is the latest status on the failing tests? Seems like there is a dangling
layer count
assert, and some unexpected changes to offsetFromLayoutObject for some layers.

https://codereview.chromium.org/2194273002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
(right):

https://codereview.chromium.org/2194273002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:637:
// m_ancestorClippingLayer
Please update this chart.

https://codereview.chromium.org/2194273002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/2194273002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:254: void
adjustOffsetForAncestorClippingMask(
This is now dead code.

[Stephen Chennney, 2016-12-07 19:33:11]

On 2016/12/07 19:16:24, chrishtr wrote:
> Code looks good now.
> 
> What is the latest status on the failing tests? Seems like there is a dangling
> layer count
> assert, and some unexpected changes to offsetFromLayoutObject for some layers.

Thanks. I haven't even looked at the tests yet while I sort out the stuff that
belongs in other patches.
 
>
https://codereview.chromium.org/2194273002/diff/200001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
> (right):
> 
>
https://codereview.chromium.org/2194273002/diff/200001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:637:
> // m_ancestorClippingLayer
> Please update this chart.

Will do.

> 
>
https://codereview.chromium.org/2194273002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):
> 
>
https://codereview.chromium.org/2194273002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:254: void
> adjustOffsetForAncestorClippingMask(
> This is now dead code.

Will fix.

[Stephen Chennney, 2016-12-07 21:36:53]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 21:37:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stephen Chennney, 2016-12-07 21:39:38]

This patch probably doesn't need review. It's basically fixing all the smaller
issues.

I'm waiting on new test results and there are 2 more tests I would like to try.

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:501:
if (clippingContainer->enclosingLayer()->hasRootScrollerAsDescendant())
On 2016/12/02 19:21:47, chrishtr wrote:
> This looks identical to 495-496.

Done.

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1727:
} else if (m_ancestorClippingLayer && m_ancestorClippingMaskLayer) {
On 2016/12/02 19:21:47, chrishtr wrote:
> Is it strictly necessary to check for m_ancestorClippingLayer here?

No, it's not necessary, despite me thinking it was. We can only have a
m_ancestorClippingMaskLayer if we have a m_ancestorClippingLayer because if the
latter doesn't exist it's because it never existed (and hence no mask) or we
removed it just above, in which case we removed its mask. Either way the
m_ancestorClippingMaskLayer will be null.

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.h:836: 
On 2016/12/02 19:21:47, chrishtr wrote:
> Nit: spurious.

Done.

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp (right):

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:351:
foregroundRect.intersect(offsetOverflowClipRect);
On 2016/12/02 19:21:47, chrishtr wrote:
> Is this just increasing clarity?

It was for debugging output. I've reverted it.

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right):

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:431: // TODO(trchen):
We haven't decided how to handle visual fragmentation with
On 2016/12/02 19:21:47, chrishtr wrote:
> This comment belongs down by line 446.

Done.

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:442:
paintLayerForFragmentsAndClip->convertToLayerCoords(
On 2016/12/02 19:21:47, chrishtr wrote:
> You can't map from an ancestor to a descendant PaintLayer with this method.
> If that is desired, switch the two positions and negate.

Done.

https://codereview.chromium.org/2194273002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:645: 
On 2016/12/02 19:21:47, chrishtr wrote:
> Super nit: spurious.

Done.

[Stephen Chennney, 2016-12-07 22:06:26]

Nested border radius element with a composited child don't work (I think I can
fix that) and the borders on the composited child are not drawn. That might be a
bug with any ancestor clipping layer.

So still not ready to land.

[commit-bot: I haz the power, 2016-12-07 22:25:09]

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

[commit-bot: I haz the power, 2016-12-07 22:25:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Stephen Chennney, 2016-12-08 15:33:12]

On 2016/12/07 22:06:26, Stephen Chennney wrote:
> Nested border radius element with a composited child don't work (I think I can
> fix that) and the borders on the composited child are not drawn. That might be
a
> bug with any ancestor clipping layer.
> 
> So still not ready to land.

On the other hand, this is strictly an improvement in the common case and the
failing cases are much less common. We do not regress anything with this patch.

So I propose moving forward and landing this, filing the following 3 bugs:
Nested border-radius clips do not apply correctly to composited children
Borders are not drawn correctly on composited children of border-radius elements
CSS clips do not apply to composited children

[chrishtr, 2016-12-08 17:49:15]

On 2016/12/08 at 15:33:12, schenney wrote:
> On 2016/12/07 22:06:26, Stephen Chennney wrote:
> > Nested border radius element with a composited child don't work (I think I
can
> > fix that) and the borders on the composited child are not drawn. That might
be a
> > bug with any ancestor clipping layer.
> > 
> > So still not ready to land.
> 
> On the other hand, this is strictly an improvement in the common case and the
failing cases are much less common. We do not regress anything with this patch.
> 
> So I propose moving forward and landing this,

Agreed.

filing the following 3 bugs:
> Nested border-radius clips do not apply correctly to composited children

This is when there are two border-radius clips between a composited layer and
its stacking ancestor, right? I think that is solvable by using the first child
of the stacking
ancestor, rather than the clipping container, for computing the clips for the
mask. Ok to put this as a TODO though.

> Borders are not drawn correctly on composited children of border-radius
elements

Is the border clipped out?

> CSS clips do not apply to composited children

This is orthogonal to your patch though, so definitely ok to delay.

I think CSS clip can be handled in m_ancestorClippingLayer (i.e. the non-mask
one) because CSS clips are always rects. Maybe even a simple patch...

[Stephen Chennney, 2016-12-08 19:43:17]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 19:43:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stephen Chennney, 2016-12-08 19:46:36]

On 2016/12/08 17:49:15, chrishtr wrote:
> On 2016/12/08 at 15:33:12, schenney wrote:
> > On 2016/12/07 22:06:26, Stephen Chennney wrote:
> > On the other hand, this is strictly an improvement in the common case and
the
> failing cases are much less common. We do not regress anything with this
patch.
> > 
> > So I propose moving forward and landing this,
> 
> Agreed.

This patch should be ready to land. Hopefully the trybots agree.

> 
> filing the following 3 bugs:
> > Nested border-radius clips do not apply correctly to composited children
> 
> This is when there are two border-radius clips between a composited layer and
> its stacking ancestor, right? I think that is solvable by using the first
child
> of the stacking
> ancestor, rather than the clipping container, for computing the clips for the
> mask. Ok to put this as a TODO though.

TODO is in, and yes I agree that fixing it is a matter of changing the layer
used for computing the clips.

> > Borders are not drawn correctly on composited children of border-radius
> elements
> 
> Is the border clipped out?

yesterday when I was looking at tests I thought it was, but now I can't repro.
I'm not adding a todo or bug until I see it again.

> > CSS clips do not apply to composited children
> 
> This is orthogonal to your patch though, so definitely ok to delay.
> 
> I think CSS clip can be handled in m_ancestorClippingLayer (i.e. the non-mask
> one) because CSS clips are always rects. Maybe even a simple patch...

CSS clips can be "shapes" so it's not a simple case of axis-aligned rects. But I
still think it's relatively straightforward once this patch is in place. I added
the TODO.

[commit-bot: I haz the power, 2016-12-08 20:52:11]

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

[commit-bot: I haz the power, 2016-12-08 20:52:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chrishtr, 2016-12-08 21:01:59]

On 2016/12/08 at 19:46:36, schenney wrote:
> On 2016/12/08 17:49:15, chrishtr wrote:
> > On 2016/12/08 at 15:33:12, schenney wrote:
> > > On 2016/12/07 22:06:26, Stephen Chennney wrote:
> > > On the other hand, this is strictly an improvement in the common case and
the
> > failing cases are much less common. We do not regress anything with this
patch.
> > > 
> > > So I propose moving forward and landing this,
> > 
> > Agreed.
> 
> This patch should be ready to land. Hopefully the trybots agree.
> 
> > 
> > filing the following 3 bugs:
> > > Nested border-radius clips do not apply correctly to composited children
> > 
> > This is when there are two border-radius clips between a composited layer
and
> > its stacking ancestor, right? I think that is solvable by using the first
child
> > of the stacking
> > ancestor, rather than the clipping container, for computing the clips for
the
> > mask. Ok to put this as a TODO though.
> 
> TODO is in, and yes I agree that fixing it is a matter of changing the layer
used for computing the clips.
> 
> > > Borders are not drawn correctly on composited children of border-radius
> > elements
> > 
> > Is the border clipped out?
> 
> yesterday when I was looking at tests I thought it was, but now I can't repro.
I'm not adding a todo or bug until I see it again.
> 
> > > CSS clips do not apply to composited children
> > 
> > This is orthogonal to your patch though, so definitely ok to delay.
> > 
> > I think CSS clip can be handled in m_ancestorClippingLayer (i.e. the
non-mask
> > one) because CSS clips are always rects. Maybe even a simple patch...
> 
> CSS clips can be "shapes" so it's not a simple case of axis-aligned rects. But
I still think it's relatively straightforward once this patch is in place. I
added the TODO.

CSS clip can only be an axis-aligned rect. By CSS clip I mean "clip: rect(...)".
clip-path and mask can have other shapes, but those induce stacking contexts.

[Stephen Chennney, 2016-12-08 21:13:02]

The win perf tests are crashes in ~DrawingRecorder, for some reason. Incorrect
cull rect is my guess.

The SPv2 failure is due to no code to handle that pathway. That we should punt
on.

[Stephen Chennney, 2016-12-09 20:55:44]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 20:56:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 21:00:42]

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

[commit-bot: I haz the power, 2016-12-09 21:00:43]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Stephen Chennney, 2016-12-09 21:30:39]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2016-12-09 21:31:00]

Finally, hopefully, ready to go.

[commit-bot: I haz the power, 2016-12-09 21:31:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 21:31:26]

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

[commit-bot: I haz the power, 2016-12-09 21:31:27]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Stephen Chennney, 2016-12-09 21:58:20]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 21:59:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chrishtr, 2016-12-09 23:09:35]

https://codereview.chromium.org/2194273002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DrawingRecorder.cpp
(right):

https://codereview.chromium.org/2194273002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DrawingRecorder.cpp:93:
m_displayItemType !=
I thought about this and tried out a few alternatives for avoiding this
situation,
but could find nothing simple that would work better than the current
implementation
of painting masks.

However, rather than this method of avoiding an assert, I would like this one:

return false from paintedOutputOfObjectHasNoEffectRegardlessOfSize() if 
layer()->compositingState() == PaintsIntoOwnBacking.

[commit-bot: I haz the power, 2016-12-10 00:03:53]

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

[commit-bot: I haz the power, 2016-12-10 00:03:54]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Stephen Chennney, 2016-12-12 19:17:22]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[Stephen Chennney, 2016-12-12 19:18:03]

The end is in sight!

https://codereview.chromium.org/2194273002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DrawingRecorder.cpp
(right):

https://codereview.chromium.org/2194273002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DrawingRecorder.cpp:93:
m_displayItemType !=
On 2016/12/09 23:09:34, chrishtr wrote:
> I thought about this and tried out a few alternatives for avoiding this
> situation,
> but could find nothing simple that would work better than the current
> implementation
> of painting masks.
> 
> However, rather than this method of avoiding an assert, I would like this one:
> 
> return false from paintedOutputOfObjectHasNoEffectRegardlessOfSize() if 
> layer()->compositingState() == PaintsIntoOwnBacking.

Done.

[commit-bot: I haz the power, 2016-12-12 19:18:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Stephen Chennney, 2016-12-12 19:19:01]

Description was changed from

==========
Fix border radius on composited children.

When a child of an overflow: hidden object is composited (and the ancestor is
not) we were failing to apply the border-radius clip. This patch fixes the
problem by adding a new masking layer for this situation, and modifying the
objects used for painting to set the clip appropriately.

New tests are added to verify the behavior for border styles, and existing
layout tests cover the bug (and fail now). Unit testing is used for verifying
update behavior.

BUG=157218
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix border radius on composited children.

When a child of an overflow: hidden object is composited (and the ancestor is
not) we were failing to apply the border-radius clip. This patch fixes the
problem by adding a new masking layer for this situation, and modifying the
layers used for painting to set the clip appropriately.

New tests are added to verify the behavior for border styles, and existing
layout tests cover the bug (and fail before this patch). Unit testing is used
for verifying update behavior.

There are remaining issues tracked in TODOs with corresponding bugs.

BUG=157218
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2016-12-12 19:41:46]

lgtm

https://codereview.chromium.org/2194273002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2194273002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2261: // must have some
effect.
s/can/must/. Also mention one such case, which is applying a clip inherited from
an ancestor.

[Stephen Chennney, 2016-12-12 19:49:22]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2016-12-12 19:49:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2194273002/#ps320001
(title: "Update the comment on why we say an empty div can paint")

[Stephen Chennney, 2016-12-12 19:49:47]

Thanks for all the hard work and advice on this one.

https://codereview.chromium.org/2194273002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/2194273002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:2261: // must have some
effect.
On 2016/12/12 19:41:46, chrishtr wrote:
> s/can/must/. Also mention one such case, which is applying a clip inherited
from
> an ancestor.

Done.

[commit-bot: I haz the power, 2016-12-12 19:50:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 23:16:53]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1481572162375400,
"parent_rev": "ffb90ef93eb58238d733647fdc89600dfc52737f", "commit_rev":
"e4579fe199aad27d9cb3dacecf957b95eddeccd2"}

[commit-bot: I haz the power, 2016-12-12 23:17:23]

Description was changed from

==========
Fix border radius on composited children.

When a child of an overflow: hidden object is composited (and the ancestor is
not) we were failing to apply the border-radius clip. This patch fixes the
problem by adding a new masking layer for this situation, and modifying the
layers used for painting to set the clip appropriately.

New tests are added to verify the behavior for border styles, and existing
layout tests cover the bug (and fail before this patch). Unit testing is used
for verifying update behavior.

There are remaining issues tracked in TODOs with corresponding bugs.

BUG=157218
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix border radius on composited children.

When a child of an overflow: hidden object is composited (and the ancestor is
not) we were failing to apply the border-radius clip. This patch fixes the
problem by adding a new masking layer for this situation, and modifying the
layers used for painting to set the clip appropriately.

New tests are added to verify the behavior for border styles, and existing
layout tests cover the bug (and fail before this patch). Unit testing is used
for verifying update behavior.

There are remaining issues tracked in TODOs with corresponding bugs.

BUG=157218
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2194273002
==========

[commit-bot: I haz the power, 2016-12-12 23:17:25]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-12-12 23:20:29]

Description was changed from

==========
Fix border radius on composited children.

When a child of an overflow: hidden object is composited (and the ancestor is
not) we were failing to apply the border-radius clip. This patch fixes the
problem by adding a new masking layer for this situation, and modifying the
layers used for painting to set the clip appropriately.

New tests are added to verify the behavior for border styles, and existing
layout tests cover the bug (and fail before this patch). Unit testing is used
for verifying update behavior.

There are remaining issues tracked in TODOs with corresponding bugs.

BUG=157218
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2194273002
==========

to

==========
Fix border radius on composited children.

When a child of an overflow: hidden object is composited (and the ancestor is
not) we were failing to apply the border-radius clip. This patch fixes the
problem by adding a new masking layer for this situation, and modifying the
layers used for painting to set the clip appropriately.

New tests are added to verify the behavior for border styles, and existing
layout tests cover the bug (and fail before this patch). Unit testing is used
for verifying update behavior.

There are remaining issues tracked in TODOs with corresponding bugs.

BUG=157218
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/0cfffd46dfaa1ee93d3d667f82b2fb3721078b34
Cr-Commit-Position: refs/heads/master@{#437948}
==========

[commit-bot: I haz the power, 2016-12-12 23:20:31]

Patchset 17 (id:??) landed as
https://crrev.com/0cfffd46dfaa1ee93d3d667f82b2fb3721078b34
Cr-Commit-Position: refs/heads/master@{#437948}

[Stephen Chennney, 2016-12-14 15:43:50]

This patch was reverted in https://codereview.chromium.org/2569253002

The revert was due to at least 3 issues:
https://bugs.chromium.org/p/chromium/issues/detail?id=673668
https://bugs.chromium.org/p/chromium/issues/detail?id=673725
https://bugs.chromium.org/p/chromium/issues/detail?id=673810