cc: Enable composited border-radius scrolling.

cc: Enable composited border-radius scrolling.

Enable composited border-radius scrolling as mask tiling is implemented.
We'll revert this CL if anything breaks.

BUG=567293, 567296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[sunxd, 2017-05-04 15:10:45]

Description was changed from

==========
cc: Enable composited border-radius scrolling.

Enable composited border-radius scrolling as mask tiling is implemented.
We'll revert this CL if anything breaks.

BUG=567293, 567296
==========

to

==========
cc: Enable composited border-radius scrolling.

Enable composited border-radius scrolling as mask tiling is implemented.
We'll revert this CL if anything breaks.

BUG=567293, 567296
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[sunxd, 2017-05-04 15:10:47]

Description was changed from

==========
cc: Enable composited border-radius scrolling.

Enable composited border-radius scrolling as mask tiling is implemented.
We'll revert this CL if anything breaks.

BUG=567293, 567296
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
cc: Enable composited border-radius scrolling.

Enable composited border-radius scrolling as mask tiling is implemented.
We'll revert this CL if anything breaks.

BUG=567293, 567296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[sunxd, 2017-05-04 15:12:14]

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

[commit-bot: I haz the power, 2017-05-04 15:12:34]

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-05-04 16:47:18]

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

[commit-bot: I haz the power, 2017-05-04 16:47:18]

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

[sunxd, 2017-05-10 20:13:38]

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

[commit-bot: I haz the power, 2017-05-10 20:14:28]

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-05-10 21:40:45]

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

[commit-bot: I haz the power, 2017-05-10 21:40:46]

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

[sunxd, 2017-05-11 19:16:07]

Description was changed from

==========
cc: Enable composited border-radius scrolling.

Enable composited border-radius scrolling as mask tiling is implemented.
We'll revert this CL if anything breaks.

BUG=567293, 567296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
cc: Enable composited border-radius scrolling.

Enable composited border-radius scrolling as mask tiling is implemented.
We'll revert this CL if anything breaks.

BUG=567293, 567296
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[sunxd, 2017-05-11 19:16:08]

sunxd@chromium.org changed reviewers:
+ chrishtr@chromium.org, enne@chromium.org, trchen@chromium.org

[sunxd, 2017-05-11 19:19:13]

There are unexpected test failures due to compositing border-radius scrolling
elements. Like I discussed with Chris offline, one of the layout tests fails due
to a layer not having an ancestor mask layer.

I'm not very familiar with the composited layer mapping but I also don't think
this is caused by mask tiling. Do you have any idea or suggestion on it?

[chrishtr, 2017-05-11 20:28:46]

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

[chrishtr, 2017-05-11 20:28:46]

Stephen, adding you as an expert on the ancestor clipping mask layer. It seems
the clipping
is not working in this case, could you advise?

[chrishtr, 2017-05-11 20:29:46]

On 2017/05/11 at 20:28:46, chrishtr wrote:
> Stephen, adding you as an expert on the ancestor clipping mask layer. It seems
the clipping
> is not working in this case, could you advise?

See for example:

https://jsfiddle.net/786m1og3/

with this CL applied, and on a high-DPI monitor.

[sunxd, 2017-05-11 20:32:13]

On 2017/05/11 20:28:46, chrishtr wrote:
> Stephen, adding you as an expert on the ancestor clipping mask layer. It seems
> the clipping
> is not working in this case, could you advise?

For more info: I attached a debugger and found that the green layer didn't get
its ancestor clipping mask, it returned here:

CompositedLayerMapping.cpp:

 579│   const PaintLayer* compositing_ancestor =
 580│      
owning_layer_.EnclosingLayerWithCompositedLayerMapping(kExcludeSelf);
 581│   if (!compositing_ancestor)
 582│     return;
 583│
 584│   const LayoutBoxModelObject* clipping_container =
 585│       owning_layer_.ClippingContainer();
 586│   if (!clipping_container)
 587│     return;
 588│
 589│   if (clipping_container->EnclosingLayer() == scroll_parent)
 590├>    return;
 591│
 592│   if (compositing_ancestor->GetLayoutObject().IsDescendantOf(
 593│           clipping_container))
 594│     return;

[sunxd, 2017-05-12 15:37:10]

On 2017/05/11 20:32:13, sunxd wrote:
> On 2017/05/11 20:28:46, chrishtr wrote:
> > Stephen, adding you as an expert on the ancestor clipping mask layer. It
seems
> > the clipping
> > is not working in this case, could you advise?
> 
> For more info: I attached a debugger and found that the green layer didn't get
> its ancestor clipping mask, it returned here:
> 
> CompositedLayerMapping.cpp:
> 
>  579│   const PaintLayer* compositing_ancestor =
>  580│      
> owning_layer_.EnclosingLayerWithCompositedLayerMapping(kExcludeSelf);
>  581│   if (!compositing_ancestor)
>  582│     return;
>  583│
>  584│   const LayoutBoxModelObject* clipping_container =
>  585│       owning_layer_.ClippingContainer();
>  586│   if (!clipping_container)
>  587│     return;
>  588│
>  589│   if (clipping_container->EnclosingLayer() == scroll_parent)
>  590├>    return;
>  591│
>  592│   if (compositing_ancestor->GetLayoutObject().IsDescendantOf(
>  593│           clipping_container))
>  594│     return;

