Update StyleValue.cssString() to use existing methods instead of rolling our own.

All StyleValues now use toCSSValue()->cssText() instead of defining custom methods in subclasses. Conveniently, this effectively allows toCSSValue to be tested in LayoutTests. Also get rid of re-parsing unit types in SimpleLength.toCSSValue, since the code there had a bug anyway which was uncovered by the LayoutTests. BUG=545318 Review URL: https://codereview.chromium.org/1602843003 Cr-Commit-Position: refs/heads/master@{#370341}

Update StyleValue.cssString() to use existing methods instead of rolling our own.
All StyleValues now use toCSSValue()->cssText() instead of defining custom methods in subclasses. Conveniently, this effectively allows toCSSValue to be tested in LayoutTests. Also get rid of re-parsing unit types in SimpleLength.toCSSValue, since the code there had a bug anyway which was uncovered by the LayoutTests. BUG=545318 Review URL: https://codereview.chromium.org/1602843003 Cr-Commit-Position: refs/heads/master@{#370341}
13eeea65 · meade · Commit bot · d6b55d45 · 13eeea65 · 13eeea65
Commit 13eeea65 authored Jan 20, 2016 by meade Committed by Commit bot Jan 20, 2016
11 changed files
--- a/third_party/WebKit/LayoutTests/typedcssom/calclength.html
+++ b/third_party/WebKit/LayoutTests/typedcssom/calclength.html
@@ -36,23 +36,40 @@ test(function() {
 }, 'Test that subtracting two CalcLengths produces a new CalcLength with the correct values.');
 test(function() {
-        var calcLength = new CalcLength({px: 1, percent: 5.2});
+  var calcLength = new CalcLength({px: 1, percent: 5.2});
-        var result = calcLength.multiply(3);
+  var result = calcLength.multiply(3);
-        assert_not_equals(calcLength, result);
+  assert_not_equals(calcLength, result);
-        assert_equals(result.px, 3);
+  assert_equals(result.px, 3);
-        assert_approx_equals(result.percent, 15.6, 0.000001);
+  assert_approx_equals(result.percent, 15.6, 0.000001);
 }, 'Test that multiplying a CalcLength produces a new CalcLength with the correct values.');
 test(function() {
-        var calcLength = new CalcLength({px: 3, percent: 15.6});
+  var calcLength = new CalcLength({px: 3, percent: 15.6});
-        var result = calcLength.divide(3);
+  var result = calcLength.divide(3);
-        assert_not_equals(calcLength, result);
+  assert_not_equals(calcLength, result);
-        assert_equals(result.px, 1);
+  assert_equals(result.px, 1);
-        assert_equals(result.percent, 5.2);
+  assert_equals(result.percent, 5.2);
 }, 'Test that dividing a CalcLength produces a new CalcLength with the correct values.');
+test(function() {
+  var values = [
+    {input: new CalcLength({px: 1}), cssString: 'calc(1px)'},
+    {input: new CalcLength({px: -1}), cssString: 'calc(-1px)'},
+    {input: new CalcLength({px: 1, percent: 15.6}), cssString: 'calc(1px + 15.6%)'},
+    {input: new CalcLength({px: 1, percent: -15.6}), cssString: 'calc(1px - 15.6%)'},
+    {input: new CalcLength({px: -1, percent: -15.6}), cssString: 'calc(-1px - 15.6%)'},
+    {input: new CalcLength({px: -1, percent: -15.6, vw: 5}), cssString: 'calc((-1px - 15.6%) + 5vw)'},
+    {input: new CalcLength({px: -1, percent: -15.6, vw: -5}), cssString: 'calc((-1px - 15.6%) - 5vw)'},
+  ];
+  for (var i = 0; i < values.length; ++i) {
+    assert_equals(values[i].input.cssString, values[i].cssString);
+  }
+}, 'Test that the CSS string method for a CalcLength produces the correct result');
 </script>
 <body>

--- a/third_party/WebKit/Source/core/css/cssom/KeywordValue.cpp
+++ b/third_party/WebKit/Source/core/css/cssom/KeywordValue.cpp
@@ -30,11 +30,6 @@ KeywordTable& keywordTable()
 } // namespace
