[SP] Don't invalidate the whole scrolled tree

[SP] Don't invalidate the whole scrolled tree

We added the whole-tree invalidation before we used ScrollRecorder
for all modes because the cull rects of all scrolled contents were
changed after scroll at that time.

The whole-tree invalidation is longer needed.

Also fixed a bug about scrolled offset compensation in non-composited
scrolling path. We need to compensate the accumulated scroll offset
from the root paint layer.

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

[Xianzhu, 2015-06-02 21:49:31]

Patchset #2 (id:20001) has been deleted

[Xianzhu, 2015-06-02 21:49:38]

Patchset #1 (id:1) has been deleted

[Xianzhu, 2015-06-02 21:53:25]

Patchset #4 (id:100001) has been deleted

[Xianzhu, 2015-06-02 21:53:37]

Patchset #2 (id:60001) has been deleted

[Xianzhu, 2015-06-02 21:53:43]

Patchset #1 (id:40001) has been deleted

[Xianzhu, 2015-06-03 17:56:12]

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

[Xianzhu, 2015-06-03 17:56:12]

We'll have crbug.com/496239 (not covered by current layout tests) with this CL.
Will land the fix for crbug.com/496239 first.

[chrishtr, 2015-06-03 18:46:09]

https://codereview.chromium.org/1159113003/diff/170001/Source/core/paint/Depr...
File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right):

https://codereview.chromium.org/1159113003/diff/170001/Source/core/paint/Depr...
Source/core/paint/DeprecatedPaintLayerPainter.cpp:648: if
(!paintingInfo.scrollOffsetAccumulation.isZero()) {
Can you explain more why we need to do this accumulation?

Also, test?

https://codereview.chromium.org/1159113003/diff/170001/Source/core/paint/Depr...
File Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp (left):

https://codereview.chromium.org/1159113003/diff/170001/Source/core/paint/Depr...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:428: if
(requiresPaintInvalidation && RuntimeEnabledFeatures::slimmingPaintEnabled()) {
Doesn't the culling code for scrolled content still try to be smart about the
clip of the scroller?

[Xianzhu, 2015-06-03 19:07:00]

https://codereview.chromium.org/1159113003/diff/170001/Source/core/paint/Depr...
File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right):

https://codereview.chromium.org/1159113003/diff/170001/Source/core/paint/Depr...
Source/core/paint/DeprecatedPaintLayerPainter.cpp:648: if
(!paintingInfo.scrollOffsetAccumulation.isZero()) {
On 2015/06/03 18:46:08, chrishtr wrote:
> Can you explain more why we need to do this accumulation?
> 
> Also, test?

This is covered by existing tests. Previously the failures were hidden because
we invalidate the whole scrolled tree.

I should have kept the comments at line 637-639 of the old code, and modified it
into:
  // As a descendant of the root layer, m_paintLayer's painting is not
controlled by the ScrollRecorders
  // created by BlockPainter of the ancestor layers up to the root layer, so we
need to issue ScrollRecorder
  // for this layer seperately, with the scroll offset (accumulated from the
root layer to the parent of this
  // layer), to get the same result as ScrollRecorder in BlockPainter.

Will insert this comment below.

Previously (code at left) we compensate the scroll offset of the root painting
layer only, missing the intermediate scroll layers between the root layer and
this layer.

https://codereview.chromium.org/1159113003/diff/170001/Source/core/paint/Depr...
File Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp (left):

https://codereview.chromium.org/1159113003/diff/170001/Source/core/paint/Depr...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:428: if
(requiresPaintInvalidation && RuntimeEnabledFeatures::slimmingPaintEnabled()) {
On 2015/06/03 18:46:08, chrishtr wrote:
> Doesn't the culling code for scrolled content still try to be smart about the
> clip of the scroller?

Do you mean the 'if (paintInfo.rect.intersects()) return' checkings in the
painters, where the rect is passed as the clip rect of the scroller? If yes,
this doesn't actually matter:
- For non-slimming paint, the scrolled contents are covered by the invalidation
at line 435 below.
- For slimming paint, we'll just treat the scroller in the same way as
composited scrolling, though for now the clip rect is much smaller than
composited scrolling. The merge algorithm can handle appearing and disappeared
display items because of scrolling. This doesn't need any invalidation.

[Xianzhu, 2015-06-03 19:42:42]

https://codereview.chromium.org/1159113003/diff/170001/Source/core/paint/Depr...
File Source/core/paint/DeprecatedPaintLayerPainter.cpp (right):

https://codereview.chromium.org/1159113003/diff/170001/Source/core/paint/Depr...
Source/core/paint/DeprecatedPaintLayerPainter.cpp:648: if
(!paintingInfo.scrollOffsetAccumulation.isZero()) {
On 2015/06/03 19:06:59, Xianzhu wrote:
> On 2015/06/03 18:46:08, chrishtr wrote:
> > Can you explain more why we need to do this accumulation?
> > 
> > Also, test?
> 
> This is covered by existing tests. Previously the failures were hidden because
> we invalidate the whole scrolled tree.
> 
> I should have kept the comments at line 637-639 of the old code, and modified
it
> into:
>   // As a descendant of the root layer, m_paintLayer's painting is not
> controlled by the ScrollRecorders
>   // created by BlockPainter of the ancestor layers up to the root layer, so
we
> need to issue ScrollRecorder
>   // for this layer seperately, with the scroll offset (accumulated from the
> root layer to the parent of this
>   // layer), to get the same result as ScrollRecorder in BlockPainter.
> 
> Will insert this comment below.
> 

Done.

[chrishtr, 2015-06-03 21:07:54]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-06-03 21:07:54]

lgtm

[commit-bot: I haz the power, 2015-06-03 21:08:22]

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

[commit-bot: I haz the power, 2015-06-03 22:02:26]

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

[commit-bot: I haz the power, 2015-06-03 22:02:26]

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

[Xianzhu, 2015-06-03 22:07:48]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-06-03 22:08:10]

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

[commit-bot: I haz the power, 2015-06-03 22:54:34]

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

[commit-bot: I haz the power, 2015-06-03 22:54:35]

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

[Xianzhu, 2015-06-04 00:25:15]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-06-04 00:25:36]

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

[commit-bot: I haz the power, 2015-06-04 01:13:15]

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

[commit-bot: I haz the power, 2015-06-04 01:13:16]

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

[Xianzhu, 2015-06-04 02:03:14]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-06-04 02:03:44]

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

[commit-bot: I haz the power, 2015-06-04 03:24:15]

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