Separate repaint and layout requirements of StyleDifference (Step 4)

Source/core/rendering/RenderObject.h

Index: Source/core/rendering/RenderObject.h
diff --git a/Source/core/rendering/RenderObject.h b/Source/core/rendering/RenderObject.h
index bd27cc85d31a91811bfb79949d3c105b1ab14e10..b3da13ed7cd4a29e857f3acf24fbf51f0bcd6fa7 100644
--- a/Source/core/rendering/RenderObject.h
+++ b/Source/core/rendering/RenderObject.h
@@ -633,6 +633,7 @@ public:
     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);
@@ -646,6 +647,11 @@ public:
         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)
     {
@@ -1294,7 +1300,7 @@ 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();
@@ -1305,10 +1311,14 @@ inline void RenderObject::setNeedsLayout(MarkingBehavior markParents, SubtreeLay
     }
 }
 
+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);