Fix shouldPaint issue when a composited floating iframe becomes non-self-painting

Fix shouldPaint issue when a composited floating iframe becomes non-self-painting

Previously we forced selfPainting flags (but let selfPainting() function
return false) on all blocks into which a composited (for certain reason)
floating iframe overhangs or intrudes. When the iframe became non-self-
painting, all these blocks will paint the float. This caused multiple
issues, such as duplicated painting, display items with duplicated ids,
subsequence caching disorders, etc.

This CL contains two parts:
1. Revert crrev.com/413649 which caused the problems;
2. Set PaintLayer::selfPaintingStatusChanged() when self-painting status
   changes, and check it and clear it during compositing update.
   If the status changed for a floating layer, update ancestor
   shouldPaint flags to reflect the new status.

BUG=680423
TEST=compositing/iframes/floating-self-painting-frame.html
TEST=compositing/iframes/floating-self-painting-frame-expected.html
TEST=fast/block/float/float-change-composited-scrolling.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
R=chrishtr@chromium.org

Review-Url: https://codereview.chromium.org/2654843004 .
Cr-Commit-Position: refs/heads/master@{#446226}
Committed: https://chromium.googlesource.com/chromium/src/+/574d079a3f53ebfae2d34cc304fe8613cbcdf593

[Xianzhu, 2017-01-25 07:12:25]

Description was changed from

==========
Fix shouldPaint issue when a composited floating iframe becomes
non-self-painting

Previously we forced selfPainting flags (but let selfPainting() function
return false) on all blocks into which a composited (for certain reason)
floating iframe overhangs or intrudes. When the iframe became non-self-
painting, all these blocks will paint the float. This caused multiple
issues, such as duplicated painting, display items with duplicated ids,
subsequence caching disorders, etc.

This CL contains two parts:
1. Revert crrev.com/413649 which caused the problems;
2. Set PaintLayer::selfPaintingStatusChanged() when self-painting status
   changes after style change, and check it during compositing update.
   If the status chagned for a floating layer, update ancestor
   shouldPaint flags to reflect the new status.

BUG=680423
TEST=compositing/iframes/floating-self-painting-frame.html
TEST=compositing/iframes/floating-self-painting-frame-expected.html
TEST=fast/block/float/float-change-composited-scrolling.html
==========

to

==========
Fix shouldPaint issue when a composited floating iframe becomes
non-self-painting

Previously we forced selfPainting flags (but let selfPainting() function
return false) on all blocks into which a composited (for certain reason)
floating iframe overhangs or intrudes. When the iframe became non-self-
painting, all these blocks will paint the float. This caused multiple
issues, such as duplicated painting, display items with duplicated ids,
subsequence caching disorders, etc.

This CL contains two parts:
1. Revert crrev.com/413649 which caused the problems;
2. Set PaintLayer::selfPaintingStatusChanged() when self-painting status
   changes after style change, and check it during compositing update.
   If the status chagned for a floating layer, update ancestor
   shouldPaint flags to reflect the new status.

BUG=680423
TEST=compositing/iframes/floating-self-painting-frame.html
TEST=compositing/iframes/floating-self-painting-frame-expected.html
TEST=fast/block/float/float-change-composited-scrolling.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-01-25 07:12:27]

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

[commit-bot: I haz the power, 2017-01-25 07:12:42]

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-01-25 09:19:34]

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

[commit-bot: I haz the power, 2017-01-25 09:19:34]

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

[Xianzhu, 2017-01-25 17:37:16]

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

[commit-bot: I haz the power, 2017-01-25 17:37:34]

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

[Xianzhu, 2017-01-25 17:44:27]

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

[Xianzhu, 2017-01-25 17:44:28]

Most of the changes are reverts of crrev.com/413649.

The change in CompositingInputsUpdater now correctly handles the cases that a
floating layer changes self-painting status.

https://codereview.chromium.org/2654843004/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp
(right):

https://codereview.chromium.org/2654843004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:260:
}
This handles both the case that was handled by
PaintLayer::didUpdateNeedsCompositedScrolling() (tested by
fast/block/float/float-change-composited-scrolling.htm), and the case that was
handled by the change in FloatingObjects (tested by
compositing/iframes/floating-self-painting-frame.html and
compositing/iframes/floating-self-painting-frame-complex.html).

https://codereview.chromium.org/2654843004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left):

