Move clearPaintInvalidatinFlags after subtree walk

Move clearPaintInvalidatinFlags after subtree walk

This allows we change paint invalidation flags using
setShouldDoFullPaintInvalidation() etc. during paint invalidation to
deal with some complex situations, for example:
- https://codereview.chromium.org/2430313004/ which sets paint
  invalidation flag in LayoutTable::recalcCollapsedBordersIfNeeded().

Previously we clear flags before walking subtrees. If some descendant
set paint invalidation flags, we would also set flags along the ancestor
path causing dirty flags after paint invalidation.

This will be needed by several bug fixes and cleanups of paint
invalidation.

Also removed some redundant checks of slimmingPaintInvalidationEnabled
in PrePaintTreeWalker.

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

Committed: https://crrev.com/6cd03514645fe54b0c2010e51a8adfdce7646db4
Cr-Commit-Position: refs/heads/master@{#431402}

[Xianzhu, 2016-11-10 18:44:08]

Description was changed from

==========
Move clearPaintInvalidatinFlags after subtree walk

This allows we change paint invalidation flags using
setShouldDoFullPaintInvalidation() etc. during paint invalidation to
deal with some complex situations.

Previously we clear flags before walking subtrees. If some descendant
set paint invalidation flags, we would also set flags along the ancestor
path causing dirty flags after paint invalidation.

This will be needed by several bug fixes and cleanups of paint
invalidation.

BUG=660195
==========

to

==========
Move clearPaintInvalidatinFlags after subtree walk

This allows we change paint invalidation flags using
setShouldDoFullPaintInvalidation() etc. during paint invalidation to
deal with some complex situations.

Previously we clear flags before walking subtrees. If some descendant
set paint invalidation flags, we would also set flags along the ancestor
path causing dirty flags after paint invalidation.

This will be needed by several bug fixes and cleanups of paint
invalidation.

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

[Xianzhu, 2016-11-10 18:44:10]

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

[commit-bot: I haz the power, 2016-11-10 18:44:53]

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, 2016-11-10 21:29:56]

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

[commit-bot: I haz the power, 2016-11-10 21:29:57]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-11-10 21:33:43]

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

[chrishtr, 2016-11-10 23:04:36]

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

[wkorman, 2016-11-10 23:26:41]

From the CL description: what are a couple of the complex cases, can we state
one or two for examples in the description?

Is it worth adding a unit test that illustrates the case you mention:

"If some descendant set paint invalidation flags, we would also set flags along
the ancestor path causing dirty flags after paint invalidation."

that fails before this patch (due to leaving dirty flags after paint
invalidation) but passes after?

https://codereview.chromium.org/2488343002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (left):

https://codereview.chromium.org/2488343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:47: if
(RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled()) {
Is this change also enabling SP invalidation for all cases? If so we should
update CL description.

[Xianzhu, 2016-11-10 23:35:07]

Description was changed from

==========
Move clearPaintInvalidatinFlags after subtree walk

This allows we change paint invalidation flags using
setShouldDoFullPaintInvalidation() etc. during paint invalidation to
deal with some complex situations.

Previously we clear flags before walking subtrees. If some descendant
set paint invalidation flags, we would also set flags along the ancestor
path causing dirty flags after paint invalidation.

This will be needed by several bug fixes and cleanups of paint
invalidation.

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

to

==========
Move clearPaintInvalidatinFlags after subtree walk

This allows we change paint invalidation flags using
setShouldDoFullPaintInvalidation() etc. during paint invalidation to
deal with some complex situations, for example:
- https://codereview.chromium.org/2430313004/ which sets paint
  invalidation flag in LayoutTable::recalcCollapsedBordersIfNeeded().

Previously we clear flags before walking subtrees. If some descendant
set paint invalidation flags, we would also set flags along the ancestor
path causing dirty flags after paint invalidation.

This will be needed by several bug fixes and cleanups of paint
invalidation.

Also removed some redundant checks of slimmingPaintInvalidationEnabled
in PrePaintTreeWalker.

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

[Xianzhu, 2016-11-10 23:35:47]

https://codereview.chromium.org/2488343002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (left):

https://codereview.chromium.org/2488343002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:47: if
(RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled()) {
On 2016/11/10 23:26:41, wkorman wrote:
> Is this change also enabling SP invalidation for all cases? If so we should
> update CL description.

The check was just redundant. The whole code is executed only when
slimmingPaintInvalidationEnabled().

Updated CL description.

[wkorman, 2016-11-10 23:38:16]

On 2016/11/10 23:26:41, wkorman wrote:
> Is it worth adding a unit test that illustrates the case you mention:
> 
> "If some descendant set paint invalidation flags, we would also set flags
along
> the ancestor path causing dirty flags after paint invalidation."
> 
> that fails before this patch (due to leaving dirty flags after paint
> invalidation) but passes after?

What was your thought on the above? It seems like we'd want something like this
to catch/prevent this from happening in the future.

[Xianzhu, 2016-11-10 23:38:52]

On 2016/11/10 23:26:41, wkorman wrote:
> From the CL description: what are a couple of the complex cases, can we state
> one or two for examples in the description?
> 

Updated description.

> Is it worth adding a unit test that illustrates the case you mention:
> 
> "If some descendant set paint invalidation flags, we would also set flags
along
> the ancestor path causing dirty flags after paint invalidation."
> 
> that fails before this patch (due to leaving dirty flags after paint
> invalidation) but passes after?

It's not easy to add a unit test for this because the test needs to mock some
layout object to change paint invalidation flags during paint invalidation.

The changed code will be tested indirectly with
https://codereview.chromium.org/2430313004/.

[wkorman, 2016-11-10 23:51:06]

On 2016/11/10 23:38:52, Xianzhu wrote:
> It's not easy to add a unit test for this because the test needs to mock some
> layout object to change paint invalidation flags during paint invalidation.
> 
> The changed code will be tested indirectly with
> https://codereview.chromium.org/2430313004/.

OK, we can't do it with a RenderingTest and poking at existing layout object
internals in some manner? That's unfortunate.

FWIW I'd be supportive of adding facility for mocking layout objects, I think
I've wanted to do similar in the past. Ask chrishtr@ how he feels about mocks.
:) We can just keep it in mind for now.

[wkorman, 2016-11-10 23:51:22]

lgtm

[Xianzhu, 2016-11-10 23:57:21]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-11-10 23:58:09]

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