@schenney: gentle ping, can you verify that on this webpage
(https://lh3.googleusercontent.com/--NU9yrAZ9c4/WRNlk4diTCI/AAAAAAAAAaU/1uS6XJ...),
the clipping container's enclosing layer of layer (.spacer1.bottom) should be
the scroll parent (the first div#outer 400x400)?

[Stephen Chennney, 2017-05-12 15:38:29]

On 2017/05/11 20:32:13, sunxd wrote:
> On 2017/05/11 20:28:46, chrishtr wrote:
> > Stephen, adding you as an expert on the ancestor clipping mask layer. It
seems
> > the clipping
> > is not working in this case, could you advise?
> 
> For more info: I attached a debugger and found that the green layer didn't get
> its ancestor clipping mask, it returned here:
> 
> CompositedLayerMapping.cpp:
> 
>  579│   const PaintLayer* compositing_ancestor =
>  580│      
> owning_layer_.EnclosingLayerWithCompositedLayerMapping(kExcludeSelf);
>  581│   if (!compositing_ancestor)
>  582│     return;
>  583│
>  584│   const LayoutBoxModelObject* clipping_container =
>  585│       owning_layer_.ClippingContainer();
>  586│   if (!clipping_container)
>  587│     return;
>  588│
>  589│   if (clipping_container->EnclosingLayer() == scroll_parent)
>  590├>    return;
>  591│
>  592│   if (compositing_ancestor->GetLayoutObject().IsDescendantOf(
>  593│           clipping_container))
>  594│     return;

I have no specific insight in this situation.

I guess we early out here because we do not want to apply the clip from a
scrolling parent layer to the child, so there is no ancestor clipping layer and
hence we can have no border radius mask. Where is the clip applied for the
clipping_container->EnclosingLayer() == scroll_parent situation?

[sunxd, 2017-05-12 15:39:01]

On 2017/05/12 15:37:10, sunxd wrote:
> On 2017/05/11 20:32:13, sunxd wrote:
> > On 2017/05/11 20:28:46, chrishtr wrote:
> > > Stephen, adding you as an expert on the ancestor clipping mask layer. It
> seems
> > > the clipping
> > > is not working in this case, could you advise?
> > 
> > For more info: I attached a debugger and found that the green layer didn't
get
> > its ancestor clipping mask, it returned here:
> > 
> > CompositedLayerMapping.cpp:
> > 
> >  579│   const PaintLayer* compositing_ancestor =
> >  580│      
> > owning_layer_.EnclosingLayerWithCompositedLayerMapping(kExcludeSelf);
> >  581│   if (!compositing_ancestor)
> >  582│     return;
> >  583│
> >  584│   const LayoutBoxModelObject* clipping_container =
> >  585│       owning_layer_.ClippingContainer();
> >  586│   if (!clipping_container)
> >  587│     return;
> >  588│
> >  589│   if (clipping_container->EnclosingLayer() == scroll_parent)
> >  590├>    return;
> >  591│
> >  592│   if (compositing_ancestor->GetLayoutObject().IsDescendantOf(
> >  593│           clipping_container))
> >  594│     return;
> 
> @schenney: gentle ping, can you verify that on this webpage
>
(https://lh3.googleusercontent.com/--NU9yrAZ9c4/WRNlk4diTCI/AAAAAAAAAaU/1uS6XJ...),
> the clipping container's enclosing layer of layer (.spacer1.bottom) should be
> the scroll parent (the first div#outer 400x400)?

The HTML of the page can be found at https://jsfiddle.net/786m1og3/

[Stephen Chennney, 2017-05-12 15:44:34]

On 2017/05/12 15:38:29, Stephen Chennney wrote:
> On 2017/05/11 20:32:13, sunxd wrote:
> > On 2017/05/11 20:28:46, chrishtr wrote:
> > > Stephen, adding you as an expert on the ancestor clipping mask layer. It
> seems
> > > the clipping
> > > is not working in this case, could you advise?
> > 
> > For more info: I attached a debugger and found that the green layer didn't
get
> > its ancestor clipping mask, it returned here:
> > 
> > CompositedLayerMapping.cpp:
> > 
> >  579│   const PaintLayer* compositing_ancestor =
> >  580│      
> > owning_layer_.EnclosingLayerWithCompositedLayerMapping(kExcludeSelf);
> >  581│   if (!compositing_ancestor)
> >  582│     return;
> >  583│
> >  584│   const LayoutBoxModelObject* clipping_container =
> >  585│       owning_layer_.ClippingContainer();
> >  586│   if (!clipping_container)
> >  587│     return;
> >  588│
> >  589│   if (clipping_container->EnclosingLayer() == scroll_parent)
> >  590├>    return;
> >  591│
> >  592│   if (compositing_ancestor->GetLayoutObject().IsDescendantOf(
> >  593│           clipping_container))
> >  594│     return;
> 
> I have no specific insight in this situation.
> 
> I guess we early out here because we do not want to apply the clip from a
> scrolling parent layer to the child, so there is no ancestor clipping layer
and
> hence we can have no border radius mask. Where is the clip applied for the
> clipping_container->EnclosingLayer() == scroll_parent situation?

I presume we're looking at
compositing/overflow/nested-border-radius-clipping.html?

It's known that the border radius clipping mask does not work for nested border
radius, though that does not explain the early out shown by the debugger.

https://bugs.chromium.org/p/chromium/issues/detail?id=672561

[Stephen Chennney, 2017-05-12 15:53:25]

On 2017/05/12 15:44:34, Stephen Chennney wrote:
> On 2017/05/12 15:38:29, Stephen Chennney wrote:
> > On 2017/05/11 20:32:13, sunxd wrote:
> > > On 2017/05/11 20:28:46, chrishtr wrote:
> > > > Stephen, adding you as an expert on the ancestor clipping mask layer. It
> > seems
> > > > the clipping
> > > > is not working in this case, could you advise?
> > > 
> > > For more info: I attached a debugger and found that the green layer didn't
> get
> > > its ancestor clipping mask, it returned here:
> > > 
> > > CompositedLayerMapping.cpp:
> > > 
> > >  579│   const PaintLayer* compositing_ancestor =
> > >  580│      
> > > owning_layer_.EnclosingLayerWithCompositedLayerMapping(kExcludeSelf);
> > >  581│   if (!compositing_ancestor)
> > >  582│     return;
> > >  583│
> > >  584│   const LayoutBoxModelObject* clipping_container =
> > >  585│       owning_layer_.ClippingContainer();
> > >  586│   if (!clipping_container)
> > >  587│     return;
> > >  588│
> > >  589│   if (clipping_container->EnclosingLayer() == scroll_parent)
> > >  590├>    return;
> > >  591│
> > >  592│   if (compositing_ancestor->GetLayoutObject().IsDescendantOf(
> > >  593│           clipping_container))
> > >  594│     return;
> > 
> > I have no specific insight in this situation.
> > 
> > I guess we early out here because we do not want to apply the clip from a
> > scrolling parent layer to the child, so there is no ancestor clipping layer
> and
> > hence we can have no border radius mask. Where is the clip applied for the
> > clipping_container->EnclosingLayer() == scroll_parent situation?
> 
> I presume we're looking at
> compositing/overflow/nested-border-radius-clipping.html?
> 
> It's known that the border radius clipping mask does not work for nested
border
> radius, though that does not explain the early out shown by the debugger.
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=672561

And yes, as I understand it the scroll parent layer for the .spacer1.bottom
layer is the outer.border-radius element. It's also the clipping container with
the border radius, so not surprising that we early out in the test for needing
clipping layers. I stand by my guess that the clip to keep the scrolling layer
within the scroll area is applied elsewhere, and hence a clipping layer is not
needed. That "elsewhere" would need to apply the border radius clip. I suspect
that's a CC clip and we can't do it.

[sunxd, 2017-05-12 15:53:44]

On 2017/05/12 15:44:34, Stephen Chennney wrote:
> On 2017/05/12 15:38:29, Stephen Chennney wrote:
> > On 2017/05/11 20:32:13, sunxd wrote:
> > > On 2017/05/11 20:28:46, chrishtr wrote:
> > > > Stephen, adding you as an expert on the ancestor clipping mask layer. It
> > seems
> > > > the clipping
> > > > is not working in this case, could you advise?
> > > 
> > > For more info: I attached a debugger and found that the green layer didn't
> get
> > > its ancestor clipping mask, it returned here:
> > > 
> > > CompositedLayerMapping.cpp:
> > > 
> > >  579│   const PaintLayer* compositing_ancestor =
> > >  580│      
> > > owning_layer_.EnclosingLayerWithCompositedLayerMapping(kExcludeSelf);
> > >  581│   if (!compositing_ancestor)
> > >  582│     return;
> > >  583│
> > >  584│   const LayoutBoxModelObject* clipping_container =
> > >  585│       owning_layer_.ClippingContainer();
> > >  586│   if (!clipping_container)
> > >  587│     return;
> > >  588│
> > >  589│   if (clipping_container->EnclosingLayer() == scroll_parent)
> > >  590├>    return;
> > >  591│
> > >  592│   if (compositing_ancestor->GetLayoutObject().IsDescendantOf(
> > >  593│           clipping_container))
> > >  594│     return;
> > 
> > I have no specific insight in this situation.
> > 
> > I guess we early out here because we do not want to apply the clip from a
> > scrolling parent layer to the child, so there is no ancestor clipping layer
> and
> > hence we can have no border radius mask. Where is the clip applied for the
> > clipping_container->EnclosingLayer() == scroll_parent situation?
> 
> I presume we're looking at
> compositing/overflow/nested-border-radius-clipping.html?
> 
> It's known that the border radius clipping mask does not work for nested
border
> radius, though that does not explain the early out shown by the debugger.
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=672561

Yes. The jsfiddle page is a simplified version of that layout test. I think it's
a separate issue of nested border radius clipping?
To describe it, I would say: layer with a border-radius scroll parent does not
get clipped by border radius.

In the pic I show above:

#document
-div#outer (400x400) <- scroll parent, the div with border-radius property
--div#outer (385x385) <- the layer with render surface, and it has a mask layer
---div#outer (384x403) <- I believe this is the scroll content
--div#outer (400x400) <- These are scrollbar related layers
---div#outer (385x15)
---div#outer (15x385)
---div#outer (15x15)
--.spacer1.bottom (103x103)

If I make an exception that we do not early out when we meet .spacer1.bottom,
the layer gets correctly clipped.

[Stephen Chennney, 2017-05-12 15:58:07]

On 2017/05/12 15:53:44, sunxd wrote:
> On 2017/05/12 15:44:34, Stephen Chennney wrote:
> > On 2017/05/12 15:38:29, Stephen Chennney wrote:
> > > On 2017/05/11 20:32:13, sunxd wrote:
> > > > On 2017/05/11 20:28:46, chrishtr wrote:
> > > > > Stephen, adding you as an expert on the ancestor clipping mask layer.
It
> > > seems
> > > > > the clipping
> > > > > is not working in this case, could you advise?
> > > > 
> > > > For more info: I attached a debugger and found that the green layer
didn't
> > get
> > > > its ancestor clipping mask, it returned here:
> > > > 
> > > > CompositedLayerMapping.cpp:
> > > > 
> > > >  579│   const PaintLayer* compositing_ancestor =
> > > >  580│      
> > > > owning_layer_.EnclosingLayerWithCompositedLayerMapping(kExcludeSelf);
> > > >  581│   if (!compositing_ancestor)
> > > >  582│     return;
> > > >  583│
> > > >  584│   const LayoutBoxModelObject* clipping_container =
> > > >  585│       owning_layer_.ClippingContainer();
> > > >  586│   if (!clipping_container)
> > > >  587│     return;
> > > >  588│
> > > >  589│   if (clipping_container->EnclosingLayer() == scroll_parent)
> > > >  590├>    return;
> > > >  591│
> > > >  592│   if (compositing_ancestor->GetLayoutObject().IsDescendantOf(
> > > >  593│           clipping_container))
> > > >  594│     return;
> > > 
> > > I have no specific insight in this situation.
> > > 
> > > I guess we early out here because we do not want to apply the clip from a
> > > scrolling parent layer to the child, so there is no ancestor clipping
layer
> > and
> > > hence we can have no border radius mask. Where is the clip applied for the
> > > clipping_container->EnclosingLayer() == scroll_parent situation?
> > 
> > I presume we're looking at
> > compositing/overflow/nested-border-radius-clipping.html?
> > 
> > It's known that the border radius clipping mask does not work for nested
> border
> > radius, though that does not explain the early out shown by the debugger.
> > 
> > https://bugs.chromium.org/p/chromium/issues/detail?id=672561
> 
> Yes. The jsfiddle page is a simplified version of that layout test. I think
it's
> a separate issue of nested border radius clipping?
> To describe it, I would say: layer with a border-radius scroll parent does not
> get clipped by border radius.
> 
> In the pic I show above:
> 
> #document
> -div#outer (400x400) <- scroll parent, the div with border-radius property
> --div#outer (385x385) <- the layer with render surface, and it has a mask
layer
> ---div#outer (384x403) <- I believe this is the scroll content
> --div#outer (400x400) <- These are scrollbar related layers
> ---div#outer (385x15)
> ---div#outer (15x385)
> ---div#outer (15x15)
> --.spacer1.bottom (103x103)
> 
> If I make an exception that we do not early out when we meet .spacer1.bottom,
> the layer gets correctly clipped.

Does it paint properly if the scrolling area is much larger? i.e. make it 1000
pixels long and move the spacer down to still be in the corner.

We still need to understand exactly why the code had that early out. Efficiency?
Correctness?

[enne (OOO), 2017-05-12 17:51:33]

https://codereview.chromium.org/2859483006/diff/20001/cc/layers/solid_color_l...
File cc/layers/solid_color_layer_impl.cc (right):

https://codereview.chromium.org/2859483006/diff/20001/cc/layers/solid_color_l...
cc/layers/solid_color_layer_impl.cc:42: mask_type !=
Layer::LayerMaskType::MULTI_TEXTURE_MASK)
Why must these noop solid color quads not be skipped for multi texture masks?

[sunxd, 2017-05-15 17:57:37]

PTAL.

I chat with schenney@ offline. Layers inside border radius divs are not clipped
by mask because we early out when ancestor clipping layer is the scroll parent
of the layer. This line of code was added due to crbug.com/450756. I made an
exception for mask layer and did not see lags on webpages mentioned in that bug
thread.

However, the layout test still fails because of crbug.com/672561. We want to
figure out what to do with that bug.

https://codereview.chromium.org/2859483006/diff/20001/cc/layers/solid_color_l...
File cc/layers/solid_color_layer_impl.cc (right):

https://codereview.chromium.org/2859483006/diff/20001/cc/layers/solid_color_l...
cc/layers/solid_color_layer_impl.cc:42: mask_type !=
Layer::LayerMaskType::MULTI_TEXTURE_MASK)
On 2017/05/12 17:51:33, enne wrote:
> Why must these noop solid color quads not be skipped for multi texture masks?

It was because mask layer's opacity is not calculated. I reverted the change
here and add the computation code.

[sunxd, 2017-05-15 17:58:29]

On 2017/05/15 17:57:37, sunxd wrote:
> PTAL.
> 
> I chat with schenney@ offline. Layers inside border radius divs are not
clipped
> by mask because we early out when ancestor clipping layer is the scroll parent
> of the layer. This line of code was added due to crbug.com/450756. I made an
> exception for mask layer and did not see lags on webpages mentioned in that
bug
> thread.
> 
> However, the layout test still fails because of crbug.com/672561. We want to
> figure out what to do with that bug.
> 
>
https://codereview.chromium.org/2859483006/diff/20001/cc/layers/solid_color_l...
> File cc/layers/solid_color_layer_impl.cc (right):
> 
>
https://codereview.chromium.org/2859483006/diff/20001/cc/layers/solid_color_l...
> cc/layers/solid_color_layer_impl.cc:42: mask_type !=
> Layer::LayerMaskType::MULTI_TEXTURE_MASK)
> On 2017/05/12 17:51:33, enne wrote:
> > Why must these noop solid color quads not be skipped for multi texture
masks?
> 
> It was because mask layer's opacity is not calculated. I reverted the change
> here and add the computation code.

Oops I forgot to remove the interface change of SolidColorLayerImpl. I'll do it
now.

[enne (OOO), 2017-05-15 18:01:44]

https://codereview.chromium.org/2859483006/diff/40001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2859483006/diff/40001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:1019: mask_layer->draw_properties().opacity =
Sorry for my confusion, but I'm not sure why you need to do this.  Can you walk
me through why this is an equivalent for not skipping quads before?

[sunxd, 2017-05-15 18:49:28]

On 2017/05/15 18:01:44, enne wrote:
>
https://codereview.chromium.org/2859483006/diff/40001/cc/trees/draw_property_...
> File cc/trees/draw_property_utils.cc (right):
> 
>
https://codereview.chromium.org/2859483006/diff/40001/cc/trees/draw_property_...
> cc/trees/draw_property_utils.cc:1019: mask_layer->draw_properties().opacity =
> Sorry for my confusion, but I'm not sure why you need to do this.  Can you
walk
> me through why this is an equivalent for not skipping quads before?

No problem. So in RenderSurfaceImpl we use PictureLayerImpl::AppendQuads logic
to tile a mask layer. Then we convert the tile quads into RPDQs.
In PictureLayerImpl::AppendQuads, solid quads are only added to temp render pass
if its alpha value is not 0.

We used to skip opacity calculation for mask layers, so any solid color quads
coming from a mask layer will be skipped by that logic. I added the
opacity calculation code in draw_property_utils for mask layers so that we have
the correct opacity when deciding whether to skip solid quads.

[enne (OOO), 2017-05-15 19:01:44]

On 2017/05/15 at 18:49:28, sunxd wrote:
> On 2017/05/15 18:01:44, enne wrote:
> >
https://codereview.chromium.org/2859483006/diff/40001/cc/trees/draw_property_...
> > File cc/trees/draw_property_utils.cc (right):
> > 
> >
https://codereview.chromium.org/2859483006/diff/40001/cc/trees/draw_property_...
> > cc/trees/draw_property_utils.cc:1019: mask_layer->draw_properties().opacity
=
> > Sorry for my confusion, but I'm not sure why you need to do this.  Can you
walk
> > me through why this is an equivalent for not skipping quads before?
> 
> No problem. So in RenderSurfaceImpl we use PictureLayerImpl::AppendQuads logic
to tile a mask layer. Then we convert the tile quads into RPDQs.
> In PictureLayerImpl::AppendQuads, solid quads are only added to temp render
pass if its alpha value is not 0.
> 
> We used to skip opacity calculation for mask layers, so any solid color quads
coming from a mask layer will be skipped by that logic. I added the
> opacity calculation code in draw_property_utils for mask layers so that we
have the correct opacity when deciding whether to skip solid quads.

But this is the opacity on the layer.  So, if the layer with a mask had an
opacity of zero, we would skip the layer and the render pass.  If opacity
calculate was skipped, I would have expected quads to *not* be skipped (or to
default to opacity 1).

I think skipping solid color quads should only happen if they're zero (which
means that the mask  has completed masked out the contents).  I think also we
should not include a mask at all when the mask is a solid color with alpha=1,
although that's for a follow up pass.  However, both of these cases seem related
to the content in the solid color quad and not related to the opacity on the
layer.

