Changed Pair to be a CSSValue

Source/core/css/CSSValuePair.h

Index: Source/core/css/CSSValuePair.h
diff --git a/Source/core/css/Pair.h b/Source/core/css/CSSValuePair.h
similarity index 58%
rename from Source/core/css/Pair.h
rename to Source/core/css/CSSValuePair.h
index 4df5efec58f714f5d1a0436552a52558d82b5d55..290b51c6a91ea24312e22d25e548920b0cf8259f 100644
--- a/Source/core/css/Pair.h
+++ b/Source/core/css/CSSValuePair.h
@@ -18,11 +18,14 @@
  * Boston, MA 02110-1301, USA.
  */
 
-#ifndef Pair_h
-#define Pair_h
+#ifndef CSSValuePair_h
+#define CSSValuePair_h
 
 #include "core/CoreExport.h"
 #include "core/css/CSSPrimitiveValue.h"
+#include "core/css/CSSValue.h"
+#include "core/style/ComputedStyle.h"
+#include "platform/Length.h"
 #include "wtf/PassRefPtr.h"
 #include "wtf/RefCounted.h"
 #include "wtf/text/StringBuilder.h"
@@ -33,51 +36,56 @@ namespace blink {
 // and border-spacing (all of which are space-separated sets of two values).  At the moment we are only using it for
 // border-radius and background-size, but (FIXME) border-spacing and background-position could be converted over to use
 // it (eliminating some extra -webkit- internal properties).
-class CORE_EXPORT Pair final : public RefCountedWillBeGarbageCollected<Pair> {
+class CORE_EXPORT CSSValuePair : public CSSValue {
 public:
     enum IdenticalValuesPolicy { DropIdenticalValues, KeepIdenticalValues };
 
-    static PassRefPtrWillBeRawPtr<Pair> create(PassRefPtrWillBeRawPtr<CSSPrimitiveValue> first, PassRefPtrWillBeRawPtr<CSSPrimitiveValue> second,
+    static PassRefPtrWillBeRawPtr<CSSValuePair> create(PassRefPtrWillBeRawPtr<CSSValue> first, PassRefPtrWillBeRawPtr<CSSValue> second,
         IdenticalValuesPolicy identicalValuesPolicy)
     {
-        return adoptRefWillBeNoop(new Pair(first, second, identicalValuesPolicy));
+        return adoptRefWillBeNoop(new CSSValuePair(first, second, identicalValuesPolicy));
     }
 
-    CSSPrimitiveValue* first() const { return m_first.get(); }
-    CSSPrimitiveValue* second() const { return m_second.get(); }
+    static PassRefPtrWillBeRawPtr<CSSValuePair> create(const LengthSize& lengthSize, const ComputedStyle& style)
+    {
+        return adoptRefWillBeNoop(new CSSValuePair(CSSPrimitiveValue::create(lengthSize.width(), style.effectiveZoom()), CSSPrimitiveValue::create(lengthSize.height(), style.effectiveZoom()), KeepIdenticalValues));
+    }
+
+    CSSValue* first() const { return m_first.get(); }

[Timothy Loh, 2015/08/28 05:51:46]

can these be const references?

[sashab, 2015/08/31 00:33:51]

Sure, but this is kind of a big change -- I'll do it in another patch.

Is this something we want to try and do in general (make functions take const
CSSFooValue&s instead of CSSFooValue*s, like in StyleBuilderConverter,
CSSToStyleMap)? There are lots of callsites that do the equivalent of
*toCSSFooValue(foo) (storing intermediary pointers in variables/as function
arguments), maybe we should change them to dereference earlier (i.e.
toCSSFooValue(*foo)).

[Timothy Loh, 2015/08/31 01:08:04]

We discussed a while ago that most usages of CSSValue are with non-null values,
so short of some big overhaul we should at some point change all the pointers to
const refs.

[sashab, 2015/08/31 01:29:47]

I'll upload a patch in a minute that changes pair to use const refs. Also happy
to do the overhaul at some point, maybe when I get some spare time.

Would be a giant review though, might be nice if there was a way to split it up.

+    CSSValue* second() const { return m_second.get(); }
 
-    String cssText() const
+    String customCSSText() const
     {
-        return generateCSSString(first()->cssText(), second()->cssText(), m_identicalValuesPolicy);
+        String first = m_first->cssText();
+        String second = m_second->cssText();
+        if (m_identicalValuesPolicy == DropIdenticalValues && first == second)
+            return first;
+        return first + ' ' + second;
     }
 
-    bool equals(const Pair& other) const
+    bool equals(const CSSValuePair& other) const
     {
         return compareCSSValuePtr(m_first, other.m_first)
             && compareCSSValuePtr(m_second, other.m_second)
             && m_identicalValuesPolicy == other.m_identicalValuesPolicy;

[Timothy Loh, 2015/08/28 05:51:46]

probably we can even assert these are the same.

[sashab, 2015/08/31 00:33:51]

The equals() method should work on all CSSValuePairs, whether they have the same
identical values policy or not. Is it really invalid to compare two Pairs that
have different identical values policies? I think it should return false but not
error.

[Timothy Loh, 2015/08/31 01:08:04]

We should only be comparing values that were parsed for the same property, I
think these will always the same serialization strategy.

[sashab, 2015/08/31 01:29:47]

Ok, added the assert.

     }
 
-    DECLARE_TRACE();
+    DECLARE_TRACE_AFTER_DISPATCH();
 
 private:
-    Pair(PassRefPtrWillBeRawPtr<CSSPrimitiveValue> first, PassRefPtrWillBeRawPtr<CSSPrimitiveValue> second, IdenticalValuesPolicy identicalValuesPolicy)
-        : m_first(first)
+    CSSValuePair(PassRefPtrWillBeRawPtr<CSSValue> first, PassRefPtrWillBeRawPtr<CSSValue> second, IdenticalValuesPolicy identicalValuesPolicy)
+        : CSSValue(ValuePairClass)
+        , m_first(first)
         , m_second(second)
         , m_identicalValuesPolicy(identicalValuesPolicy) { }
 
-    static String generateCSSString(const String& first, const String& second, IdenticalValuesPolicy identicalValuesPolicy)
-    {
-        if (identicalValuesPolicy == DropIdenticalValues && first == second)
-            return first;
-        return first + ' ' + second;
-    }
-
-    RefPtrWillBeMember<CSSPrimitiveValue> m_first;
-    RefPtrWillBeMember<CSSPrimitiveValue> m_second;
+    RefPtrWillBeMember<CSSValue> m_first;
+    RefPtrWillBeMember<CSSValue> m_second;
     IdenticalValuesPolicy m_identicalValuesPolicy;
 };
 
+DEFINE_CSS_VALUE_TYPE_CASTS(CSSValuePair, isValuePair());
+
 } // namespace
 
-#endif
+#endif // CSSValuePair_h