Optimize layout/repaint on FrameView resize

Source/core/frame/FrameView.cpp

Index: Source/core/frame/FrameView.cpp
diff --git a/Source/core/frame/FrameView.cpp b/Source/core/frame/FrameView.cpp
index 9a97c1587c4718977f395f810cb98e89f1b5ebdf..73650ec7a15e2c5ac5fed1f50bb1c0879d0cb485 100644
--- a/Source/core/frame/FrameView.cpp
+++ b/Source/core/frame/FrameView.cpp
@@ -220,7 +220,6 @@ void FrameView::reset()
     m_contentIsOpaque = false;
     m_hasPendingLayout = false;
     m_layoutSubtreeRoot = 0;
-    m_doFullRepaint = false;
     m_layoutSchedulingEnabled = true;
     m_inPerformLayout = false;
     m_canRepaintDuringPerformLayout = false;
@@ -950,25 +949,20 @@ void FrameView::layout(bool allowSubtree)
 
             m_size = LayoutSize(layoutSize().width(), layoutSize().height());
 
-            if (oldSize != m_size) {
-                shouldDoFullLayout = true;
-                if (!m_firstLayout) {
-                    RenderBox* rootRenderer = document->documentElement() ? document->documentElement()->renderBox() : 0;
-                    RenderBox* bodyRenderer = rootRenderer && document->body() ? document->body()->renderBox() : 0;
-                    if (bodyRenderer && bodyRenderer->stretchesToViewport())
-                        bodyRenderer->setChildNeedsLayout();
-                    else if (rootRenderer && rootRenderer->stretchesToViewport())
-                        rootRenderer->setChildNeedsLayout();
-                }
+            if (oldSize != m_size && !m_firstLayout) {
+                RenderBox* rootRenderer = document->documentElement() ? document->documentElement()->renderBox() : 0;
+                RenderBox* bodyRenderer = rootRenderer && document->body() ? document->body()->renderBox() : 0;
+                if (bodyRenderer && bodyRenderer->stretchesToViewport())
+                    bodyRenderer->setChildNeedsLayout();
+                else if (rootRenderer && rootRenderer->stretchesToViewport())
+                    rootRenderer->setChildNeedsLayout();
             }
         }
 
         layer = rootForThisLayout->enclosingLayer();
 
-        // We need to set m_doFullRepaint before triggering layout as RenderObject::checkForRepaint
-        // checks the boolean to disable local repaints.
-        m_doFullRepaint |= shouldDoFullLayout;
-
+        if (shouldDoFullLayout)
+            setNeedsLayout(); // May override previous setNeedsResizeLayout().

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

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.

[Xianzhu, 2014/03/20 17:47:43]

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

         performLayout(rootForThisLayout, inSubtreeLayout);
 
         m_layoutSubtreeRoot = 0;
@@ -977,7 +971,7 @@ void FrameView::layout(bool allowSubtree)
     if (!inSubtreeLayout && !toRenderView(rootForThisLayout)->document().printing())
         adjustViewSize();
 
-    layer->updateLayerPositionsAfterLayout(renderView()->layer(), updateLayerPositionFlags(layer, inSubtreeLayout, m_doFullRepaint));
+    layer->updateLayerPositionsAfterLayout(renderView()->layer(), updateLayerPositionFlags(layer, inSubtreeLayout, false));

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

I believe this is the only caller for updateLayerPositionFlags which means you
can just remove the 3rd param instead of setting it false.

 
     updateCompositingLayersAfterLayout();
 
@@ -1000,22 +994,6 @@ void FrameView::layout(bool allowSubtree)
     if (m_nestedLayoutCount)
         return;
 
-    if (RuntimeEnabledFeatures::repaintAfterLayoutEnabled()) {
-        if (m_doFullRepaint)
-            renderView()->setShouldDoFullRepaintAfterLayout(true);
-
-        if (m_doFullRepaint)
-            repaintTree(rootForThisLayout);

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

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.

[Xianzhu, 2014/03/20 17:47:43]

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

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

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.

[Xianzhu, 2014/03/21 16:17:09]

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

-
-    } else if (m_doFullRepaint) {
-        // FIXME: This isn't really right, since the RenderView doesn't fully encompass
-        // the visibleContentRect(). It just happens to work out most of the time,
-        // since first layouts and printing don't have you scrolled anywhere.
-        renderView()->repaint();
-    }
-
-    m_doFullRepaint = false;
-
 #ifndef NDEBUG
     // Post-layout assert that nobody was re-marked as needing layout during layout.
     document->renderer()->assertSubtreeIsLaidOut();
@@ -1750,7 +1728,7 @@ void FrameView::repaintContentRectangle(const IntRect& r)
 void FrameView::contentsResized()
 {
     ScrollView::contentsResized();
-    setNeedsLayout();
+    setNeedsResizeLayout();
 }
 
 void FrameView::scrollbarExistenceDidChange()
@@ -1892,6 +1870,12 @@ void FrameView::setNeedsLayout()
         renderView->setNeedsLayout();
 }
 
+void FrameView::setNeedsResizeLayout()
+{
+    if (RenderView* renderView = this->renderView())
+        renderView->setNeedsResizeLayout();
+}
+
 bool FrameView::isTransparent() const
 {
     return m_isTransparent;