[sunxd, 2017-05-15 21:05:21]

On 2017/05/15 19:01:44, enne wrote:
> On 2017/05/15 at 18:49:28, sunxd wrote:
> > On 2017/05/15 18:01:44, enne wrote:
> > >
>
https://codereview.chromium.org/2859483006/diff/40001/cc/trees/draw_property_...
> > > File cc/trees/draw_property_utils.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/2859483006/diff/40001/cc/trees/draw_property_...
> > > cc/trees/draw_property_utils.cc:1019:
mask_layer->draw_properties().opacity
> =
> > > Sorry for my confusion, but I'm not sure why you need to do this.  Can you
> walk
> > > me through why this is an equivalent for not skipping quads before?
> > 
> > No problem. So in RenderSurfaceImpl we use PictureLayerImpl::AppendQuads
logic
> to tile a mask layer. Then we convert the tile quads into RPDQs.
> > In PictureLayerImpl::AppendQuads, solid quads are only added to temp render
> pass if its alpha value is not 0.
> > 
> > We used to skip opacity calculation for mask layers, so any solid color
quads
> coming from a mask layer will be skipped by that logic. I added the
> > opacity calculation code in draw_property_utils for mask layers so that we
> have the correct opacity when deciding whether to skip solid quads.
> 
> But this is the opacity on the layer.  So, if the layer with a mask had an
> opacity of zero, we would skip the layer and the render pass.  If opacity
> calculate was skipped, I would have expected quads to *not* be skipped (or to
> default to opacity 1).
> 
> I think skipping solid color quads should only happen if they're zero (which
> means that the mask  has completed masked out the contents).  I think also we
> should not include a mask at all when the mask is a solid color with alpha=1,
> although that's for a follow up pass.  However, both of these cases seem
related
> to the content in the solid color quad and not related to the opacity on the
> layer.