https://codereview.chromium.org/2654843004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:1527:
LayoutBlockFlow::setAncestorShouldPaintFloatingObject(*layoutBox());
This logic is moved into CompositingInputsUpdater.cpp.

Other changes in this file are reverts of crrev.com/413649.

[commit-bot: I haz the power, 2017-01-25 20:22:48]

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

[commit-bot: I haz the power, 2017-01-25 20:22:50]

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

[chrishtr, 2017-01-25 20:47:57]

https://codereview.chromium.org/2654843004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2654843004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:4002:
!floatingObject.layoutObject()->hasSelfPaintingLayer()) {
Why do you need this?

https://codereview.chromium.org/2654843004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BlockFlowPainter.cpp (right):

https://codereview.chromium.org/2654843004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BlockFlowPainter.cpp:57: if
(floatingLayoutObject->hasSelfPaintingLayer())
In my original patch, I removed this because it was impossible to trigger for
non-self-painting layers. Why would it trigger now?

[chrishtr, 2017-01-25 20:48:39]

Please check that the testcase in crbug.com/610906 still works here. I added a
layout
test, but good to triple-check.

[Xianzhu, 2017-01-25 21:03:25]

https://codereview.chromium.org/2654843004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/2654843004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:4002:
!floatingObject.layoutObject()->hasSelfPaintingLayer()) {
On 2017/01/25 20:47:57, chrishtr wrote:
> Why do you need this?

This is a part of the revert, which I didn't look very closely at.
This is like the revert in BlockFlowPainter.cpp.
Perhaps it should be a DCHECK.

Added a TODO here, like I did in BlockFlowPainter.cpp.

This CL needs to be merged into M56, so I would like to take the safe side.

https://codereview.chromium.org/2654843004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BlockFlowPainter.cpp (right):

https://codereview.chromium.org/2654843004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BlockFlowPainter.cpp:57: if
(floatingLayoutObject->hasSelfPaintingLayer())
On 2017/01/25 20:47:57, chrishtr wrote:
> In my original patch, I removed this because it was impossible to trigger for
> non-self-painting layers. Why would it trigger now?

I didn't look at this closely when applying the revert. This CL needs to be
merged into M56, so I would like to take the safe side.

[Xianzhu, 2017-01-25 21:04:44]

On 2017/01/25 20:48:39, chrishtr wrote:
> Please check that the testcase in crbug.com/610906 still works here. I added a
> layout
> test, but good to triple-check.

Yes. I listed the tests that might be affected by the CL in the CL description.

[chrishtr, 2017-01-25 21:08:55]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-01-25 21:08:56]

lgtm

[commit-bot: I haz the power, 2017-01-25 21:09:23]

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-01-26 00:55:23]

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

[commit-bot: I haz the power, 2017-01-26 00:55:24]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Xianzhu, 2017-01-26 01:07:45]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-01-26 01:09:00]

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-01-26 04:22:39]

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

[commit-bot: I haz the power, 2017-01-26 04:22:41]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Xianzhu, 2017-01-26 04:31:39]

Description was changed from

==========
Fix shouldPaint issue when a composited floating iframe becomes
non-self-painting

Previously we forced selfPainting flags (but let selfPainting() function
return false) on all blocks into which a composited (for certain reason)
floating iframe overhangs or intrudes. When the iframe became non-self-
painting, all these blocks will paint the float. This caused multiple
issues, such as duplicated painting, display items with duplicated ids,
subsequence caching disorders, etc.

This CL contains two parts:
1. Revert crrev.com/413649 which caused the problems;
2. Set PaintLayer::selfPaintingStatusChanged() when self-painting status
   changes after style change, and check it during compositing update.
   If the status chagned for a floating layer, update ancestor
   shouldPaint flags to reflect the new status.

BUG=680423
TEST=compositing/iframes/floating-self-painting-frame.html
TEST=compositing/iframes/floating-self-painting-frame-expected.html
TEST=fast/block/float/float-change-composited-scrolling.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix shouldPaint issue when a composited floating iframe becomes
non-self-painting

Previously we forced selfPainting flags (but let selfPainting() function
return false) on all blocks into which a composited (for certain reason)
floating iframe overhangs or intrudes. When the iframe became non-self-
painting, all these blocks will paint the float. This caused multiple
issues, such as duplicated painting, display items with duplicated ids,
subsequence caching disorders, etc.

