[SPInvalidation] Invalidate all paint and properties for printing

[SPInvalidation] Invalidate all paint and properties for printing

Previously we recursively call setShouldDoFullPaintInvalidation()
on all objects of the views entering/exiting print mode.

For SPInvalidation we also need to rebuild all paint properties.
Use a flag on FrameView to indicate it has just entered or exited
printing mode. Invalidate paint and update paint properties of the
whole tree if the flag is set during PrePaintTreeWalk.

BUG=645667
TEST=virtual/spinvalidation/printing/overflow-auto.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[Xianzhu, 2016-12-13 23:33:26]

Description was changed from

==========
[SPInvalidation] Invalidate all paint and properties for printing

Previously we recursively call setShouldDoFullPaintInvalidation()
on all objects of the views entering/exiting print mode.

For SPInvalidation we also need to rebuild all paint properties.
Use a flag on FrameView to indicate it has just entered or exited
printing mode. Invalidate paint and update paint properties of the
whole tree if the flag is set during PrePaintTreeWalk.

BUG=645667
TEST=virtual/spinvalidation/printing/overflow-auto.html
==========

to

==========
[SPInvalidation] Invalidate all paint and properties for printing

Previously we recursively call setShouldDoFullPaintInvalidation()
on all objects of the views entering/exiting print mode.

For SPInvalidation we also need to rebuild all paint properties.
Use a flag on FrameView to indicate it has just entered or exited
printing mode. Invalidate paint and update paint properties of the
whole tree if the flag is set during PrePaintTreeWalk.

BUG=645667
TEST=virtual/spinvalidation/printing/overflow-auto.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2016-12-13 23:33:58]

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

[commit-bot: I haz the power, 2016-12-13 23:34: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-12-14 01:57:06]

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

[commit-bot: I haz the power, 2016-12-14 01:57:07]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-12-14 05:11:45]

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

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

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-12-14 06:43:15]

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

[commit-bot: I haz the power, 2016-12-14 06:43:16]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-12-14 16:56:50]

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

[Xianzhu, 2016-12-14 23:53:35]

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

[commit-bot: I haz the power, 2016-12-14 23:54:24]

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-12-15 01:41:15]

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

[commit-bot: I haz the power, 2016-12-15 01:41:16]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-12-15 06:07:25]

The implementation looks good overall, just some questions about why such a
heavy hammer is needed.

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:106: crbug.com/646176
virtual/spinvalidation/printing/overflow-auto.html [ Pass ]
I'm surprised only one new test passes. Do the other tests pass with this as
well but you left them off for efficiency?

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:752: // Set when the whole
frame subtree needs full paint invalidation and paint
Why is a full paint property tree rebuild necessary, beyond the rebuilding that
occurs when printing causes a layout?

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.h (right):

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.h:52: // For SPv1, all of
the above flags don't cross paint invalidation container
Similar question as above--why do we need to invalidate paint in more cases than
spv1?

[Xianzhu, 2016-12-15 16:59:11]

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:106: crbug.com/646176
virtual/spinvalidation/printing/overflow-auto.html [ Pass ]
On 2016/12/15 06:07:24, pdr. wrote:
> I'm surprised only one new test passes. Do the other tests pass with this as
> well but you left them off for efficiency?

The other tests also pass, but some of them are already virtual tests which
can't be virtual/spinvalidation. We can add
virtual/spinvalidation/printing/fixed-positioned-scrolled.html though.

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:752: // Set when the whole
frame subtree needs full paint invalidation and paint
On 2016/12/15 06:07:24, pdr. wrote:
> Why is a full paint property tree rebuild necessary, beyond the rebuilding
that
> occurs when printing causes a layout?

There would be many under-invalidations in paint and paint properties just based
on layout. We are also doing the similar thing in spv1 with a heavier hammer:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Loc....

This change replaces the whole tree walk setting shouldDoFullPaintInvalidation
on every object with a flag, and let it also work for paint property
invalidation.

It would be great if we knew exactly why the forced layouts before and after
printing don't cause enough paint invalidations without the forced paint
invalidation.

[Xianzhu, 2016-12-15 17:04:35]

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

[commit-bot: I haz the power, 2016-12-15 17:04:55]

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

[Xianzhu, 2016-12-15 17:05:42]

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:106: crbug.com/646176
virtual/spinvalidation/printing/overflow-auto.html [ Pass ]
On 2016/12/15 16:59:11, Xianzhu wrote:
> On 2016/12/15 06:07:24, pdr. wrote:
> > I'm surprised only one new test passes. Do the other tests pass with this as
> > well but you left them off for efficiency?
> 
> The other tests also pass, but some of them are already virtual tests which
> can't be virtual/spinvalidation. We can add
> virtual/spinvalidation/printing/fixed-positioned-scrolled.html though.

Done.

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:752: // Set when the whole
frame subtree needs full paint invalidation and paint
On 2016/12/15 16:59:11, Xianzhu wrote:
> On 2016/12/15 06:07:24, pdr. wrote:
> > Why is a full paint property tree rebuild necessary, beyond the rebuilding
> that
> > occurs when printing causes a layout?
> 
> There would be many under-invalidations in paint and paint properties just
based
> on layout. We are also doing the similar thing in spv1 with a heavier hammer:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Loc....
> 