The mask layer gets applied due to a 1px border-radius property. In that layout
test, the raster_source_ of the mask layer is solid color. So do you mean that
we keep the mask type change and do not skip mask layers?

[enne (OOO), 2017-05-15 21:11:58]

On 2017/05/15 at 21:05:21, sunxd wrote:
> On 2017/05/15 19:01:44, enne wrote:
> > On 2017/05/15 at 18:49:28, sunxd wrote:
> > > On 2017/05/15 18:01:44, enne wrote:
> > > >
> >
https://codereview.chromium.org/2859483006/diff/40001/cc/trees/draw_property_...
> > > > File cc/trees/draw_property_utils.cc (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/2859483006/diff/40001/cc/trees/draw_property_...
> > > > cc/trees/draw_property_utils.cc:1019:
mask_layer->draw_properties().opacity
> > =
> > > > Sorry for my confusion, but I'm not sure why you need to do this.  Can
you
> > walk
> > > > me through why this is an equivalent for not skipping quads before?
> > > 
> > > No problem. So in RenderSurfaceImpl we use PictureLayerImpl::AppendQuads
logic
> > to tile a mask layer. Then we convert the tile quads into RPDQs.
> > > In PictureLayerImpl::AppendQuads, solid quads are only added to temp
render
> > pass if its alpha value is not 0.
> > > 
> > > We used to skip opacity calculation for mask layers, so any solid color
quads
> > coming from a mask layer will be skipped by that logic. I added the
> > > opacity calculation code in draw_property_utils for mask layers so that we
> > have the correct opacity when deciding whether to skip solid quads.
> > 
> > But this is the opacity on the layer.  So, if the layer with a mask had an
> > opacity of zero, we would skip the layer and the render pass.  If opacity
> > calculate was skipped, I would have expected quads to *not* be skipped (or
to
> > default to opacity 1).
> > 
> > I think skipping solid color quads should only happen if they're zero (which
> > means that the mask  has completed masked out the contents).  I think also
we
> > should not include a mask at all when the mask is a solid color with
alpha=1,
> > although that's for a follow up pass.  However, both of these cases seem
related
> > to the content in the solid color quad and not related to the opacity on the
> > layer.
> 
> The mask layer gets applied due to a 1px border-radius property. In that
layout test, the raster_source_ of the mask layer is solid color. So do you mean
that we keep the mask type change and do not skip mask layers?

