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 605b6b7788fead6ce250f8c8ccfd811a1c7fcd9e..cbb78864113215b7fdd7e2f37b83392260d18dcd 100644
--- a/Source/core/rendering/RenderObject.cpp
+++ b/Source/core/rendering/RenderObject.cpp
@@ -1901,6 +1901,7 @@ StyleDifference RenderObject::adjustStyleDifference(StyleDifference diff, unsign
                 diff.setNeedsFullLayout(); // FIXME: Do this for now since SimplifiedLayout cannot handle updating floating objects lists.
             else
                 diff.setNeedsSimplifiedLayout();
+            diff.setNeedsRepaintLayer();

[Julien - ping for review, 2014/04/25 15:43:04]

Is this line always needed? Even when we trigger a full layout above?

If so, I would like a comment to be added as to why we need it.

[Xianzhu, 2014/04/25 17:32:39]

The above change will disappear if rebased on trchen@'s
https://codereview.chromium.org/250773002/ (diff.setNeedsRepaintLayer() is the
only thing left).

For now this is related to the change in RenderLayerModelObject. I removed the
original code that recovered needsRepaintLayer from needsLayout on transform
change, so we need to set needsRepaintLayer separately.

[Julien - ping for review, 2014/04/28 17:40:28]

I am still requesting a code comment or waiting on trchen's change so that this
hack disappears.

[Xianzhu, 2014/04/28 18:11:50]

Done.

         } else {
             diff.setNeedsRecompositeLayer();
         }
@@ -1908,7 +1909,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
@@ -1916,7 +1917,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();
@@ -1924,14 +1925,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();
@@ -2039,7 +2040,10 @@ void RenderObject::setStyle(PassRefPtr<RenderStyle> style)
         }
     }
 
-    if (updatedDiff.needsRepaint()) {
+    // FIXME: For now if needsFullLayout() or needsSimplifiedLayout(), repaint is handled during layout(),

[Julien - ping for review, 2014/04/25 15:43:04]

What's the long term plan?

My view of the code is that we will keep invalidating during layout as it's the
right things to do and we definitely will need to a way to force an invalidation
from time to time regardless (e.g. a background-color change + a box size change
that ends up with the same box size due to other constrains)

[Xianzhu, 2014/04/25 17:32:39]

Yes.

In this CL, repaint-only needs are always set (e.g. background-color change),
but the changed condition below prevents duplicated repaint rects otherwise we
need to rebaseline many layout tests.

For layout-caused repaint, I haven't explored in depth. May have the following
choices:
a) let RenderStyle::visualInvalidationDiff() set all repaint needs that may be
caused by layout in advance;
b) let RenderObject::layout() only repaint if needed;
c) mix a) and b):
   - RenderStyle sets explicit repaint needs (e.g change of border-width will
set both needsFullLayout and needsRepaintObject)
   - For geometry-only changes, RenderStyle sets needsFullLayout only, and
RenderObject repaints if needed.

c) seems the most feasible way.

wdyt?

[Julien - ping for review, 2014/04/28 17:40:28]

border-width is a bad example as we already keep track of it in our border-box
so layout can detect these changes. A better example is transform where we have
lost all the required information by layout time and can't infer it anymore.
That means we have no other option than c) in the foreseeable future.
That's what I had in mind. However the new comment in the code is confusing and
should be improved.

[Xianzhu, 2014/04/28 18:11:50]

How about the updated comments?

+    // so don't repaint here to avoid duplicate repaints.
+    // Need to optimize repaint during layout() to reduce unnecessary repaints and duplicate code.
+    if (updatedDiff.needsRepaint() && !updatedDiff.needsFullLayout() && !updatedDiff.needsSimplifiedLayout()) {
         // 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();
@@ -2079,8 +2083,12 @@ void RenderObject::styleWillChange(StyleDifference diff, const RenderStyle& newS
             }
         }
 
-        if (m_parent && diff.needsRepaintObjectOnly())
+        // FIXME: For now if needsFullLayout() or needsSimplifiedLayout(), repaint is handled during layout(),
+        // so don't repaint here to avoid duplicate repaints.
+        // Need to optimize repaint during layout() to reduce unnecessary repaints and duplicate code.
+        if (m_parent && diff.needsRepaintObject() && !diff.needsFullLayout() && !diff.needsSimplifiedLayout())
             repaint();
+
         if (isFloating() && (m_style->floating() != newStyle.floating()))
             // For changes in float styles, we need to conceivably remove ourselves
             // from the floating objects list.