Avoid repainting non needs-self-layout container

Source/core/rendering/LayoutRepainter.cpp

 /*
  * Copyright (C) 2000 Lars Knoll (knoll@kde.org)
  *           (C) 2000 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Dirk Mueller (mueller@kde.org)
  *           (C) 2004 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Library General Public License for more details.
  *
  * You should have received a copy of the GNU Library General Public License
  * along with this library; see the file COPYING.LIB.  If not, write to
  * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
  * Boston, MA 02110-1301, USA.
  *
  */
 
 #include "config.h"
 #include "core/rendering/LayoutRepainter.h"
 
+#include "core/rendering/RenderBlockFlow.h"
+#include "core/rendering/RenderBox.h"
 #include "core/rendering/RenderObject.h"
 
 namespace WebCore {
 
 LayoutRepainter::LayoutRepainter(RenderObject& object, bool checkForRepaint)
     : m_object(object)
     , m_repaintContainer(0)
     , m_checkForRepaint(checkForRepaint)
+    , m_logicalWidth(-1)

[eseidel, 2014/02/24 21:45:45]

My instinct would have been to preface these with "old" or similar to indicate
that they're saved on construction, but maybe that's not the pattern for this
object?

[Julien - ping for review, 2014/03/06 21:03:02]

Good suggestion.

+    , m_logicalHeight(-1)

[eseidel, 2014/02/24 21:45:45]

I would use a const int kDidNotChange = -1; to make the later if checks clearer.

[Julien - ping for review, 2014/03/06 21:03:02]

Done.

+    , m_selfNeededLayout(object.selfNeedsLayout())
 {
     if (RuntimeEnabledFeatures::repaintAfterLayoutEnabled())
         return;
 
     if (m_checkForRepaint) {
         m_repaintContainer = m_object.containerForRepaint();
         m_oldBounds = m_object.clippedOverflowRectForRepaint(m_repaintContainer);
         m_oldOutlineBox = m_object.outlineBoundsForRepaint(m_repaintContainer);
+        if (m_object.isBox()) {
+            const RenderBox& box = *toRenderBox(&m_object);
+            m_logicalWidth = box.logicalWidth();
+            m_logicalHeight = box.logicalHeight();
+        }
     }
 }
 
 bool LayoutRepainter::repaintAfterLayout()
 {
     if (RuntimeEnabledFeatures::repaintAfterLayoutEnabled())
         return false;
 
-    return m_checkForRepaint ? m_object.repaintAfterLayoutIfNeeded(m_repaintContainer, m_object.selfNeedsLayout(), m_oldBounds, m_oldOutlineBox) : false;
+    if (!m_checkForRepaint)
+        return false;
+
+    if (canSkipRepaint())
+        return false;
+
+    return m_object.repaintAfterLayoutIfNeeded(m_repaintContainer, m_object.selfNeedsLayout(), m_oldBounds, m_oldOutlineBox);
+}
+
+bool LayoutRepainter::canSkipRepaint() const
+{
+    // If we needed layout, our content could have changed so we have to repaint.
+    if (m_selfNeededLayout)
+        return false;
+
+    if (m_logicalWidth == -1) {
+        ASSERT(m_logicalHeight == -1);
+        return false;
+    }
+
+    ASSERT(m_logicalWidth != -1);
+    ASSERT(m_logicalHeight != -1);
+
+    // We don't repaint box containers that are not marked for self-layout and didn't change size.
+    // This is valid because:
+    // - if their content changed, they would be marked for self-layout.
+    // - their descendant(s) that actually changed should repaint themselves correctly.
+    // FIXME: It should be possible to avoid repainting size changes but we have to be careful with
+    // some cases (e.g. gradient or repeated backgrounds).
+    const RenderBox& box = *toRenderBox(&m_object);
+    if (m_logicalWidth != box.logicalWidth() || m_logicalHeight != box.logicalHeight())
+        return false;
+
+    if (!m_object.isRenderBlockFlow())
+        return true;
+
+    const RenderBlockFlow& block = *toRenderBlockFlow(&box);
+    // FIXME: We currently disable this optimization for floats as they need to be
+    // correctly repainted by their (not marked for self-layout) block-flow container.
+    return !block.containsFloats();

[eseidel, 2014/02/24 21:45:45]

I might have abstracted this into a nicely named helper function.

return hasChildrenNeedingRepaintFromParent();

or something?

[Julien - ping for review, 2014/03/06 21:03:02]

Added hasChildrenNeedingRepaintFromContainingBlock.

 }
 
 } // namespace WebCore