Fixed-position repaint invalidations should be sent to the correct composited layer.

Fixed-position repaint invalidations should be sent to the correct composited layer.

When scrolling, there is logic in FrameView.cpp that sends
repaint invalidations for all viewport-constrained objects.
This logic was always sending repaints to the RenderView's
layer, and did not account for the scenario where the
non-composited fixed-position element paints into a different
composited layer.

This patch makes that code path recognize this situation,
and computes a correct repaint invalidation and sends it
to the correct composited layer so that non-composited
fixed position elements can be repainted corectly.

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

[shawnsingh, 2013-09-11 02:14:45]

Enne, seems like you have touched this code in the past, can you please review?

Thanks!

[enne (OOO), 2013-09-11 21:09:39]

https://codereview.chromium.org/23523046/diff/6001/Source/core/page/FrameView...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/23523046/diff/6001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:1372: // then we have to invlidate that enclosing
layer, not the RenderView.
typo: invalidate

https://codereview.chromium.org/23523046/diff/6001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:1373: updateRect.moveBy(scrollPosition());
Can you convince me that the scrollPosition is the only thing you need to look
at to get into the space of the parent enclosing composited layer?

[shawnsingh, 2013-09-11 23:10:38]

Thanks for review.   I'll fix the typo in a new patch soon.

https://codereview.chromium.org/23523046/diff/6001/Source/core/page/FrameView...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/23523046/diff/6001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:1373: updateRect.moveBy(scrollPosition());
On 2013/09/11 21:09:39, enne wrote:
> Can you convince me that the scrollPosition is the only thing you need to look
> at to get into the space of the parent enclosing composited layer?

The function that computes the update rect 8 lines above this -->
repaintRectIncludingNonCompositingDescendants() is by definition intended to
return the rect in the space of its repaint container.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Double-checking the implementation of containerForRepaint(), if compositing is
enabled it is exactly equivalent to enclosingCompositingLayer().
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[enne (OOO), 2013-09-12 00:07:53]

https://codereview.chromium.org/23523046/diff/6001/Source/core/page/FrameView...
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/23523046/diff/6001/Source/core/page/FrameView...
Source/core/page/FrameView.cpp:1373: updateRect.moveBy(scrollPosition());
On 2013/09/11 23:10:38, shawnsingh wrote:
> On 2013/09/11 21:09:39, enne wrote:
> > Can you convince me that the scrollPosition is the only thing you need to
look
> > at to get into the space of the parent enclosing composited layer?
> 
> The function that computes the update rect 8 lines above this -->
> repaintRectIncludingNonCompositingDescendants() is by definition intended to
> return the rect in the space of its repaint container.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Double-checking the implementation of containerForRepaint(), if compositing is
> enabled it is exactly equivalent to enclosingCompositingLayer().
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Hrm. In that case, why do you need to add scrollPosition() in as well? If this
updateRect was in the space of the repaint container, then I would expect that
it would only need to move by scrollDelta.

[shawnsingh, 2013-09-12 23:42:52]

> Hrm. In that case, why do you need to add scrollPosition() in as well? If this
> updateRect was in the space of the repaint container, then I would expect that
> it would only need to move by scrollDelta.

Yeah, that's an excellent question that led me down a rabbit hole =)  Indeed it
would be more correct if repaintRectIncludingNonCompositingDescendants() already
included the correctly scrolled rect.

However, as I tried to code it, I ran into some fundamental issues that need to
be addressed in separate refactors.  I posted a question to blink-dev about it,
subject titled "FrameView vs RenderView".   Basically, FrameView does the
document-level scrolling, and the render tree doesn't really know about it.  I
believe it is a layering violation to have RenderLayer reach into FrameView to
get that scroll position - and on top of that, there seems to be a bug in
RenderLayer::enclosingScrollableArea that incorrectly returns the document's
RenderLayer that has no scrolling, so there's no way to identify the scenario
where we need to get the FrameView's scrollPosition.

So, if reviewers are willing, I think the current patch is still an appropriate
fix for now, and there needs to be some hefty refactoring of scrolling logic and
roles of FrameView vs RenderView to make repaint rects correct more generally.

[enne (OOO), 2013-09-13 00:36:58]

Thanks for all the investigation.  lgtm!

[commit-bot: I haz the power, 2013-09-13 01:22:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/23523046/6001

[commit-bot: I haz the power, 2013-09-13 01:57:04]

Change committed as 157723