Optimize layout/repaint on FrameView resize

Source/core/frame/FrameView.cpp

 /*
  * Copyright (C) 1998, 1999 Torben Weis <weis@kde.org>
  *                     1999 Lars Knoll <knoll@kde.org>
  *                     1999 Antti Koivisto <koivisto@kde.org>
  *                     2000 Dirk Mueller <mueller@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
  *           (C) 2006 Graham Dennis (graham.dennis@gmail.com)
  *           (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
@@ skipping 202 matching lines @@
         renderer->setWidget(nullptr);
 }
 
 void FrameView::reset()
 {
     m_cannotBlitToWindow = false;
     m_isOverlapped = false;
     m_contentIsOpaque = false;
     m_hasPendingLayout = false;
     m_layoutSubtreeRoot = 0;
-    m_doFullRepaint = false;
     m_layoutSchedulingEnabled = true;
     m_inPerformLayout = false;
     m_canRepaintDuringPerformLayout = false;
     m_doingPreLayoutStyleUpdate = false;
     m_inSynchronousPostLayout = false;
     m_layoutCount = 0;
     m_nestedLayoutCount = 0;
     m_postLayoutTasksTimer.stop();
     m_updateWidgetsTimer.stop();
     m_firstLayout = true;
@@ skipping 709 matching lines @@
                 setScrollbarModes(hMode, vMode);
                 setScrollbarsSuppressed(false, true);
             } else if (hMode != currentHMode || vMode != currentVMode) {
                 setScrollbarModes(hMode, vMode);
             }
 
             LayoutSize oldSize = m_size;
 
             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;
     } // Reset m_layoutSchedulingEnabled to its previous value.
 
     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();
 
     m_layoutCount++;
 
     if (AXObjectCache* cache = rootForThisLayout->document().existingAXObjectCache())
         cache->postNotification(rootForThisLayout, AXObjectCache::AXLayoutComplete, true);
     updateAnnotatedRegions();
 
     ASSERT(!rootForThisLayout->needsLayout());
 
     if (document->hasListenerType(Document::OVERFLOWCHANGED_LISTENER))
         updateOverflowStatus(layoutSize().width() < contentsWidth(), layoutSize().height() < contentsHeight());
 
     scheduleOrPerformPostLayoutTasks();
 
     InspectorInstrumentation::didLayout(cookie, rootForThisLayout);
 
     m_nestedLayoutCount--;
     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();
 #endif
 
     // FIXME: It should be not possible to remove the FrameView from the frame/page during layout
     // however m_inPerformLayout is not set for most of this function, so none of our RELEASE_ASSERTS
     // in LocalFrame/Page will fire. One of the post-layout tasks is disconnecting the LocalFrame from
     // the page in fast/frames/crash-remove-iframe-during-object-beforeload-2.html
     // necessitating this check here.
@@ skipping 714 matching lines @@
         repaintRect.move(-scrollOffset());
         m_trackedRepaintRects.append(repaintRect);
     }
 
     ScrollView::repaintContentRectangle(r);
 }
 
 void FrameView::contentsResized()
 {
     ScrollView::contentsResized();
-    setNeedsLayout();
+    setNeedsResizeLayout();
 }
 
 void FrameView::scrollbarExistenceDidChange()
 {
     // We check to make sure the view is attached to a frame() as this method can
     // be triggered before the view is attached by LocalFrame::createView(...) setting
     // various values such as setScrollBarModes(...) for example.  An ASSERT is
     // triggered when a view is layout before being attached to a frame().
     if (!frame().view())
         return;
@@ skipping 121 matching lines @@
         || (renderView && renderView->needsLayout())
         || isSubtreeLayout();
 }
 
 void FrameView::setNeedsLayout()
 {
     if (RenderView* renderView = this->renderView())
         renderView->setNeedsLayout();
 }
 
+void FrameView::setNeedsResizeLayout()
+{
+    if (RenderView* renderView = this->renderView())
+        renderView->setNeedsResizeLayout();
+}
+
 bool FrameView::isTransparent() const
 {
     return m_isTransparent;
 }
 
 void FrameView::setTransparent(bool isTransparent)
 {
     m_isTransparent = isTransparent;
     DisableCompositingQueryAsserts disabler;
     if (renderView() && renderView()->layer()->hasCompositedLayerMapping())
@@ skipping 1312 matching lines @@
 void FrameView::willRemoveScrollbar(Scrollbar* scrollbar, ScrollbarOrientation orientation)
 {
     ScrollableArea::willRemoveScrollbar(scrollbar, orientation);
     if (AXObjectCache* cache = axObjectCache()) {
         cache->remove(scrollbar);
         cache->handleScrollbarUpdate(this);
     }
 }
 
 } // namespace WebCore