Implement rational overflow clipping in SPv2

Implement rational overflow clipping in SPv2

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

[chrishtr, 2017-05-03 18:33:35]

Description was changed from

==========
none


none


none


none


none


none

BUG=
==========

to

==========
none


none


none


none


none


none

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

[chrishtr, 2017-05-03 18:35:35]

Description was changed from

==========
none


none


none


none


none


none

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

to

==========
Implement rational overflow clipping in SPv2

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

[trchen, 2017-05-03 21:13:21]

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

[trchen, 2017-05-03 21:13:22]

This looks like a good first step! This will apply rounded clip to effects. Are
you going to extend this to chunks soon?

IMO we should think ahead how to group clips. For example:
<div style="overflow:hidden; border-radius:5px;">
  <div style="opacity:0.5">P1</div>
  <div style="opacity:0.5">P2</div>
</div>
Ideally we should be able to detect this and create just one rounded clip for
both child effects.

And in the presence of child chunks:
<div style="overflow:hidden; border-radius:5px;">
  <div style="opacity:0.5">P1</div>
  P2
  <div style="opacity:0.5">P3</div>
</div>
----
Also for heads up, things that are backdrop-dependent (blend mode and backdrop
filter) cannot be enclosed by extra grouping. Special care needs to be taken for
them. For example:

<div style="position:relative;">P1</div>
<div style="border-radius:5px; overflow:hidden;">
  <div style="mix-blend-mode:multiply;">P2</div>
</div>
<div style="position:relative;">P3</div>

    [   P2  ]
[P1][E_blend][P3]
[     E_root    ]

I guess we're going to enclose E_blend with an effect for applying rounded
corner:

    [   P2  ][P_corner]
    [E_blend][ E_mask ]
[P1][    E_isolate    ][P3]
[          E_root         ]

Where P_corner paints a rounded corner mask, E_mask applies it with kDstIn.
E_isolate is needed because the mask applies only to P2 but not P1, but now
E_blend has a problem because it won't be able to see its backdrop P1. For such
special case, I think E_isolate should steal E_blend's blend mode, and reset
E_blend's blend mode to kSrc.

https://codereview.chromium.org/2859673004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2859673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:588:
compositing_reasons |= kCompositingReasonIsolateCompositedDescendants;
(Unrelated) I think this no longer applies. I shall try remove this hack in a
separate CL.

https://codereview.chromium.org/2859673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:601:
output_clip = context.fixed_position.clip;
fixed_position.clip is not necessarily shallower than in-flow clip, due to CSS
clip. Why is this needed though? The default from line #560 is as shallow as you
could get.

It may be beneficial to push it further down, e.g. allocating a smaller buffer
with canvas->saveLayer, but I think PaintChunksToCcLayer is in a better position
for it (for composited effects, cc already does it when computing render surface
bounds).

[chrishtr, 2017-05-03 21:18:00]

On 2017/05/03 at 21:13:22, trchen wrote:
> This looks like a good first step! This will apply rounded clip to effects.
Are you going to extend this to chunks soon?

Agreed, yes. It needs to apply to all chunks with border radius under its effect
as well, still need to implement that.
I think it can be done in the property tree manager also.
> 
> IMO we should think ahead how to group clips. For example:

Does this matter for SPv1 compat? I think not but you have looked into it more
closely than I have.

