Include scroll offset in scrolling contents painted background rect invalidation.

Include scroll offset in scrolling contents painted background rect invalidation.

BUG=643741
TEST=paint/invalidation/composited-overflow-with-local-background.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[flackr, 2016-09-02 16:32:41]

Description was changed from

==========
Include scroll offset in scrolling contents painted background rect
invalidation.

BUG=None
TEST=paint/invalidation/composited-overflow-with-local-background.html
==========

to

==========
Include scroll offset in scrolling contents painted background rect
invalidation.

BUG=None
TEST=paint/invalidation/composited-overflow-with-local-background.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[flackr, 2016-09-02 16:35:11]

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

[flackr, 2016-09-02 16:35:11]

PTAL, this fixes the invalidation bug that led to
https://codereview.chromium.org/2264663002 being reverted. We need to include
the scroll offset just as we do when we draw the background.

[chrishtr, 2016-09-02 17:08:41]

Please attach a bug.

Also, why does it need to be adjusted for scroll in this case? We do not include
scroll offset
for composited scrolling.

[flackr, 2016-09-02 18:06:57]

Description was changed from

==========
Include scroll offset in scrolling contents painted background rect
invalidation.

BUG=None
TEST=paint/invalidation/composited-overflow-with-local-background.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Include scroll offset in scrolling contents painted background rect
invalidation.

BUG=643741
TEST=paint/invalidation/composited-overflow-with-local-background.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[flackr, 2016-09-02 18:18:12]

On 2016/09/02 at 17:08:41, chrishtr wrote:
> Please attach a bug.

Done.

> 
> Also, why does it need to be adjusted for scroll in this case? We do not
include scroll offset
> for composited scrolling.

We're adjusting for the same reason we include the scroll offset in
BoxPainter::paintBoxDecorationBackground(
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Box...
), because we're painting into the scrolling contents layer from context of the
composited scroll container.

I believe in the invalidation in scrolling contents case, the passed in rect
already includes this offset, but the rect in this case is for the overflow:
scroll element so it's not in the context of the scroll which is why we create a
new rect.

[flackr, 2016-09-02 18:30:34]

On 2016/09/02 at 18:18:12, flackr wrote:
> On 2016/09/02 at 17:08:41, chrishtr wrote:
> > Please attach a bug.
> 
> Done.
> 
> > 
> > Also, why does it need to be adjusted for scroll in this case? We do not
include scroll offset
> > for composited scrolling.
> 
> We're adjusting for the same reason we include the scroll offset in
BoxPainter::paintBoxDecorationBackground(
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Box...
), because we're painting into the scrolling contents layer from context of the
composited scroll container.
> 
> I believe in the invalidation in scrolling contents case, the passed in rect
already includes this offset, but the rect in this case is for the overflow:
scroll element so it's not in the context of the scroll which is why we create a
new rect.

Actually, looking at the tests I think this is incorrect. The reason seems to be
that we're issuing a paint invalidation in the scrolling contents layer of the
actual scroller. Relative to the scroller we are at -scrollOffset. There are no
other cases were we do this.

[chrishtr, 2016-09-02 18:45:32]

On 2016/09/02 at 18:18:12, flackr wrote:
> On 2016/09/02 at 17:08:41, chrishtr wrote:
> > Please attach a bug.
> 
> Done.
> 
> > 
> > Also, why does it need to be adjusted for scroll in this case? We do not
include scroll offset
> > for composited scrolling.
> 
> We're adjusting for the same reason we include the scroll offset in
BoxPainter::paintBoxDecorationBackground(
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Box...
), because we're painting into the scrolling contents layer from context of the
composited scroll container.
> 
> I believe in the invalidation in scrolling contents case, the passed in rect
already includes this offset, but the rect in this case is for the overflow:
scroll element so it's not in the context of the scroll which is why we create a
new rect.

It's not already included. The reason is that m_scrollingContentsLayer is
positioned in the same coordinate space as m_graphicsLayer (this was a change I
made a couple of months ago). Thus composited scrolling should not be
included in paint invalidation rects. As a result, I'm not sure why your test is
failing...


In the case of BoxPainter, the scroll offset is added because the scrolling
content paints in a scrolled space. For the composited path, this scroll offset
is exactly counteracted by this code:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...

so that you in effect paint at the same offset for every scroll offset, if the
content is composited. (And if it's not composited, scroll offsets must be
applied to get the right coordinates.)

[flackr, 2016-09-02 18:58:22]

On 2016/09/02 at 18:45:32, chrishtr wrote:
> On 2016/09/02 at 18:18:12, flackr wrote:
> > On 2016/09/02 at 17:08:41, chrishtr wrote:
> > > Please attach a bug.
> > 
> > Done.
> > 
> > > 
> > > Also, why does it need to be adjusted for scroll in this case? We do not
include scroll offset
> > > for composited scrolling.
> > 
> > We're adjusting for the same reason we include the scroll offset in
BoxPainter::paintBoxDecorationBackground(
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Box...
), because we're painting into the scrolling contents layer from context of the
composited scroll container.
> > 
> > I believe in the invalidation in scrolling contents case, the passed in rect
already includes this offset, but the rect in this case is for the overflow:
scroll element so it's not in the context of the scroll which is why we create a
new rect.
> 
> It's not already included. The reason is that m_scrollingContentsLayer is
positioned in the same coordinate space as m_graphicsLayer (this was a change I
made a couple of months ago). Thus composited scrolling should not be
> included in paint invalidation rects. As a result, I'm not sure why your test
is failing...
> 
> 
> In the case of BoxPainter, the scroll offset is added because the scrolling
content paints in a scrolled space. For the composited path, this scroll offset
is exactly counteracted by this code:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...
> 
> so that you in effect paint at the same offset for every scroll offset, if the
content is composited. (And if it's not composited, scroll offsets must be
applied to get the right coordinates.)

I'm not sure the exact cause, but the paint under-invalidation check thinks that
the scrolling contents background is being drawn at -scrollOffset, which doesn't
match the invalidation. Perhaps those paints need to be recorded differently
somehow?