Don't include scroll offset in offsetFromLayoutObject for scrolling contents layers.

Don't include scroll offset in offsetFromLayoutObject for scrolling contents layers.

As a side-effect, this patch changes the system to no longer invalidate both the scrolling contents layer and m_graphicsLayer, since they now have different coordinate systems. (This has been a long-standing bug.)

BUG=529938, 416535
Committed: https://crrev.com/66797e1dd0c3366619ebcc6a5ce3f546055d9103
Cr-Commit-Position: refs/heads/master@{#396951}

[chrishtr, 2016-05-27 03:45:17]

Description was changed from

==========
none


none


none


none


none


none


none

BUG=
==========

to

==========
Don't include scroll offset in offsetFromLayoutObject for scrolling contents
layers.

To achieve this, also avoids invalidating both the scrolling contents layer and
m_graphicsLayer, since they now have different coordinate systems.

BUG=529938
==========

[chrishtr, 2016-05-27 03:46:34]

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

[chrishtr, 2016-05-27 03:46:35]

Sending early for feedback on the approach. Hopefully we can make this work.
Fixes
fast/repaint/overflow-move-after-scroll.html for the feature in the referenced
bug, because
otherwise the visual rect would incorrectly not be offset by scroll. Making it
offset by
scroll would be to inefficient/ugly since it would involve finding the
compositing scrolling
paint invalidation container and offsetting by its scroll offset.

[chrishtr, 2016-05-27 15:32:50]

On 2016/05/27 at 03:46:35, chrishtr wrote:
> Sending early for feedback on the approach. Hopefully we can make this work.
Fixes
> fast/repaint/overflow-move-after-scroll.html for the feature in the referenced
bug, because
> otherwise the visual rect would incorrectly not be offset by scroll. Making it
offset by
> scroll would be to inefficient/ugly since it would involve finding the
compositing scrolling
> paint invalidation container and offsetting by its scroll offset.

Only a small number of failing tests:

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

And several of those just need rebaselines.

[chrishtr, 2016-05-28 20:49:30]

Description was changed from

==========
Don't include scroll offset in offsetFromLayoutObject for scrolling contents
layers.

To achieve this, also avoids invalidating both the scrolling contents layer and
m_graphicsLayer, since they now have different coordinate systems.

BUG=529938
==========

to

==========
Don't include scroll offset in offsetFromLayoutObject for scrolling contents
layers.

As a side-effect, this patch changes the system to no longer invalidate both the
scrolling contents layer and m_graphicsLayer, since they now have different
coordinate systems. (This has been a long-standing bug.)

BUG=529938,416535
==========

[Xianzhu, 2016-05-31 16:06:20]

https://codereview.chromium.org/2019583002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2019583002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1226: bool
compositedScrolling = paintInvalidationContainer.usesCompositedScrolling() &&
&paintInvalidationContainer != this;
You can move this into
LayoutBoxModelObject::setBackingNeedsPaintInvalidationInRect() to avoid the
additional bool parameter.

[chrishtr, 2016-05-31 16:36:43]

Done.

https://codereview.chromium.org/2019583002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2019583002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1226: bool
compositedScrolling = paintInvalidationContainer.usesCompositedScrolling() &&
&paintInvalidationContainer != this;
On 2016/05/31 at 16:06:20, Xianzhu wrote:
> You can move this into
LayoutBoxModelObject::setBackingNeedsPaintInvalidationInRect() to avoid the
additional bool parameter.

Done.

[chrishtr, 2016-05-31 16:37:36]

FYI I tried to add a test for
https://bugs.chromium.org/p/chromium/issues/detail?id=416535#c22
but failed, because the internals methods available to set selection cause more
invalidation
than real selection. I'd like to do that as a followup.

[Xianzhu, 2016-05-31 16:45:53]

On 2016/05/31 16:37:36, chrishtr wrote:
> FYI I tried to add a test for
> https://bugs.chromium.org/p/chromium/issues/detail?id=416535#c22
> but failed, because the internals methods available to set selection cause
more
> invalidation
> than real selection. I'd like to do that as a followup.

What about crbug.com/429845?

[Xianzhu, 2016-05-31 16:46:27]

https://codereview.chromium.org/2019583002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (left):

https://codereview.chromium.org/2019583002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:434: }
?

https://codereview.chromium.org/2019583002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2019583002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1009:
// // change can be propagated to compositor.
Nit: //

[chrishtr, 2016-05-31 16:57:26]

On 2016/05/31 at 16:45:53, wangxianzhu wrote:
> On 2016/05/31 16:37:36, chrishtr wrote:
> > FYI I tried to add a test for
> > https://bugs.chromium.org/p/chromium/issues/detail?id=416535#c22
> > but failed, because the internals methods available to set selection cause
more
> > invalidation
> > than real selection. I'd like to do that as a followup.
> 
> What about crbug.com/429845?

Good point, working on that now.

[chrishtr, 2016-05-31 16:58:40]

https://codereview.chromium.org/2019583002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (left):

https://codereview.chromium.org/2019583002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:434: }
On 2016/05/31 at 16:46:27, Xianzhu wrote:
> ?

Done.

https://codereview.chromium.org/2019583002/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2019583002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1009:
// // change can be propagated to compositor.
On 2016/05/31 at 16:46:27, Xianzhu wrote:
> Nit: //

Done.

[chrishtr, 2016-05-31 18:14:16]

Added a test. Arguably, this test applies to both 567875 and 429845, since the
underlying
bug in both cases was that the foreground layer was not invalidated.

[chrishtr, 2016-05-31 18:14:47]

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/invalidation-on-foreground-graphics-layer-expected.txt
(right):

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/invalidation-on-foreground-graphics-layer-expected.txt:27:
"rect": [100, 100, 200, 200],
This is the scrolling contents layer.

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/invalidation-on-foreground-graphics-layer-expected.txt:43:
"object": "LayoutBlockFlow DIV id='target'",
This is the foreground layer.

[chrishtr, 2016-05-31 18:15:51]

I also updated the code for invalidating display item clients to match behavior
for actual
rect invalidations, so that debugging is not confused in the future as to the
mismatch.

[Xianzhu, 2016-05-31 18:27:04]

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/invalidation-on-foreground-graphics-layer.html
(right):

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/invalidation-on-foreground-graphics-layer.html:9:
target.style.background = "green";
Nit: too much indent

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/invalidation-on-foreground-graphics-layer.html:12:
runRepaintAndPixelTest();
Ditto.

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp
(right):

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp:95:
paintInvalidationContainer.invalidateDisplayItemClientOnBacking(*scrollbar,
PaintInvalidationScroll);
We should invalidate the scrollbar on container layer. It seems that we'll
invalidate the scrolling layer because a nullptr is passed as the layoutObject.
I think we should force passing the parameter.

[wkorman, 2016-05-31 18:34:15]

LGTM pending resolution of Xianzhu's comments of course.

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:415: static bool
compositedScrollsWithRespectTo(const LayoutObject* layoutObject, const
LayoutBoxModelObject& paintInvalidationContainer)
If layoutObject is never null can use const &?

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h (right):

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h:295: // The third
parameter should be equal to the DisplayItemClient if it happens to be a
LayoutObjert subclass. It is only used
LayoutObjert -> LayoutObject

[chrishtr, 2016-05-31 18:35:30]

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/invalidation-on-foreground-graphics-layer.html
(right):

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/invalidation-on-foreground-graphics-layer.html:9:
target.style.background = "green";
On 2016/05/31 at 18:27:04, Xianzhu wrote:
> Nit: too much indent

Fixed.

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/invalidation-on-foreground-graphics-layer.html:12:
runRepaintAndPixelTest();
On 2016/05/31 at 18:27:04, Xianzhu wrote:
> Ditto.

Done.

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp
(right):

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp:95:
paintInvalidationContainer.invalidateDisplayItemClientOnBacking(*scrollbar,
PaintInvalidationScroll);
On 2016/05/31 at 18:27:04, Xianzhu wrote:
> We should invalidate the scrollbar on container layer. It seems that we'll
invalidate the scrolling layer because a nullptr is passed as the layoutObject.
I think we should force passing the parameter.

But it has no effect except debugging no? Is this important enough to fix?

[Xianzhu, 2016-05-31 18:43:46]

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp
(right):

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintInvalidationCapableScrollableArea.cpp:95:
paintInvalidationContainer.invalidateDisplayItemClientOnBacking(*scrollbar,
PaintInvalidationScroll);
On 2016/05/31 18:35:29, chrishtr wrote:
> On 2016/05/31 at 18:27:04, Xianzhu wrote:
> > We should invalidate the scrollbar on container layer. It seems that we'll
> invalidate the scrolling layer because a nullptr is passed as the
layoutObject.
> I think we should force passing the parameter.
> 
> But it has no effect except debugging no? Is this important enough to fix?

Oh I forgot my short-cut change and thought we were still invalidate display
item clients through GraphicsLayers.

I think you can leave display item client as-is because it's not clear on which
layer the display item client is invalidated with the current short-cut display
item client invalidation method. I should change how we track display item
client invalidation (maybe one registry for a page or main frame).

[Xianzhu, 2016-05-31 18:47:55]

lgtm.

For display item client tracking, either the latest patch or as-is looks good to
me. I'll work on change the tracking method.

[chrishtr, 2016-05-31 18:51:54]

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:415: static bool
compositedScrollsWithRespectTo(const LayoutObject* layoutObject, const
LayoutBoxModelObject& paintInvalidationContainer)
On 2016/05/31 at 18:34:15, wkorman wrote:
> If layoutObject is never null can use const &?

It can be null, due to the default parameter of
invalidateDisplayItemClientOnBacking.

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h (right):

https://codereview.chromium.org/2019583002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h:295: // The third
parameter should be equal to the DisplayItemClient if it happens to be a
LayoutObjert subclass. It is only used
On 2016/05/31 at 18:34:15, wkorman wrote:
> LayoutObjert -> LayoutObject

Done.

[chrishtr, 2016-05-31 18:55:20]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-05-31 18:55:21]

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

