Separate repaint and layout requirements of StyleDifference (Step 4)

Source/core/rendering/style/SVGRenderStyle.cpp

 /*
     Copyright (C) 2004, 2005, 2007 Nikolas Zimmermann <zimmermann@kde.org>
                   2004, 2005, 2010 Rob Buis <buis@kde.org>
     Copyright (C) Research In Motion Limited 2010. All rights reserved.
 
     Based on khtml code by:
     Copyright (C) 1999 Antti Koivisto (koivisto@kde.org)
     Copyright (C) 1999-2003 Lars Knoll (knoll@kde.org)
     Copyright (C) 2002-2003 Dirk Mueller (mueller@kde.org)
     Copyright (C) 2002 Apple Computer, Inc.
@@ skipping 104 matching lines @@
     svg_noninherited_flags = other->svg_noninherited_flags;
     stops = other->stops;
     misc = other->misc;
     resources = other->resources;
 }
 
 StyleDifference SVGRenderStyle::diff(const SVGRenderStyle* other) const
 {
     StyleDifference styleDifference;
 
-    if (diffNeedsLayout(other))
+    if (diffNeedsLayoutAndRepaint(other)) {
         styleDifference.setNeedsFullLayout();
-    if (diffNeedsRepaint(other))
         styleDifference.setNeedsRepaintObject();

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

We didn't do that before, why do we want to do this change?

Furthermore, SVG has its own way of invalidating that doesn't really use this
flag AFAICT.

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

I'm a bit confused. Doesn't RenderObject::setStyle() and
RenderObject::shouldDoFullRepaintAfterLayout() apply to SVG objects?

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

They both do. However this is forcing the object to repaint, just because it
needs layout seems like a conceptual mistake (layout doesn't imply calling
repaint() during style change as we can infer the old / new rectangles).

The only reason this is not generating more invalidation is because there is a
check for needsRepaintObject() && ! needsLayout() because calling repaint().

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

The code in RenderObject.cpp is changed to the following in this CL, no
!needsLayout() any more:

        if (m_parent && diff.needsRepaintObject()) {
            if (RuntimeEnabledFeatures::repaintAfterLayoutEnabled() &&
diff.needsLayout())
                setShouldDoFullRepaintAfterLayout(true);
            else
                repaint();
        }

+    } else if (diffNeedsRepaint(other)) {
+        styleDifference.setNeedsRepaintObject();
+    }
 
     return styleDifference;
 }
 
-bool SVGRenderStyle::diffNeedsLayout(const SVGRenderStyle* other) const
+bool SVGRenderStyle::diffNeedsLayoutAndRepaint(const SVGRenderStyle* other) const
 {
     // If resources change, we need a relayout, as the presence of resources influences the repaint rect.
     if (resources != other->resources)
         return true;
 
     // If markers change, we need a relayout, as marker boundaries are cached in RenderSVGPath.
     if (inheritedResources != other->inheritedResources)
         return true;
 
     // All text related properties influence layout.
@@ skipping 83 matching lines @@
 }
 
 EPaintOrderType SVGRenderStyle::paintOrderType(unsigned index) const
 {
     ASSERT(index < ((1 << kPaintOrderBitwidth)-1));
     unsigned pt = (paintOrder() >> (kPaintOrderBitwidth*index)) & ((1u << kPaintOrderBitwidth) - 1);
     return (EPaintOrderType)pt;
 }
 
 }