Separate repaint and layout requirements of StyleDifference (Step 4)

Source/core/rendering/RenderObject.h

 /*
  * 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, 2012 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
@@ skipping 615 matching lines @@
     // Returns the object containing this one. Can be different from parent for positioned elements.
     // If repaintContainer and repaintContainerSkipped are not null, on return *repaintContainerSkipped
     // is true if the renderer returned is an ancestor of repaintContainer.
     RenderObject* container(const RenderLayerModelObject* repaintContainer = 0, bool* repaintContainerSkipped = 0) const;
 
     virtual RenderObject* hoverAncestor() const { return parent(); }
 
     Element* offsetParent() const;
 
     void markContainingBlocksForLayout(bool scheduleRelayout = true, RenderObject* newRoot = 0, SubtreeLayoutScope* = 0);
+    void setNeedsLayoutWithoutForcingFullRepaint(MarkingBehavior = MarkContainingBlockChain, SubtreeLayoutScope* = 0);

[Julien - ping for review, 2014/05/12 15:08:46]

This function is not used outside RenderObject so it could make sense to make it
private.

[Xianzhu, 2014/05/12 21:31:51]

We will need this function (in new opting-in-repaint code, it's just
setNeedsLayout()) from outside RenderObject in the following steps.

     void setNeedsLayout(MarkingBehavior = MarkContainingBlockChain, SubtreeLayoutScope* = 0);
     void clearNeedsLayout();
     void setChildNeedsLayout(MarkingBehavior = MarkContainingBlockChain, SubtreeLayoutScope* = 0);
     void setNeedsPositionedMovementLayout();
     void setPreferredLogicalWidthsDirty(MarkingBehavior = MarkContainingBlockChain);
     void clearPreferredLogicalWidthsDirty();
     void invalidateContainerPreferredLogicalWidths();
 
     void setNeedsLayoutAndPrefWidthsRecalc()
     {
         setNeedsLayout();
         setPreferredLogicalWidthsDirty();
     }
+    void setNeedsLayoutAndPrefWidthsRecalcWithoutForcingFullRepaint()

[Julien - ping for review, 2014/05/12 15:08:46]

I think we should make this the default, not an opt-out.

[Xianzhu, 2014/05/12 21:31:51]

Done.

+    {
+        setNeedsLayoutWithoutForcingFullRepaint();
+        setPreferredLogicalWidthsDirty();
+    }
 
     void setPositionState(EPosition position)
     {
         ASSERT((position != AbsolutePosition && position != FixedPosition) || isBox());
         m_bitfields.setPositionedState(position);
     }
     void clearPositionedState() { m_bitfields.clearPositionedState(); }
 
     void setFloating(bool isFloating) { m_bitfields.setFloating(isFloating); }
     void setInline(bool isInline) { m_bitfields.setIsInline(isInline); }
@@ skipping 628 matching lines @@
     if (isText() && !isBR())
         return false;
     return true;
 }
 
 inline bool RenderObject::isBeforeOrAfterContent() const
 {
     return isBeforeContent() || isAfterContent();
 }
 
-inline void RenderObject::setNeedsLayout(MarkingBehavior markParents, SubtreeLayoutScope* layouter)
+inline void RenderObject::setNeedsLayoutWithoutForcingFullRepaint(MarkingBehavior markParents, SubtreeLayoutScope* layouter)
 {
     ASSERT(!isSetNeedsLayoutForbidden());
     bool alreadyNeededLayout = m_bitfields.selfNeedsLayout();
     setSelfNeedsLayout(true);
     if (!alreadyNeededLayout) {
         if (markParents == MarkContainingBlockChain && (!layouter || layouter->root() != this))
             markContainingBlocksForLayout(true, 0, layouter);
     }
 }
 
+inline void RenderObject::setNeedsLayout(MarkingBehavior markParents, SubtreeLayoutScope* layouter)
+{
+    setNeedsLayoutWithoutForcingFullRepaint(markParents, layouter);
+    setShouldDoFullRepaintAfterLayout(true);
+}

[Julien - ping for review, 2014/05/07 00:24:32]

For me, this bit is total madness: we *don't* want each renderer marking to
trigger a full layout by default and I am surprised that the viewport doesn't
get invalidated all the time after this change.

Anything that does a several pass layout calls setNeedsLayout on its children.
You will force them to be invalidated, even if they don't move nor change size
:(

[Xianzhu, 2014/05/07 00:39:22]

Could you give more details about how this CL changes the current behavior? I
thought we do always fully repaint on selfNeedsLayout and this change just moves
the setShouldDoFullRepaintAfterLayout() call from clearNeedsLayout() into
setNeedsLayout().

[Julien - ping for review, 2014/05/08 21:03:49]

We have 2 ways to generate invalidations: incremental (invalidate the difference
between the old and the new rectangles) and full (invalidate the old and new
rects). Incremental is the default and setShouldDoFullRepaintAfterLayout is used
to force a full invalidation.

A full invalidation should only be done when we know we can't do incremental
ones (like if the object moved, ...). This code is basically forcing multi pass
layout into a full invalidation, which was not the old behavior and is just
conceptually wrong.

[Xianzhu, 2014/05/08 21:52:55]

Incremental invalidation is only possible when there is no selfNeedsLayout().
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).

The above change is equivalent with the original code, but just set
shouldDoFullRepaintAfterLayout eariler.

[Julien - ping for review, 2014/05/12 15:08:46]

OK, I see your point. It makes an assumption in the code very clear (that if we
set the layout flags, we assume that we are changing past the point of
incremental invalidation). This is unfortunate from my perspective that we keep
this bad assumption silent in setNeedsLayout: I would prefer if setNeedsLayout
would be renamed to setNeedsLayoutAndDoFullRepaint to make it obvious that we
are doing more than just layout. I expect this to be a significant refactoring
though so let's have a big FIXME there that the default shouldn't be silent.

+
 inline void RenderObject::clearNeedsLayout()
 {
-    if (!shouldDoFullRepaintAfterLayout())
-        setShouldDoFullRepaintAfterLayout(selfNeedsLayout());
     if (needsPositionedMovementLayoutOnly())
         setOnlyNeededPositionedMovementLayout(true);
     setLayoutDidGetCalled(true);
     setSelfNeedsLayout(false);
     setEverHadLayout(true);
     setPosChildNeedsLayout(false);
     setNeedsSimplifiedNormalFlowLayout(false);
     setNormalChildNeedsLayout(false);
     setNeedsPositionedMovementLayout(false);
     setAncestorLineBoxDirty(false);
@@ skipping 120 matching lines @@
 void showTree(const WebCore::RenderObject*);
 void showLineTree(const WebCore::RenderObject*);
 void showRenderTree(const WebCore::RenderObject* object1);
 // We don't make object2 an optional parameter so that showRenderTree
 // can be called from gdb easily.
 void showRenderTree(const WebCore::RenderObject* object1, const WebCore::RenderObject* object2);
 
 #endif
 
 #endif // RenderObject_h