[Xianzhu, 2016-11-10 23:59:00]

Fwiw the logic is ensured by
LayoutObject::assertSubtreeClearedPaintInvalidationFlags() which is called after
a FrameView finishes paint invalidation.

[commit-bot: I haz the power, 2016-11-11 00:23:33]

Description was changed from

==========
Move clearPaintInvalidatinFlags after subtree walk

This allows we change paint invalidation flags using
setShouldDoFullPaintInvalidation() etc. during paint invalidation to
deal with some complex situations, for example:
- https://codereview.chromium.org/2430313004/ which sets paint
  invalidation flag in LayoutTable::recalcCollapsedBordersIfNeeded().

Previously we clear flags before walking subtrees. If some descendant
set paint invalidation flags, we would also set flags along the ancestor
path causing dirty flags after paint invalidation.

This will be needed by several bug fixes and cleanups of paint
invalidation.

Also removed some redundant checks of slimmingPaintInvalidationEnabled
in PrePaintTreeWalker.

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

to

==========
Move clearPaintInvalidatinFlags after subtree walk

This allows we change paint invalidation flags using
setShouldDoFullPaintInvalidation() etc. during paint invalidation to
deal with some complex situations, for example:
- https://codereview.chromium.org/2430313004/ which sets paint
  invalidation flag in LayoutTable::recalcCollapsedBordersIfNeeded().

Previously we clear flags before walking subtrees. If some descendant
set paint invalidation flags, we would also set flags along the ancestor
path causing dirty flags after paint invalidation.

This will be needed by several bug fixes and cleanups of paint
invalidation.

Also removed some redundant checks of slimmingPaintInvalidationEnabled
in PrePaintTreeWalker.

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

[commit-bot: I haz the power, 2016-11-11 00:23:34]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-11 00:29:18]

Description was changed from

==========
Move clearPaintInvalidatinFlags after subtree walk

This allows we change paint invalidation flags using
setShouldDoFullPaintInvalidation() etc. during paint invalidation to
deal with some complex situations, for example:
- https://codereview.chromium.org/2430313004/ which sets paint
  invalidation flag in LayoutTable::recalcCollapsedBordersIfNeeded().

Previously we clear flags before walking subtrees. If some descendant
set paint invalidation flags, we would also set flags along the ancestor
path causing dirty flags after paint invalidation.

This will be needed by several bug fixes and cleanups of paint
invalidation.

Also removed some redundant checks of slimmingPaintInvalidationEnabled
in PrePaintTreeWalker.

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

to

==========
Move clearPaintInvalidatinFlags after subtree walk

This allows we change paint invalidation flags using
setShouldDoFullPaintInvalidation() etc. during paint invalidation to
deal with some complex situations, for example:
- https://codereview.chromium.org/2430313004/ which sets paint
  invalidation flag in LayoutTable::recalcCollapsedBordersIfNeeded().

Previously we clear flags before walking subtrees. If some descendant
set paint invalidation flags, we would also set flags along the ancestor
path causing dirty flags after paint invalidation.

This will be needed by several bug fixes and cleanups of paint
invalidation.

Also removed some redundant checks of slimmingPaintInvalidationEnabled
in PrePaintTreeWalker.

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

Committed: https://crrev.com/6cd03514645fe54b0c2010e51a8adfdce7646db4
Cr-Commit-Position: refs/heads/master@{#431402}
==========

[commit-bot: I haz the power, 2016-11-11 00:29:19]

Patchset 1 (id:??) landed as
https://crrev.com/6cd03514645fe54b0c2010e51a8adfdce7646db4
Cr-Commit-Position: refs/heads/master@{#431402}