CSSValue Immediates: Change CSSValue to an object instead of a pointer

Source/core/css/CSSValue.h

Index: Source/core/css/CSSValue.h
diff --git a/Source/core/css/CSSValue.h b/Source/core/css/CSSValue.h
index 1c0f64f7244171c87332c0632fc4e4e7b64a6aee..c51eb2f873038254df2077d9476dd555c90ae8ad 100644
--- a/Source/core/css/CSSValue.h
+++ b/Source/core/css/CSSValue.h
@@ -22,218 +22,311 @@
 #define CSSValue_h
 
 #include "core/CoreExport.h"
+#include "core/css/CSSValueObject.h"
 #include "platform/heap/Handle.h"
-#include "platform/weborigin/KURL.h"
-#include "wtf/HashMap.h"
-#include "wtf/ListHashSet.h"
 #include "wtf/RefCounted.h"
 #include "wtf/RefPtr.h"
 
 namespace blink {
 
-class CORE_EXPORT CSSValue : public RefCountedWillBeGarbageCollectedFinalized<CSSValue> {
+// A non-nullable container for CSSValueObject. Takes ownership of the passed
+// CSSValueObject.
+class CORE_EXPORT CSSValue {
+    ALLOW_ONLY_INLINE_ALLOCATION();
+    friend class NullableCSSValue;
 public:
-#if ENABLE(OILPAN)
-    // Override operator new to allocate CSSValue subtype objects onto
-    // a dedicated heap.
-    GC_PLUGIN_IGNORE("crbug.com/443854")
-    void* operator new(size_t size)
-    {
-        return allocateObject(size);
-    }
-    static void* allocateObject(size_t size)
-    {
-        ThreadState* state = ThreadStateFor<ThreadingTrait<CSSValue>::Affinity>::state();
-        return Heap::allocateOnHeapIndex(state, size, CSSValueHeapIndex, GCInfoTrait<CSSValue>::index());
-    }
-#else
-    // Override RefCounted's deref() to ensure operator delete is called on
-    // the appropriate subclass type.
-    void deref()
-    {
-        if (derefBase())
-            destroy();
-    }
-#endif // !ENABLE(OILPAN)
-
-    String cssText() const;
-
-    bool isPrimitiveValue() const { return m_classType == PrimitiveClass; }
-    bool isValueList() const { return m_classType >= ValueListClass; }
-
-    bool isBaseValueList() const { return m_classType == ValueListClass; }
-
-    bool isBorderImageSliceValue() const { return m_classType == BorderImageSliceClass; }
-    bool isCanvasValue() const { return m_classType == CanvasClass; }
-    bool isCursorImageValue() const { return m_classType == CursorImageClass; }
-    bool isCrossfadeValue() const { return m_classType == CrossfadeClass; }
-    bool isFontFeatureValue() const { return m_classType == FontFeatureClass; }
-    bool isFontFaceSrcValue() const { return m_classType == FontFaceSrcClass; }
-    bool isFunctionValue() const { return m_classType == FunctionClass; }
-    bool isImageGeneratorValue() const { return m_classType >= CanvasClass && m_classType <= RadialGradientClass; }
-    bool isGradientValue() const { return m_classType >= LinearGradientClass && m_classType <= RadialGradientClass; }
-    bool isImageSetValue() const { return m_classType == ImageSetClass; }
-    bool isImageValue() const { return m_classType == ImageClass; }
-    bool isImplicitInitialValue() const;
-    bool isInheritedValue() const { return m_classType == InheritedClass; }
-    bool isInitialValue() const { return m_classType == InitialClass; }
-    bool isUnsetValue() const { return m_classType == UnsetClass; }
-    bool isCSSWideKeyword() const { return m_classType >= InheritedClass && m_classType <= UnsetClass; }
-    bool isLinearGradientValue() const { return m_classType == LinearGradientClass; }
-    bool isPathValue() const { return m_classType == PathClass; }
-    bool isRadialGradientValue() const { return m_classType == RadialGradientClass; }
-    bool isReflectValue() const { return m_classType == ReflectClass; }
-    bool isShadowValue() const { return m_classType == ShadowClass; }
-    bool isCubicBezierTimingFunctionValue() const { return m_classType == CubicBezierTimingFunctionClass; }
-    bool isStepsTimingFunctionValue() const { return m_classType == StepsTimingFunctionClass; }
-    bool isLineBoxContainValue() const { return m_classType == LineBoxContainClass; }
-    bool isCalcValue() const {return m_classType == CalculationClass; }
-    bool isGridTemplateAreasValue() const { return m_classType == GridTemplateAreasClass; }
-    bool isSVGDocumentValue() const { return m_classType == CSSSVGDocumentClass; }
-    bool isContentDistributionValue() const { return m_classType == CSSContentDistributionClass; }
-    bool isUnicodeRangeValue() const { return m_classType == UnicodeRangeClass; }
-    bool isGridLineNamesValue() const { return m_classType == GridLineNamesClass; }
-
-    bool hasFailedOrCanceledSubresources() const;
-
-    bool equals(const CSSValue&) const;
-
-    void finalizeGarbageCollectedObject();
-    DEFINE_INLINE_TRACE_AFTER_DISPATCH() { }
+    // Not explicit to allow for casting.
+    CSSValue(const CSSValueObject& cssValueObject)
+        : m_data(const_cast<CSSValueObject*>(&cssValueObject))
+    {
+        ASSERT(m_data);
+        ref();
+    }
+
+    // TODO(sashab): Remove these; construction should only be available with
+    // CSSValueObject&.
+    template<class T> CSSValue(PassRefPtrWillBeRawPtr<T> cssValueObject)
+        : m_data(cssValueObject.leakRef())
+    {
+        static_assert(WTF::IsSubclass<T, CSSValueObject>::value, "Parameter must be a container for CSSValueObject");
+        ASSERT(m_data);
+    }
+
+#if !ENABLE(OILPAN)
+    template<class T> CSSValue(RefPtrWillBeRawPtr<T> cssValueObject)
+        : m_data(cssValueObject.release().leakRef())
+    {
+        static_assert(WTF::IsSubclass<T, CSSValueObject>::value, "Parameter must be a container for CSSValueObject");
+        ASSERT(m_data);
+    }
+#endif
+
+    CSSValue(const CSSValue& cssValue)
+        : m_data(cssValue.m_data)
+    {
+        ref();
+    }
+
+    ~CSSValue()
+    {
+        deref();
+    }
+
+    CSSValue& operator=(const CSSValue& rhs)
+    {
+        rhs.ref();
+        deref();
+        m_data = rhs.m_data;
+        return *this;
+    }
+
+    bool operator==(const CSSValue& other) const
+    {
+        return m_data->equals(*other.m_data);
+    }
+
+    // TODO: Remove this and update callsites to use operator== instead.
+    bool equals(const CSSValue& other) const
+    {
+        return m_data->equals(*other.m_data);
+    }
+
+    bool ptrEquals(const CSSValue& other) const
+    {
+        return m_data == other.m_data;
+    }
+
+    // Methods that proxy to CSSValueObject.
+    String cssText() const { return m_data->cssText(); }
+    bool hasFailedOrCanceledSubresources() const { return m_data->hasFailedOrCanceledSubresources(); };
+    bool isPrimitiveValue() const { return m_data->isPrimitiveValue(); }
+    bool isValueList() const { return m_data->isValueList(); }
+    bool isBaseValueList() const { return m_data->isBaseValueList(); }
+    bool isBorderImageSliceValue() const { return m_data->isBorderImageSliceValue(); }
+    bool isCanvasValue() const { return m_data->isCanvasValue(); }
+    bool isCursorImageValue() const { return m_data->isCursorImageValue(); }
+    bool isCrossfadeValue() const { return m_data->isCrossfadeValue(); }
+    bool isFontFeatureValue() const { return m_data->isFontFeatureValue(); }
+    bool isFontFaceSrcValue() const { return m_data->isFontFaceSrcValue(); }
+    bool isFunctionValue() const { return m_data->isFunctionValue(); }
+    bool isImageGeneratorValue() const { return m_data->isImageGeneratorValue(); }
+    bool isGradientValue() const { return m_data->isGradientValue(); }
+    bool isImageSetValue() const { return m_data->isImageSetValue(); }
+    bool isImageValue() const { return m_data->isImageValue(); }
+    bool isImplicitInitialValue() const { return m_data->isImplicitInitialValue(); };
+    bool isInheritedValue() const { return m_data->isInheritedValue(); };
+    bool isInitialValue() const { return m_data->isInitialValue(); };
+    bool isUnsetValue() const { return m_data->isUnsetValue(); };
+    bool isCSSWideKeyword() const { return m_data->isCSSWideKeyword(); };
+    bool isLinearGradientValue() const { return m_data->isLinearGradientValue(); };
+    bool isPathValue() const { return m_data->isPathValue(); };
+    bool isRadialGradientValue() const { return m_data->isRadialGradientValue(); };
+    bool isReflectValue() const { return m_data->isReflectValue(); };
+    bool isShadowValue() const { return m_data->isShadowValue(); };
+    bool isCubicBezierTimingFunctionValue() const { return m_data->isCubicBezierTimingFunctionValue(); };
+    bool isStepsTimingFunctionValue() const { return m_data->isStepsTimingFunctionValue(); };
+    bool isLineBoxContainValue() const { return m_data->isLineBoxContainValue(); };
+    bool isCalcValue() const { return m_data->isCalcValue(); };
+    bool isGridTemplateAreasValue() const { return m_data->isGridTemplateAreasValue(); };
+    bool isSVGDocumentValue() const { return m_data->isSVGDocumentValue(); };
+    bool isContentDistributionValue() const { return m_data->isContentDistributionValue(); };
+    bool isUnicodeRangeValue() const { return m_data->isUnicodeRangeValue(); };
+    bool isGridLineNamesValue() const { return m_data->isGridLineNamesValue(); };
+
     DECLARE_TRACE();
 
-    // ~CSSValue should be public, because non-public ~CSSValue causes C2248
-    // error: 'blink::CSSValue::~CSSValue' : cannot access protected member
-    // declared in class 'blink::CSSValue' when compiling
-    // 'source\wtf\refcounted.h' by using msvc.
-    ~CSSValue() { }
-
-protected:
-
-    static const size_t ClassTypeBits = 6;
-    enum ClassType {
-        PrimitiveClass,
-
-        // Image classes.
-        ImageClass,
-        CursorImageClass,
-
-        // Image generator classes.
-        CanvasClass,
-        CrossfadeClass,
-        LinearGradientClass,
-        RadialGradientClass,
-
-        // Timing function classes.
-        CubicBezierTimingFunctionClass,
-        StepsTimingFunctionClass,
-
-        // Other class types.
-        BorderImageSliceClass,
-        FontFeatureClass,
-        FontFaceSrcClass,
-
-        InheritedClass,
-        InitialClass,
-        UnsetClass,
-
-        ReflectClass,
-        ShadowClass,
-        UnicodeRangeClass,
-        LineBoxContainClass,
-        CalculationClass,
-        GridTemplateAreasClass,
-        PathClass,
-
-        // SVG classes.
-        CSSSVGDocumentClass,
-
-        CSSContentDistributionClass,
-
-        // List class types must appear after ValueListClass.
-        ValueListClass,
-        FunctionClass,
-        ImageSetClass,
-        GridLineNamesClass,
-        // Do not append non-list class types here.
-    };
+private:
+    // To ensure these are never called. Without this, the compiler may accidentally call
+    // CSSValue(nullptr) with the CSSValueObject* constructor to store an intermediate value.
+    // e.g. NullableCSSValue n = x ? CSSValue(foo) : nullptr;
+    CSSValue() = delete;
+    CSSValue(std::nullptr_t) = delete;
+
+    void ref() const
+    {
+#if !ENABLE(OILPAN)
+        if (m_data)
+            m_data->ref();
+#endif
+    }
+
+    void deref() const
+    {
+#if !ENABLE(OILPAN)
+        if (m_data)
+            m_data->deref();
+#endif
+    }
+
+    CSSValueObject* m_data;

[haraken, 2015/06/11 02:48:53]

You need to use RawPtrWillBeMember<CSSValueObject>. I guess the GC plugin will
complain about this with oilpan builds, but didn't it?

[sashab, 2015/06/11 23:05:35]

I wasn't compiling with blink_gc_plugin=1, so maybe I didn't get it :) Thanks
though; changed it now.

+};
 
-    static const size_t ValueListSeparatorBits = 2;
-    enum ValueListSeparator {
-        SpaceSeparator,
-        CommaSeparator,
-        SlashSeparator
+// A nullable container for CSSValueObject. Contents are the same as CSSValue.
+// Behavior is similar to a CSSValue*.
+class CORE_EXPORT NullableCSSValue {
+    ALLOW_ONLY_INLINE_ALLOCATION();
+public:
+    // Not explicit to allow for casting.
+    NullableCSSValue()
+        : m_data(nullptr)
+    {
+    }
+
+    NullableCSSValue(const CSSValueObject* cssValueObject)
+        : m_data(const_cast<CSSValueObject*>(cssValueObject))
+    {
+        ref();
+    }
+
+    NullableCSSValue(const CSSValueObject& cssValueObject)
+        : m_data(const_cast<CSSValueObject*>(&cssValueObject))
+    {
+        ref();
+    }
+
+    // TODO(sashab): Remove these; construction should only be available with
+    // CSSValueObject& or CSSValueObject*.
+    template <class T> NullableCSSValue(PassRefPtrWillBeRawPtr<T> cssValueObject)
+        : m_data(cssValueObject.leakRef())
+    {
+        static_assert(WTF::IsSubclass<T, CSSValueObject>::value, "Parameter must be a container for CSSValueObject");
+    }
+
+#if !ENABLE(OILPAN)
+    template <class T> NullableCSSValue(RefPtrWillBeRawPtr<T> cssValueObject)
+        : m_data(cssValueObject.release().leakRef())
+    {
+        static_assert(WTF::IsSubclass<T, CSSValueObject>::value, "Parameter must be a container for CSSValueObject");
+    }
+#endif
+
+    NullableCSSValue(const NullableCSSValue& cssValue)
+        : m_data(cssValue.m_data)
+    {
+        ref();
+    }
+
+    NullableCSSValue(const CSSValue& cssValue)
+        : m_data(cssValue.m_data)
+    {
+        ref();
+    }
+
+    ~NullableCSSValue()
+    {
+        deref();
     };
 
-    ClassType classType() const { return static_cast<ClassType>(m_classType); }
+    operator bool() const
+    {
+        return m_data;
+    }
 
-    explicit CSSValue(ClassType classType)
-        : m_primitiveUnitType(0)
-        , m_hasCachedCSSText(false)
-        , m_isQuirkValue(false)
-        , m_valueListSeparator(SpaceSeparator)
-        , m_classType(classType)
+    NullableCSSValue& operator=(const NullableCSSValue& rhs)
     {
+        rhs.ref();
+        deref();
+        m_data = rhs.m_data;
+        return *this;
     }
 
-    // NOTE: This class is non-virtual for memory and performance reasons.
-    // Don't go making it virtual again unless you know exactly what you're doing!
+    bool operator==(const NullableCSSValue& rhs)
+    {
+        return m_data ? rhs.m_data && m_data->equals(*rhs.m_data) : bool(rhs.m_data);
+    }
 
-private:
-    void destroy();
+    bool operator!=(const NullableCSSValue& rhs)
+    {
+        return !(*this == rhs);
+    }
 
-protected:
-    // The bits in this section are only used by specific subclasses but kept here
-    // to maximize struct packing.
+    CSSValue& operator*()
+    {
+        ASSERT(m_data);
+        // reinterpret_casts used to avoid ref-churn.
+        return *reinterpret_cast<CSSValue*>(this);
+    }
 
-    // CSSPrimitiveValue bits:
-    unsigned m_primitiveUnitType : 7; // CSSPrimitiveValue::UnitType
-    mutable unsigned m_hasCachedCSSText : 1;
-    unsigned m_isQuirkValue : 1;
+    const CSSValue& operator*() const
+    {
+        ASSERT(m_data);
+        return *reinterpret_cast<const CSSValue*>(this);
+    }
 
-    unsigned m_valueListSeparator : ValueListSeparatorBits;
+    CSSValue* operator->()
+    {
+        ASSERT(m_data);
+        return reinterpret_cast<CSSValue*>(this);
+    }
+
+    const CSSValue* operator->() const
+    {
+        ASSERT(m_data);
+        return reinterpret_cast<const CSSValue*>(this);
+    }
+
+    void swap(NullableCSSValue& rhs)
+    {
+        std::swap(this->m_data, rhs.m_data);
+    }
+
+    DECLARE_TRACE();
 
 private:
-    unsigned m_classType : ClassTypeBits; // ClassType
+    void ref() const
+    {
+#if !ENABLE(OILPAN)
+        if (m_data)
+            m_data->ref();
+#endif
+    }
+
+    void deref() const
+    {
+#if !ENABLE(OILPAN)
+        if (m_data)
+            m_data->deref();
+#endif
+    }
+
+    CSSValueObject* m_data;

[haraken, 2015/06/11 02:48:53]

Ditto. You need to use RawPtrWillBeMember<CSSValueObject>.

[sashab, 2015/06/11 23:05:35]

Done.

 };
 
-template<typename CSSValueType, size_t inlineCapacity>
-inline bool compareCSSValueVector(const WillBeHeapVector<RefPtrWillBeMember<CSSValueType>, inlineCapacity>& firstVector, const WillBeHeapVector<RefPtrWillBeMember<CSSValueType>, inlineCapacity>& secondVector)
+static_assert(sizeof(CSSValue) == sizeof(void*), "CSSValue should be pointer-sized");
+static_assert(sizeof(NullableCSSValue) == sizeof(void*), "CSSValue should be pointer-sized");
+static_assert(sizeof(CSSValue) == sizeof(NullableCSSValue), "Both CSSValue containers must contain the same data");
+
+template<size_t inlineCapacity>
+inline bool compareCSSValueVector(const WillBeHeapVector<CSSValue, inlineCapacity>& firstVector, const WillBeHeapVector<CSSValue, inlineCapacity>& secondVector)
 {
     size_t size = firstVector.size();
     if (size != secondVector.size())
         return false;
 
     for (size_t i = 0; i < size; i++) {
-        const RefPtrWillBeMember<CSSValueType>& firstPtr = firstVector[i];
-        const RefPtrWillBeMember<CSSValueType>& secondPtr = secondVector[i];
-        if (firstPtr == secondPtr || (firstPtr && secondPtr && firstPtr->equals(*secondPtr)))
+        const CSSValue& firstPtr = firstVector[i];
+        const CSSValue& secondPtr = secondVector[i];
+        if (firstPtr.ptrEquals(secondPtr) || firstPtr.equals(secondPtr))
             continue;
         return false;
     }
     return true;
 }
 
-template<typename CSSValueType>
-inline bool compareCSSValuePtr(const RefPtr<CSSValueType>& first, const RefPtr<CSSValueType>& second)
-{
-    return first ? second && first->equals(*second) : !second;
-}
-
-template<typename CSSValueType>
-inline bool compareCSSValuePtr(const RawPtr<CSSValueType>& first, const RawPtr<CSSValueType>& second)
-{
-    return first ? second && first->equals(*second) : !second;
-}
-
-template<typename CSSValueType>
-inline bool compareCSSValuePtr(const Member<CSSValueType>& first, const Member<CSSValueType>& second)
-{
-    return first ? second && first->equals(*second) : !second;
-}
-
 #define DEFINE_CSS_VALUE_TYPE_CASTS(thisType, predicate) \
-    DEFINE_TYPE_CASTS(thisType, CSSValue, value, value->predicate, value.predicate)
+    DEFINE_TYPE_CASTS(thisType, CSSValueObject, value, value->predicate, value.predicate); \
+    inline thisType& to##thisType(CSSValue value) \
+    { \
+        ASSERT_WITH_SECURITY_IMPLICATION(value.predicate); \
+        return **reinterpret_cast<thisType**>(&value); \
+    } \
+    inline thisType* to##thisType(NullableCSSValue value) \
+    { \
+        if (!value) \
+            return nullptr; \
+        ASSERT_WITH_SECURITY_IMPLICATION(value->predicate); \
+        return *reinterpret_cast<thisType**>(&value); \
+    }
 
 } // namespace blink