[SPInvalidation] Ignore clip failure in GeometryMapper::localToVisualRectInAncestorSpace()

[SPInvalidation] Ignore clip failure in GeometryMapper::localToVisualRectInAncestorSpace()

On SPv1 GeometryMapper may fail to apply clip when the paint
invalidation container creates an overflow clip (in ancestorState)
which is not in localState of an out-of-flow positioned descendant.

See crbug.com/513108 and layout test
compositing/overflow/handle-non-ancestor-clip-parent.html (run with
--enable-prefer-compositing-to-lcd-text) and crbug.com/ for details.

Ignore clip failure for SPv1 for now.

BUG=646176
TEST=virtual/prefer_compositing_to_lcd_text/compositing/overflow/handle-non-ancestor-clip-parent.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/af159bbc73992952138753b2e2cc17865ba602b8
Cr-Commit-Position: refs/heads/master@{#423592}

[Xianzhu, 2016-10-04 23:45:27]

Description was changed from

==========
[SPInvalidation] Ignore clip failure in
GeometryMapper::localToVisualRectInAncestorSpace()

This may be caused by the fundamental compositing issue.
Ignore it for SPv1.

BUG=646176
TEST=virtual/prefer_compositing_to_lcd_text/compositing/overflow/handle-non-ancestor-clip-parent.html
==========

to

==========
[SPInvalidation] Ignore clip failure in
GeometryMapper::localToVisualRectInAncestorSpace()

This may be caused by the fundamental compositing issue.
Ignore it for SPv1.

BUG=646176
TEST=virtual/prefer_compositing_to_lcd_text/compositing/overflow/handle-non-ancestor-clip-parent.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-10-04 23:46:17]

Description was changed from

==========
[SPInvalidation] Ignore clip failure in
GeometryMapper::localToVisualRectInAncestorSpace()

This may be caused by the fundamental compositing issue.
Ignore it for SPv1.

BUG=646176
TEST=virtual/prefer_compositing_to_lcd_text/compositing/overflow/handle-non-ancestor-clip-parent.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPInvalidation] Ignore clip failure in
GeometryMapper::localToVisualRectInAncestorSpace()

This may be caused by the fundamental compositing issue (the paint
invalidation container has more clipping node than a descendant in
the test case).

Ignore it for SPv1.

BUG=646176
TEST=virtual/prefer_compositing_to_lcd_text/compositing/overflow/handle-non-ancestor-clip-parent.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-10-04 23:46:39]

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

[chrishtr, 2016-10-05 00:54:00]

Could you spell out the exact situation along with property trees? Would like to
understand better this scenario in case we see it again, and for SPv2.

[Xianzhu, 2016-10-05 18:09:19]

Description was changed from

==========
[SPInvalidation] Ignore clip failure in
GeometryMapper::localToVisualRectInAncestorSpace()

This may be caused by the fundamental compositing issue (the paint
invalidation container has more clipping node than a descendant in
the test case).

Ignore it for SPv1.

BUG=646176
TEST=virtual/prefer_compositing_to_lcd_text/compositing/overflow/handle-non-ancestor-clip-parent.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPInvalidation] Ignore clip failure in
GeometryMapper::localToVisualRectInAncestorSpace()

On SPv1 GeometryMapper may fail to apply clip when the paint
invalidation container creates an overflow clip (in ancestorState)
which is not in localState of an out-of-flow positioned descendant.

See crbug.com/513108 and layout test
compositing/overflow/handle-non-ancestor-clip-parent.html (run with
--enable-prefer-compositing-to-lcd-text) and crbug.com/ for details.
Ignore it for SPv1 for now.

BUG=646176
TEST=virtual/prefer_compositing_to_lcd_text/compositing/overflow/handle-non-ancestor-clip-parent.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-10-05 18:10:38]

On 2016/10/05 00:54:00, chrishtr wrote:
> Could you spell out the exact situation along with property trees? Would like
to
> understand better this scenario in case we see it again, and for SPv2.

Updated description and comments to contain more details.

[Xianzhu, 2016-10-05 18:11:33]

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

[commit-bot: I haz the power, 2016-10-05 18:12:00]

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

[chrishtr, 2016-10-05 19:21:32]

So the problem is that localToVisualRectInAncestorSpace is not mapping to an
actual ancestor
clip node? Should we just change whatever call site to
mapToVisualRectInDestinationSpace
instead?