Sorry, I just don't understand what the opacity has to do with it.

The total opacity is used as an early out to determine if no quads should be
emitted.  That opacity is the layer opacity * the solid color opacity.  In other
words, early out if either layer or solid color opacity is zero.  I think if the
layer opacity is zero, then the render surface would already be skipped. 
Therefore, it seems like unneeded complication to set the opacity of the mask
layer.

Or, said differently, I would expect quads to be skipped or not skipped based on
their solid color only, and not based on the opacity of the layer that they're
masking.

[sunxd, 2017-05-16 14:31:04]

On 2017/05/15 21:11:58, enne wrote:
> On 2017/05/15 at 21:05:21, sunxd wrote:
> > On 2017/05/15 19:01:44, enne wrote:
> > > On 2017/05/15 at 18:49:28, sunxd wrote:
> > > > On 2017/05/15 18:01:44, enne wrote:
> > > > >
> > >
>
https://codereview.chromium.org/2859483006/diff/40001/cc/trees/draw_property_...
> > > > > File cc/trees/draw_property_utils.cc (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/2859483006/diff/40001/cc/trees/draw_property_...
> > > > > cc/trees/draw_property_utils.cc:1019:
> mask_layer->draw_properties().opacity
> > > =
> > > > > Sorry for my confusion, but I'm not sure why you need to do this.  Can
> you
> > > walk
> > > > > me through why this is an equivalent for not skipping quads before?
> > > > 
> > > > No problem. So in RenderSurfaceImpl we use PictureLayerImpl::AppendQuads
> logic
> > > to tile a mask layer. Then we convert the tile quads into RPDQs.
> > > > In PictureLayerImpl::AppendQuads, solid quads are only added to temp
> render
> > > pass if its alpha value is not 0.
> > > > 
> > > > We used to skip opacity calculation for mask layers, so any solid color
> quads
> > > coming from a mask layer will be skipped by that logic. I added the
> > > > opacity calculation code in draw_property_utils for mask layers so that
we
> > > have the correct opacity when deciding whether to skip solid quads.
> > > 
> > > But this is the opacity on the layer.  So, if the layer with a mask had an
> > > opacity of zero, we would skip the layer and the render pass.  If opacity
> > > calculate was skipped, I would have expected quads to *not* be skipped (or
> to
> > > default to opacity 1).
> > > 
> > > I think skipping solid color quads should only happen if they're zero
(which
> > > means that the mask  has completed masked out the contents).  I think also
> we
> > > should not include a mask at all when the mask is a solid color with
> alpha=1,
> > > although that's for a follow up pass.  However, both of these cases seem
> related
> > > to the content in the solid color quad and not related to the opacity on
the
> > > layer.
> > 
> > The mask layer gets applied due to a 1px border-radius property. In that
> layout test, the raster_source_ of the mask layer is solid color. So do you
mean
> that we keep the mask type change and do not skip mask layers?
> 
> Sorry, I just don't understand what the opacity has to do with it.
> 
> The total opacity is used as an early out to determine if no quads should be
> emitted.  That opacity is the layer opacity * the solid color opacity.  In
other
> words, early out if either layer or solid color opacity is zero.  I think if
the
> layer opacity is zero, then the render surface would already be skipped. 
> Therefore, it seems like unneeded complication to set the opacity of the mask
> layer.
> 
> Or, said differently, I would expect quads to be skipped or not skipped based
on
> their solid color only, and not based on the opacity of the layer that they're
> masking.

If I understand correctly, do you mean that we want to replace this line:
38   float alpha =
39       (SkColorGetA(color) * (1.0f / 255.0f)) * shared_quad_state->opacity;
with:
38   float alpha = SkColorGetA(color) * (1.0f / 255.0f)?

[enne (OOO), 2017-05-16 17:10:17]

> If I understand correctly, do you mean that we want to replace this line:
> 38   float alpha =
> 39       (SkColorGetA(color) * (1.0f / 255.0f)) * shared_quad_state->opacity;
> with:
> 38   float alpha = SkColorGetA(color) * (1.0f / 255.0f)?

I was assuming that the mask layer's shared_quad_state->opacity was 1 here.  Is
that not true?

[enne (OOO), 2017-05-16 17:19:07]

Sorry for my ongoing confusion.  I feel like I must be missing something.  My
questions here are:
(1) Why were you trying to avoid skipping solid color quads in a previous patch?
(2) Why are you setting the mask layer opacity here (which makes it seem like it
would skip *more* quads and not fewer)?

What happens in your current patch if you remove the code in draw_property_utils
to set the mask layer opacity?

[sunxd, 2017-05-16 18:59:56]

On 2017/05/16 17:19:07, enne wrote:
> Sorry for my ongoing confusion.  I feel like I must be missing something.  My
> questions here are:
> (1) Why were you trying to avoid skipping solid color quads in a previous
patch?
> (2) Why are you setting the mask layer opacity here (which makes it seem like
it
> would skip *more* quads and not fewer)?
> 
> What happens in your current patch if you remove the code in
draw_property_utils
> to set the mask layer opacity?

Oh sorry I thought the problem was known.
In layout test
respect-clip-for-non-composited-scrollers-when-prefering-compositing-over-lcd-text.html,
it creates a 1px solid black border-radius div "scrolled".
As a result, we get a solid color mask layer (I don't understand why but it's a
rectangular mask layer).

This solid color mask layer's opacity is set to 0 and not updated when computing
draw properties. So, as a result the mask layer is skipped, no quads are
produced, and we get an empty "scrolled" div.

[enne (OOO), 2017-05-16 20:15:41]

On 2017/05/16 at 18:59:56, sunxd wrote:
> On 2017/05/16 17:19:07, enne wrote:
> > Sorry for my ongoing confusion.  I feel like I must be missing something. 
My
> > questions here are:
> > (1) Why were you trying to avoid skipping solid color quads in a previous
patch?
> > (2) Why are you setting the mask layer opacity here (which makes it seem
like it
> > would skip *more* quads and not fewer)?
> > 
> > What happens in your current patch if you remove the code in
draw_property_utils
> > to set the mask layer opacity?
> 
> Oh sorry I thought the problem was known.
> In layout test
respect-clip-for-non-composited-scrollers-when-prefering-compositing-over-lcd-text.html,
it creates a 1px solid black border-radius div "scrolled".
> As a result, we get a solid color mask layer (I don't understand why but it's
a rectangular mask layer).
> 
> This solid color mask layer's opacity is set to 0 and not updated when
computing draw properties. So, as a result the mask layer is skipped, no quads
are produced, and we get an empty "scrolled" div.

What sets the mask layer's opacity to 0?

[sunxd, 2017-05-16 20:39:57]

On 2017/05/16 20:15:41, enne wrote:
> On 2017/05/16 at 18:59:56, sunxd wrote:
> > On 2017/05/16 17:19:07, enne wrote:
> > > Sorry for my ongoing confusion.  I feel like I must be missing something. 
> My
> > > questions here are:
> > > (1) Why were you trying to avoid skipping solid color quads in a previous
> patch?
> > > (2) Why are you setting the mask layer opacity here (which makes it seem
> like it
> > > would skip *more* quads and not fewer)?
> > > 
> > > What happens in your current patch if you remove the code in
> draw_property_utils
> > > to set the mask layer opacity?
> > 
> > Oh sorry I thought the problem was known.
> > In layout test
>
respect-clip-for-non-composited-scrollers-when-prefering-compositing-over-lcd-text.html,
> it creates a 1px solid black border-radius div "scrolled".
> > As a result, we get a solid color mask layer (I don't understand why but
it's
> a rectangular mask layer).
> > 
> > This solid color mask layer's opacity is set to 0 and not updated when
> computing draw properties. So, as a result the mask layer is skipped, no quads
> are produced, and we get an empty "scrolled" div.
> 
> What sets the mask layer's opacity to 0?

I think it's set to 0 when initializing draw properties, and it's never changed
afterwards.

[enne (OOO), 2017-05-16 20:50:07]

> > What sets the mask layer's opacity to 0?
> 
> I think it's set to 0 when initializing draw properties, and it's never
changed afterwards.

Aha, thanks.  I get it now.

Could you set the mask layer's opacity to 1 instead of 0? I think that makes
more sense than the layer's opacity and is cleaner then informing
SolidColorLayerImpl about mask types.

Could you also add a cc unit test with a solid color mask and make sure that its
quads are not skipped? Maybe I'm just slow on the uptake, but I feel like this
is a bit subtle.

I think with that, this is good to go.  Sorry for all the confusion.

[sunxd, 2017-05-18 18:39:15]

sunxd@chromium.org changed reviewers:
+ ericrk@chromium.org

[sunxd, 2017-05-18 18:39:15]

PTAL.

I added a line in draw_property_utils that arbitrarily set mask layer opacity to
1, so that they can avoid being skipped in solid color layer's append quads.

As discussed offline, I'm going to remove the mask with larger layer pixel test.

[enne (OOO), 2017-05-18 18:43:37]

Thanks for the changes and the extra test. The cc side of things lgtm

[chrishtr, 2017-05-19 00:27:53]

Tien-Ren, could you review this in my place while I am out?

@sunxd: you could land this behind a RuntimeEnabledFlag in order to commit it
while nested-border-radius is being fixed.

[chrishtr, 2017-05-19 00:28:15]

https://codereview.chromium.org/2859483006/diff/60001/cc/trees/layer_tree_set...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/2859483006/diff/60001/cc/trees/layer_tree_set...
cc/trees/layer_tree_settings.h:94: bool enable_mask_tiling = true;
You could also turn this on without border-radius scrolling right?

[sunxd, 2017-05-19 15:07:50]

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

[commit-bot: I haz the power, 2017-05-19 15:08:15]

Dry run: CQ is trying da patch.

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

[trchen, 2017-05-22 23:00:06]

https://codereview.chromium.org/2859483006/diff/100001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2859483006/diff/100001/cc/layers/layer.cc#new...
cc/layers/layer.cc:429: DCHECK_EQ(bounds().ToString(),
inputs_.mask_layer->bounds().ToString());
CLM updates layer geometry after attaching mask layer. I think this check should
be done during commit. Also I think this is a spin-off from another discussion.
Should we split this into a different CL?

https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(left):

https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:555:
const LayoutBoxModelObject& clipping_ancestor) {
This has to be changed as well. If there is anything that can change relative
position between child and clipping_ancestor, overlap must be assumed (thus clip
cannot be omitted). For example: http://jsbin.com/rexelunugo/

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

https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:626:
owning_layer_is_clipped = owning_layer_is_masked;
Could you document what does this line do? Actually I don't understand this
either before your CL or after your CL.

[sunxd, 2017-05-24 14:37:25]

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

[commit-bot: I haz the power, 2017-05-24 14:37:45]

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-05-24 15:39:21]

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

