Separate repaint and layout requirements of StyleDifference (Step 3)

Source/core/rendering/RenderObject.cpp

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Dirk Mueller (mueller@kde.org)
  *           (C) 2004 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2009 Google Inc. All rights reserved.
  * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 1883 matching lines @@
         // Text nodes share style with their parents but transforms don't apply to them,
         // hence the !isText() check.
         // FIXME: when transforms are taken into account for overflow, we will need to do a layout.
         if (!isText() && (!hasLayer() || !toRenderLayerModelObject(this)->layer()->hasDirectReasonsForCompositing())) {
             // We need to set at least SimplifiedLayout, but if PositionedMovementOnly is already set
             // then we actually need SimplifiedLayoutAndPositionedMovement.
             if (!hasLayer())
                 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();
         }
     }
 
     // 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
             diff.setNeedsRecompositeLayer();
     }
 
     // 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();
         else
             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();
     }
 
     // If we have no layer(), just treat a RepaintLayer hint as a normal Repaint.
     if (diff.needsRepaintLayer() && !hasLayer()) {
         diff.clearNeedsRepaint();
         diff.setNeedsRepaintObject();
     }
@@ skipping 87 matching lines @@
         if (updatedDiff.needsFullLayout()) {
             setNeedsLayoutAndPrefWidthsRecalc();
         } else {
             if (updatedDiff.needsPositionedMovementLayout())
                 setNeedsPositionedMovementLayout();
             if (updatedDiff.needsSimplifiedLayout())
                 setNeedsSimplifiedNormalFlowLayout();
         }
     }
 
-    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();
     }
 }
 
 static inline bool rendererHasBackground(const RenderObject* renderer)
 {
     return renderer && renderer->hasBackground();
 }
@@ skipping 19 matching lines @@
                 if (newStyle.visibility() == VISIBLE) {
                     layer->setHasVisibleContent();
                 } else if (layer->hasVisibleContent() && (this == layer->renderer() || layer->renderer()->style()->visibility() != VISIBLE)) {
                     layer->dirtyVisibleContentStatus();
                     if (diff.needsLayout())
                         repaint();
                 }
             }
         }
 
-        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.
             toRenderBox(this)->removeFloatingOrPositionedChildFromBlockLists();
         else if (isOutOfFlowPositioned() && (m_style->position() != newStyle.position()))
             // For changes in positioning styles, we need to conceivably remove ourselves
             // from the positioned objects list.
             toRenderBox(this)->removeFloatingOrPositionedChildFromBlockLists();
 
         s_affectsParentBlock = isFloatingOrOutOfFlowPositioned()
@@ skipping 1328 matching lines @@
 {
     if (object1) {
         const WebCore::RenderObject* root = object1;
         while (root->parent())
             root = root->parent();
         root->showRenderTreeAndMark(object1, "*", object2, "-", 0);
     }
 }
 
 #endif