setMayNeedPaintInvalidation in setNeedsOverflowRecalcAfterStyleChange()

setMayNeedPaintInvalidation in setNeedsOverflowRecalcAfterStyleChange()

This is to ensure needsPaintOffsetAndVisualRectUpdate() when overflow
will change because of style change.

It was added in https://codereview.chromium.org/2751523011/ then
removed in https://codereview.chromium.org/2776583003/ because it
caused paint time regression when scrolling google search result page.

This CL adds it back, and added a condition to avoid unnecessary paint
invalidation when a container's location changes.

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

Review-Url: https://codereview.chromium.org/2791933003
Cr-Commit-Position: refs/heads/master@{#461829}
Committed: https://chromium.googlesource.com/chromium/src/+/347d17a607d7f379ba66bb4904869c1b530aac2b

[Xianzhu, 2017-04-03 21:59:49]

Description was changed from

==========
setMayNeedPaintInvalidation in setNeedsOverflowRecalcAfterStyleChange()

This is to ensure needsPaintOffsetAndVisualRectUpdate() when overflow
will change because of style change.

It was added in https://codereview.chromium.org/2751523011/ then
removed in https://codereview.chromium.org/2776583003/ because it
caused paint time regression when scrolling google search result page.

This CL adds it back, and added a condition to avoid unnecessary paint
invalidation when a container's location changes.

BUG=704182
==========

to

==========
setMayNeedPaintInvalidation in setNeedsOverflowRecalcAfterStyleChange()

This is to ensure needsPaintOffsetAndVisualRectUpdate() when overflow
will change because of style change.

It was added in https://codereview.chromium.org/2751523011/ then
removed in https://codereview.chromium.org/2776583003/ because it
caused paint time regression when scrolling google search result page.

This CL adds it back, and added a condition to avoid unnecessary paint
invalidation when a container's location changes.

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

[Xianzhu, 2017-04-03 22:00:16]

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

[Xianzhu, 2017-04-04 01:19:38]

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

[chrishtr, 2017-04-04 16:26:05]

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

https://codereview.chromium.org/2791933003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintInvalidationTest.cpp:87: // This is a
simlified test case for crbug.com/704182.
nit: simplified

https://codereview.chromium.org/2791933003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintInvalidationTest.cpp:91: "<div
style='height: 2000px'></div>"
This test doesn't seem to result in a user-visible change. Is that
possible? I assume so because of this CL, but what is the testcase 
which demonstrates it?

[Xianzhu, 2017-04-04 16:58:31]

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

https://codereview.chromium.org/2791933003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintInvalidationTest.cpp:87: // This is a
simlified test case for crbug.com/704182.
On 2017/04/04 16:26:05, chrishtr wrote:
> nit: simplified

Done.

https://codereview.chromium.org/2791933003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintInvalidationTest.cpp:91: "<div
style='height: 2000px'></div>"
On 2017/04/04 16:26:05, chrishtr wrote:
> This test doesn't seem to result in a user-visible change. Is that
> possible? I assume so because of this CL, but what is the testcase 
> which demonstrates it?

This test case ensures no paint time regression for the case of
crbug.com/704182. Line 114 checks the result of the new
setMayNeedPaintInvalidation() call, and line 117 checks we don't repaint because
there is no actual visual change.

The new setMayNeedPaintInvalidation() call is for
https://codereview.chromium.org/2792063002/ which actually uses the
needsPaintOffsetAndVisualRectUpdate flag. After that CL, several layout tests
will show the necessity of the setMayNeedPaintInvalidation call.

[chrishtr, 2017-04-04 17:03:45]

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

https://codereview.chromium.org/2791933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:491:
!context.paintingLayer->subtreeIsInvisible()) {
Could you add a test checking that making the tree visible again
will correctly update all these things?

Also, what is the mechanism that does that, especially after
https://codereview.chromium.org/2792063002/?

[Xianzhu, 2017-04-04 18:03:34]

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

https://codereview.chromium.org/2791933003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:491:
!context.paintingLayer->subtreeIsInvisible()) {
On 2017/04/04 17:03:45, chrishtr wrote:
> Could you add a test checking that making the tree visible again
> will correctly update all these things?
> 

Done.

> Also, what is the mechanism that does that, especially after
> https://codereview.chromium.org/2792063002/

The mechanism ensuring correct paint invalidation is existing:
- visibility change will cause full paint invalidation through style change
paint invalidation;
- subtreeIsInvisible flag is updated in
PaintLayer::updateDescendantDependentFlags().

[chrishtr, 2017-04-04 18:10:28]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-04-04 18:10:29]

lgtm

[commit-bot: I haz the power, 2017-04-04 18:11:02]

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-04-04 20:53:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491329428901350,
"parent_rev": "a70d7175ade7962c719eed61838d483c572fc137", "commit_rev":
"347d17a607d7f379ba66bb4904869c1b530aac2b"}

[commit-bot: I haz the power, 2017-04-04 20:54:49]

Description was changed from

==========
setMayNeedPaintInvalidation in setNeedsOverflowRecalcAfterStyleChange()

This is to ensure needsPaintOffsetAndVisualRectUpdate() when overflow
will change because of style change.

It was added in https://codereview.chromium.org/2751523011/ then
removed in https://codereview.chromium.org/2776583003/ because it
caused paint time regression when scrolling google search result page.

This CL adds it back, and added a condition to avoid unnecessary paint
invalidation when a container's location changes.

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

to

==========
setMayNeedPaintInvalidation in setNeedsOverflowRecalcAfterStyleChange()

This is to ensure needsPaintOffsetAndVisualRectUpdate() when overflow
will change because of style change.

It was added in https://codereview.chromium.org/2751523011/ then
removed in https://codereview.chromium.org/2776583003/ because it
caused paint time regression when scrolling google search result page.

This CL adds it back, and added a condition to avoid unnecessary paint
invalidation when a container's location changes.

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

Review-Url: https://codereview.chromium.org/2791933003
Cr-Commit-Position: refs/heads/master@{#461829}
Committed:
https://chromium.googlesource.com/chromium/src/+/347d17a607d7f379ba66bb490486...
==========

[commit-bot: I haz the power, 2017-04-04 20:54:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/347d17a607d7f379ba66bb490486...