[commit-bot: I haz the power, 2017-05-24 15:39:22]

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

[chrishtr, 2017-06-12 23:28:12]

schenney's patch just landed. I think this CL is now unblocked!

[sunxd, 2017-06-13 15:59:04]

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

[commit-bot: I haz the power, 2017-06-13 15:59:20]

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-13 16:58:42]

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

[commit-bot: I haz the power, 2017-06-13 16:58:43]

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

[sunxd, 2017-06-13 22:15:15]

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

[commit-bot: I haz the power, 2017-06-13 22:15:54]

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-13 22:32:03]

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

[commit-bot: I haz the power, 2017-06-13 22:32:04]

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

[sunxd, 2017-06-14 15:12:02]

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

[commit-bot: I haz the power, 2017-06-14 15:12:26]

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-14 15:38:42]

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

[commit-bot: I haz the power, 2017-06-14 15:38:48]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  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...)

[sunxd, 2017-06-14 17:49:05]

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

[commit-bot: I haz the power, 2017-06-14 17:49:35]

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-14 19:05:12]

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

[commit-bot: I haz the power, 2017-06-14 19:05:14]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunxd, 2017-06-14 19:11:06]

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

[commit-bot: I haz the power, 2017-06-14 19:11:43]

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-14 21:34:38]

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

