Use floating_object's parent object to check for overhanging floats.

Use floating_object's parent object to check for overhanging floats.

This fixes the "Double-painting of floats" issue caught on this page
http://www.alz.org/what-is-dementia.asp?gclid=Cj0KEQjw0IvIBRDF0Yzq4qGE4IwBEiQATMQlMcdGdUB3bX7An9ifdWLx8RUy7o6FlVy5rx_NB7i_CwAaAiQh8P8HAQ

1) On that page a floating iframe overhangs over several parents and gets
copied by AddOverhangingFloats to the block with self-painting layer.
Once the float reaches the block with self-painting layer boundary
its ShouldPaint flag gets flipped.

2) Because the float is wrapped inside of anonymous block
during the composition step UpdateAncestorShouldPaintFloatingObject uses
a not-direct parent of the float. As a result IsOverhangingFloat returns
a wrong result and flips ShouldPaint flag for the FloatingObject associated
with the float's layout object which is already marked for
paint by another FloatingObject created in step 1.

BUG=717755
Review-Url: https://codereview.chromium.org/2875163002
Cr-Commit-Position: refs/heads/master@{#471337}
Committed: https://chromium.googlesource.com/chromium/src/+/bd5ed3446133bfe50bc9e0194c1fe8d271e8fcf0

[Gleb Lanbin, 2017-05-11 21:40:14]

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

[commit-bot: I haz the power, 2017-05-11 21:41:39]

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-11 23:13:17]

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

[commit-bot: I haz the power, 2017-05-11 23:13:18]

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

[Gleb Lanbin, 2017-05-11 23:48:08]

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

[commit-bot: I haz the power, 2017-05-11 23:49:08]

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-12 02:26:52]

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

[commit-bot: I haz the power, 2017-05-12 02:26:52]

Dry run: This issue passed the CQ dry run.

[Gleb Lanbin, 2017-05-12 03:50:20]

Patchset #1 (id:1) has been deleted

[Gleb Lanbin, 2017-05-12 04:10:43]

glebl@chromium.org changed reviewers:
+ eae@chromium.org

[Gleb Lanbin, 2017-05-12 04:10:43]

I didn't add any tests because 1) we already have multiple tests that cover
floating iframes(compositing/iframes/floating-self-painting-frame-complex.html,
compositing/iframes/floating-self-painting-frame.html etc.) 2) I think it's
pointless to test that newly introduced DCHECK because the approach where we
copy FloatingObjects between ancestors and share ShouldPaint flag looks very
error prone to me. Hopefully it will go away with LayoutNG.

[eae, 2017-05-12 16:41:58]

OK, LGTM

[eae, 2017-05-12 16:42:09]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2017-05-12 16:42:48]

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-12 16:49:43]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494607329486140,
"parent_rev": "7cb6da294bb27263270813c11cd8f0a62c40e112", "commit_rev":
"bd5ed3446133bfe50bc9e0194c1fe8d271e8fcf0"}

[commit-bot: I haz the power, 2017-05-12 16:49:52]

Description was changed from

==========
Use floating_object's parent object to check for overhanging floats.

This fixes the "Double-painting of floats" issue caught on this page
http://www.alz.org/what-is-dementia.asp?gclid=Cj0KEQjw0IvIBRDF0Yzq4qGE4IwBEiQ...

1) On that page a floating iframe overhangs over several parents and gets
copied by AddOverhangingFloats to the block with self-painting layer.
Once the float reaches the block with self-painting layer boundary
its ShouldPaint flag gets flipped.

2) Because the float is wrapped inside of anonymous block
during the composition step UpdateAncestorShouldPaintFloatingObject uses
a not-direct parent of the float. As a result IsOverhangingFloat returns
a wrong result and flips ShouldPaint flag for the FloatingObject associated
with the float's layout object which is already marked for
paint by another FloatingObject created in step 1.

BUG=717755
==========

to

==========
Use floating_object's parent object to check for overhanging floats.

This fixes the "Double-painting of floats" issue caught on this page
http://www.alz.org/what-is-dementia.asp?gclid=Cj0KEQjw0IvIBRDF0Yzq4qGE4IwBEiQ...

1) On that page a floating iframe overhangs over several parents and gets
copied by AddOverhangingFloats to the block with self-painting layer.
Once the float reaches the block with self-painting layer boundary
its ShouldPaint flag gets flipped.

2) Because the float is wrapped inside of anonymous block
during the composition step UpdateAncestorShouldPaintFloatingObject uses
a not-direct parent of the float. As a result IsOverhangingFloat returns
a wrong result and flips ShouldPaint flag for the FloatingObject associated
with the float's layout object which is already marked for
paint by another FloatingObject created in step 1.

BUG=717755

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

[commit-bot: I haz the power, 2017-05-12 16:49:53]

Committed patchset #1 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/bd5ed3446133bfe50bc9e0194c1f...

[chrishtr, 2017-05-12 17:01:14]

On 2017/05/12 at 04:10:43, glebl wrote:
> I didn't add any tests because 1) we already have multiple tests that cover
floating iframes(compositing/iframes/floating-self-painting-frame-complex.html,
compositing/iframes/floating-self-painting-frame.html etc.) 2) I think it's
pointless to test that newly introduced DCHECK because the approach where we
copy FloatingObjects between ancestors and share ShouldPaint flag looks very
error prone to me. Hopefully it will go away with LayoutNG.

Are you referring to the DCHECK in PaintController? Testing that DCHECK is not
so useful, but there is an underlying bug in floats that
was not exhibited by any layout test. It would be very useful to have a reduced
version of the page exhibiting this bug to go with this
patch.