This CL contains two parts:
1. Revert crrev.com/413649 which caused the problems;
2. Set PaintLayer::selfPaintingStatusChanged() when self-painting status
   changes, and check it and clear it during compositing update.
   If the status changed for a floating layer, update ancestor
   shouldPaint flags to reflect the new status.

BUG=680423
TEST=compositing/iframes/floating-self-painting-frame.html
TEST=compositing/iframes/floating-self-painting-frame-expected.html
TEST=fast/block/float/float-change-composited-scrolling.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-01-26 04:37:55]

Description was changed from

==========
Fix shouldPaint issue when a composited floating iframe becomes
non-self-painting

Previously we forced selfPainting flags (but let selfPainting() function
return false) on all blocks into which a composited (for certain reason)
floating iframe overhangs or intrudes. When the iframe became non-self-
painting, all these blocks will paint the float. This caused multiple
issues, such as duplicated painting, display items with duplicated ids,
subsequence caching disorders, etc.

This CL contains two parts:
1. Revert crrev.com/413649 which caused the problems;
2. Set PaintLayer::selfPaintingStatusChanged() when self-painting status
   changes, and check it and clear it during compositing update.
   If the status changed for a floating layer, update ancestor
   shouldPaint flags to reflect the new status.

BUG=680423
TEST=compositing/iframes/floating-self-painting-frame.html
TEST=compositing/iframes/floating-self-painting-frame-expected.html
TEST=fast/block/float/float-change-composited-scrolling.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix shouldPaint issue when a composited floating iframe becomes
non-self-painting

Previously we forced selfPainting flags (but let selfPainting() function
return false) on all blocks into which a composited (for certain reason)
floating iframe overhangs or intrudes. When the iframe became non-self-
painting, all these blocks will paint the float. This caused multiple
issues, such as duplicated painting, display items with duplicated ids,
subsequence caching disorders, etc.

This CL contains two parts:
1. Revert crrev.com/413649 which caused the problems;
2. Set PaintLayer::selfPaintingStatusChanged() when self-painting status
   changes, and check it and clear it during compositing update.
   If the status changed for a floating layer, update ancestor
   shouldPaint flags to reflect the new status.

BUG=680423
TEST=compositing/iframes/floating-self-painting-frame.html
TEST=compositing/iframes/floating-self-painting-frame-expected.html
TEST=fast/block/float/float-change-composited-scrolling.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
R=chrishtr@chromium.org

Review-Url: https://codereview.chromium.org/2654843004 .
Cr-Commit-Position: refs/heads/master@{#446226}
Committed:
https://chromium.googlesource.com/chromium/src/+/574d079a3f53ebfae2d34cc304fe...
==========

[Xianzhu, 2017-01-26 04:37:56]

Committed patchset #4 (id:60001) manually as
574d079a3f53ebfae2d34cc304fe8613cbcdf593 (presubmit successful).

[kjellander_chromium, 2017-01-26 07:52:21]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2651253003/ by kjellander@chromium.org.

The reason for reverting is: Fails on
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/49858
Snippet:
20:56:41.220 1868 worker/4
compositing/iframes/floating-self-painting-frame-complex.html output stderr
lines:
20:56:41.220 1868  
[5864:3688:0125/205640.620:601633:ERROR:graphicslayer.cpp(1294)] "LayoutView
#document" Uninvalidated old/new pixels mismatch at 411,8 old:ffc4c4c4
new:fec4c4c4
20:56:41.294 1868 "E:\b\c\b\win_layout\src\out\Release\image_diff.exe --diff
c:\users\chrome~2\appdata\local\temp\tmpous4g4\actual.png
c:\users\chrome~2\appdata\local\temp\tmpous4g4\expected.png
c:\users\chrome~2\appdata\local\temp\tmpous4g4\diff.png" took 0.07s
20:56:41.389 1868 "E:\b\c\b\win_layout\src\out\Release\image_diff.exe --diff
c:\users\chrome~2\appdata\local\temp\tmpaddjzm\actual.png
c:\users\chrome~2\appdata\local\temp\tmpaddjzm\expected.png
c:\users\chrome~2\appdata\local\temp\tmpaddjzm\diff.png" took 0.09s
20:56:41.393 3184 [6230/47861]
compositing/iframes/floating-self-painting-frame-complex.html failed
unexpectedly (reference mismatch)
20:56:41.392 1868 worker/4
compositing/iframes/floating-self-painting-frame-complex.html failed:
20:56:41.392 1868 worker/4  reference mismatch.