[commit-bot: I haz the power, 2017-06-14 21:34:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunxd, 2017-06-15 15:59:10]

PTAL.

I'm going to add a comment about scroll_parent skipping in ancestor clipping
layers.

Also I'm not sure if I can simply rebaseline the test failures on mac and win
try bots. Do you have any suggestion?

https://codereview.chromium.org/2859483006/diff/100001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2859483006/diff/100001/cc/layers/layer.cc#new...
cc/layers/layer.cc:429: DCHECK_EQ(bounds().ToString(),
inputs_.mask_layer->bounds().ToString());
On 2017/05/22 23:00:06, trchen wrote:
> CLM updates layer geometry after attaching mask layer. I think this check
should
> be done during commit. Also I think this is a spin-off from another
discussion.
> Should we split this into a different CL?

Done.

https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(left):

https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:555:
const LayoutBoxModelObject& clipping_ancestor) {
On 2017/05/22 23:00:06, trchen wrote:
> This has to be changed as well. If there is anything that can change relative
> position between child and clipping_ancestor, overlap must be assumed (thus
clip
> cannot be omitted). For example: http://jsbin.com/rexelunugo/

There was a bug about nested border-radius clipping, schenney@ recently landed
the fix. I think the position issue is fixed?

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

https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:626:
owning_layer_is_clipped = owning_layer_is_masked;
On 2017/05/22 23:00:06, trchen wrote:
> Could you document what does this line do? Actually I don't understand this
> either before your CL or after your CL.

Sure. It was added due to this bug: crbug.com/450756.
I think it's trying to avoid applying scroll parent as the ancestor clipping
layer to composited scrollers.

[Stephen Chennney, 2017-06-15 17:44:25]

I think the failures on th ebots are OK to rebaseline. It's a bit annoying that
we are chopping off a row of thumb pixels on the mac scrollbars, but as far as I
can tell we are already doing that even without this change (at least on my
mac).

https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(left):

https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:555:
const LayoutBoxModelObject& clipping_ancestor) {
On 2017/06/15 15:59:10, sunxd wrote:
> On 2017/05/22 23:00:06, trchen wrote:
> > This has to be changed as well. If there is anything that can change
relative
> > position between child and clipping_ancestor, overlap must be assumed (thus
> clip
> > cannot be omitted). For example: http://jsbin.com/rexelunugo/
> 
> There was a bug about nested border-radius clipping, schenney@ recently landed
> the fix. I think the position issue is fixed?

Yes, the nested border radius mask code will look at every layer up to the
composited ancestor now, not just the clipping ancestor.

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

https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:626:
owning_layer_is_clipped = owning_layer_is_masked;
On 2017/06/15 15:59:10, sunxd wrote:
> On 2017/05/22 23:00:06, trchen wrote:
> > Could you document what does this line do? Actually I don't understand this
> > either before your CL or after your CL.
> 
> Sure. It was added due to this bug: crbug.com/450756.
> I think it's trying to avoid applying scroll parent as the ancestor clipping
> layer to composited scrollers.

Will we not now have the same problem as crbug.com/450756 in cases where the
mask is required? Could you make a test based off
compositing/overflow/avoid-ancestor-clip-for-scroll-children.html that has a
border radius to invoke the mask?

https://codereview.chromium.org/2859483006/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2859483006/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2842: 
I would like to see these changes. Could you remove these NeedsRebaseline
expectations, generate the new results and then use webkit-patch rebaseline-cl
to get new baselines to add to the patch?

[sunxd, 2017-06-15 19:59:20]

I've uploaded the layout test results, PTAL.

https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(left):

https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:555:
const LayoutBoxModelObject& clipping_ancestor) {
On 2017/06/15 17:44:24, Stephen Chennney wrote:
> On 2017/06/15 15:59:10, sunxd wrote:
> > On 2017/05/22 23:00:06, trchen wrote:
> > > This has to be changed as well. If there is anything that can change
> relative
> > > position between child and clipping_ancestor, overlap must be assumed
(thus
> > clip
> > > cannot be omitted). For example: http://jsbin.com/rexelunugo/
> > 
> > There was a bug about nested border-radius clipping, schenney@ recently
landed
> > the fix. I think the position issue is fixed?
> 
> Yes, the nested border radius mask code will look at every layer up to the
> composited ancestor now, not just the clipping ancestor.

Done.

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

https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:626:
owning_layer_is_clipped = owning_layer_is_masked;
On 2017/06/15 17:44:24, Stephen Chennney wrote:
> On 2017/06/15 15:59:10, sunxd wrote:
> > On 2017/05/22 23:00:06, trchen wrote:
> > > Could you document what does this line do? Actually I don't understand
this
> > > either before your CL or after your CL.
> > 
> > Sure. It was added due to this bug: crbug.com/450756.
> > I think it's trying to avoid applying scroll parent as the ancestor clipping
> > layer to composited scrollers.
> 
> Will we not now have the same problem as crbug.com/450756 in cases where the
> mask is required? Could you make a test based off
> compositing/overflow/avoid-ancestor-clip-for-scroll-children.html that has a
> border radius to invoke the mask?

Oh sorry I think this change should be reverted as the new fix is going to take
care of the mask problem of scroll_parents.

https://codereview.chromium.org/2859483006/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2859483006/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2842: 
On 2017/06/15 17:44:25, Stephen Chennney wrote:
> I would like to see these changes. Could you remove these NeedsRebaseline
> expectations, generate the new results and then use webkit-patch rebaseline-cl
> to get new baselines to add to the patch?

Acknowledged.

[Stephen Chennney, 2017-06-16 14:21:13]

On 2017/06/15 19:59:20, sunxd wrote:
> I've uploaded the layout test results, PTAL.
> 
>
https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
> (left):
> 
>
https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:555:
> const LayoutBoxModelObject& clipping_ancestor) {
> On 2017/06/15 17:44:24, Stephen Chennney wrote:
> > On 2017/06/15 15:59:10, sunxd wrote:
> > > On 2017/05/22 23:00:06, trchen wrote:
> > > > This has to be changed as well. If there is anything that can change
> > relative
> > > > position between child and clipping_ancestor, overlap must be assumed
> (thus
> > > clip
> > > > cannot be omitted). For example: http://jsbin.com/rexelunugo/
> > > 
> > > There was a bug about nested border-radius clipping, schenney@ recently
> landed
> > > the fix. I think the position issue is fixed?
> > 
> > Yes, the nested border radius mask code will look at every layer up to the
> > composited ancestor now, not just the clipping ancestor.
> 
> Done.
> 
>
https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
> (right):
> 
>
https://codereview.chromium.org/2859483006/diff/100001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:626:
> owning_layer_is_clipped = owning_layer_is_masked;
> On 2017/06/15 17:44:24, Stephen Chennney wrote:
> > On 2017/06/15 15:59:10, sunxd wrote:
> > > On 2017/05/22 23:00:06, trchen wrote:
> > > > Could you document what does this line do? Actually I don't understand
> this
> > > > either before your CL or after your CL.
> > > 
> > > Sure. It was added due to this bug: crbug.com/450756.
> > > I think it's trying to avoid applying scroll parent as the ancestor
clipping
> > > layer to composited scrollers.
> > 
> > Will we not now have the same problem as crbug.com/450756 in cases where the
> > mask is required? Could you make a test based off
> > compositing/overflow/avoid-ancestor-clip-for-scroll-children.html that has a
> > border radius to invoke the mask?
> 
> Oh sorry I think this change should be reverted as the new fix is going to
take
> care of the mask problem of scroll_parents.
> 
>
https://codereview.chromium.org/2859483006/diff/200001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2859483006/diff/200001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/TestExpectations:2842: 
> On 2017/06/15 17:44:25, Stephen Chennney wrote:
> > I would like to see these changes. Could you remove these NeedsRebaseline
> > expectations, generate the new results and then use webkit-patch
rebaseline-cl
> > to get new baselines to add to the patch?
> 
> Acknowledged.

Great stuff. lgtm.

[sunxd, 2017-06-16 19:32:39]

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

[commit-bot: I haz the power, 2017-06-16 19:32:54]

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-16 20:06:44]

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

