Fix paint invalidation rect tracking within composited scrolling layers.

Fix paint invalidation rect tracking within composited scrolling layers.

Previously, paint invalidation rect tracking (aka repaint-after-layout rects)
were always stored in the space of the paint invalidation container. If the
paint invalidation container is a composited scroller, this is incorrect. They
should be stored in the space of the graphics layer backing the content. In
the case of a composited scroller, the graphics layer backing never moves when
scrolls occur.

This had the effect that any paint invalidation anywhere in the scroller that
happens after a scroll would end up invalidating the entire scrolled content,
because it appeared to have moved. This change is a huge performance improvement
in some such cases. In addition, this patch fixes incorrect invalidations which
may result in missed invalidations after scrolling.

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

[chrishtr, 2015-08-13 20:40:05]

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

[chrishtr, 2015-08-13 20:41:30]

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

[Xianzhu, 2015-08-13 21:24:42]

https://codereview.chromium.org/1287413002/diff/40001/LayoutTests/platform/wi...
File
LayoutTests/platform/win/fast/repaint/overflow-move-after-scroll-expected.txt
(right):

https://codereview.chromium.org/1287413002/diff/40001/LayoutTests/platform/wi...
LayoutTests/platform/win/fast/repaint/overflow-move-after-scroll-expected.txt:29:
[300, 100, 120, 50],
This looks incorrect. The old location of the block should be at 300,200,120,50
in the scrolling contents layer.

https://codereview.chromium.org/1287413002/diff/40001/LayoutTests/platform/wi...
File
LayoutTests/platform/win/fast/repaint/overflow-scroll-after-move-expected.txt
(right):

https://codereview.chromium.org/1287413002/diff/40001/LayoutTests/platform/wi...
LayoutTests/platform/win/fast/repaint/overflow-scroll-after-move-expected.txt:15:
[50, -100, 200, 50]
FYI This is because crbug.com/416535, which I didn't find a good solution
without slimming paint.

https://codereview.chromium.org/1287413002/diff/40001/LayoutTests/platform/wi...
LayoutTests/platform/win/fast/repaint/overflow-scroll-after-move-expected.txt:29:
[50, 50, 200, 50]
This seems incorrect, too.

[Xianzhu, 2015-08-13 21:25:29]

P.S. The CL looks great!

[skobes, 2015-08-13 21:26:10]

Thanks for fixing this!

The underlying problem is that a composited scroller has multiple
GraphicsLayers, and
CompositedDeprecatedPaintLayerMapping::setContentsNeedDisplayInRect doesn't let
the caller specify which one the invalidation should apply to.  I think
wangxianzhu tried to address some of this in http://crbug.com/416535, hopefully
slimming paint will improve the situation.

DeprecatedPaintLayer::updateLayerPositionsAfterScrollRecursive has code to
adjust the scroller's previousPaintInvalidationRect after a scroll.  Should that
code be removed now that we have this?

https://codereview.chromium.org/1287413002/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1287413002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.h:1434: // This stores the paint invalidation
rect from the previous frame. This rect does *not* account for composited
scrolling. See
It's a little unclear what it means to "account for composited scrolling". 
Could we just say that "the rect is in the coordinate space of the paint
invalidation container's scrolling contents layer"?

[skobes, 2015-08-13 21:31:03]

On 2015/08/13 21:26:10, skobes wrote:
> "the rect is in the coordinate space of the paint
> invalidation container's scrolling contents layer"

"... if the paint invalidation container is a scroller" :)

[chrishtr, 2015-08-13 21:44:27]

https://codereview.chromium.org/1287413002/diff/40001/LayoutTests/platform/wi...
File
LayoutTests/platform/win/fast/repaint/overflow-move-after-scroll-expected.txt
(right):

https://codereview.chromium.org/1287413002/diff/40001/LayoutTests/platform/wi...
LayoutTests/platform/win/fast/repaint/overflow-move-after-scroll-expected.txt:29:
[300, 100, 120, 50],
On 2015/08/13 at 21:24:42, Xianzhu wrote:
> This looks incorrect. The old location of the block should be at
300,200,120,50 in the scrolling contents layer.

Yep you're right. If the child is absolutely positioned, including scroll offset
is wrong somehow as I've written it. Investigating.

