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..6749dd6bc73a7fbba84810f9595e31853d68a70d 100644
--- a/Source/core/css/CSSValue.h
+++ b/Source/core/css/CSSValue.h
@@ -31,10 +31,10 @@
 
 namespace blink {
 

[Timothy Loh, 2015/06/02 04:54:03]

This stuff should be moved to CSSValueObject.{h,cpp}

[sashab, 2015/06/05 06:16:29]

Done.

-class CORE_EXPORT CSSValue : public RefCountedWillBeGarbageCollectedFinalized<CSSValue> {
+class CORE_EXPORT CSSValueObject : public RefCountedWillBeGarbageCollectedFinalized<CSSValueObject> {
 public:
 #if ENABLE(OILPAN)
-    // Override operator new to allocate CSSValue subtype objects onto
+    // Override operator new to allocate CSSValueObject subtype objects onto
     // a dedicated heap.
     GC_PLUGIN_IGNORE("crbug.com/443854")
     void* operator new(size_t size)
@@ -43,8 +43,8 @@ public:
     }
     static void* allocateObject(size_t size)
     {
-        ThreadState* state = ThreadStateFor<ThreadingTrait<CSSValue>::Affinity>::state();
-        return Heap::allocateOnHeapIndex(state, size, CSSValueHeapIndex, GCInfoTrait<CSSValue>::index());
+        ThreadState* state = ThreadStateFor<ThreadingTrait<CSSValueObject>::Affinity>::state();
+        return Heap::allocateOnHeapIndex(state, size, CSSValueHeapIndex, GCInfoTrait<CSSValueObject>::index());
     }
 #else
     // Override RefCounted's deref() to ensure operator delete is called on
@@ -96,17 +96,17 @@ public:
 
     bool hasFailedOrCanceledSubresources() const;
 
-    bool equals(const CSSValue&) const;
+    bool equals(const CSSValueObject&) const;
 
     void finalizeGarbageCollectedObject();
     DEFINE_INLINE_TRACE_AFTER_DISPATCH() { }
     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
+    // ~CSSValueObject should be public, because non-public ~CSSValueObject causes C2248
+    // error: 'blink::CSSValueObject::~CSSValueObject' : cannot access protected member
+    // declared in class 'blink::CSSValueObject' when compiling
     // 'source\wtf\refcounted.h' by using msvc.
-    ~CSSValue() { }
+    ~CSSValueObject() { }
 
 protected:
 
@@ -167,7 +167,7 @@ protected:
 
     ClassType classType() const { return static_cast<ClassType>(m_classType); }
 
-    explicit CSSValue(ClassType classType)
+    explicit CSSValueObject(ClassType classType)
         : m_primitiveUnitType(0)
         , m_hasCachedCSSText(false)
         , m_isQuirkValue(false)
@@ -197,16 +197,269 @@ private:
     unsigned m_classType : ClassTypeBits; // ClassType
 };
 
-template<typename CSSValueType, size_t inlineCapacity>
-inline bool compareCSSValueVector(const WillBeHeapVector<RefPtrWillBeMember<CSSValueType>, inlineCapacity>& firstVector, const WillBeHeapVector<RefPtrWillBeMember<CSSValueType>, inlineCapacity>& secondVector)
+// A non-nullable container for CSSValueObject. Takes ownership of the passed
+// CSSValueObject.
+class CORE_EXPORT CSSValue : public NoBaseWillBeGarbageCollectedFinalized<CSSValue> {
+    friend class NullableCSSValue;
+public:
+    // Not explicit to allow for casting.
+    CSSValue(const CSSValueObject* cssValueObject)

[Timothy Loh, 2015/06/02 04:54:03]

This ctor seems wrong, I think we should be creating these from references
instead?

[sashab, 2015/06/05 06:16:30]

Done.

+        : m_data(const_cast<CSSValueObject*>(cssValueObject))
+    {
+        ASSERT(m_data);
+        m_data->ref();
+    }
+
+    template<class T> CSSValue(PassRefPtrWillBeRawPtr<T> cssValueObject)
+        : m_data(cssValueObject.leakRef())
+    {
+        ASSERT(m_data);
+        m_data->ref();
+    }
+
+    template <class T> CSSValue(RefPtrWillBeRawPtr<T> cssValueObject)

[Timothy Loh, 2015/06/02 04:54:03]

extra space after template :|

[sashab, 2015/06/05 06:16:29]

Ty! Didn't notice

+        : m_data(cssValueObject.release().leakRef())
+    {
+        ASSERT(m_data);
+        m_data->ref();
+    }
+
+    CSSValue(const CSSValue& cssValue)
+        : m_data(cssValue.m_data)
+    {
+        m_data->ref();
+    }
+
+    ~CSSValue()
+    {
+        m_data->deref();
+    }
+
+    CSSValue& operator=(const CSSValue& rhs)
+    {
+        m_data->deref();

[Timothy Loh, 2015/06/02 04:54:03]

Should switch order of ref and deref so that assigning a value to itself doesn't
fail.

[sashab, 2015/06/05 06:16:30]

Ah, good one!

+        rhs.m_data->ref();
+        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(); };
+

[Timothy Loh, 2015/06/02 04:54:04]

Unused? Should call these in the above functions instead of writing
m_data->ref/deref().

[sashab, 2015/06/05 06:16:30]

ref() is used in:
ImmutableStylePropertySet::ImmutableStylePropertySet

deref() is used in:
ImmutableStylePropertySet::~ImmutableStylePropertySet

I'll make a TODO to get rid of them, though, and investigate the listed bug
(http://crbug.com/449032) in a follow-up patch.

+#if !ENABLE(OILPAN)
+    void ref()
+    {
+        m_data->ref();
+    }
+    void deref()
+    {
+        m_data->deref();
+    }
+#endif
+
+    DECLARE_TRACE();
+
+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() { ASSERT_NOT_REACHED(); }
+    CSSValue(std::nullptr_t null) { ASSERT_NOT_REACHED(); }

[Timothy Loh, 2015/06/02 04:54:03]

drop null argument name

[sashab, 2015/06/05 06:16:28]

Didn't know you could do that. Done.

+
+    CSSValueObject* m_data;
+};
+
+// A nullable container for CSSValueObject. Contents are the same as CSSValue.
+// Behavior is similar to a CSSValue*.
+class CORE_EXPORT NullableCSSValue : public NoBaseWillBeGarbageCollectedFinalized<NullableCSSValue> {
+public:
+    // Not explicit to allow for casting.
+    NullableCSSValue()
+        : m_data(nullptr)
+    {
+    };

[Timothy Loh, 2015/06/02 04:54:04]

drop semicolon

[sashab, 2015/06/05 06:16:30]

Done.

+
+    NullableCSSValue(std::nullptr_t null)

[Timothy Loh, 2015/06/02 04:54:03]

Is this ctor needed?

[sashab, 2015/06/05 06:16:30]

Not anymore. Removed.

+        : m_data(nullptr)
+    {
+    };
+
+    NullableCSSValue(const CSSValueObject* cssValueObject)
+        : m_data(const_cast<CSSValueObject*>(cssValueObject))
+    {
+        if (m_data)

[Timothy Loh, 2015/06/02 04:54:03]

should have (inline) ref/deref functions which do this, instead of having to
explicitly check m_data everywhere

[sashab, 2015/06/05 06:16:29]

Done.

+            m_data->ref();
+    }
+
+    template <class T>
+    NullableCSSValue(PassRefPtrWillBeRawPtr<T> cssValueObject)
+        : m_data(cssValueObject.leakRef())
+    {
+        if (m_data)
+            m_data->ref();
+    }
+
+    template <class T>
+    NullableCSSValue(RefPtrWillBeRawPtr<T> cssValueObject)
+        : m_data(cssValueObject.release().leakRef())
+    {
+        if (m_data)
+            m_data->ref();
+    }
+
+    NullableCSSValue(const NullableCSSValue& cssValue)
+        : m_data(cssValue.m_data)
+    {
+        if (m_data)
+            m_data->ref();
+    }
+
+    // CSSValues can be 'auto-cast' to NullableCSSValues, but not the other way around.

[Timothy Loh, 2015/06/02 04:54:04]

don't think this comment helps (it should be obvious?)

[sashab, 2015/06/05 06:16:29]

Removed.

+    NullableCSSValue(const CSSValue& cssValue)
+        : m_data(cssValue.m_data)
+    {
+        m_data->ref();
+    }
+
+    ~NullableCSSValue()
+    {
+        if (m_data)
+            m_data->deref();
+    };
+
+    operator bool() const
+    {
+        return m_data;
+    }
+
+    NullableCSSValue& operator=(const NullableCSSValue& rhs)
+    {
+        if (m_data)

[Timothy Loh, 2015/06/02 04:54:04]

swap order of deref/ref

[sashab, 2015/06/05 06:16:29]

Done, as before.

+            m_data->deref();
+        if (rhs.m_data)
+            rhs.m_data->ref();
+        m_data = rhs.m_data;
+        return *this;
+    }
+
+    bool operator==(const NullableCSSValue& rhs)
+    {
+        return m_data ? rhs.m_data && m_data->equals(*rhs.m_data) : bool(rhs.m_data);
+    }
+
+    CSSValue& operator*()
+    {
+        return *reinterpret_cast<CSSValue*>(this);

[Timothy Loh, 2015/06/02 04:54:03]

Is the reinterpret_cast to avoid ref-churn? If so, a comment would be useful.

Should add ASSERT(m_data) at least in these.

[sashab, 2015/06/05 06:16:29]

Done.

+    }
+
+    const CSSValue& operator*() const
+    {
+        return *reinterpret_cast<const CSSValue*>(this);
+    }
+
+    CSSValue* operator->()
+    {
+        return reinterpret_cast<CSSValue*>(this);
+    }
+
+    const CSSValue* operator->() const
+    {
+        return reinterpret_cast<const CSSValue*>(this);
+    }
+
+    void swap(NullableCSSValue& rhs)
+    {
+        std::swap(this->m_data, rhs.m_data);
+    }
+
+    DECLARE_TRACE();
+
+private:
+    CSSValueObject* m_data;
+};
+
+static_assert(sizeof(CSSValue) == sizeof(CSSValueObject*), "CSSValue must only contain a CSSValueObject*, and no vtable");

[Timothy Loh, 2015/06/02 04:54:03]

maybe:

== sizeof(void*), "CSSValue should be pointer-sized"

[sashab, 2015/06/05 06:16:28]

Done.

+static_assert(sizeof(NullableCSSValue) == sizeof(CSSValueObject*), "NullableCSSValue must only contain a CSSValueObject*, and no vtable");
+static_assert(sizeof(CSSValue) == sizeof(NullableCSSValue), "Both CSSValue containers must contain the same data members");
+
+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];
+        const CSSValue& firstPtr = firstVector[i];
+        const CSSValue& secondPtr = secondVector[i];
+        if (firstPtr.ptrEquals(secondPtr) || firstPtr.equals(secondPtr))
+            continue;
+        return false;
+    }
+    return true;
+}
+
+// TODO: Rename these to CompareCSSValueObjectPtr or make them take a CSSValue
+template<typename CSSValueObjectType, size_t inlineCapacity>
+inline bool compareCSSValueObjectVector(const WillBeHeapVector<RefPtrWillBeMember<CSSValueObjectType>, inlineCapacity>& firstVector, const WillBeHeapVector<RefPtrWillBeMember<CSSValueObjectType>, inlineCapacity>& secondVector)
+{
+    size_t size = firstVector.size();
+    if (size != secondVector.size())
+        return false;
+
+    for (size_t i = 0; i < size; i++) {
+        const RefPtrWillBeMember<CSSValueObjectType>& firstPtr = firstVector[i];
+        const RefPtrWillBeMember<CSSValueObjectType>& secondPtr = secondVector[i];
         if (firstPtr == secondPtr || (firstPtr && secondPtr && firstPtr->equals(*secondPtr)))
             continue;
         return false;
@@ -233,7 +486,19 @@ inline bool compareCSSValuePtr(const Member<CSSValueType>& first, const Member<C
 }
 
 #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