Separate repaint and layout requirements of StyleDifference (Step 1)

Source/core/rendering/style/StyleDifference.h

Index: Source/core/rendering/style/StyleDifference.h
diff --git a/Source/core/rendering/style/StyleDifference.h b/Source/core/rendering/style/StyleDifference.h
new file mode 100644
index 0000000000000000000000000000000000000000..678513e8a30854e78a958e419c8fd7784e8bd46e
--- /dev/null
+++ b/Source/core/rendering/style/StyleDifference.h
@@ -0,0 +1,159 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef StyleDifference_h
+#define StyleDifference_h
+
+#include "core/rendering/style/RenderStyleConstants.h"

[Julien - ping for review, 2014/04/18 20:35:15]

Once you've removed the legacy type shouldn't this go away (ie do we want a
FIXME for that?)

[Xianzhu, 2014/04/18 22:02:27]

Done.

+
+namespace WebCore {
+
+// The difference between two styles.

[Julien - ping for review, 2014/04/18 20:35:15]

Let's have more class comments:

// This class represents the difference between two computed styles
(RenderStyle).
// The difference can be of 3 types:
// - Layout difference
// - Repaint difference
// - Recompositing difference

[Xianzhu, 2014/04/18 22:02:27]

Done.

+class StyleDifference {
+public:
+    StyleDifference()
+        : m_needsRecompositeLayer(0)

[Julien - ping for review, 2014/04/18 20:35:15]

Let's use false here.

[Xianzhu, 2014/04/18 22:02:27]

Done.

+        , m_repaintType(NoRepaint)
+        , m_layoutType(NoLayout)
+        , m_contextSensitiveFlags(NotContextSensitive) { }
+
+    // Temporary constructor to convert StyleDifferenceLegacy to new StyleDifference.
+    // At this step, implicit requirements (e.g. StyleDifferenceLayout implies StyleDifferenceRepaint),
+    // is not handled by StyleDifference but need to be handled by the callers.
+    StyleDifference(StyleDifferenceLegacy legacyDiff, unsigned context)
+        : m_needsRecompositeLayer(0)
+        , m_repaintType(NoRepaint)
+        , m_layoutType(NoLayout)
+        , m_contextSensitiveFlags(NotContextSensitive)
+    {
+        switch (legacyDiff) {
+        case StyleDifferenceEqual:
+            break;
+        case StyleDifferenceRecompositeLayer:
+            m_needsRecompositeLayer = 1;
+            break;
+        case StyleDifferenceRepaint:
+            m_repaintType = RepaintObjectOnly;
+            break;
+        case StyleDifferenceRepaintLayer:
+            m_repaintType = RepaintLayer;
+            break;
+        case StyleDifferenceLayoutPositionedMovementOnly:
+            m_layoutType = PositionedMovementOnly;
+            break;
+        case StyleDifferenceLayout:
+            m_layoutType = FullLayout;
+            break;
+        }
+
+        if (context & ContextSensitivePropertyTransform)
+            m_contextSensitiveFlags |= TransformChanged;
+        if (context & ContextSensitivePropertyOpacity)
+            m_contextSensitiveFlags |= OpacityChanged;
+        if (context & ContextSensitivePropertyFilter)
+            m_contextSensitiveFlags |= FilterChanged;
+        if (context & ContextSensitivePropertyTextOrColor)
+            m_contextSensitiveFlags |= TextOrColorChanged;

[Julien - ping for review, 2014/04/18 20:35:15]

Why do we need an extra enum instead of reusing the existing one (including the
value names)?

[Xianzhu, 2014/04/18 22:02:27]

This change was reduced from my original whole big change (Patch Set 4) which
also removes ContextSensitiveStyleDifferenceProperty from
StyleDifferenceConstants.h.

Reverted changes about context sensitive properties.

+    }
+
+    // The two styles are identical.
+    bool hasNoChange() const
+    {
+        return !m_needsRecompositeLayer
+            && m_repaintType == NoRepaint
+            && m_layoutType == NoLayout
+            && !isContextSensitive();

[Julien - ping for review, 2014/04/18 20:35:15]

The context sensitive flags are really not differences per se so including them
looks suspicious.

They are just used to temporarily keep track of a specific change in order to
determine the correct course of action (usually to avoid having a style
difference as it's unneeded). Why do we include it here?

[Xianzhu, 2014/04/18 22:02:27]

Actually directly treating StyleDifferenceEqual as no-change while there is
context sensitive flag set caused a bug in SimplifyMarkupCommand. Some changed
properties (such as color and text) were ignored. The resolution was to also
check !context. Now still use the original method.

+    }
+
+    // The layer needs its position and transform updated. Implied by other repaint and layout flags.
+    bool needsRecompositeLayer() const { return m_needsRecompositeLayer || needsRepaint() || needsLayout(); }
+    void setNeedsRecompositeLayer() { m_needsRecompositeLayer = 1; }

[Julien - ping for review, 2014/04/18 20:35:15]

Please use the boolean 'true' here.

[Xianzhu, 2014/04/18 22:02:27]

Done.

+
+    bool needsRepaint() const { return m_repaintType != NoRepaint; }
+    void resetNeedsRepaint() { m_repaintType = NoRepaint; }

[Julien - ping for review, 2014/04/18 20:35:15]

See comment below.

[Xianzhu, 2014/04/18 22:02:27]

Done.

+
+    // The object just needs to be repainted.
+    bool needsRepaintObjectOnly() const { return m_repaintType == RepaintObjectOnly; }
+    void setNeedsRepaintObject()
+    {
+        if (!needsRepaintLayer())
+            m_repaintType = RepaintObjectOnly;
+    }
+
+    // The layer and its descendant layers need to be repainted.
+    bool needsRepaintLayer() const { return m_repaintType == RepaintLayer; }
+    void setNeedsRepaintLayer() { m_repaintType = RepaintLayer; }
+
+    bool needsLayout() const { return m_layoutType != NoLayout; }
+    void resetNeedsLayout() { m_layoutType = NoLayout; }

[Julien - ping for review, 2014/04/18 20:35:15]

These getters are named needsLayout / clearNeedsLayout on RenderObject so I
think we should be consistent.

[Xianzhu, 2014/04/18 22:02:27]

Done.

+
+    // The position of this positioned object has been updated.

[Julien - ping for review, 2014/04/18 20:35:15]

I would change position to "offset" as this is what we usually use for that
(position having a meaning in CSS).

[Xianzhu, 2014/04/18 22:02:27]

Done.

+    bool needsPositionedMovementLayoutOnly() const { return m_layoutType == PositionedMovementOnly; }
+    void setNeedsPositionedMovementLayout()
+    {
+        if (!needsFullLayout())
+            m_layoutType = PositionedMovementOnly;
+        // FIXME: This is temporary to keep the StyleDifferenceLegacy behavior.
+        m_repaintType = NoRepaint;
+    }
+
+    // A full layout is required.

[Julien - ping for review, 2014/04/18 20:35:15]

Not really a helpful comment. A "full layout" is kind of a generic name that
doesn't mean much to the reader.

[Xianzhu, 2014/04/18 22:02:27]

Removed. The function name seems descriptive enough.

+    bool needsFullLayout() const { return m_layoutType == FullLayout; }
+    void setNeedsFullLayout()
+    {
+        m_layoutType = FullLayout;
+        // FIXME: This is temporary to keep the StyleDifferenceLegacy behavior.
+        m_repaintType = NoRepaint;
+    }
+
+    // When some style properties change, different amounts of work have to be done depending on
+    // context (e.g. whether the property is changing on an element which has a compositing layer).
+    // Caller of RenderStyle::visualInvalidationDiff() needs to check the context and determine
+    // what work is needed.

[Julien - ping for review, 2014/04/18 20:35:15]

I really think this should be a FIXME: these flags should be internal and not
exposed. What should be important is the final answer as to what was modified,
not these intermediate details.

[Xianzhu, 2014/04/18 22:02:27]

Reverted changes about context sensitive properties.

+    bool isContextSensitive() const { return m_contextSensitiveFlags != NotContextSensitive; }
+    void resetContextSensitiveFlags() { m_contextSensitiveFlags = NotContextSensitive; }
+
+    bool transformChanged() const { return m_contextSensitiveFlags & TransformChanged; }
+    void setTransformChanged() { m_contextSensitiveFlags |= TransformChanged; }
+
+    bool opacityChanged() const { return m_contextSensitiveFlags & OpacityChanged; }

[Julien - ping for review, 2014/04/18 20:35:15]

Again those getters should have a conjugated verb: opacityDidChange() /
transformDidChange() / ...

+    void setOpacityChanged() { m_contextSensitiveFlags |= OpacityChanged; }
+
+    bool filterChanged() const { return m_contextSensitiveFlags & FilterChanged; }
+    void setFilterChanged() { m_contextSensitiveFlags |= FilterChanged; }

[Julien - ping for review, 2014/04/18 20:35:15]

The setters for these properties are unused so we shouldn't add them.

+
+    // Text decoration or color changed. Needs repaint if the object contains text or properties dependent or color (e.g., border or outline).
+    bool textOrColorChanged() const { return m_contextSensitiveFlags & TextOrColorChanged; }
+    void setTextOrColorChanged() { m_contextSensitiveFlags |= TextOrColorChanged; }
+
+private:
+    unsigned m_needsRecompositeLayer : 1;
+
+    enum RepaintType {
+        NoRepaint,

[Julien - ping for review, 2014/04/18 20:35:15]

We should probably make these start at 0, it would ensure that !m_repaintType
returns what you expect (do I need no repaint?)

[Xianzhu, 2014/04/18 22:02:27]

Done.

+        RepaintObjectOnly,
+        RepaintLayer
+    };
+    unsigned m_repaintType : 2;
+
+    enum LayoutType {
+        NoLayout,

[Julien - ping for review, 2014/04/18 20:35:15]

Same comment.

[Xianzhu, 2014/04/18 22:02:27]

Done.

+        PositionedMovementOnly,
+        FullLayout
+    };
+    unsigned m_layoutType : 2;
+
+    enum ContextSensitiveFlags {
+        NotContextSensitive = 0,
+        TransformChanged = 1 << 0,
+        OpacityChanged = 1 << 1,
+        FilterChanged = 1 << 2,
+        TextOrColorChanged = 1 << 3
+    };
+    unsigned m_contextSensitiveFlags : 4;

[Julien - ping for review, 2014/04/18 20:35:15]

I think this is my biggest gripe with the change (and the reason I mentioned
splitting it): I don't think we should add it to the class as it is written as
it's orthogonal and doesn't give us much. I could see a future where we move the
diff function here and it would make sense to move this enum too but for now, it
seems very artificial to me.

[Xianzhu, 2014/04/18 22:02:27]

Agreed. Reverted the changes about context sensitive properties.

+};
+
+} // namespace WebCore
+
+#endif // StyleDifference_h