Separate repaint and layout requirements of StyleDifference (Step 3)

Source/core/rendering/RenderObject.cpp

Index: Source/core/rendering/RenderObject.cpp
diff --git a/Source/core/rendering/RenderObject.cpp b/Source/core/rendering/RenderObject.cpp
index f8e8c059436e6af372f63e27aec3799ba5562da8..3bb9fb57c841e71824ee634f8fac7a144d15c93e 100644
--- a/Source/core/rendering/RenderObject.cpp
+++ b/Source/core/rendering/RenderObject.cpp
@@ -1878,7 +1878,7 @@ StyleDifference RenderObject::adjustStyleDifference(StyleDifference diff, unsign
 
     // If opacity or zIndex changed, and the layer does not paint into its own separate backing, then we need to repaint (also
     // ignoring text nodes)
-    if ((contextSensitiveProperties & (ContextSensitivePropertyOpacity | ContextSensitivePropertyZIndex)) && !diff.needsLayout()) {
+    if (contextSensitiveProperties & (ContextSensitivePropertyOpacity | ContextSensitivePropertyZIndex)) {
         if (!isText() && (!hasLayer() || !toRenderLayerModelObject(this)->layer()->hasDirectReasonsForCompositing()))
             diff.setNeedsRepaintLayer();
         else
@@ -1886,7 +1886,7 @@ StyleDifference RenderObject::adjustStyleDifference(StyleDifference diff, unsign
     }
 
     // If filter changed, and the layer does not paint into its own separate backing or it paints with filters, then we need to repaint.
-    if ((contextSensitiveProperties & ContextSensitivePropertyFilter) && hasLayer() && !diff.needsLayout()) {
+    if ((contextSensitiveProperties & ContextSensitivePropertyFilter) && hasLayer()) {
         RenderLayer* layer = toRenderLayerModelObject(this)->layer();
         if (!layer->hasDirectReasonsForCompositing() || layer->paintsWithFilters())
             diff.setNeedsRepaintLayer();
@@ -1894,14 +1894,14 @@ StyleDifference RenderObject::adjustStyleDifference(StyleDifference diff, unsign
             diff.setNeedsRecompositeLayer();
     }
 
-    if ((contextSensitiveProperties & ContextSensitivePropertyTextOrColor) && !diff.needsRepaint() && !diff.needsLayout()
+    if ((contextSensitiveProperties & ContextSensitivePropertyTextOrColor) && !diff.needsRepaint()
         && hasImmediateNonWhitespaceTextChildOrPropertiesDependentOnColor())
         diff.setNeedsRepaintObject();
 
     // The answer to layerTypeRequired() for plugins, iframes, and canvas can change without the actual
     // style changing, since it depends on whether we decide to composite these elements. When the
     // layer status of one of these elements changes, we need to force a layout.
-    if (diff.hasNoChange() && style() && isLayerModelObject()) {
+    if (!diff.needsFullLayout() && style() && isLayerModelObject()) {
         bool requiresLayer = toRenderLayerModelObject(this)->layerTypeRequired() != NoLayer;
         if (hasLayer() != requiresLayer)
             diff.setNeedsFullLayout();
@@ -2026,7 +2026,10 @@ void RenderObject::setStyle(PassRefPtr<RenderStyle> style)
             toRenderBox(this)->updateLayerTransform();
     }
 
-    if (updatedDiff.needsRepaint()) {
+    // FIXME: For now if needsFullLayout is set, layout always triggers repaint,
+    // so don't repaint here to avoid duplicate repaint rects.
+    // Revisit the condition when we optimize layout not to always repaint.

[Julien - ping for review, 2014/04/30 18:02:26]

I don't think this comment makes sense per our discussion. If we have to force
an invalidation here, we should allow it and not care about layout generating
invalidations too.

Also a layout invalidation doesn't guarantee a FULL invalidation which will be
wrong in some cases.

[Xianzhu, 2014/04/30 18:40:15]

Updated the comments to mention this is temporary to keep the original
StyleDifference behavior and later we should force repaint here if
diff.needsRepaint().

+    if (updatedDiff.needsRepaint() && !updatedDiff.needsFullLayout()) {
         // Do a repaint with the new style now, e.g., for example if we go from
         // not having an outline to having an outline.
         repaint();
@@ -2066,8 +2069,12 @@ void RenderObject::styleWillChange(StyleDifference diff, const RenderStyle& newS
             }
         }
 
-        if (m_parent && diff.needsRepaintObjectOnly())
+        // FIXME: For now if needsFullLayout is set, layout always triggers repaint,
+        // so don't repaint here to avoid duplicate repaint rects.
+        // Revisit the condition when we optimize layout not to always repaint.
+        if (m_parent && diff.needsRepaintObject() && !diff.needsFullLayout())
             repaint();
+
         if (isFloating() && (m_style->floating() != newStyle.floating()))
             // For changes in float styles, we need to conceivably remove ourselves
             // from the floating objects list.
@@ -2083,7 +2090,7 @@ void RenderObject::styleWillChange(StyleDifference diff, const RenderStyle& newS
 
         // Clearing these bits is required to avoid leaving stale renderers.
         // FIXME: We shouldn't need that hack if our logic was totally correct.
-        if (diff.needsFullLayout() || diff.needsPositionedMovementLayout()) {
+        if (diff.needsLayout()) {
             setFloating(false);
             clearPositionedState();
         }