Changed Pair to be a CSSValue

Source/core/css/CSSValuePair.h

 /*
  * (C) 1999-2003 Lars Knoll (knoll@kde.org)
  * Copyright (C) 2004, 2005, 2006 Apple Computer, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Library General Public License for more details.
  *
  * You should have received a copy of the GNU Library General Public License
  * along with this library; see the file COPYING.LIB.  If not, write to
  * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
  * 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"
 
 namespace blink {
 
 // A primitive value representing a pair.  This is useful for properties like border-radius, background-size/position,

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

This comment is super outdated, can we drop it?

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

Sure, gone.

 // 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(); }
-
-    String cssText() const
+    static PassRefPtrWillBeRawPtr<CSSValuePair> create(const LengthSize& lengthSize, const ComputedStyle& style)
     {
-        return generateCSSString(first()->cssText(), second()->cssText(), m_identicalValuesPolicy);
+        return adoptRefWillBeNoop(new CSSValuePair(CSSPrimitiveValue::create(lengthSize.width(), style.effectiveZoom()), CSSPrimitiveValue::create(lengthSize.height(), style.effectiveZoom()), KeepIdenticalValues));
     }
 
-    bool equals(const Pair& other) const
+    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 customCSSText() const
+    {
+        String first = m_first->cssText();
+        String second = m_second->cssText();
+        if (m_identicalValuesPolicy == DropIdenticalValues && first == second)
+            return first;
+        return first + ' ' + second;
+    }
+
+    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