Fix nested border radius with composited child.

Fix nested border radius with composited child.

Walk up the layer tree from a layer that needs an
AncestorChildClippingMaskLayer to find all border radius clips to
apply. Several auxiliary changes were required. The code in CLM for
determining if a mask layer is needed has also been reworked to
test against all clips.

Make LayerClipRecorder::CollectRoundedRectClips a public static member
so that CLM can use it to determine if the mask layer is required. Also
expand the conditions in which LayerClipRecorder uses the method to
catch all cases where masks need rounded clips.

Make several PaintLayer methods const, because they can be and we need
them so.

New tests for various cases of border radius where not just the parent
layer needs checking, or where we need to skip over the parent layer and
still check.

R=chrishtr@chromium.org
BUG=672561
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2923683002
Cr-Commit-Position: refs/heads/master@{#478814}
Committed: https://chromium.googlesource.com/chromium/src/+/e4208d105f75a14f9719ae35c19d7104f08a5f0b

[Stephen Chennney, 2017-06-05 21:01:00]

Description was changed from

==========
Fixing nested border radius with composited child.

WIP do not commit

Basically walks up the tree to find all border radius clips to apply.
Have to turn off an optimization in LayerClipRecorder because it
prevents us finding all the clips.

R=chrishtr@chromium.org
BUG=672561
==========

to

==========
Fixing nested border radius with composited child.

WIP do not commit

Basically walks up the tree to find all border radius clips to apply.
Have to turn off an optimization in LayerClipRecorder because it
prevents us finding all the clips.

R=chrishtr@chromium.org
BUG=672561
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-06-09 17:29:30]

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

https://codereview.chromium.org/2923683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:594:
owning_layer_.Clipper(PaintLayer::kDoNotUseGeometryMapper)
1. clip_rect will be in the space of compositing_ancestor. That's because you
(correctly) set compositing_ancestor as the root for the ClipRectsContext.

https://codereview.chromium.org/2923683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:604:
LayoutRect local_visual_rect = composited_bounds_;
2. local_visual_rect is in the space of owning_layer_'s PaintLayer, which is
the same as it's LayoutObject's border box space.

https://codereview.chromium.org/2923683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:605:
GetLayoutObject().MapToVisualRectInAncestorSpace(clipping_container,
(1) and (2) lead me to conclude that you should use compositing_ancestor as
the first parameter to MapToVisualRectInAncestorSpace. I think that should
be the only fix needed to this code.

[Stephen Chennney, 2017-06-09 20:51:20]

Description was changed from

==========
Fixing nested border radius with composited child.

WIP do not commit

Basically walks up the tree to find all border radius clips to apply.
Have to turn off an optimization in LayerClipRecorder because it
prevents us finding all the clips.

R=chrishtr@chromium.org
BUG=672561
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix nested border radius with composited child.

Walk up the layer tree from a layer that needs an
AncestorChildClippingMaskLayer to find all border radius clips to
apply. Several auxiliary changes were required. The code in CLM for
determining if a mask layer is needed has also been reworked to
test against all clips.

Make LayerClipRecorder::CollectRoundedRectClips a public static member
so that CLM can use it to determine if the mask layer is required. Also
expand the conditions in which LayerClipRecorder uses the method to
catch all cases where masks need rounded clips.

Make several PaintLayer methods const, because they can be and we need
them so.

New tests for various cases of border radius where not just the parent layer
needs checking, or where we need to skip over the parent layer and still check.

R=chrishtr@chromium.org
BUG=672561
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Stephen Chennney, 2017-06-09 20:52:37]

I think this is good to go. I'll need one more iteration to get the new
baselines, but the code could be reviewed now.

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

https://codereview.chromium.org/2923683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:594:
owning_layer_.Clipper(PaintLayer::kDoNotUseGeometryMapper)
On 2017/06/09 17:29:30, chrishtr wrote:
> 1. clip_rect will be in the space of compositing_ancestor. That's because you
> (correctly) set compositing_ancestor as the root for the ClipRectsContext.

Acknowledged.

https://codereview.chromium.org/2923683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:604:
LayoutRect local_visual_rect = composited_bounds_;
On 2017/06/09 17:29:30, chrishtr wrote:
> 2. local_visual_rect is in the space of owning_layer_'s PaintLayer, which is
> the same as it's LayoutObject's border box space.

Acknowledged.