[Xianzhu, 2016-10-05 19:32:47]

On 2016/10/05 19:21:32, chrishtr wrote:
> So the problem is that localToVisualRectInAncestorSpace is not mapping to an
> actual ancestor
> clip node? Should we just change whatever call site to
> mapToVisualRectInDestinationSpace
> instead?

This is actually called from mapToVisualRectInDestinationSpace() (which tries
localToVisualRectInAncestorSpace() first) in the case. The problem is that
localToAncestorMatrix succeeded, but localToAncestorClipRect() failed, and the
DCHECK(success) will crash. We could remove the DCHECK(success) here, but that
would cause the succeeded localToAncestorMatrix and mapRect to be done again in
the slow path which will produce the same result, so the current patch has
better performance than removing DCHECK(success).

I wonder if it's a valid case for SPv2 to map a rect from an out-of-flow
descendant to a non-containing-block scrolling ancestor. For now this happens
for SPv1 only.

[chrishtr, 2016-10-05 19:40:29]

On 2016/10/05 at 19:32:47, wangxianzhu wrote:
> On 2016/10/05 19:21:32, chrishtr wrote:
> > So the problem is that localToVisualRectInAncestorSpace is not mapping to an
> > actual ancestor
> > clip node? Should we just change whatever call site to
> > mapToVisualRectInDestinationSpace
> > instead?
> 
> This is actually called from mapToVisualRectInDestinationSpace() (which tries
localToVisualRectInAncestorSpace() first) in the case. The problem is that
localToAncestorMatrix succeeded, but localToAncestorClipRect() failed, and the
DCHECK(success) will crash. We could remove the DCHECK(success) here, but that
would cause the succeeded localToAncestorMatrix and mapRect to be done again in
the slow path which will produce the same result, so the current patch has
better performance than removing DCHECK(success).

Ah I see. In this case it would produce the same result, but not in general
right? It seems just removing the DCHECK is the cleanest solution.
 
> I wonder if it's a valid case for SPv2 to map a rect from an out-of-flow
descendant to a non-containing-block scrolling ancestor. For now this happens
for SPv1 only.

[Xianzhu, 2016-10-05 19:58:13]

On 2016/10/05 19:40:29, chrishtr wrote:
> On 2016/10/05 at 19:32:47, wangxianzhu wrote:
> > On 2016/10/05 19:21:32, chrishtr wrote:
> > > So the problem is that localToVisualRectInAncestorSpace is not mapping to
an
> > > actual ancestor
> > > clip node? Should we just change whatever call site to
> > > mapToVisualRectInDestinationSpace
> > > instead?
> > 
> > This is actually called from mapToVisualRectInDestinationSpace() (which
tries
> localToVisualRectInAncestorSpace() first) in the case. The problem is that
> localToAncestorMatrix succeeded, but localToAncestorClipRect() failed, and the
> DCHECK(success) will crash. We could remove the DCHECK(success) here, but that
> would cause the succeeded localToAncestorMatrix and mapRect to be done again
in
> the slow path which will produce the same result, so the current patch has
> better performance than removing DCHECK(success).
> 
> Ah I see. In this case it would produce the same result, but not in general
> right? It seems just removing the DCHECK is the cleanest solution.
>  
> > I wonder if it's a valid case for SPv2 to map a rect from an out-of-flow
> descendant to a non-containing-block scrolling ancestor. For now this happens
> for SPv1 only.

Tried just removing DCHECK(success), and the test still failed because the slow
path just repeats what the fast path failed to do and sets success to false. In
the case, we are unable to apply clip. If we treat this as a failure, it should
fail in both the fast path and the slow path, so the caller always get a
failure. Perhaps we can change the boolean to a enum to let the caller know
whether transform or clip failed, or just treat clip failure a success.

[commit-bot: I haz the power, 2016-10-05 21:27:33]

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

[commit-bot: I haz the power, 2016-10-05 21:27:33]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2016-10-05 22:55:56]

