SP: Invalidate all non-compositing descendants in the slow scroll path.

SP: Invalidate all non-compositing descendants in the slow scroll path.

In non-slimming-paint mode we can get away with just invalidating the scroller
because its bounds contain those of any non-composited descendants. In slimming
paint mode, however, this won't work. We should be able to improve this later
in Slimming Paint phase 2.

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

[chrishtr, 2015-06-22 23:05:53]

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

[Xianzhu, 2015-06-22 23:32:03]

lgtm

https://codereview.chromium.org/1202523006/diff/1/LayoutTests/paint/invalidat...
File
LayoutTests/paint/invalidation/non-stacking-scroller-with-abspos-descendant.html
(right):

https://codereview.chromium.org/1202523006/diff/1/LayoutTests/paint/invalidat...
LayoutTests/paint/invalidation/non-stacking-scroller-with-abspos-descendant.html:14:
</script>
Nit: It will work but looks weird to use text-based-repaint.js for a ref test. I
think run-after-layout-and-paint.js would be better.

[trchen, 2015-06-22 23:41:29]

I think this is a little bit too aggressive. While we need to invalidate the
display item of all non-composited descendants, we don't necessarily need to
re-raster all of them.

LayoutObject::invalidateDisplayItemClientForNonCompositingDescendants() was
added for this purpose. It is not postponed until invalidation phase though.
Perhaps we should optimize it and use it instead.

[Xianzhu, 2015-06-22 23:46:39]

On 2015/06/22 23:41:29, trchen wrote:
> I think this is a little bit too aggressive. While we need to invalidate the
> display item of all non-composited descendants, we don't necessarily need to
> re-raster all of them.
> 
> LayoutObject::invalidateDisplayItemClientForNonCompositingDescendants() was
> added for this purpose. It is not postponed until invalidation phase though.
> Perhaps we should optimize it and use it instead.

I think there is no difference about rasterization for the two methods, because
the combined invalidation rects is still the same.

[chrishtr, 2015-06-22 23:46:55]

On 2015/06/22 at 23:41:29, trchen wrote:
> I think this is a little bit too aggressive. While we need to invalidate the
display item of all non-composited descendants, we don't necessarily need to
re-raster all of them.
> 
> LayoutObject::invalidateDisplayItemClientForNonCompositingDescendants() was
added for this purpose. It is not postponed until invalidation phase though.
Perhaps we should optimize it and use it instead.

What do you mean by optimize?
setShouldDoFullPaintInvalidationIncludingNonCompositingDescendants does an eager
recursion also.

[trchen, 2015-06-22 23:50:59]

On 2015/06/22 23:46:39, Xianzhu wrote:
> On 2015/06/22 23:41:29, trchen wrote:
> > I think this is a little bit too aggressive. While we need to invalidate the
> > display item of all non-composited descendants, we don't necessarily need to
> > re-raster all of them.
> > 
> > LayoutObject::invalidateDisplayItemClientForNonCompositingDescendants() was
> > added for this purpose. It is not postponed until invalidation phase though.
> > Perhaps we should optimize it and use it instead.
> 
> I think there is no difference about rasterization for the two methods,
because
> the combined invalidation rects is still the same.

I'm not very sure about that. Haven't read the scroll invalidation code in
detail. Do we always invalidate the whole unscrolled padding rect?

On 2015/06/22 23:46:55, chrishtr wrote:
> On 2015/06/22 at 23:41:29, trchen wrote:
> > I think this is a little bit too aggressive. While we need to invalidate the
> display item of all non-composited descendants, we don't necessarily need to
> re-raster all of them.
> > 
> > LayoutObject::invalidateDisplayItemClientForNonCompositingDescendants() was
> added for this purpose. It is not postponed until invalidation phase though.
> Perhaps we should optimize it and use it instead.
> 
> What do you mean by optimize?
> setShouldDoFullPaintInvalidationIncludingNonCompositingDescendants does an
eager
> recursion also.

Oops, didn't know that. Then using
LayoutObject::invalidateDisplayItemClientForNonCompositingDescendants() is
definitely better. :)

[trchen, 2015-06-22 23:55:46]

On 2015/06/22 23:50:59, trchen wrote:
> On 2015/06/22 23:46:39, Xianzhu wrote:
> > On 2015/06/22 23:41:29, trchen wrote:
> > > I think this is a little bit too aggressive. While we need to invalidate
the
> > > display item of all non-composited descendants, we don't necessarily need
to
> > > re-raster all of them.
> > > 
> > > LayoutObject::invalidateDisplayItemClientForNonCompositingDescendants()
was
> > > added for this purpose. It is not postponed until invalidation phase
though.
> > > Perhaps we should optimize it and use it instead.
> > 
> > I think there is no difference about rasterization for the two methods,
> because
> > the combined invalidation rects is still the same.
> 
> I'm not very sure about that. Haven't read the scroll invalidation code in
> detail. Do we always invalidate the whole unscrolled padding rect?