https://codereview.chromium.org/2923683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:605:
GetLayoutObject().MapToVisualRectInAncestorSpace(clipping_container,
On 2017/06/09 17:29:30, chrishtr wrote:
> (1) and (2) lead me to conclude that you should use compositing_ancestor as
> the first parameter to MapToVisualRectInAncestorSpace. I think that should
> be the only fix needed to this code.

Acknowledged.

[chrishtr, 2017-06-09 20:59:04]

Description was changed from

==========
Fix nested border radius with composited child.

Walk up the layer tree from a layer that needs an
AncestorChildClippingMaskLayer to find all border radius clips to
apply. Several auxiliary changes were required. The code in CLM for
determining if a mask layer is needed has also been reworked to
test against all clips.

Make LayerClipRecorder::CollectRoundedRectClips a public static member
so that CLM can use it to determine if the mask layer is required. Also
expand the conditions in which LayerClipRecorder uses the method to
catch all cases where masks need rounded clips.

Make several PaintLayer methods const, because they can be and we need
them so.

New tests for various cases of border radius where not just the parent layer
needs checking, or where we need to skip over the parent layer and still check.

R=chrishtr@chromium.org
BUG=672561
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix nested border radius with composited child.

Walk up the layer tree from a layer that needs an
AncestorChildClippingMaskLayer to find all border radius clips to
apply. Several auxiliary changes were required. The code in CLM for
determining if a mask layer is needed has also been reworked to
test against all clips.

Make LayerClipRecorder::CollectRoundedRectClips a public static member
so that CLM can use it to determine if the mask layer is required. Also
expand the conditions in which LayerClipRecorder uses the method to
catch all cases where masks need rounded clips.

Make several PaintLayer methods const, because they can be and we need
them so.

New tests for various cases of border radius where not just the parent
layer needs checking, or where we need to skip over the parent layer and
still check.

R=chrishtr@chromium.org
BUG=672561
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2017-06-09 22:45:28]

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

https://codereview.chromium.org/2923683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:554:
bool CompositedLayerMapping::AncestorRoundedCornersWontClip(
Please unittest this directly.

https://codereview.chromium.org/2923683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:555:
const LayoutBoxModelObject* clipping_container) {
I don't think this argument is necessary. Just iterate over clipping container
until you reach an element higher in the DOM than compositing_container.

https://codereview.chromium.org/2923683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:557:
// the root to correctly handle nested clips.
You don't need to look above above compositing_ancestor.
Those clips don't matter because they are applied by other
composited layers.

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

https://codereview.chromium.org/2923683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/LayerClipRecorder.h:56: // Build a vector
of the border radius clips that shoudl be applied to
nit: should

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

https://codereview.chromium.org/2923683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:440: while (const
PaintLayer* new_clip_root =
You can stop at the PaintLayer indicated by CompositingContainer().

[Stephen Chennney, 2017-06-12 14:43:30]

Thanks for the input.

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

https://codereview.chromium.org/2923683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:554:
bool CompositedLayerMapping::AncestorRoundedCornersWontClip(
On 2017/06/09 22:45:28, chrishtr wrote:
> Please unittest this directly.

Will do.

https://codereview.chromium.org/2923683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:555:
const LayoutBoxModelObject* clipping_container) {
On 2017/06/09 22:45:28, chrishtr wrote:
> I don't think this argument is necessary. Just iterate over clipping container
> until you reach an element higher in the DOM than compositing_container.

The argument is just because I happen to have already computed this thing so it
seems simpler to pass it in than compute it again.

I'll cut short the iteration at the compositing_container.

https://codereview.chromium.org/2923683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:557:
// the root to correctly handle nested clips.
On 2017/06/09 22:45:28, chrishtr wrote:
> You don't need to look above above compositing_ancestor.
> Those clips don't matter because they are applied by other
> composited layers.

Yes. I get that now.

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

https://codereview.chromium.org/2923683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/LayerClipRecorder.h:56: // Build a vector
of the border radius clips that shoudl be applied to
On 2017/06/09 22:45:28, chrishtr wrote:
> nit: should

Done.

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

https://codereview.chromium.org/2923683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:440: while (const
PaintLayer* new_clip_root =
On 2017/06/09 22:45:28, chrishtr wrote:
> You can stop at the PaintLayer indicated by CompositingContainer().

Will do.

[Stephen Chennney, 2017-06-12 15:04:56]

Working on the unit tests. Then done.

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

https://codereview.chromium.org/2923683002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:555:
const LayoutBoxModelObject* clipping_container) {
On 2017/06/12 14:43:29, Stephen Chennney wrote:
> On 2017/06/09 22:45:28, chrishtr wrote:
> > I don't think this argument is necessary. Just iterate over clipping
container
> > until you reach an element higher in the DOM than compositing_container.
> 
> The argument is just because I happen to have already computed this thing so
it
> seems simpler to pass it in than compute it again.
> 
> I'll cut short the iteration at the compositing_container.

Misunderstood. I see what you mean now and have made the change.

[chrishtr, 2017-06-12 15:16:23]

I don't see a new patchset yet...

[Stephen Chennney, 2017-06-12 18:20:25]

On 2017/06/12 15:16:23, chrishtr wrote:
> I don't see a new patchset yet...

Sorry I wasn't clear. I was waiting until it was all done. Now all done.

[Stephen Chennney, 2017-06-12 18:20:31]

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

[commit-bot: I haz the power, 2017-06-12 18:21:01]

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, 2017-06-12 19:53:05]

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

[commit-bot: I haz the power, 2017-06-12 19:53:06]

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

[chrishtr, 2017-06-12 19:58:47]

Looking great!

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

https://codereview.chromium.org/2923683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:498:
bool AncestorRoundedCornersWontClip(const PaintLayer*);
Name this arg compositing_ancestor and add documentation for it.

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

https://codereview.chromium.org/2923683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1432:
SetBodyInnerHTML(
Make each of these its own testcase.

https://codereview.chromium.org/2923683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1459:
EXPECT_TRUE(child_mapping->AncestorClippingMaskLayer());
Add an expectation for the size of the mask layer.

https://codereview.chromium.org/2923683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1523:
EXPECT_TRUE(child_mapping->AncestorClippingMaskLayer());
ditto

[Stephen Chennney, 2017-06-12 20:00:52]

The current patch addresses all the review comments, I think.

There's more space for optimizations on creating the mask layer, and ancestor
clipping layers too, but I don't think they are worth it right now. For example,
a grandparent may have border radius but the parent entirely clips with a
rectangle. To do better we would need to combine all the ancestor clips to see
if the total result needs a border radius mask, but I suspect pages that mix
border-radius and no-border radius are rare for design aesthetic reasons.

[Stephen Chennney, 2017-06-12 20:02:09]

On 2017/06/12 20:00:52, Stephen Chennney wrote:
> The current patch addresses all the review comments, I think.
> 
> There's more space for optimizations on creating the mask layer, and ancestor
> clipping layers too, but I don't think they are worth it right now. For
example,
> a grandparent may have border radius but the parent entirely clips with a
> rectangle. To do better we would need to combine all the ancestor clips to see
> if the total result needs a border radius mask, but I suspect pages that mix
> border-radius and no-border radius are rare for design aesthetic reasons.

Sorry, I didn't get your last round of comments before writing my previous
message. I'm still working to address those latest review comments.

[Stephen Chennney, 2017-06-12 20:41:54]

I think I got it all. I also changed some EXPECT_BLAH to ASSERT_BLAH when we are
relying on the pointer existing in a subsequent check.

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

https://codereview.chromium.org/2923683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h:498:
bool AncestorRoundedCornersWontClip(const PaintLayer*);
On 2017/06/12 19:58:46, chrishtr wrote:
> Name this arg compositing_ancestor and add documentation for it.

Done.

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

https://codereview.chromium.org/2923683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1432:
SetBodyInnerHTML(
On 2017/06/12 19:58:47, chrishtr wrote:
> Make each of these its own testcase.

Done.

https://codereview.chromium.org/2923683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1459:
EXPECT_TRUE(child_mapping->AncestorClippingMaskLayer());
On 2017/06/12 19:58:46, chrishtr wrote:
> Add an expectation for the size of the mask layer.

Done.

https://codereview.chromium.org/2923683002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp:1523:
EXPECT_TRUE(child_mapping->AncestorClippingMaskLayer());
On 2017/06/12 19:58:46, chrishtr wrote:
> ditto

Done.

[chrishtr, 2017-06-12 20:43:33]

lgtm

[Stephen Chennney, 2017-06-12 20:48:35]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2017-06-12 20:49:02]

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, 2017-06-12 23:18:15]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497300515242540,
"parent_rev": "fc37c2192f75423124df19f8f2bf31709682d723", "commit_rev":
"e4208d105f75a14f9719ae35c19d7104f08a5f0b"}

