[blink] Refactor PaintLayerClipper::CalculateClipRects

[blink] Refactor PaintLayerClipper::CalculateClipRects

This CL refactors PaintLayerClipper::CalculateClipRects so the code
structure becomes more similar to PaintPropertyTreeBuilder.

One behavior change is introduced. When the kIgnoreOverflowClip is
specified, PLC should skip overflow clip of the root layer. The
intention is to be able to paint the whole scrolled contents for
composited scrolling. Before this CL, kIgnoreOverflowClip was not
honored if the element also has a CSS clip. In such case, both CSS clip
and overflow clip would still be applied. With this CL, both CSS clip
and overflow clip would be skipped if kIgnoreOverflowClip is specified.
This is believed to be a bug that forced us to disable composited
scrolling if CSS clip is present.

Other than that, no behavior change is introduced.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[trchen, 2017-04-13 19:34:13]

Description was changed from

==========
[blink] Refactor PaintLayerClipper::CalculateClipRects

This CL refactors PaintLayerClipper::CalculateClipRects so the code
structure becomes more similar to PaintPropertyTreeBuilder.

One behavior change is introduced. When the kIgnoreOverflowClip is
specified, PLC should skip overflow clip of the root layer. The
intention is to be able to paint the whole scrolled contents for
composited scrolling. Before this CL, kIgnoreOverflowClip was not
honored if the element also has a CSS clip. In such case, both CSS clip
and overflow clip would still be applied. With this CL, both CSS clip
and overflow clip would be skipped if kIgnoreOverflowClip is specified.
This is believed to be a bug that forced us to disable composited
scrolling if CSS clip is present.

Other than that, no behavior change is introduced.
==========

to

==========
[blink] Refactor PaintLayerClipper::CalculateClipRects

This CL refactors PaintLayerClipper::CalculateClipRects so the code
structure becomes more similar to PaintPropertyTreeBuilder.

One behavior change is introduced. When the kIgnoreOverflowClip is
specified, PLC should skip overflow clip of the root layer. The
intention is to be able to paint the whole scrolled contents for
composited scrolling. Before this CL, kIgnoreOverflowClip was not
honored if the element also has a CSS clip. In such case, both CSS clip
and overflow clip would still be applied. With this CL, both CSS clip
and overflow clip would be skipped if kIgnoreOverflowClip is specified.
This is believed to be a bug that forced us to disable composited
scrolling if CSS clip is present.

Other than that, no behavior change is introduced.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[trchen, 2017-04-13 19:41:58]

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

[trchen, 2017-04-13 19:41:59]

I tried to make sure the code works provably equivalent on each of the
refactoring step, except for the kIgnoreOverflowClip + CSS clip change.

As we worked around the kIgnoreOverflowClip + CSS clip issue by disabling
composited scrolling when CSS clip is present. The change can't be tested by a
layout test. I'll make a unit test instead. (A followup CL will be made to
remove the workaround to see what happens.)

I think this CL is beneficial because it should make it easier to verify
GeometryMapper version of PLC works identical to non-GM version, thus improves
our confidence in SPv2 backward compatibility

[trchen, 2017-04-13 19:46:38]

FYI the workaround I talked about:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...

[chrishtr, 2017-04-13 21:33:44]

The code here is going to be half-dead after M59, if SPInvalidation succeeds.
Is it really worth it to refactor at this very late point? Won't it just
introduce potential bugs in SPv1?

[trchen, 2017-04-13 22:22:01]

On 2017/04/13 21:33:44, chrishtr wrote:
> The code here is going to be half-dead after M59, if SPInvalidation succeeds.
> Is it really worth it to refactor at this very late point? Won't it just
> introduce potential bugs in SPv1?

If SPInvalidation sticks, why would it introduce bugs despite the code doesn't
even run? :>

I'm aware the non-GM version is in maintenance mode now. Yet I still believe it
is beneficial to keep code clean whenever possible. (Unless we are deleting the
code tomorrow.)
The motive was to reduce the discrepancy between GM version and non-GM version.
When I worked on the other PLC refactoring for the GM version, I noticed that
the interaction between CSS clip and kIgnoreOverflowClip looked questionable
(and did not match non-GM version). I think fixing both would reduce some noises
for development in this area.

[chrishtr, 2017-04-14 17:35:53]

On 2017/04/13 at 22:22:01, trchen wrote:
> On 2017/04/13 21:33:44, chrishtr wrote:
> > The code here is going to be half-dead after M59, if SPInvalidation
succeeds.
> > Is it really worth it to refactor at this very late point? Won't it just
> > introduce potential bugs in SPv1?
> 
> If SPInvalidation sticks, why would it introduce bugs despite the code doesn't
even run? :>

It will run - PaintLayerCompositor will keep calling it.

> When I worked on the other PLC refactoring for the GM version, I noticed that
the interaction between CSS clip and kIgnoreOverflowClip looked questionable
(and did not match non-GM version).

I thought we concluded they were the same after all? Do you still see a
difference?

> I think fixing both would reduce some noises for development in this area.

It is good to keep code clean. I'm just wondering whether there is a specific
goal other than just cleaning the code somewhat.
Along the same lines, this CL also makes a non-refactoring change - why is that
important? And if it is important, it should
be in a separate patch other than the refactoring.