Ah I see what you mean. If we use
invalidateDisplayItemClientForNonCompositingDescendants(), it has to be added in
extra to box().setShouldDoFullPaintInvalidation(); that's already there. Yea
then I guess it doesn't make all that much difference.

I would still recommend using
invalidateDisplayItemClientForNonCompositingDescendants() because it is clearer
about the intention (we won't need to think whether the invalidation rects
really matters).

[Xianzhu, 2015-06-23 00:16:14]

On 2015/06/22 23:55:46, trchen wrote:
> On 2015/06/22 23:50:59, trchen wrote:
> > On 2015/06/22 23:46:39, Xianzhu wrote:
> > > On 2015/06/22 23:41:29, trchen wrote:
> > > > I think this is a little bit too aggressive. While we need to invalidate
> the
> > > > display item of all non-composited descendants, we don't necessarily
need
> to
> > > > re-raster all of them.
> > > > 
> > > > LayoutObject::invalidateDisplayItemClientForNonCompositingDescendants()
> was
> > > > added for this purpose. It is not postponed until invalidation phase
> though.
> > > > Perhaps we should optimize it and use it instead.
> > > 
> > > I think there is no difference about rasterization for the two methods,
> > because
> > > the combined invalidation rects is still the same.
> > 
> > I'm not very sure about that. Haven't read the scroll invalidation code in
> > detail. Do we always invalidate the whole unscrolled padding rect?
> 
> Ah I see what you mean. If we use
> invalidateDisplayItemClientForNonCompositingDescendants(), it has to be added
in
> extra to box().setShouldDoFullPaintInvalidation(); that's already there. Yea
> then I guess it doesn't make all that much difference.
> 
> I would still recommend using
> invalidateDisplayItemClientForNonCompositingDescendants() because it is
clearer
> about the intention (we won't need to think whether the invalidation rects
> really matters).

I think we should do the contrary.