On 2016/10/05 at 19:58:13, wangxianzhu wrote:
> On 2016/10/05 19:40:29, chrishtr wrote:
> > On 2016/10/05 at 19:32:47, wangxianzhu wrote:
> > > On 2016/10/05 19:21:32, chrishtr wrote:
> > > > So the problem is that localToVisualRectInAncestorSpace is not mapping
to an
> > > > actual ancestor
> > > > clip node? Should we just change whatever call site to
> > > > mapToVisualRectInDestinationSpace
> > > > instead?
> > > 
> > > This is actually called from mapToVisualRectInDestinationSpace() (which
tries
> > localToVisualRectInAncestorSpace() first) in the case. The problem is that
> > localToAncestorMatrix succeeded, but localToAncestorClipRect() failed, and
the
> > DCHECK(success) will crash. We could remove the DCHECK(success) here, but
that
> > would cause the succeeded localToAncestorMatrix and mapRect to be done again
in
> > the slow path which will produce the same result, so the current patch has
> > better performance than removing DCHECK(success).
> > 
> > Ah I see. In this case it would produce the same result, but not in general
> > right? It seems just removing the DCHECK is the cleanest solution.
> >  
> > > I wonder if it's a valid case for SPv2 to map a rect from an out-of-flow
> > descendant to a non-containing-block scrolling ancestor. For now this
happens
> > for SPv1 only.
> 
> Tried just removing DCHECK(success), and the test still failed because the
slow path just repeats what the fast path failed to do and sets success to
false. In the case, we are unable to apply clip. If we treat this as a failure,
it should fail in both the fast path and the slow path, so the caller always get
a failure. Perhaps we can change the boolean to a enum to let the caller know
whether transform or clip failed, or just treat clip failure a success.

Why did the clip fail? It should clip to the LCA of the two clip states, then
transform down to the destination transform space. What went wrong?

[Xianzhu, 2016-10-05 23:15:14]

On 2016/10/05 22:55:56, chrishtr wrote:
> On 2016/10/05 at 19:58:13, wangxianzhu wrote:
> > On 2016/10/05 19:40:29, chrishtr wrote:
> > > On 2016/10/05 at 19:32:47, wangxianzhu wrote:
> > > > On 2016/10/05 19:21:32, chrishtr wrote:
> > > > > So the problem is that localToVisualRectInAncestorSpace is not mapping
> to an
> > > > > actual ancestor
> > > > > clip node? Should we just change whatever call site to
> > > > > mapToVisualRectInDestinationSpace
> > > > > instead?
> > > > 
> > > > This is actually called from mapToVisualRectInDestinationSpace() (which
> tries
> > > localToVisualRectInAncestorSpace() first) in the case. The problem is that
> > > localToAncestorMatrix succeeded, but localToAncestorClipRect() failed, and
> the
> > > DCHECK(success) will crash. We could remove the DCHECK(success) here, but
> that
> > > would cause the succeeded localToAncestorMatrix and mapRect to be done
again
> in
> > > the slow path which will produce the same result, so the current patch has
> > > better performance than removing DCHECK(success).
> > > 
> > > Ah I see. In this case it would produce the same result, but not in
general
> > > right? It seems just removing the DCHECK is the cleanest solution.
> > >  
> > > > I wonder if it's a valid case for SPv2 to map a rect from an out-of-flow
> > > descendant to a non-containing-block scrolling ancestor. For now this
> happens
> > > for SPv1 only.
> > 
> > Tried just removing DCHECK(success), and the test still failed because the
> slow path just repeats what the fast path failed to do and sets success to
> false. In the case, we are unable to apply clip. If we treat this as a
failure,
> it should fail in both the fast path and the slow path, so the caller always
get
> a failure. Perhaps we can change the boolean to a enum to let the caller know
> whether transform or clip failed, or just treat clip failure a success.
> 
> Why did the clip fail? It should clip to the LCA of the two clip states, then
> transform down to the destination transform space. What went wrong?

The LCA state contains LCA of transform and other nodes from destination state:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

It looks incorrect to use LCA(clip) as the destination clip because we may apply
more clips (Clip1 and LCA(clip) in the following example) than expected.
    
    LCA(Clip)
      /  \
   Clip2 Clip1
    /      \
  dest    source

It seems to make no sense to apply any clip if there are dest side clips (e.g.
the bug case), i.e the clip of the dest state is not an ancestor of the clip of
the source state. Perhaps we should just treat clip failures as successes?

For SPv2 we may ignore all overflow clips for visual rects, and will avoid such
problems.

[Xianzhu, 2016-10-06 15:54:48]

Description was changed from

==========
[SPInvalidation] Ignore clip failure in
GeometryMapper::localToVisualRectInAncestorSpace()

On SPv1 GeometryMapper may fail to apply clip when the paint
invalidation container creates an overflow clip (in ancestorState)
which is not in localState of an out-of-flow positioned descendant.

