Delay scrollContents until the next paint

Delay scrollContents until the next paint
    
    Fix a ChickenAndEgg issue by delaying scroll until the
    next paint when the compositing layers are up to date.
    The amount scrolled but not painted is stored in
    ScrollView::m_pendingScrollDelta.
    
    Rearrange the code such that if there are fixed position
    elements, do the following before painting:
    
    1. Walk the tree and do updateLayerPosition
    2. Mark that we need to update compositing layers since
       layers may have changed as a result of scrolling if
         there are fixed elements.
    3. Update compositing layers.
    4. Invalidate fixed position elements.
    5. Update repaint rects for fixed position elements to
       reflect the new scroll position.
    
    Also, when calculating repaint rects for fixed elements,
    take into account both the scroll position and the
    scroll delta. This way we can invalidate the previous
    scroll position for the fixed element if it is
    promoted to a layer as a result of scrolling.

BUG=343766
BUG=343767
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170086

[ykyyip, 2014-03-08 00:10:44]

Can you PTAL at this patchset? If this approach looks OK to you, I think I can
remove FrameView::invalidateFixedElementsBeforeScrolling. I also looked into
allowing you to query stale compositing state in that function, but that quickly
exploded into "add a parameter to 20 functions" which seems gross.

Things to do for me:
- investigate some layout test failures
- try to understand RenderLayer::updateLayerPositionsAfterScroll better

[abarth-chromium, 2014-03-08 06:45:49]

> B=343766 
> BUG=343766

I think the "B" line isn't needed.  The BUG line should be enough.

[abarth-chromium, 2014-03-08 06:54:51]

I don't feel like I'm that much of an expert in this code.  Below are some
shallow comments about your patch.

https://codereview.chromium.org/191693002/diff/1/Source/core/frame/FrameView.h
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/191693002/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.h:150: virtual void
repaintFixedElementsAfterScrolling() OVERRIDE;
OVERRIDE?

https://codereview.chromium.org/191693002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/191693002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayer.cpp:505:
repainter().repaintIncludingNonCompositingDescendants(renderer()->containerForRepaint());
I'm not sure I understand what's changing here...

https://codereview.chromium.org/191693002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/191693002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderView.cpp:501: // If we're in the middle of
scrolling, also invalidate the previous scroll position.
I might say that we have a pending scroll delta rather than saying we're in the
middle of a scroll.

https://codereview.chromium.org/191693002/diff/1/Source/platform/scroll/Scrol...
File Source/platform/scroll/ScrollView.cpp (right):

https://codereview.chromium.org/191693002/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.cpp:279: //
invalidateFixedElementsBeforeScrolling(scrollDelta, rectToCopyOnScroll());
I couldn't find where we actually call this function.  Do you intend to comment
it out here?

https://codereview.chromium.org/191693002/diff/1/Source/platform/scroll/Scrol...
File Source/platform/scroll/ScrollView.h (right):

https://codereview.chromium.org/191693002/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.h:300: IntSize m_scrollDelta;
m_pendingScrollDelta?

The word "pending" might hint that we're waiting to apply this scroll delta.

[ykyyip, 2014-03-11 01:59:38]

https://codereview.chromium.org/191693002/diff/1/Source/core/frame/FrameView.h
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/191693002/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.h:150: virtual void
repaintFixedElementsAfterScrolling() OVERRIDE;
On 2014/03/08 06:54:52, abarth wrote:
> OVERRIDE?

Done.

https://codereview.chromium.org/191693002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/191693002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderView.cpp:501: // If we're in the middle of
scrolling, also invalidate the previous scroll position.
On 2014/03/08 06:54:52, abarth wrote:
> I might say that we have a pending scroll delta rather than saying we're in
the
> middle of a scroll.

Done.

https://codereview.chromium.org/191693002/diff/1/Source/platform/scroll/Scrol...
File Source/platform/scroll/ScrollView.cpp (right):

https://codereview.chromium.org/191693002/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.cpp:279: //
invalidateFixedElementsBeforeScrolling(scrollDelta, rectToCopyOnScroll());
On 2014/03/08 06:54:52, abarth wrote:
> I couldn't find where we actually call this function.  Do you intend to
comment
> it out here?

Yes, I actually don't need this function with the changes in
RenderView::computeRectForRepaint. I will remove this code.

