Optimize layout/repaint on FrameView resize

Optimize layout/repaint on FrameView resize

Use the new setNeedsResizeLayout() (https://codereview.chromium.org/197283038/)
to schedule layout on FrameView resize.

BUG=258219
TEST=WebFrameTest.sizeChangeRepaint

[Xianzhu, 2014-03-19 17:45:52]

The difference between Patch Set 1 and Patch Set 2 is about
FrameView::needsFullRepaint(). In Patch Set 1, I still set needsFullRepaint on
logical width change, while Patch Set 2 removes FrameView::needsFullRepaint().
I'm not sure which is better and how this is related to repaint-after-layout.

[Xianzhu, 2014-03-19 17:46:41]

+dsinclair@ about repaint.

[dsinclair, 2014-03-20 10:41:41]

+leviw as he has been digging into LayoutState and can better comment on that
bit of the change.

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1008: repaintTree(rootForThisLayout);
This will break repaintAfterLayout as this is the only way that a repaint is
triggered. Without this we won't issue any invalidations.

I think, for repaint-after-layout, you want to change the m_doFullRepaint to
shouldDoFullRepaint to make this work.

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

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:965: setNeedsLayout(); // May override previous
setNeedsResizeLayout().
It seems strange to me that we're in FrameView::layout and we're telling the
frameview that it needs layout? Is this just to set the needsResizeLayout flag?
If so, it'd be better to set it directly to make this clearer.

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:974:
layer->updateLayerPositionsAfterLayout(renderView()->layer(),
updateLayerPositionFlags(layer, inSubtreeLayout, false));
I believe this is the only caller for updateLayerPositionFlags which means you
can just remove the 3rd param instead of setting it false.

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:267: const Vector<IntRect>& trackedRepaintRects()
const { return m_trackedRepaintRects; }
This seems unrelated to the current cl?

https://codereview.chromium.org/204813002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (left):

https://codereview.chromium.org/204813002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1625: return
!document().view()->needsFullRepaint() && !hasLayer() && everHadLayout();
Won't this end up triggering more invalidations? Previously if the view was
marked as doing a full repaint we'd skip issuing invalidations for children.
With this change, we'll always issue the child invalidations even if the full
renderView will be invalidated.

Should it be checking something like the needsRepaintLayout flag instead?

[Xianzhu, 2014-03-20 16:52:36]

On 2014/03/20 10:41:41, dsinclair wrote:
> +leviw as he has been digging into LayoutState and can better comment on that
> bit of the change.
> 

Thanks for review.

Realized I should only go the optimized path when needsResizeLayoutOnly(), and
keep m_doFullRepaint when setNeedsLayout() is requested.

[Xianzhu, 2014-03-20 17:47:42]

PTAL.

Patch Set 3 reverted Patch Set 2 about FrameView::m_doFullRepaint. It does child
invalidations instead of FrameView full repaint when needsResizeLayoutOnly() and
only logical height changed.

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1008: repaintTree(rootForThisLayout);
On 2014/03/20 10:41:41, dsinclair wrote:
> This will break repaintAfterLayout as this is the only way that a repaint is
> triggered. Without this we won't issue any invalidations.
> 
> I think, for repaint-after-layout, you want to change the m_doFullRepaint to
> shouldDoFullRepaint to make this work.

Previously when m_doFullRepaint is not true, wouldn't the descendants issue
invalidations if necessary?

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

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:965: setNeedsLayout(); // May override previous
setNeedsResizeLayout().
On 2014/03/20 10:41:41, dsinclair wrote:
> It seems strange to me that we're in FrameView::layout and we're telling the
> frameview that it needs layout? Is this just to set the needsResizeLayout
flag?
> If so, it'd be better to set it directly to make this clearer.

I thought if shouldDoFullLayout but only resizeLayout is scheduled, should force
a full layout. However this is not applicable in Patch Set 3.

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:267: const Vector<IntRect>& trackedRepaintRects()
const { return m_trackedRepaintRects; }
On 2014/03/20 10:41:41, dsinclair wrote:
> This seems unrelated to the current cl?

The new unit test needs it.

https://codereview.chromium.org/204813002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (left):

https://codereview.chromium.org/204813002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1625: return
!document().view()->needsFullRepaint() && !hasLayer() && everHadLayout();
On 2014/03/20 10:41:41, dsinclair wrote:
> Won't this end up triggering more invalidations? Previously if the view was
> marked as doing a full repaint we'd skip issuing invalidations for children.
> With this change, we'll always issue the child invalidations even if the full
> renderView will be invalidated.

It seems hard to weigh between full repaint and child repaints in
FrameView::layout(). In some cases we can be sure that full repaint is better
and mark full repaint. For other cases, perhaps we can fall back to full repaint
when number or area of child repaints exceeds some threshold during layout.

> 
> Should it be checking something like the needsRepaintLayout flag instead?

Good idea. I think this would help improve the situation. Would like to consider
it in some later change.

[dsinclair, 2014-03-21 09:39:27]

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1008: repaintTree(rootForThisLayout);
On 2014/03/20 17:47:43, Xianzhu wrote:
> On 2014/03/20 10:41:41, dsinclair wrote:
> > This will break repaintAfterLayout as this is the only way that a repaint is
> > triggered. Without this we won't issue any invalidations.
> > 
> > I think, for repaint-after-layout, you want to change the m_doFullRepaint to
> > shouldDoFullRepaint to make this work.
> 
> Previously when m_doFullRepaint is not true, wouldn't the descendants issue
> invalidations if necessary?

No, this was actually broken with the partial layout removal. If you sync your
repo you'll see the m_doFullRepaint is gone from this if statement. This is the
only place that invalidations should be issued in repaint-after-layout. We walk
down from the frame after layout has happened.

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

https://codereview.chromium.org/204813002/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:269: const Vector<IntRect>& trackedRepaintRects()
const { return m_trackedRepaintRects; }
Can this be protected or private and we friend in the test class?

[Xianzhu, 2014-03-21 16:17:08]

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/204813002/diff/70001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1008: repaintTree(rootForThisLayout);
On 2014/03/21 09:39:27, dsinclair wrote:
> On 2014/03/20 17:47:43, Xianzhu wrote:
> > On 2014/03/20 10:41:41, dsinclair wrote:
> > > This will break repaintAfterLayout as this is the only way that a repaint
is
> > > triggered. Without this we won't issue any invalidations.
> > > 
> > > I think, for repaint-after-layout, you want to change the m_doFullRepaint
to
> > > shouldDoFullRepaint to make this work.
> > 
> > Previously when m_doFullRepaint is not true, wouldn't the descendants issue
> > invalidations if necessary?
> 
> No, this was actually broken with the partial layout removal. If you sync your
> repo you'll see the m_doFullRepaint is gone from this if statement. This is
the
> only place that invalidations should be issued in repaint-after-layout. We
walk
> down from the frame after layout has happened.

Thanks for explanation. So Patch Set 3 is doing the right thing :)

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

https://codereview.chromium.org/204813002/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:269: const Vector<IntRect>& trackedRepaintRects()
const { return m_trackedRepaintRects; }
On 2014/03/21 09:39:27, dsinclair wrote:
> Can this be protected or private and we friend in the test class?

I think it is along with the other trackRepaints methods. It looks weird that it
is protected but others are public.

[eseidel, 2014-04-01 18:28:30]

So this "resize layout" would be a temporary fix until we taught
setNeedsLayout(true) to not always cause a repaint?  In general the time spent
actually laying out even a large complicated rendering tree is extremely small. 
It's been my experiance that it's more the consequences of layout which are
costly (like the Raster jobs triggered by the invalidations).

I'd like to understand if this is a temporary fix, or the permenent solution. 
If this is only temporary, do we agree on a longer term vision for making
self-layout's not unconditionally cause repaints?