See crbug.com/513108 and layout test
compositing/overflow/handle-non-ancestor-clip-parent.html (run with
--enable-prefer-compositing-to-lcd-text) and crbug.com/ for details.
Ignore it for SPv1 for now.

BUG=646176
TEST=virtual/prefer_compositing_to_lcd_text/compositing/overflow/handle-non-ancestor-clip-parent.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPInvalidation] Ignore clip failure in
GeometryMapper::localToVisualRectInAncestorSpace()

On SPv1 GeometryMapper may fail to apply clip when the paint
invalidation container creates an overflow clip (in ancestorState)
which is not in localState of an out-of-flow positioned descendant.

See crbug.com/513108 and layout test
compositing/overflow/handle-non-ancestor-clip-parent.html (run with
--enable-prefer-compositing-to-lcd-text) and crbug.com/ for details.

Ignore clip failure for SPv1 for now.

BUG=646176
TEST=virtual/prefer_compositing_to_lcd_text/compositing/overflow/handle-non-ancestor-clip-parent.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[chrishtr, 2016-10-06 18:14:07]

lgtm

[chrishtr, 2016-10-06 18:14:10]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2016-10-06 18:14:41]

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

[commit-bot: I haz the power, 2016-10-06 18:21:48]

Description was changed from

==========
[SPInvalidation] Ignore clip failure in
GeometryMapper::localToVisualRectInAncestorSpace()

On SPv1 GeometryMapper may fail to apply clip when the paint
invalidation container creates an overflow clip (in ancestorState)
which is not in localState of an out-of-flow positioned descendant.

See crbug.com/513108 and layout test
compositing/overflow/handle-non-ancestor-clip-parent.html (run with
--enable-prefer-compositing-to-lcd-text) and crbug.com/ for details.

Ignore clip failure for SPv1 for now.

BUG=646176
TEST=virtual/prefer_compositing_to_lcd_text/compositing/overflow/handle-non-ancestor-clip-parent.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPInvalidation] Ignore clip failure in
GeometryMapper::localToVisualRectInAncestorSpace()

On SPv1 GeometryMapper may fail to apply clip when the paint
invalidation container creates an overflow clip (in ancestorState)
which is not in localState of an out-of-flow positioned descendant.

See crbug.com/513108 and layout test
compositing/overflow/handle-non-ancestor-clip-parent.html (run with
--enable-prefer-compositing-to-lcd-text) and crbug.com/ for details.

Ignore clip failure for SPv1 for now.

BUG=646176
TEST=virtual/prefer_compositing_to_lcd_text/compositing/overflow/handle-non-ancestor-clip-parent.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-10-06 18:21:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-06 18:23:36]

Description was changed from

==========
[SPInvalidation] Ignore clip failure in
GeometryMapper::localToVisualRectInAncestorSpace()

On SPv1 GeometryMapper may fail to apply clip when the paint
invalidation container creates an overflow clip (in ancestorState)
which is not in localState of an out-of-flow positioned descendant.

See crbug.com/513108 and layout test
compositing/overflow/handle-non-ancestor-clip-parent.html (run with
--enable-prefer-compositing-to-lcd-text) and crbug.com/ for details.

Ignore clip failure for SPv1 for now.

BUG=646176
TEST=virtual/prefer_compositing_to_lcd_text/compositing/overflow/handle-non-ancestor-clip-parent.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPInvalidation] Ignore clip failure in
GeometryMapper::localToVisualRectInAncestorSpace()

On SPv1 GeometryMapper may fail to apply clip when the paint
invalidation container creates an overflow clip (in ancestorState)
which is not in localState of an out-of-flow positioned descendant.

See crbug.com/513108 and layout test
compositing/overflow/handle-non-ancestor-clip-parent.html (run with
--enable-prefer-compositing-to-lcd-text) and crbug.com/ for details.

Ignore clip failure for SPv1 for now.

BUG=646176
TEST=virtual/prefer_compositing_to_lcd_text/compositing/overflow/handle-non-ancestor-clip-parent.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/af159bbc73992952138753b2e2cc17865ba602b8
Cr-Commit-Position: refs/heads/master@{#423592}
==========

[commit-bot: I haz the power, 2016-10-06 18:23:37]

Patchset 4 (id:??) landed as
https://crrev.com/af159bbc73992952138753b2e2cc17865ba602b8
Cr-Commit-Position: refs/heads/master@{#423592}