https://codereview.chromium.org/1287413002/diff/40001/LayoutTests/platform/wi...
File
LayoutTests/platform/win/fast/repaint/overflow-scroll-after-move-expected.txt
(right):

https://codereview.chromium.org/1287413002/diff/40001/LayoutTests/platform/wi...
LayoutTests/platform/win/fast/repaint/overflow-scroll-after-move-expected.txt:29:
[50, 50, 200, 50]
On 2015/08/13 at 21:24:42, Xianzhu wrote:
> This seems incorrect, too.

Yep. Same issue it seems: absolute positioning.

[Xianzhu, 2015-08-13 21:47:02]

On 2015/08/13 21:26:10, skobes wrote:
> 
> DeprecatedPaintLayer::updateLayerPositionsAfterScrollRecursive has code to
> adjust the scroller's previousPaintInvalidationRect after a scroll.  Should
that
> code be removed now that we have this?
> 

Chris, I suspect this is the reason of the wrong new test results.

[chrishtr, 2015-08-13 21:57:37]

On 2015/08/13 at 21:47:02, wangxianzhu wrote:
> On 2015/08/13 21:26:10, skobes wrote:
> > 
> > DeprecatedPaintLayer::updateLayerPositionsAfterScrollRecursive has code to
> > adjust the scroller's previousPaintInvalidationRect after a scroll.  Should
that
> > code be removed now that we have this?
> > 
> 
> Chris, I suspect this is the reason of the wrong new test results.

ah. good point. I'll check that. pos:absolute has a DeprecatedPaintLayer.!

[chrishtr, 2015-08-13 23:37:37]

Fixed by turning off the functionality in
DPL::updateLayerPositionsAfterScrollRecursive when scrolling is composited.

[Xianzhu, 2015-08-14 04:31:28]

lgtm except the comments for layout tests, the timeout and need of rebaselines.

I just remembered why I failed to fix the bug before: I tried to always use
backing coordinates everywhere but blocked by crbug.com/416535. Your CL avoids
the issue by just changing the coordinates of paint invalidation rect/location
but still using paint invalidation container coordinates when doing
invalidation. Didn't thought of this way :(

https://codereview.chromium.org/1287413002/diff/80001/LayoutTests/paint/inval...
File LayoutTests/paint/invalidation/invalidate-after-composited-scroll.html
(right):

https://codereview.chromium.org/1287413002/diff/80001/LayoutTests/paint/inval...
LayoutTests/paint/invalidation/invalidate-after-composited-scroll.html:14:
requestAnimationFrame(function() {
Why is requestAnimationFrame needed?

https://codereview.chromium.org/1287413002/diff/80001/LayoutTests/paint/inval...
LayoutTests/paint/invalidation/invalidate-after-composited-scroll.html:22:
window.testRunner.waitUntilDone();
The above two lines are unnecessary.

[skobes, 2015-08-14 05:42:02]

lgtm

[chrishtr, 2015-08-14 18:26:41]

https://codereview.chromium.org/1287413002/diff/80001/LayoutTests/paint/inval...
File LayoutTests/paint/invalidation/invalidate-after-composited-scroll.html
(right):

https://codereview.chromium.org/1287413002/diff/80001/LayoutTests/paint/inval...
LayoutTests/paint/invalidation/invalidate-after-composited-scroll.html:14:
requestAnimationFrame(function() {
On 2015/08/14 at 04:31:28, Xianzhu wrote:
> Why is requestAnimationFrame needed?

Fixed.

https://codereview.chromium.org/1287413002/diff/80001/LayoutTests/paint/inval...
LayoutTests/paint/invalidation/invalidate-after-composited-scroll.html:22:
window.testRunner.waitUntilDone();
On 2015/08/14 at 04:31:28, Xianzhu wrote:
> The above two lines are unnecessary.

Fixed

[chrishtr, 2015-08-14 18:36:40]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-08-14 18:36:40]

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

[commit-bot: I haz the power, 2015-08-14 18:36:49]

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

[commit-bot: I haz the power, 2015-08-14 20:05:24]

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

[commit-bot: I haz the power, 2015-08-14 20:05:25]

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

[chrishtr, 2015-08-14 20:07:30]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2015-08-14 20:07:42]

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

[commit-bot: I haz the power, 2015-08-14 21:38:59]

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