Fix under-invalidation about view background change caused by body style change

Fix under-invalidation about view background change caused by body style change

We need to invalidate the document element layout object when view
background changes caused by body style change.

BUG=482177
TEST=virtual/slimmingpaint/fast/repaint/view-background-from-body-2.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194779

[Xianzhu, 2015-04-29 21:23:18]

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

[chrishtr, 2015-04-30 01:23:57]

https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:1782: if
(RuntimeEnabledFeatures::slimmingPaintEnabled() && layoutView && isBody() &&
layoutView->backgroundLayoutObject() == this) {
How about always invalidating the LayoutView and the LayoutObject for its
background layout object as a pair, regardless of the reason?

[Xianzhu, 2015-04-30 04:46:40]

https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:1782: if
(RuntimeEnabledFeatures::slimmingPaintEnabled() && layoutView && isBody() &&
layoutView->backgroundLayoutObject() == this) {
On 2015/04/30 01:23:57, chrishtr wrote:
> How about always invalidating the LayoutView and the LayoutObject for its
> background layout object as a pair, regardless of the reason?

We don't need to invalidate the LayoutView, but the documentElementLayoutObject
which may paint the viewport background using body's background style.

Do you mean:
  if (isBody() && shouldDoFullPaintInvalidation()) // caused by style change
     documentElementLayoutObject->setShouldDoFullPaintInvalidation()
?

I'm afraid this would cause unnecessary invalidation of the document element
when e.g.
- the body is animating a style other than background
- the document element has background and the body is animating its own
background

Hopefully the code added in this CL will be removed when we resolve
crbug.com/475115.

[chrishtr, 2015-04-30 17:48:36]

https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:1782: if
(RuntimeEnabledFeatures::slimmingPaintEnabled() && layoutView && isBody() &&
layoutView->backgroundLayoutObject() == this) {
On 2015/04/30 at 04:46:39, Xianzhu wrote:
> On 2015/04/30 01:23:57, chrishtr wrote:
> > How about always invalidating the LayoutView and the LayoutObject for its
> > background layout object as a pair, regardless of the reason?
> 
> We don't need to invalidate the LayoutView, but the
documentElementLayoutObject which may paint the viewport background using body's
background style.
> 
> Do you mean:
>   if (isBody() && shouldDoFullPaintInvalidation()) // caused by style change
>      documentElementLayoutObject->setShouldDoFullPaintInvalidation()
> ?
> 
> I'm afraid this would cause unnecessary invalidation of the document element
when e.g.
> - the body is animating a style other than background
> - the document element has background and the body is animating its own
background
> 
> Hopefully the code added in this CL will be removed when we resolve
crbug.com/475115.

Yeah I mean just always invalidating both if one of them is invalidated. Will it
really be a performance problem?

[Xianzhu, 2015-04-30 17:59:59]

On 2015/04/30 17:48:36, chrishtr wrote:
>
https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
> File Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
> Source/core/layout/LayoutObject.cpp:1782: if
> (RuntimeEnabledFeatures::slimmingPaintEnabled() && layoutView && isBody() &&
> layoutView->backgroundLayoutObject() == this) {
> On 2015/04/30 at 04:46:39, Xianzhu wrote:
> > On 2015/04/30 01:23:57, chrishtr wrote:
> > > How about always invalidating the LayoutView and the LayoutObject for its
> > > background layout object as a pair, regardless of the reason?
> > 
> > We don't need to invalidate the LayoutView, but the
> documentElementLayoutObject which may paint the viewport background using
body's
> background style.
> > 
> > Do you mean:
> >   if (isBody() && shouldDoFullPaintInvalidation()) // caused by style change
> >      documentElementLayoutObject->setShouldDoFullPaintInvalidation()
> > ?
> > 
> > I'm afraid this would cause unnecessary invalidation of the document element
> when e.g.
> > - the body is animating a style other than background
> > - the document element has background and the body is animating its own
> background
> > 
> > Hopefully the code added in this CL will be removed when we resolve
> crbug.com/475115.
> 
> Yeah I mean just always invalidating both if one of them is invalidated. Will
it
> really be a performance problem?

Might be a performance problem for some special cases, e.g.
<html style="background: gradient-or-other-expensive-background">
  <body animating>
  </body>
</html>

[Xianzhu, 2015-04-30 18:06:02]

On 2015/04/30 17:59:59, Xianzhu wrote:
> On 2015/04/30 17:48:36, chrishtr wrote:
> >
>
https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
> > File Source/core/layout/LayoutObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
> > Source/core/layout/LayoutObject.cpp:1782: if
> > (RuntimeEnabledFeatures::slimmingPaintEnabled() && layoutView && isBody() &&
> > layoutView->backgroundLayoutObject() == this) {
> > On 2015/04/30 at 04:46:39, Xianzhu wrote:
> > > On 2015/04/30 01:23:57, chrishtr wrote:
> > > > How about always invalidating the LayoutView and the LayoutObject for
its
> > > > background layout object as a pair, regardless of the reason?
> > > 
> > > We don't need to invalidate the LayoutView, but the
> > documentElementLayoutObject which may paint the viewport background using
> body's
> > background style.
> > > 
> > > Do you mean:
> > >   if (isBody() && shouldDoFullPaintInvalidation()) // caused by style
change
> > >      documentElementLayoutObject->setShouldDoFullPaintInvalidation()
> > > ?
> > > 
> > > I'm afraid this would cause unnecessary invalidation of the document
element
> > when e.g.
> > > - the body is animating a style other than background
> > > - the document element has background and the body is animating its own
> > background
> > > 
> > > Hopefully the code added in this CL will be removed when we resolve
> > crbug.com/475115.
> > 
> > Yeah I mean just always invalidating both if one of them is invalidated.
Will
> it
> > really be a performance problem?
> 
> Might be a performance problem for some special cases, e.g.
> <html style="background: gradient-or-other-expensive-background">
>   <body animating>
>   </body>
> </html>

However, your suggested method won't cause regressions because in
non-slimming-paint mode we always repaint document element when body is
invalidated.
Will upload a simpler patch soon.

[chrishtr, 2015-04-30 18:06:37]

On 2015/04/30 at 17:59:59, wangxianzhu wrote:
> On 2015/04/30 17:48:36, chrishtr wrote:
> >
https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
> > File Source/core/layout/LayoutObject.cpp (right):
> > 
> >
https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
> > Source/core/layout/LayoutObject.cpp:1782: if
> > (RuntimeEnabledFeatures::slimmingPaintEnabled() && layoutView && isBody() &&
> > layoutView->backgroundLayoutObject() == this) {
> > On 2015/04/30 at 04:46:39, Xianzhu wrote:
> > > On 2015/04/30 01:23:57, chrishtr wrote:
> > > > How about always invalidating the LayoutView and the LayoutObject for
its
> > > > background layout object as a pair, regardless of the reason?
> > > 
> > > We don't need to invalidate the LayoutView, but the
> > documentElementLayoutObject which may paint the viewport background using
body's
> > background style.
> > > 
> > > Do you mean:
> > >   if (isBody() && shouldDoFullPaintInvalidation()) // caused by style
change
> > >      documentElementLayoutObject->setShouldDoFullPaintInvalidation()
> > > ?
> > > 
> > > I'm afraid this would cause unnecessary invalidation of the document
element
> > when e.g.
> > > - the body is animating a style other than background
> > > - the document element has background and the body is animating its own
> > background
> > > 
> > > Hopefully the code added in this CL will be removed when we resolve
> > crbug.com/475115.
> > 
> > Yeah I mean just always invalidating both if one of them is invalidated.
Will it
> > really be a performance problem?
> 
> Might be a performance problem for some special cases, e.g.
> <html style="background: gradient-or-other-expensive-background">
>   <body animating>
>   </body>
> </html>

In this case it would be a non-background animation? And in this case the view
paints the background?

[chrishtr, 2015-04-30 18:08:21]

On 2015/04/30 at 18:06:37, chrishtr wrote:
> On 2015/04/30 at 17:59:59, wangxianzhu wrote:
> > On 2015/04/30 17:48:36, chrishtr wrote:
> > >
https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
> > > File Source/core/layout/LayoutObject.cpp (right):
> > > 
> > >
https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
> > > Source/core/layout/LayoutObject.cpp:1782: if
> > > (RuntimeEnabledFeatures::slimmingPaintEnabled() && layoutView && isBody()
&&
> > > layoutView->backgroundLayoutObject() == this) {
> > > On 2015/04/30 at 04:46:39, Xianzhu wrote:
> > > > On 2015/04/30 01:23:57, chrishtr wrote:
> > > > > How about always invalidating the LayoutView and the LayoutObject for
its
> > > > > background layout object as a pair, regardless of the reason?
> > > > 
> > > > We don't need to invalidate the LayoutView, but the
> > > documentElementLayoutObject which may paint the viewport background using
body's
> > > background style.
> > > > 
> > > > Do you mean:
> > > >   if (isBody() && shouldDoFullPaintInvalidation()) // caused by style
change
> > > >      documentElementLayoutObject->setShouldDoFullPaintInvalidation()
> > > > ?
> > > > 
> > > > I'm afraid this would cause unnecessary invalidation of the document
element
> > > when e.g.
> > > > - the body is animating a style other than background
> > > > - the document element has background and the body is animating its own
> > > background
> > > > 
> > > > Hopefully the code added in this CL will be removed when we resolve
> > > crbug.com/475115.
> > > 
> > > Yeah I mean just always invalidating both if one of them is invalidated.
Will it
> > > really be a performance problem?
> > 
> > Might be a performance problem for some special cases, e.g.
> > <html style="background: gradient-or-other-expensive-background">
> >   <body animating>
> >   </body>
> > </html>
> 
> In this case it would be a non-background animation? And in this case the view
paints the background?

How common is that though?

And am I right that this CL would get significantly simpler with the suggested
change? If not, then there is not much point, let me know. Also trchen is
working in this area...

[Xianzhu, 2015-04-30 18:36:35]

On 2015/04/30 18:08:21, chrishtr wrote:
> On 2015/04/30 at 18:06:37, chrishtr wrote:
> > On 2015/04/30 at 17:59:59, wangxianzhu wrote:
> > > On 2015/04/30 17:48:36, chrishtr wrote:
> > > >
>
https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
> > > > File Source/core/layout/LayoutObject.cpp (right):
> > > > 
> > > >
>
https://codereview.chromium.org/1117753003/diff/1/Source/core/layout/LayoutOb...
> > > > Source/core/layout/LayoutObject.cpp:1782: if
> > > > (RuntimeEnabledFeatures::slimmingPaintEnabled() && layoutView &&
isBody()
> &&
> > > > layoutView->backgroundLayoutObject() == this) {
> > > > On 2015/04/30 at 04:46:39, Xianzhu wrote:
> > > > > On 2015/04/30 01:23:57, chrishtr wrote:
> > > > > > How about always invalidating the LayoutView and the LayoutObject
for
> its
> > > > > > background layout object as a pair, regardless of the reason?
> > > > > 
> > > > > We don't need to invalidate the LayoutView, but the
> > > > documentElementLayoutObject which may paint the viewport background
using
> body's
> > > > background style.
> > > > > 
> > > > > Do you mean:
> > > > >   if (isBody() && shouldDoFullPaintInvalidation()) // caused by style
> change
> > > > >      documentElementLayoutObject->setShouldDoFullPaintInvalidation()
> > > > > ?
> > > > > 
> > > > > I'm afraid this would cause unnecessary invalidation of the document
> element
> > > > when e.g.
> > > > > - the body is animating a style other than background
> > > > > - the document element has background and the body is animating its
own
> > > > background
> > > > > 
> > > > > Hopefully the code added in this CL will be removed when we resolve
> > > > crbug.com/475115.
> > > > 
> > > > Yeah I mean just always invalidating both if one of them is invalidated.
> Will it
> > > > really be a performance problem?
> > > 
> > > Might be a performance problem for some special cases, e.g.
> > > <html style="background: gradient-or-other-expensive-background">
> > >   <body animating>
> > >   </body>
> > > </html>
> > 
> > In this case it would be a non-background animation? And in this case the
view
> paints the background?
> 
> How common is that though?

I'm not sure. It seems that every over-invalidation bug
(https://code.google.com/p/chromium/issues/list?q=label:SlowBecauseOfRepaintSt...)
looks to me like special case :)
 
> And am I right that this CL would get significantly simpler with the suggested
> change? If not, then there is not much point, let me know. Also trchen is
> working in this area...

Yes. Uploaded simpler CL. PTAL.

(trchen@ is working on crbug.com/475115 which is to refactor background painting
code to be clearer. This CL is to fix the under-invalidation bug required by
slimming paint phase 1. Fixing crbug.com/475115 may result this CL to be
removed.)

[chrishtr, 2015-04-30 20:20:35]

I vote for this latest patchset, especially given trchen is working on the issue
more generally which may make it obsolete.

[chrishtr, 2015-04-30 20:20:37]

lgtm

[Xianzhu, 2015-04-30 20:58:33]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-04-30 20:58:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1117753003/#ps40001
(title: "Update test expectations")

[commit-bot: I haz the power, 2015-04-30 20:59:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1117753003/40001

[commit-bot: I haz the power, 2015-05-01 00:53:43]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194779