https://codereview.chromium.org/191693002/diff/1/Source/platform/scroll/Scrol...
File Source/platform/scroll/ScrollView.h (right):

https://codereview.chromium.org/191693002/diff/1/Source/platform/scroll/Scrol...
Source/platform/scroll/ScrollView.h:300: IntSize m_scrollDelta;
On 2014/03/08 06:54:52, abarth wrote:
> m_pendingScrollDelta?
> 
> The word "pending" might hint that we're waiting to apply this scroll delta.

Done.

[Ian Vollick, 2014-03-11 02:20:05]

On 2014/03/11 01:59:38, ykyyip wrote:
> https://codereview.chromium.org/191693002/diff/1/Source/core/frame/FrameView.h
> File Source/core/frame/FrameView.h (right):
> 
>
https://codereview.chromium.org/191693002/diff/1/Source/core/frame/FrameView....
> Source/core/frame/FrameView.h:150: virtual void
> repaintFixedElementsAfterScrolling() OVERRIDE;
> On 2014/03/08 06:54:52, abarth wrote:
> > OVERRIDE?
> 
> Done.
> 
>
https://codereview.chromium.org/191693002/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderView.cpp (right):
> 
>
https://codereview.chromium.org/191693002/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderView.cpp:501: // If we're in the middle of
> scrolling, also invalidate the previous scroll position.
> On 2014/03/08 06:54:52, abarth wrote:
> > I might say that we have a pending scroll delta rather than saying we're in
> the
> > middle of a scroll.
> 
> Done.
> 
>
https://codereview.chromium.org/191693002/diff/1/Source/platform/scroll/Scrol...
> File Source/platform/scroll/ScrollView.cpp (right):
> 
>
https://codereview.chromium.org/191693002/diff/1/Source/platform/scroll/Scrol...
> Source/platform/scroll/ScrollView.cpp:279: //
> invalidateFixedElementsBeforeScrolling(scrollDelta, rectToCopyOnScroll());
> On 2014/03/08 06:54:52, abarth wrote:
> > I couldn't find where we actually call this function.  Do you intend to
> comment
> > it out here?
> 
> Yes, I actually don't need this function with the changes in
> RenderView::computeRectForRepaint. I will remove this code.
> 
>
https://codereview.chromium.org/191693002/diff/1/Source/platform/scroll/Scrol...
> File Source/platform/scroll/ScrollView.h (right):
> 
>
https://codereview.chromium.org/191693002/diff/1/Source/platform/scroll/Scrol...
> Source/platform/scroll/ScrollView.h:300: IntSize m_scrollDelta;
> On 2014/03/08 06:54:52, abarth wrote:
> > m_pendingScrollDelta?
> > 
> > The word "pending" might hint that we're waiting to apply this scroll delta.
> 
> Done.

I have to admit that I'm also not very familiar with the code you've removed in
FrameView, but IIUC it was intended to invalidate the old position of
non-compositing viewport constrained objects on the shift-and-blit fast path.

(Adam, if that's correct, does this fast path still exist? I recall you'd said
that you couldn't find the code for this anymore?)

I see that you've updated the code in RenderView to expand the invalidation
rect, but I don't understand how this is guaranteed to cover all the viewport
constrained stuff. Is the rect that's passed in guaranteed to be big enough
somehow?

[abarth-chromium, 2014-03-11 02:27:59]

> Adam, if that's correct, does this fast path still exist?

The shift and blit code path still exists, at least for the time being.  The
code appears to be used even in compositing mode as well.  A good test case for
that is android.com/kitkat

[ykyyip, 2014-03-12 23:36:42]

On 2014/03/11 02:20:05, Ian Vollick wrote:

> I see that you've updated the code in RenderView to expand the invalidation
> rect, but I don't understand how this is guaranteed to cover all the viewport
> constrained stuff. Is the rect that's passed in guaranteed to be big enough
> somehow?

I understand the code a lot better now and I've put back some of the original
code to invalidate viewport constrained objects. I've also updated some layout
tests to reflect that painting is delayed after scrolling. Can you PTAnotherL?

[Ian Vollick, 2014-03-21 00:14:14]

I'm sorry it's taken me so long to respond! This change looks VERY cool, but I
feel a bit out of my depth with the FrameView and painting changes.

I've left a few comments, but Adam, can you PTAL?

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1478: scrolledRect.move(-scrollDelta);
Whoa. Was this a bug in the previous code?

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1701: // Compositing layers may change after
scrolling.
If we kill overlap testing and land squashing, I think this will no longer the
case. If that's true, perhaps add a FIXME so we remember to remove this?

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1723: RenderLayer* layer =
toRenderBoxModelObject(renderer)->layer();
We don't need to do this if the layer is composited, right? (If it's guaranteed
that the layers won't be composited, perhaps add an assert?)

[esprehn, 2014-03-24 21:50:42]

bunch of nits, but this looks okay.

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/compositing/...
File LayoutTests/compositing/squashing/squash-above-fixed-1.html (right):

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/compositing/...
LayoutTests/compositing/squashing/squash-above-fixed-1.html:131: });
You need a catch() on the end to log exceptions. Otherwise the Promise will just
swallow them.