[commit-bot: I haz the power, 2017-06-16 20:06:45]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[chrishtr, 2017-06-16 22:34:34]

https://codereview.chromium.org/2859483006/diff/260001/cc/trees/draw_property...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2859483006/diff/260001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:1035: mask_layer->draw_properties().opacity = 1;
Why this change?

https://codereview.chromium.org/2859483006/diff/260001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2859483006/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2724: crbug.com/567296
compositing/masks/mask-with-removed-filters.html [ NeedsRebaseline ]
Please use webkit-patch rebaseline-cl to upload the new baselines instead.

https://codereview.chromium.org/2859483006/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/scroller-with-border-radius.html
(left):

https://codereview.chromium.org/2859483006/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/scroller-with-border-radius.html:52:
result += "Has border radius (should not be using composited scrolling): ";
Instead of deleting, modify the test to expect composited scrolling in this case
also.

https://codereview.chromium.org/2859483006/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (left):

https://codereview.chromium.org/2859483006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1255: Element*
container2 = document->getElementById("scroller2");
Please add a unittest here that it *does* composited scroll with border-radius.

[sunxd, 2017-06-19 14:58:59]

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

[commit-bot: I haz the power, 2017-06-19 14:59:23]

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-19 16:02:18]

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

[commit-bot: I haz the power, 2017-06-19 16:02:20]

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...)

[sunxd, 2017-06-20 14:36:10]

PTAL.

There is a tiny mistake when rebasing test results, I uploaded the new patch
with partial rebasing results.

https://codereview.chromium.org/2859483006/diff/260001/cc/trees/draw_property...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/2859483006/diff/260001/cc/trees/draw_property...
cc/trees/draw_property_utils.cc:1035: mask_layer->draw_properties().opacity = 1;
On 2017/06/16 22:34:33, chrishtr wrote:
> Why this change?

SolidColorLayer deliberately skips quads with opacity = 0. Mask layer used to
have a default 0 opacity, so if the layer is a solid color one, it will be
completed ignored.

https://codereview.chromium.org/2859483006/diff/260001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2859483006/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2724: crbug.com/567296
compositing/masks/mask-with-removed-filters.html [ NeedsRebaseline ]
On 2017/06/16 22:34:33, chrishtr wrote:
> Please use webkit-patch rebaseline-cl to upload the new baselines instead.

Acknowledged.

https://codereview.chromium.org/2859483006/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/compositing/overflow/scroller-with-border-radius.html
(left):

https://codereview.chromium.org/2859483006/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/compositing/overflow/scroller-with-border-radius.html:52:
result += "Has border radius (should not be using composited scrolling): ";
On 2017/06/16 22:34:33, chrishtr wrote:
> Instead of deleting, modify the test to expect composited scrolling in this
case
> also.

Done.

https://codereview.chromium.org/2859483006/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp (left):

https://codereview.chromium.org/2859483006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp:1255: Element*
container2 = document->getElementById("scroller2");
On 2017/06/16 22:34:34, chrishtr wrote:
> Please add a unittest here that it *does* composited scroll with
border-radius.

Done.

[sunxd, 2017-06-20 18:56:45]

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

[commit-bot: I haz the power, 2017-06-20 18:57:12]

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-20 19:56:15]

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

[commit-bot: I haz the power, 2017-06-20 19:56:16]

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-21 15:26:56]

https://codereview.chromium.org/2859483006/diff/320001/cc/trees/layer_tree_se...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/2859483006/diff/320001/cc/trees/layer_tree_se...
cc/trees/layer_tree_settings.h:96: bool enable_mask_tiling = true;
It looks like this CL actually does two things: composited scroll for
border-radius, and turning on mask tiling.

I think it would be better to turn on mask tiling in one CL, and then border
radius in another.

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

https://codereview.chromium.org/2859483006/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp:503: if (const auto*
ancestor_rounded_clip =
Put #ifdef DCHECK_IS_ON around these conditionals.