Avoid use of CSSValue in MediaQueryExp and MediaQueryEvaluator

Source/core/css/MediaQueryExp.cpp

Index: Source/core/css/MediaQueryExp.cpp
diff --git a/Source/core/css/MediaQueryExp.cpp b/Source/core/css/MediaQueryExp.cpp
index a7260d16cf7904170b34a02196238efa8c0803ba..c1f4fd7aa72796cb3cfe2e7e0b982e82c8e39278 100644
--- a/Source/core/css/MediaQueryExp.cpp
+++ b/Source/core/css/MediaQueryExp.cpp
@@ -30,11 +30,12 @@
 #include "config.h"
 #include "core/css/MediaQueryExp.h"
 
-#include "CSSValueKeywords.h"
 #include "core/css/CSSAspectRatioValue.h"
 #include "core/css/CSSParserValues.h"
 #include "core/css/CSSPrimitiveValue.h"
 #include "core/html/parser/HTMLParserIdioms.h"
+#include "wtf/DecimalNumber.h"
+#include "wtf/text/StringBuffer.h"
 #include "wtf/text/StringBuilder.h"
 
 namespace WebCore {
@@ -201,13 +202,13 @@ bool MediaQueryExp::isViewportDependent() const
 
 MediaQueryExp::MediaQueryExp(const MediaQueryExp& other)
     : m_mediaFeature(other.mediaFeature())
-    , m_value(other.value())
+    , m_expValue(other.expValue())
 {
 }
 
-MediaQueryExp::MediaQueryExp(const String& mediaFeature, PassRefPtrWillBeRawPtr<CSSValue> value)
+MediaQueryExp::MediaQueryExp(const String& mediaFeature, const MediaQueryExpValue& expValue)
     : m_mediaFeature(mediaFeature)
-    , m_value(value)
+    , m_expValue(expValue)
 {
 }
 
@@ -215,9 +216,7 @@ PassOwnPtrWillBeRawPtr<MediaQueryExp> MediaQueryExp::createIfValid(const String&
 {
     ASSERT(!mediaFeature.isNull());
 
-    // FIXME - Creation of CSSValue here may not be thread safe.
-    // It should be replaced by a different way to pass the values to MediaQueryEvaluator.
-    RefPtrWillBeRawPtr<CSSValue> cssValue = nullptr;
+    MediaQueryExpValue expValue;
     bool isValid = false;
     String lowerMediaFeature = attemptStaticStringCreation(mediaFeature.lower());
 
@@ -227,29 +226,32 @@ PassOwnPtrWillBeRawPtr<MediaQueryExp> MediaQueryExp::createIfValid(const String&
             CSSParserValue* value = valueList->current();
             ASSERT(value);
 
-            if (featureWithCSSValueID(lowerMediaFeature, value)) {
+            if (featureWithCSSValueID(lowerMediaFeature, value) && featureWithValidIdent(lowerMediaFeature, value->id)) {
                 // Media features that use CSSValueIDs.
-                cssValue = CSSPrimitiveValue::createIdentifier(value->id);
-                if (!featureWithValidIdent(lowerMediaFeature, toCSSPrimitiveValue(cssValue.get())->getValueID()))
-                    cssValue.clear();
-            } else if (featureWithValidDensity(lowerMediaFeature, value)) {
-                // Media features that must have non-negative <density>, ie. dppx, dpi or dpcm.
-                cssValue = CSSPrimitiveValue::create(value->fValue, (CSSPrimitiveValue::UnitTypes) value->unit);
-            } else if (featureWithValidPositiveLength(lowerMediaFeature, value)) {
-                // Media features that must have non-negative <lenght> or number value.
-                cssValue = CSSPrimitiveValue::create(value->fValue, (CSSPrimitiveValue::UnitTypes) value->unit);
-            } else if (featureWithPositiveInteger(lowerMediaFeature, value)) {
-                // Media features that must have non-negative integer value.
-                cssValue = CSSPrimitiveValue::create(value->fValue, CSSPrimitiveValue::CSS_NUMBER);
-            } else if (featureWithPositiveNumber(lowerMediaFeature, value)) {
-                // Media features that must have non-negative number value.
-                cssValue = CSSPrimitiveValue::create(value->fValue, CSSPrimitiveValue::CSS_NUMBER);
-            } else if (featureWithZeroOrOne(lowerMediaFeature, value)) {
-                // Media features that must have (0|1) value.
-                cssValue = CSSPrimitiveValue::create(value->fValue, CSSPrimitiveValue::CSS_NUMBER);
+                expValue.id = value->id;
+                expValue.unit = CSSPrimitiveValue::CSS_VALUE_ID;
+                expValue.isID = true;
+            } else if (featureWithValidDensity(lowerMediaFeature, value)
+                || featureWithValidPositiveLength(lowerMediaFeature, value)) {
+                // Media features that must have non-negative <density>, ie. dppx, dpi or dpcm,
+                // or Media features that must have non-negative <length> or number value.
+                expValue.value = value->fValue;
+                expValue.unit = value->unit;
+                expValue.isValue = true;
+                expValue.isInteger = value->isInt;
+            } else if (featureWithPositiveInteger(lowerMediaFeature, value)
+                || featureWithPositiveNumber(lowerMediaFeature, value)
+                || featureWithZeroOrOne(lowerMediaFeature, value)) {
+                // Media features that must have non-negative integer value,
+                // or media features that must have non-negative number value,
+                // or media features that must have (0|1) value.
+                expValue.value = value->fValue;
+                expValue.unit = CSSPrimitiveValue::CSS_NUMBER;
+                expValue.isValue = true;
+                expValue.isInteger = value->isInt;
             }
 
-            isValid = cssValue;
+            isValid = (expValue.isID || expValue.isValue);
 
         } else if (valueList->size() == 3 && featureWithAspectRatio(lowerMediaFeature)) {
             // Create list of values.
@@ -274,8 +276,11 @@ PassOwnPtrWillBeRawPtr<MediaQueryExp> MediaQueryExp::createIfValid(const String&
                 }
             }
 
-            if (isValid)
-                cssValue = CSSAspectRatioValue::create(numeratorValue, denominatorValue);
+            if (isValid) {
+                expValue.numerator = (unsigned)numeratorValue;
+                expValue.denominator = (unsigned)denominatorValue;
+                expValue.isRatio = true;
+            }
         }
     } else if (featureWithoutValue(lowerMediaFeature)) {
         isValid = true;
@@ -284,25 +289,84 @@ PassOwnPtrWillBeRawPtr<MediaQueryExp> MediaQueryExp::createIfValid(const String&
     if (!isValid)
         return nullptr;
 
-    return adoptPtrWillBeNoop(new MediaQueryExp(lowerMediaFeature, cssValue));
+    return adoptPtrWillBeNoop(new MediaQueryExp(lowerMediaFeature, expValue));
 }
 
 MediaQueryExp::~MediaQueryExp()
 {
 }
 
+bool MediaQueryExp::operator==(const MediaQueryExp& other) const
+{
+    return (other.m_mediaFeature == m_mediaFeature)
+        && ((!other.m_expValue.set() && !m_expValue.set())
+            || (other.m_expValue.set() && m_expValue.set() && other.m_expValue.equals(m_expValue)));
+}
+
 String MediaQueryExp::serialize() const
 {
     StringBuilder result;
     result.append("(");
     result.append(m_mediaFeature.lower());
-    if (m_value) {
+    if (m_expValue.set()) {
         result.append(": ");
-        result.append(m_value->cssText());
+        result.append(m_expValue.cssText());
     }
     result.append(")");
 
     return result.toString();
 }
 
+static String unitToString(unsigned short unit)

[eseidel, 2014/04/22 16:22:23]

I wonder if this doesn't belong on CSSPrimitiveValue itself?

+{
+    switch (unit) {
+    case CSSPrimitiveValue::CSS_PERCENTAGE:
+        return String("%");
+    case CSSPrimitiveValue::CSS_EMS:
+        return String("em");
+    case CSSPrimitiveValue::CSS_EXS:
+        return String("ex");
+    case CSSPrimitiveValue::CSS_REMS:
+        return String("rem");
+    case CSSPrimitiveValue::CSS_CHS:
+        return String("ch");
+    case CSSPrimitiveValue::CSS_PX:
+        return String("px");
+    case CSSPrimitiveValue::CSS_CM:
+        return String("cm");
+    case CSSPrimitiveValue::CSS_DPPX:
+        return String("dppx");
+    case CSSPrimitiveValue::CSS_DPI:
+        return String("dpi");
+    case CSSPrimitiveValue::CSS_DPCM:
+        return String("dpcm");
+    }
+    return String();
+}
+
+static String printNumber(double number)
+{
+    DecimalNumber decimal(number);
+    StringBuffer<LChar> buffer(decimal.bufferLengthForStringDecimal());
+    decimal.toStringDecimal(buffer.characters(), buffer.length());
+    return String::adopt(buffer);
+}
+
+String MediaQueryExpValue::cssText() const
+{
+    StringBuilder output;
+    if (isValue) {
+        output.append(printNumber(value));
+        output.append(unitToString(unit));
+    } else if (isRatio) {
+        output.append(printNumber(numerator));
+        output.append("/");
+        output.append(printNumber(denominator));
+    } else if (isID) {
+        output.append(getValueName(id));
+    }
+
+    return output.toString();
+}
+
 } // namespace