-String KeywordValue::cssString() const
-{
-    return m_keywordValue;
-}
 const String& KeywordValue::keywordValue() const
 {
    return m_keywordValue;

--- a/third_party/WebKit/Source/core/css/cssom/KeywordValue.h
+++ b/third_party/WebKit/Source/core/css/cssom/KeywordValue.h
@@ -27,7 +27,6 @@ public:
    const String& keywordValue() const;
-    String cssString() const override;
    PassRefPtrWillBeRawPtr<CSSValue> toCSSValue() const override;
 private:

--- a/third_party/WebKit/Source/core/css/cssom/LengthValue.cpp
+++ b/third_party/WebKit/Source/core/css/cssom/LengthValue.cpp
@@ -161,6 +161,45 @@ const String& LengthValue::lengthTypeToString(LengthValue::LengthUnit unit)
    }
 }
+CSSPrimitiveValue::UnitType LengthValue::lengthTypeToPrimitiveType(LengthValue::LengthUnit unit)
+{
+    switch (unit) {
+    case Px:
+        return CSSPrimitiveValue::UnitType::Pixels;
+    case Percent:
+        return CSSPrimitiveValue::UnitType::Percentage;
+    case Em:
+        return CSSPrimitiveValue::UnitType::Ems;
+    case Ex:
+        return CSSPrimitiveValue::UnitType::Exs;
+    case Ch:
+        return CSSPrimitiveValue::UnitType::Chs;
+    case Rem:
+        return CSSPrimitiveValue::UnitType::Rems;
+    case Vw:
+        return CSSPrimitiveValue::UnitType::ViewportWidth;
+    case Vh:
+        return CSSPrimitiveValue::UnitType::ViewportHeight;
+    case Vmin:
+        return CSSPrimitiveValue::UnitType::ViewportMin;
+    case Vmax:
+        return CSSPrimitiveValue::UnitType::ViewportMax;
+    case Cm:
+        return CSSPrimitiveValue::UnitType::Centimeters;
+    case Mm:
+        return CSSPrimitiveValue::UnitType::Millimeters;
+    case In:
+        return CSSPrimitiveValue::UnitType::Inches;
+    case Pc:
+        return CSSPrimitiveValue::UnitType::Picas;
+    case Pt:
+        return CSSPrimitiveValue::UnitType::Points;
+    default:
+        ASSERT_NOT_REACHED();
+        return CSSPrimitiveValue::UnitType::Pixels;
+    }
+}
 LengthValue* LengthValue::addInternal(const LengthValue*, ExceptionState&)
 {
    ASSERT_NOT_REACHED();

--- a/third_party/WebKit/Source/core/css/cssom/LengthValue.h
+++ b/third_party/WebKit/Source/core/css/cssom/LengthValue.h
@@ -50,7 +50,8 @@ protected:
    LengthValue() {}
    static LengthUnit lengthUnitFromName(const String&);
-    static const String& lengthTypeToString(LengthUnit type);
+    static const String& lengthTypeToString(LengthUnit);
+    static CSSPrimitiveValue::UnitType lengthTypeToPrimitiveType(LengthUnit);
    virtual LengthValue* addInternal(const LengthValue* other, ExceptionState&);
    virtual LengthValue* subtractInternal(const LengthValue* other, ExceptionState&);

--- a/third_party/WebKit/Source/core/css/cssom/NumberValue.h
+++ b/third_party/WebKit/Source/core/css/cssom/NumberValue.h
@@ -28,8 +28,6 @@ public:
        m_value = value;
    }