> Also for heads up, things that are backdrop-dependent (blend mode and backdrop
filter) cannot be enclosed by extra grouping. Special care needs to be taken for
them. For example:
> 
> <div style="position:relative;">P1</div>
> <div style="border-radius:5px; overflow:hidden;">
>   <div style="mix-blend-mode:multiply;">P2</div>
> </div>
> <div style="position:relative;">P3</div>
> 
>     [   P2  ]
> [P1][E_blend][P3]
> [     E_root    ]
> 
> I guess we're going to enclose E_blend with an effect for applying rounded
corner:
> 
>     [   P2  ][P_corner]
>     [E_blend][ E_mask ]
> [P1][    E_isolate    ][P3]
> [          E_root         ]
> 
> Where P_corner paints a rounded corner mask, E_mask applies it with kDstIn.
E_isolate is needed because the mask applies only to P2 but not P1, but now
E_blend has a problem because it won't be able to see its backdrop P1. For such
special case, I think E_isolate should steal E_blend's blend mode, and reset
E_blend's blend mode to kSrc.
> 
>
https://codereview.chromium.org/2859673004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2859673004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:588:
compositing_reasons |= kCompositingReasonIsolateCompositedDescendants;
> (Unrelated) I think this no longer applies. I shall try remove this hack in a
separate CL.
> 
>
https://codereview.chromium.org/2859673004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:601:
output_clip = context.fixed_position.clip;
> fixed_position.clip is not necessarily shallower than in-flow clip, due to CSS
clip. Why is this needed though? The default from line #560 is as shallow as you
could get.
> 
> It may be beneficial to push it further down, e.g. allocating a smaller buffer
with canvas->saveLayer, but I think PaintChunksToCcLayer is in a better position
for it (for composited effects, cc already does it when computing render surface
bounds).

[chrishtr, 2017-05-03 21:20:59]

https://codereview.chromium.org/2859673004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2859673004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:601:
output_clip = context.fixed_position.clip;
On 2017/05/03 at 21:13:22, trchen wrote:
> fixed_position.clip is not necessarily shallower than in-flow clip, due to CSS
clip. Why is this needed though? The default from line #560 is as shallow as you
could get.

It's required to implement fx-containment, step 1 I think - no clips may escape
effects.

I don't get why fixed_position.clip might be deeper than in-flow clip. Can you
post an example?

[trchen, 2017-05-03 22:04:18]

On 2017/05/03 21:20:59, chrishtr wrote:
>
https://codereview.chromium.org/2859673004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2859673004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:601:
> output_clip = context.fixed_position.clip;
> On 2017/05/03 at 21:13:22, trchen wrote:
> > fixed_position.clip is not necessarily shallower than in-flow clip, due to
CSS
> clip. Why is this needed though? The default from line #560 is as shallow as
you
> could get.
> 
> It's required to implement fx-containment, step 1 I think - no clips may
escape
> effects.

Line #560 already guarantees that. It always points to a clip that is a common
ancestor of every children.
(In fact, almost always the root node.)

> I don't get why fixed_position.clip might be deeper than in-flow clip. Can you
> post an example?

<div style="position:relative; overflow:hidden;">
  <div style="position:absolute; clip:rect(...);">
    <div>P1</div>
    <div style="position:fixed;">P2</div>
  </div>
</div>

P1 wants both clip, P2 wants only the CSS clip. We ended up creating separate
clip nodes for in-flow+abs-pos children and fix-pos children.
(ObjectPaintProperties::CssClipFixedPosition())

[chrishtr, 2017-05-03 22:10:14]

On 2017/05/03 at 22:04:18, trchen wrote:
> On 2017/05/03 21:20:59, chrishtr wrote:
> >
https://codereview.chromium.org/2859673004/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):
> > 
> >
https://codereview.chromium.org/2859673004/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:601:
> > output_clip = context.fixed_position.clip;
> > On 2017/05/03 at 21:13:22, trchen wrote:
> > > fixed_position.clip is not necessarily shallower than in-flow clip, due to
CSS
> > clip. Why is this needed though? The default from line #560 is as shallow as
you
> > could get.
> > 
> > It's required to implement fx-containment, step 1 I think - no clips may
escape
> > effects.
> 
> Line #560 already guarantees that. It always points to a clip that is a common
ancestor of every children.
> (In fact, almost always the root node.)
> 
> > I don't get why fixed_position.clip might be deeper than in-flow clip. Can
you
> > post an example?
> 
> <div style="position:relative; overflow:hidden;">
>   <div style="position:absolute; clip:rect(...);">
>     <div>P1</div>
>     <div style="position:fixed;">P2</div>
>   </div>
> </div>
> 
> P1 wants both clip, P2 wants only the CSS clip. We ended up creating separate
clip nodes for in-flow+abs-pos children and fix-pos children.
(ObjectPaintProperties::CssClipFixedPosition())

Ah ok I see. The issue is that the fixed position clip node might not be an
ancestor of the other one.