The whole tree walk for SPv1 is in
LayoutView::setShouldDoFullPaintInvalidationForViewAndAllDescendants().

> This change replaces the whole tree walk setting shouldDoFullPaintInvalidation
> on every object with a flag, and let it also work for paint property
> invalidation.
> 
> It would be great if we knew exactly why the forced layouts before and after
> printing don't cause enough paint invalidations without the forced paint
> invalidation.

[Xianzhu, 2016-12-15 17:46:21]

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

[commit-bot: I haz the power, 2016-12-15 17:46:27]

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

[Xianzhu, 2016-12-15 18:06:30]

Patchset #5 (id:80001) has been deleted

[Xianzhu, 2016-12-15 18:08:41]

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

[commit-bot: I haz the power, 2016-12-15 18:09:01]

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

[pdr., 2016-12-15 19:40:32]

On 2016/12/15 at 17:05:42, wangxianzhu wrote:
>
https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/TestExpectations:106: crbug.com/646176
virtual/spinvalidation/printing/overflow-auto.html [ Pass ]
> On 2016/12/15 16:59:11, Xianzhu wrote:
> > On 2016/12/15 06:07:24, pdr. wrote:
> > > I'm surprised only one new test passes. Do the other tests pass with this
as
> > > well but you left them off for efficiency?
> > 
> > The other tests also pass, but some of them are already virtual tests which
> > can't be virtual/spinvalidation. We can add
> > virtual/spinvalidation/printing/fixed-positioned-scrolled.html though.
> 
> Done.
> 
>
https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/FrameView.h (right):
> 
>
https://codereview.chromium.org/2575653002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/FrameView.h:752: // Set when the whole
frame subtree needs full paint invalidation and paint
> On 2016/12/15 16:59:11, Xianzhu wrote:
> > On 2016/12/15 06:07:24, pdr. wrote:
> > > Why is a full paint property tree rebuild necessary, beyond the rebuilding
> > that
> > > occurs when printing causes a layout?
> > 
> > There would be many under-invalidations in paint and paint properties just
based
> > on layout. We are also doing the similar thing in spv1 with a heavier
hammer:
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Loc....
> > 
> 
> The whole tree walk for SPv1 is in
LayoutView::setShouldDoFullPaintInvalidationForViewAndAllDescendants().
> 
> > This change replaces the whole tree walk setting
shouldDoFullPaintInvalidation
> > on every object with a flag, and let it also work for paint property
> > invalidation.
> > 
> > It would be great if we knew exactly why the forced layouts before and after
> > printing don't cause enough paint invalidations without the forced paint
> > invalidation.

LGTM

[commit-bot: I haz the power, 2016-12-15 19:47:09]

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

[commit-bot: I haz the power, 2016-12-15 19:47:10]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Xianzhu, 2016-12-15 20:07:19]

Patchset #5 (id:100001) has been deleted

[Xianzhu, 2016-12-15 20:07:27]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-12-15 20:08:27]

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-12-15 20:14:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481832447760730,
"parent_rev": "d0531ffc4b1e1591f9f249191b9d586103106869", "commit_rev":
"53d57cadedc34bfa15e742baddacbed09a97c49e"}

[commit-bot: I haz the power, 2016-12-15 20:14:50]

Description was changed from

==========
[SPInvalidation] Invalidate all paint and properties for printing

Previously we recursively call setShouldDoFullPaintInvalidation()
on all objects of the views entering/exiting print mode.

For SPInvalidation we also need to rebuild all paint properties.
Use a flag on FrameView to indicate it has just entered or exited
printing mode. Invalidate paint and update paint properties of the
whole tree if the flag is set during PrePaintTreeWalk.

BUG=645667
TEST=virtual/spinvalidation/printing/overflow-auto.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[SPInvalidation] Invalidate all paint and properties for printing

Previously we recursively call setShouldDoFullPaintInvalidation()
on all objects of the views entering/exiting print mode.

For SPInvalidation we also need to rebuild all paint properties.
Use a flag on FrameView to indicate it has just entered or exited
printing mode. Invalidate paint and update paint properties of the
whole tree if the flag is set during PrePaintTreeWalk.

BUG=645667
TEST=virtual/spinvalidation/printing/overflow-auto.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2575653002
==========

[commit-bot: I haz the power, 2016-12-15 20:14:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-15 20:18:21]

Description was changed from

==========
[SPInvalidation] Invalidate all paint and properties for printing

Previously we recursively call setShouldDoFullPaintInvalidation()
on all objects of the views entering/exiting print mode.

For SPInvalidation we also need to rebuild all paint properties.
Use a flag on FrameView to indicate it has just entered or exited
printing mode. Invalidate paint and update paint properties of the
whole tree if the flag is set during PrePaintTreeWalk.

BUG=645667
TEST=virtual/spinvalidation/printing/overflow-auto.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2575653002
==========

to

==========
[SPInvalidation] Invalidate all paint and properties for printing

Previously we recursively call setShouldDoFullPaintInvalidation()
on all objects of the views entering/exiting print mode.

For SPInvalidation we also need to rebuild all paint properties.
Use a flag on FrameView to indicate it has just entered or exited
printing mode. Invalidate paint and update paint properties of the
whole tree if the flag is set during PrePaintTreeWalk.

BUG=645667
TEST=virtual/spinvalidation/printing/overflow-auto.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2016-12-15 20:18:22]

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