-    String cssString() const override { return String::number(m_value); }
    PassRefPtrWillBeRawPtr<CSSValue> toCSSValue() const override
    {
        return cssValuePool().createValue(m_value, CSSPrimitiveValue::UnitType::

--- a/third_party/WebKit/Source/core/css/cssom/SimpleLength.cpp
+++ b/third_party/WebKit/Source/core/css/cssom/SimpleLength.cpp
@@ -10,18 +10,9 @@
 namespace blink {
-String SimpleLength::cssString() const
-{
-    StringBuilder s;
-    s.appendNumber(m_value);
-    s.append(unit());
-    return s.toString();
-}
 PassRefPtrWillBeRawPtr<CSSValue> SimpleLength::toCSSValue() const
 {
-    // TODO: Don't re-parse the unit.
+    return cssValuePool().createValue(m_value, LengthValue::lengthTypeToPrimitiveType(m_unit));
-    return cssValuePool().createValue(m_value, CSSPrimitiveValue::fromName(unit()));
 }
 LengthValue* SimpleLength::addInternal(const LengthValue* other, ExceptionState& exceptionState)

--- a/third_party/WebKit/Source/core/css/cssom/SimpleLength.h
+++ b/third_party/WebKit/Source/core/css/cssom/SimpleLength.h
@@ -39,7 +39,6 @@ public:
    StyleValueType type() const override { return StyleValueType::SimpleLengthType; }
-    String cssString() const override;
    PassRefPtrWillBeRawPtr<CSSValue> toCSSValue() const override;
 protected:

--- a/third_party/WebKit/Source/core/css/cssom/StyleCalcLength.cpp
+++ b/third_party/WebKit/Source/core/css/cssom/StyleCalcLength.cpp
@@ -124,29 +124,6 @@ LengthValue* StyleCalcLength::divideInternal(double x, ExceptionState& exception
    return result;
 }
-String StyleCalcLength::cssString() const
-{
-    StringBuilder builder;
-    builder.appendLiteral("calc(");
-    for (unsigned i = 0; i < LengthUnit::Count; ++i) {
-        LengthUnit lengthUnit = static_cast<LengthUnit>(i);
-        if (has(lengthUnit)) {
-            double value = get(lengthUnit);
-            if (value >= 0 && i > 0) {
-                builder.appendLiteral(" + ");
-            } else if (value < 0 && i > 0) {
-                builder.appendLiteral(" - ");
-            } else if (value < 0) {
-                builder.append('-');
-            }
-            builder.appendNumber(std::abs(get(lengthUnit)));
-            builder.append(lengthTypeToString(lengthUnit));
-        }
-    }
-    builder.append(')');
-    return builder.toString();
-}
 PassRefPtrWillBeRawPtr<CSSValue> StyleCalcLength::toCSSValue() const
 {
    // Create a CSS Calc Value, then put it into a CSSPrimitiveValue
@@ -154,20 +131,14 @@ PassRefPtrWillBeRawPtr<CSSValue> StyleCalcLength::toCSSValue() const
    for (unsigned i = 0; i < LengthUnit::Count; ++i) {
        LengthUnit lengthUnit = static_cast<LengthUnit>(i);
        if (!has(lengthUnit))
-            break;
+            continue;
        double value = get(lengthUnit);
-        CSSPrimitiveValue::UnitType primitiveUnit;
+        CSSPrimitiveValue::UnitType primitiveUnit = lengthTypeToPrimitiveType(lengthUnit);
-        if (lengthUnit == LengthUnit::Percent) {
-            primitiveUnit = CSSPrimitiveValue::UnitType::Percentage;
-        } else {
-            // TODO: Don't re-parse the unit here.
-            primitiveUnit = CSSPrimitiveValue::fromName(lengthTypeToString(lengthUnit));
-        }
        if (node) {
            node = CSSCalcValue::createExpressionNode(
                node,
-                CSSCalcValue::createExpressionNode(CSSPrimitiveValue::create(value, primitiveUnit)),
+                CSSCalcValue::createExpressionNode(CSSPrimitiveValue::create(std::abs(value), primitiveUnit)),
-                CalcAdd);
+                value >= 0 ? CalcAdd : CalcSubtract);
        } else {
            node = CSSCalcValue::createExpressionNode(CSSPrimitiveValue::create(value, primitiveUnit));
        }

--- a/third_party/WebKit/Source/core/css/cssom/StyleCalcLength.h
+++ b/third_party/WebKit/Source/core/css/cssom/StyleCalcLength.h
@@ -44,7 +44,6 @@ public:
 #undef GETTER_MACRO
-    String cssString() const override;
    PassRefPtrWillBeRawPtr<CSSValue> toCSSValue() const override;
    StyleValueType type() const override { return CalcLengthType; }

--- a/third_party/WebKit/Source/core/css/cssom/StyleValue.h
+++ b/third_party/WebKit/Source/core/css/cssom/StyleValue.h
@@ -30,8 +30,11 @@ public:
    static StyleValue* create(const CSSValue&);
    static ScriptValue parse(ScriptState*, const String& property, const String& cssText);
-    virtual String cssString() const = 0;
    virtual PassRefPtrWillBeRawPtr<CSSValue> toCSSValue() const = 0;
+    virtual String cssString() const
+    {
+        return toCSSValue()->cssText();
+    }
    DEFINE_INLINE_VIRTUAL_TRACE() { }