Improve r136754 by hardening checks of expected values for background-position.
authoralexis@webkit.org <alexis@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 7 Dec 2012 19:23:52 +0000 (19:23 +0000)
committeralexis@webkit.org <alexis@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 7 Dec 2012 19:23:52 +0000 (19:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=104380

Reviewed by Antti Koivisto.

r136754 was landed to fix the problem of checking successively two calc
values with validUnit. It was asserting as validUnit expect you to use
the parsed value of the calc after the call. In this case we pre-check the
background-position longhand to count how many values it has to then
call the right parsing functions accordingly. While r136754 is not
wrong it is better to harden isPotentialPositionValue with the real
expected units and keywords. For this matter we can reuse the
ReleaseParsedCalcValueCondition enum which was created with the same
idea as this patch. If you are not interested of the calc parsed
value when calling validUnit() you can now specify it, I believe it is
good to have it explicit to avoid mistake in the future.

No new tests : this is covered by css3/*, fast/backgrounds/*.

* css/CSSParser.cpp:
(WebCore::CSSParser::validCalculationUnit):
(WebCore::CSSParser::validUnit):
(WebCore::CSSParser::isPotentialPositionValue):
* css/CSSParser.h:
(WebCore::CSSParser::validUnit):
(CSSParser):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@136966 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/css/CSSParser.cpp
Source/WebCore/css/CSSParser.h

index 1a0aad0..cfb63be 100644 (file)
@@ -1,3 +1,32 @@
+2012-12-07  Alexis Menard  <alexis@webkit.org>
+
+        Improve r136754 by hardening checks of expected values for background-position.
+        https://bugs.webkit.org/show_bug.cgi?id=104380
+
+        Reviewed by Antti Koivisto.
+
+        r136754 was landed to fix the problem of checking successively two calc
+        values with validUnit. It was asserting as validUnit expect you to use
+        the parsed value of the calc after the call. In this case we pre-check the
+        background-position longhand to count how many values it has to then
+        call the right parsing functions accordingly. While r136754 is not
+        wrong it is better to harden isPotentialPositionValue with the real
+        expected units and keywords. For this matter we can reuse the
+        ReleaseParsedCalcValueCondition enum which was created with the same
+        idea as this patch. If you are not interested of the calc parsed
+        value when calling validUnit() you can now specify it, I believe it is
+        good to have it explicit to avoid mistake in the future.
+
+        No new tests : this is covered by css3/*, fast/backgrounds/*.
+
+        * css/CSSParser.cpp:
+        (WebCore::CSSParser::validCalculationUnit):
+        (WebCore::CSSParser::validUnit):
+        (WebCore::CSSParser::isPotentialPositionValue):
+        * css/CSSParser.h:
+        (WebCore::CSSParser::validUnit):
+        (CSSParser):
+
 2012-12-07  Brent Fulgham  <bfulgham@webkit.org>
 
         Remove unnecessary casts in transformations.
index 756f195..afd73b7 100644 (file)
@@ -1487,7 +1487,7 @@ KURL CSSParser::completeURL(const String& url) const
     return completeURL(m_context, url);
 }
 
-bool CSSParser::validCalculationUnit(CSSParserValue* value, Units unitflags)
+bool CSSParser::validCalculationUnit(CSSParserValue* value, Units unitflags, ReleaseParsedCalcValueCondition releaseCalc)
 {
     bool mustBeNonNegative = unitflags & FNonNeg;
 
@@ -1525,7 +1525,7 @@ bool CSSParser::validCalculationUnit(CSSParserValue* value, Units unitflags)
     case CalcOther:
         break;
     }
-    if (!b)
+    if (!b || releaseCalc == ReleaseParsedCalcValue)
         m_parsedCalculation.release();
     return b;    
 }
@@ -1536,10 +1536,10 @@ inline bool CSSParser::shouldAcceptUnitLessValues(CSSParserValue* value, Units u
     return (unitflags & (FLength | FAngle | FTime)) && (!value->fValue || cssParserMode == CSSQuirksMode || cssParserMode == SVGAttributeMode);
 }
 
-bool CSSParser::validUnit(CSSParserValue* value, Units unitflags, CSSParserMode cssParserMode)
+bool CSSParser::validUnit(CSSParserValue* value, Units unitflags, CSSParserMode cssParserMode, ReleaseParsedCalcValueCondition releaseCalc)
 {
     if (isCalculation(value))
-        return validCalculationUnit(value, unitflags);
+        return validCalculationUnit(value, unitflags, releaseCalc);
         
     bool b = false;
     switch (value->unit) {
@@ -3875,7 +3875,7 @@ void CSSParser::parse3ValuesBackgroundPosition(CSSParserValueList* valueList, Re
 
 inline bool CSSParser::isPotentialPositionValue(CSSParserValue* value)
 {
-    return !value->id || isBackgroundPositionKeyword(value->id);
+    return isBackgroundPositionKeyword(value->id) || validUnit(value, FPercent | FLength, ReleaseParsedCalcValue);
 }
 
 void CSSParser::parseFillBackgroundPosition(CSSParserValueList* valueList, RefPtr<CSSValue>& value1, RefPtr<CSSValue>& value2)
index 4c6f8c5..1311875 100644 (file)
@@ -589,20 +589,20 @@ private:
         return static_cast<Units>(static_cast<unsigned>(a) | static_cast<unsigned>(b));
     }
 
-    bool validCalculationUnit(CSSParserValue*, Units);
+    enum ReleaseParsedCalcValueCondition {
+        ReleaseParsedCalcValue,
+        DoNotReleaseParsedCalcValue
+    };
+
+    bool validCalculationUnit(CSSParserValue*, Units, ReleaseParsedCalcValueCondition releaseCalc = DoNotReleaseParsedCalcValue);
 
     bool shouldAcceptUnitLessValues(CSSParserValue*, Units, CSSParserMode);
 
-    inline bool validUnit(CSSParserValue* value, Units unitflags) { return validUnit(value, unitflags, m_context.mode); }
-    bool validUnit(CSSParserValue*, Units, CSSParserMode);
+    inline bool validUnit(CSSParserValue* value, Units unitflags, ReleaseParsedCalcValueCondition releaseCalc = DoNotReleaseParsedCalcValue) { return validUnit(value, unitflags, m_context.mode, releaseCalc); }
+    bool validUnit(CSSParserValue*, Units, CSSParserMode, ReleaseParsedCalcValueCondition releaseCalc = DoNotReleaseParsedCalcValue);
 
     bool parseBorderImageQuad(Units, RefPtr<CSSPrimitiveValue>&);
     int colorIntFromValue(CSSParserValue*);
-
-    enum ReleaseParsedCalcValueCondition {
-        ReleaseParsedCalcValue,
-        DoNotReleaseParsedCalcValue
-    };    
     double parsedDouble(CSSParserValue*, ReleaseParsedCalcValueCondition releaseCalc = DoNotReleaseParsedCalcValue);
     bool isCalculation(CSSParserValue*);