[commit-bot: I haz the power, 2017-06-12 23:18:31]

Description was changed from

==========
Fix nested border radius with composited child.

Walk up the layer tree from a layer that needs an
AncestorChildClippingMaskLayer to find all border radius clips to
apply. Several auxiliary changes were required. The code in CLM for
determining if a mask layer is needed has also been reworked to
test against all clips.

Make LayerClipRecorder::CollectRoundedRectClips a public static member
so that CLM can use it to determine if the mask layer is required. Also
expand the conditions in which LayerClipRecorder uses the method to
catch all cases where masks need rounded clips.

Make several PaintLayer methods const, because they can be and we need
them so.

New tests for various cases of border radius where not just the parent
layer needs checking, or where we need to skip over the parent layer and
still check.

R=chrishtr@chromium.org
BUG=672561
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix nested border radius with composited child.

Walk up the layer tree from a layer that needs an
AncestorChildClippingMaskLayer to find all border radius clips to
apply. Several auxiliary changes were required. The code in CLM for
determining if a mask layer is needed has also been reworked to
test against all clips.

Make LayerClipRecorder::CollectRoundedRectClips a public static member
so that CLM can use it to determine if the mask layer is required. Also
expand the conditions in which LayerClipRecorder uses the method to
catch all cases where masks need rounded clips.

Make several PaintLayer methods const, because they can be and we need
them so.

New tests for various cases of border radius where not just the parent
layer needs checking, or where we need to skip over the parent layer and
still check.

R=chrishtr@chromium.org
BUG=672561
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2923683002
Cr-Commit-Position: refs/heads/master@{#478814}
Committed:
https://chromium.googlesource.com/chromium/src/+/e4208d105f75a14f9719ae35c19d...
==========

[commit-bot: I haz the power, 2017-06-12 23:18:34]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/e4208d105f75a14f9719ae35c19d...