.catch(function(e) {
  console.error(e);
});

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/overflow-auto-in-overflow-auto-scrolled.html
(right):

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/overflow-auto-in-overflow-auto-scrolled.html:10:
function setupTest()
onload = function()

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/overflow-auto-in-overflow-auto-scrolled.html:35:
window.addEventListener("load", setupTest, false);
remove this.

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/scroll-fixed-layer-with-no-visible-content.html
(right):

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/scroll-fixed-layer-with-no-visible-content.html:13:
function scrollAndRepaint()
same.

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/scroll-in-fixed-layer.html (right):

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/scroll-in-fixed-layer.html:12: function
scrollAndRepaint()
Yeah can we just switch to onload = for all these?

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1478: scrolledRect.move(-scrollDelta);
On 2014/03/21 00:14:14, Ian Vollick wrote:
> Whoa. Was this a bug in the previous code?

Yeah can you explain why do you changed this?

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1702: if (m_nestedLayoutCount <= 1) {
How do we get into here in a nested layout?

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1724:
layer->repainter().computeRepaintRects(renderer->containerForRepaint());
Does this make all fixed position things repaint constantly? If they have their
own layers we don't need to do that.

https://codereview.chromium.org/191693002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/191693002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:499: if (flags & IsOverflowScroll && flags
& HasSeenAncestorWithOverflowClip && !m_canSkipRepaintRectsUpdateOnScroll) {
Add parens around & cases. This is hard to read, we should fix the ones at the
top too.

[ykyyip, 2014-03-24 21:56:10]

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1478: scrolledRect.move(-scrollDelta);
On 2014/03/21 00:14:14, Ian Vollick wrote:
> Whoa. Was this a bug in the previous code?

This changed because RenderView::computeRectForRepaint now returns the rect
without the pending scroll, so when an object gets promoted to a layer it
repaints the previous position in the former layer it painted into. It was OK
before because it returned the scrolled rect in the past.

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1701: // Compositing layers may change after
scrolling.
On 2014/03/21 00:14:14, Ian Vollick wrote:
> If we kill overlap testing and land squashing, I think this will no longer the
> case. If that's true, perhaps add a FIXME so we remember to remove this?

Done.

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1701: // Compositing layers may change after
scrolling.
On 2014/03/21 00:14:14, Ian Vollick wrote:
> If we kill overlap testing and land squashing, I think this will no longer the
> case. If that's true, perhaps add a FIXME so we remember to remove this?

I also wonder if we can revisit this code and make it simpler after we land
those things...

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1723: RenderLayer* layer =
toRenderBoxModelObject(renderer)->layer();
On 2014/03/21 00:14:14, Ian Vollick wrote:
> We don't need to do this if the layer is composited, right? (If it's
guaranteed
> that the layers won't be composited, perhaps add an assert?)

Yes, I think we don't need this if the layer has its own backing.

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1723: RenderLayer* layer =
toRenderBoxModelObject(renderer)->layer();
On 2014/03/21 00:14:14, Ian Vollick wrote:
> We don't need to do this if the layer is composited, right? (If it's
guaranteed
> that the layers won't be composited, perhaps add an assert?)

Done.

[ykyyip, 2014-03-24 22:15:02]

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/compositing/...
File LayoutTests/compositing/squashing/squash-above-fixed-1.html (right):

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/compositing/...
LayoutTests/compositing/squashing/squash-above-fixed-1.html:131: });
On 2014/03/24 21:50:43, esprehn wrote:
> You need a catch() on the end to log exceptions. Otherwise the Promise will
just
> swallow them.
> 
> .catch(function(e) {
>   console.error(e);
> });