[commit-bot: I haz the power, 2016-05-31 18:55:57]

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

[commit-bot: I haz the power, 2016-05-31 20:19:17]

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

[commit-bot: I haz the power, 2016-05-31 20:19:18]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chrishtr, 2016-05-31 20:23:50]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-05-31 20:23:51]

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

[commit-bot: I haz the power, 2016-05-31 20:24:21]

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

[commit-bot: I haz the power, 2016-05-31 22:21:38]

Committed patchset #17 (id:160002)

[commit-bot: I haz the power, 2016-05-31 22:23:16]

Description was changed from

==========
Don't include scroll offset in offsetFromLayoutObject for scrolling contents
layers.

As a side-effect, this patch changes the system to no longer invalidate both the
scrolling contents layer and m_graphicsLayer, since they now have different
coordinate systems. (This has been a long-standing bug.)

BUG=529938,416535
==========

to

==========
Don't include scroll offset in offsetFromLayoutObject for scrolling contents
layers.

As a side-effect, this patch changes the system to no longer invalidate both the
scrolling contents layer and m_graphicsLayer, since they now have different
coordinate systems. (This has been a long-standing bug.)

BUG=529938,416535

Committed: https://crrev.com/66797e1dd0c3366619ebcc6a5ce3f546055d9103
Cr-Commit-Position: refs/heads/master@{#396951}
==========

[commit-bot: I haz the power, 2016-05-31 22:23:18]

Patchset 17 (id:??) landed as
https://crrev.com/66797e1dd0c3366619ebcc6a5ce3f546055d9103
Cr-Commit-Position: refs/heads/master@{#396951}

[scottmg, 2016-06-08 00:09:38]

A revert of this CL (patchset #17 id:160002) has been created in
https://codereview.chromium.org/2046233002/ by scottmg@chromium.org.

The reason for reverting is: Speculative revert for very small likelihood of
causing elevated crash rates as detailed in
https://bugs.chromium.org/p/chromium/issues/detail?id=616399.

[scottmg, 2016-06-08 18:28:06]

On 2016/06/08 00:09:38, scottmg wrote:
> A revert of this CL (patchset #17 id:160002) has been created in
> https://codereview.chromium.org/2046233002/ by mailto:scottmg@chromium.org.
> 
> The reason for reverting is: Speculative revert for very small likelihood of
> causing elevated crash rates as detailed in
> https://bugs.chromium.org/p/chromium/issues/detail?id=616399.

(Wasn't reverted)