If we believe the invalidaiton rects won't exceed the original invalidation
rect, then the two methods are the same; If the invalidation rects might exceed
(shouldn't the scrolling container clip?), if we just invalidate display item
clients, then some invalidated display items might not be covered by any
invalidation rects, causing under-painting.

In addition, calling invalidateDisplayItemClientForNonCompositingDescendants()
before layout may uselessly invalidate inline boxes that might be removed during
layout.

[trchen, 2015-06-23 00:29:28]

On 2015/06/23 00:16:14, Xianzhu wrote:
> On 2015/06/22 23:55:46, trchen wrote:
> > On 2015/06/22 23:50:59, trchen wrote:
> > > On 2015/06/22 23:46:39, Xianzhu wrote:
> > > > On 2015/06/22 23:41:29, trchen wrote:
> > > > > I think this is a little bit too aggressive. While we need to
invalidate
> > the
> > > > > display item of all non-composited descendants, we don't necessarily
> need
> > to
> > > > > re-raster all of them.
> > > > > 
> > > > >
LayoutObject::invalidateDisplayItemClientForNonCompositingDescendants()
> > was
> > > > > added for this purpose. It is not postponed until invalidation phase
> > though.
> > > > > Perhaps we should optimize it and use it instead.
> > > > 
> > > > I think there is no difference about rasterization for the two methods,
> > > because
> > > > the combined invalidation rects is still the same.
> > > 
> > > I'm not very sure about that. Haven't read the scroll invalidation code in
> > > detail. Do we always invalidate the whole unscrolled padding rect?
> > 
> > Ah I see what you mean. If we use
> > invalidateDisplayItemClientForNonCompositingDescendants(), it has to be
added
> in
> > extra to box().setShouldDoFullPaintInvalidation(); that's already there. Yea
> > then I guess it doesn't make all that much difference.
> > 
> > I would still recommend using
> > invalidateDisplayItemClientForNonCompositingDescendants() because it is
> clearer
> > about the intention (we won't need to think whether the invalidation rects
> > really matters).
> 
> I think we should do the contrary.
> 
> If we believe the invalidaiton rects won't exceed the original invalidation
> rect, then the two methods are the same; If the invalidation rects might
exceed
> (shouldn't the scrolling container clip?), if we just invalidate display item
> clients, then some invalidated display items might not be covered by any
> invalidation rects, causing under-painting.

Non-composited fixed-pos descendant is one notable example that will result in
over-re-rastering. http://output.jsbin.com/pituceraya/

Note: Fixed-pos descendants are not clipped by the scrolling container.

> In addition, calling invalidateDisplayItemClientForNonCompositingDescendants()
> before layout may uselessly invalidate inline boxes that might be removed
during
> layout.

[Xianzhu, 2015-06-23 00:33:51]

> 
> Non-composited fixed-pos descendant is one notable example that will result in
> over-re-rastering. http://output.jsbin.com/pituceraya/
> 
> Note: Fixed-pos descendants are not clipped by the scrolling container.
> 

Actually I think this is a good example of 'under-paint': the display item will
change, but is not covered by any invalidation rect and we may miss re-recording
of it.

[Xianzhu, 2015-06-23 01:10:29]

Discussed offline with trchen@ and we examined a slightly different case than
the case in #10 (http://output.jsbin.com/qinajopaqi):
1. The scrollable div is scrolled out of view;
2. The scrollable div scrolls programatically;

If we call invalidateDisplayItemClientForNonCompositingDescendants in step 2,
the invalidation rects are out of viewport, and there might be no next
recording. We are not sure what will happen if the tile containing the fixed
child is evicted and re-rasterized with the old display item/picture. Will do
more investigation on this case.

Except for the above corner case,
invalidateDisplayItemClientForNonCompositingDescendants() seems better.

[chrishtr, 2015-06-23 04:03:56]

On 2015/06/23 at 01:10:29, wangxianzhu wrote:
> Discussed offline with trchen@ and we examined a slightly different case than
the case in #10 (http://output.jsbin.com/qinajopaqi):
> 1. The scrollable div is scrolled out of view;
> 2. The scrollable div scrolls programatically;
> 
> If we call invalidateDisplayItemClientForNonCompositingDescendants in step 2,
the invalidation rects are out of viewport, and there might be no next
recording. We are not sure what will happen if the tile containing the fixed
child is evicted and re-rasterized with the old display item/picture. Will do
more investigation on this case.
> 
> Except for the above corner case,
invalidateDisplayItemClientForNonCompositingDescendants() seems better.

So I have to commit the patch as-is right, to be correct?

[chrishtr, 2015-06-23 04:07:27]

Sample layout test failures before rebaseline:

https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

A couple of them look pretty weird, in particular:

https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

I don't know what's up with that, invalidation should be getting larger not
smaller.

https://codereview.chromium.org/1202523006/diff/1/LayoutTests/paint/invalidat...
File
LayoutTests/paint/invalidation/non-stacking-scroller-with-abspos-descendant.html
(right):

https://codereview.chromium.org/1202523006/diff/1/LayoutTests/paint/invalidat...
LayoutTests/paint/invalidation/non-stacking-scroller-with-abspos-descendant.html:14:
</script>
On 2015/06/22 at 23:32:03, Xianzhu wrote:
> Nit: It will work but looks weird to use text-based-repaint.js for a ref test.
I think run-after-layout-and-paint.js would be better.

Done.

[Xianzhu, 2015-06-23 04:49:27]

On 2015/06/23 04:07:27, chrishtr wrote:
> Sample layout test failures before rebaseline:
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
> 
> A couple of them look pretty weird, in particular:
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
> 
> I don't know what's up with that, invalidation should be getting larger not
> smaller.

The new results look correct.

The old results seem because of the same situation described in the last
paragraph of crbug.com/416539 #0. Without this patch we skip paint invalidation
of the descendants of the scrolled container, leaving their
paintInvalidationRects not updated. During the paint invalidation triggered by
repaintTest(), the paintInvalidationRects of the scrolling contents are updated,
and we find their paintInvalidationRects change and invalidate them. I'm
expecting to create a new way to resolve it during slimming paint phase 2.

[Xianzhu, 2015-06-23 04:56:39]

https://codereview.chromium.org/1202523006/diff/40001/LayoutTests/paint/inval...
File
LayoutTests/paint/invalidation/non-stacking-scroller-with-abspos-descendant.html
(right):

https://codereview.chromium.org/1202523006/diff/40001/LayoutTests/paint/inval...
LayoutTests/paint/invalidation/non-stacking-scroller-with-abspos-descendant.html:12:
});
We need a 'true' as the second parameter here, otherwise we need explicit
waitUntilDone and notifyDone.

The crash is because of the pixel capturing after the test automatically
finishes. (I have a WIP CL to replace the crash with a warning message)

[chrishtr, 2015-06-23 16:51:37]

https://codereview.chromium.org/1202523006/diff/40001/LayoutTests/paint/inval...
File
LayoutTests/paint/invalidation/non-stacking-scroller-with-abspos-descendant.html
(right):

https://codereview.chromium.org/1202523006/diff/40001/LayoutTests/paint/inval...
LayoutTests/paint/invalidation/non-stacking-scroller-with-abspos-descendant.html:12:
});
On 2015/06/23 at 04:56:39, Xianzhu wrote:
> We need a 'true' as the second parameter here, otherwise we need explicit
waitUntilDone and notifyDone.
> 
> The crash is because of the pixel capturing after the test automatically
finishes. (I have a WIP CL to replace the crash with a warning message)

Done.

[chrishtr, 2015-06-23 16:57:43]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-06-23 16:57:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/1202523006/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-06-23 16:58:02]

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

[chrishtr, 2015-06-23 17:43:43]

The CQ bit was unchecked by chrishtr@chromium.org

[chrishtr, 2015-06-23 17:43:53]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-06-23 17:43:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/1202523006/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-06-23 17:44:15]

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

[commit-bot: I haz the power, 2015-06-23 19:00:59]

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

[commit-bot: I haz the power, 2015-06-23 19:01:01]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60356)

[chrishtr, 2015-06-23 19:03:08]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-06-23 19:03:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/1202523006/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-06-23 19:03:46]

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

[commit-bot: I haz the power, 2015-06-23 21:20:31]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197675