Done.

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/overflow-auto-in-overflow-auto-scrolled.html
(right):

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/overflow-auto-in-overflow-auto-scrolled.html:10:
function setupTest()
On 2014/03/24 21:50:43, esprehn wrote:
> onload = function()

Done.

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/overflow-auto-in-overflow-auto-scrolled.html:35:
window.addEventListener("load", setupTest, false);
On 2014/03/24 21:50:43, esprehn wrote:
> remove this.

Done.

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/scroll-fixed-layer-with-no-visible-content.html
(right):

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/scroll-fixed-layer-with-no-visible-content.html:13:
function scrollAndRepaint()
On 2014/03/24 21:50:43, esprehn wrote:
> same.

Done.

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/scroll-in-fixed-layer.html (right):

https://codereview.chromium.org/191693002/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/scroll-in-fixed-layer.html:12: function
scrollAndRepaint()
On 2014/03/24 21:50:43, esprehn wrote:
> Yeah can we just switch to onload = for all these?

Done.

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1702: if (m_nestedLayoutCount <= 1) {
On 2014/03/24 21:50:43, esprehn wrote:
> How do we get into here in a nested layout?

I'm not sure. This code was moved from a different function that was called from
the same code and already had the check in it. I'll try removing the check for
nested layouts and see if anything breaks.

https://codereview.chromium.org/191693002/diff/60001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1724:
layer->repainter().computeRepaintRects(renderer->containerForRepaint());
On 2014/03/24 21:50:43, esprehn wrote:
> Does this make all fixed position things repaint constantly? If they have
their
> own layers we don't need to do that.

As far as I understand it, RenderLayerRepainter::computeRepaintRects() doesn't
cause a repaint. It just caches a rect for next call to repaintAfterLayout(), or
whoever asks for the rect next time.

https://codereview.chromium.org/191693002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/191693002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:499: if (flags & IsOverflowScroll && flags
& HasSeenAncestorWithOverflowClip && !m_canSkipRepaintRectsUpdateOnScroll) {
On 2014/03/24 21:50:43, esprehn wrote:
> Add parens around & cases. This is hard to read, we should fix the ones at the
> top too.

Done.

[esprehn, 2014-03-25 02:37:43]

lgtm, couple comments.

https://codereview.chromium.org/191693002/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/191693002/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1729: if
(!renderer->style()->hasViewportConstrainedPosition())
It seems really weird that m_viewportConstrainedObjects can contain things that
report false to hasViewportConstrainedPosition()

https://codereview.chromium.org/191693002/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1738: if (layer->compositingState() ==
PaintsIntoOwnBacking)
Fixed position elements are always composited now, is this code even needed?

[esprehn, 2014-03-25 02:42:20]

btw can you update the BUG= line to include the chicken and egg bug?

[ykyyip, 2014-03-25 23:01:59]

https://codereview.chromium.org/191693002/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/191693002/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1729: if
(!renderer->style()->hasViewportConstrainedPosition())
On 2014/03/25 02:37:44, esprehn wrote:
> It seems really weird that m_viewportConstrainedObjects can contain things
that
> report false to hasViewportConstrainedPosition()

I don't know why this check is here either. m_viewportConstrainedObjects is
modified in RenderLayerModelObject::styleDidChange(). I'll remove the check and
add an ASSERT instead.

https://codereview.chromium.org/191693002/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1738: if (layer->compositingState() ==
PaintsIntoOwnBacking)
On 2014/03/25 02:37:44, esprehn wrote:
> Fixed position elements are always composited now, is this code even needed?

Really, that code still gets run when there are fixed position objects, e.g.
android.com/kitkat

[ykyyip, 2014-03-26 18:53:37]

The CQ bit was checked by ykyyip@chromium.org

[commit-bot: I haz the power, 2014-03-26 18:53:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ykyyip@chromium.org/191693002/130001

[commit-bot: I haz the power, 2014-03-26 18:54:19]

Change committed as 170086