Simplify CSSParser::parseFont.
authoralexis.menard@openbossa.org <alexis.menard@openbossa.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Apr 2012 14:26:55 +0000 (14:26 +0000)
committeralexis.menard@openbossa.org <alexis.menard@openbossa.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 23 Apr 2012 14:26:55 +0000 (14:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=78698

Reviewed by Antti Koivisto.

Source/WebCore:

Simplify parseFont by sharing the code we have for
the longhands of the font property.

No new tests : Extend the existing font shorthand test and modify expected files
as now the order of the longhands added in the property list of the style
has changed. It's very unlikely that some code is relying on this order though. It will
also match the way the spec order them http://www.w3.org/TR/css3-fonts/#font-prop
even though the order is arbitrary for some values.

* css/CSSParser.cpp:
(WebCore::CSSParser::parseValue):
(WebCore::CSSParser::parseFont):
(WebCore::CSSParser::parseLineHeight):
(WebCore):
(WebCore::CSSParser::parseFontSize):
(WebCore::CSSParser::parseFontWeight):  Fix a bug discovered while using parseFontWeight from
the parseFont (case font: 0/0, Arial, sans-serif; in a layout test), we should return true only
when we add something in the property list.
* css/CSSParser.h:

LayoutTests:

Added new incorrect values to improve the test coverage of the font shorthand property.

The patch changed the order the longhands are added to the list
of properties for the style so the expected files need to be updated.

* fast/css/font-shorthand-expected.txt:
* fast/css/font-shorthand.html:
* fast/inspector-support/style-expected.txt:

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

LayoutTests/ChangeLog
LayoutTests/fast/css/font-shorthand-expected.txt
LayoutTests/fast/css/font-shorthand.html
LayoutTests/fast/inspector-support/style-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/css/CSSParser.cpp
Source/WebCore/css/CSSParser.h

index b2e8104..be88d9c 100644 (file)
@@ -1,3 +1,19 @@
+2012-04-23  Alexis Menard  <alexis.menard@openbossa.org>
+
+        Simplify CSSParser::parseFont.
+        https://bugs.webkit.org/show_bug.cgi?id=78698
+
+        Reviewed by Antti Koivisto.
+
+        Added new incorrect values to improve the test coverage of the font shorthand property.
+
+        The patch changed the order the longhands are added to the list
+        of properties for the style so the expected files need to be updated.
+
+        * fast/css/font-shorthand-expected.txt:
+        * fast/css/font-shorthand.html:
+        * fast/inspector-support/style-expected.txt:
+
 2012-04-23  Mikhail Naganov  <mnaganov@chromium.org>
 
         [Chromium] Unreviewed test expectations update.
index eb2c461..7f68495 100644 (file)
@@ -1,75 +1,75 @@
 Test
 Font for '12px monospace':
-font-family: monospace (original property was font)
-font-size: 12px (original property was font)
 font-style: normal (original property was font and property was implicitly set.)
 font-variant: normal (original property was font and property was implicitly set.)
 font-weight: normal (original property was font and property was implicitly set.)
+font-size: 12px (original property was font)
 line-height: normal (original property was font and property was implicitly set.)
+font-family: monospace (original property was font)
 
 Font for '12px/24px serif':
-font-family: serif (original property was font)
-font-size: 12px (original property was font)
 font-style: normal (original property was font and property was implicitly set.)
 font-variant: normal (original property was font and property was implicitly set.)
 font-weight: normal (original property was font and property was implicitly set.)
+font-size: 12px (original property was font)
 line-height: 24px (original property was font)
+font-family: serif (original property was font)
 
 Font for 'normal 12px serif':
-font-family: serif (original property was font)
-font-size: 12px (original property was font)
 font-style: normal (original property was font)
 font-variant: normal (original property was font and property was implicitly set.)
 font-weight: normal (original property was font and property was implicitly set.)
+font-size: 12px (original property was font)
 line-height: normal (original property was font and property was implicitly set.)
+font-family: serif (original property was font)
 
 Font for 'normal normal 12px serif':
-font-family: serif (original property was font)
-font-size: 12px (original property was font)
 font-style: normal (original property was font)
 font-variant: normal (original property was font)
 font-weight: normal (original property was font and property was implicitly set.)
+font-size: 12px (original property was font)
 line-height: normal (original property was font and property was implicitly set.)
+font-family: serif (original property was font)
 
 Font for 'normal normal normal 12px serif':
-font-family: serif (original property was font)
-font-size: 12px (original property was font)
 font-style: normal (original property was font)
 font-variant: normal (original property was font)
 font-weight: normal (original property was font)
+font-size: 12px (original property was font)
 line-height: normal (original property was font and property was implicitly set.)
+font-family: serif (original property was font)
 
 Font for 'italic small-caps 12px/24px serif':
-font-family: serif (original property was font)
-font-size: 12px (original property was font)
 font-style: italic (original property was font)
 font-variant: small-caps (original property was font)
 font-weight: normal (original property was font and property was implicitly set.)
+font-size: 12px (original property was font)
 line-height: 24px (original property was font)
+font-family: serif (original property was font)
 
 Font for 'italic bold 12px/24px serif':
-font-family: serif (original property was font)
-font-size: 12px (original property was font)
 font-style: italic (original property was font)
-font-variant: normal (original property was font and property was implicitly set.)
 font-weight: bold (original property was font)
+font-variant: normal (original property was font and property was implicitly set.)
+font-size: 12px (original property was font)
 line-height: 24px (original property was font)
+font-family: serif (original property was font)
 
 Font for 'small-caps bold 14px/28px Arial, sans-serif':
-font-family: Arial, sans-serif (original property was font)
-font-size: 14px (original property was font)
-font-style: normal (original property was font and property was implicitly set.)
 font-variant: small-caps (original property was font)
 font-weight: bold (original property was font)
+font-style: normal (original property was font and property was implicitly set.)
+font-size: 14px (original property was font)
 line-height: 28px (original property was font)
+font-family: Arial, sans-serif (original property was font)
 
 Font for 'italic small-caps bold 14px/28px Arial, sans-serif':
-font-family: Arial, sans-serif (original property was font)
-font-size: 14px (original property was font)
 font-style: italic (original property was font)
 font-variant: small-caps (original property was font)
 font-weight: bold (original property was font)
+font-size: 14px (original property was font)
 line-height: 28px (original property was font)
+font-family: Arial, sans-serif (original property was font)
 
 Font for 'italic small-caps bold 12px/24px':
 
@@ -77,4 +77,10 @@ Font for 'italic small-caps bold 12px':
 
 Font for 'italic small-caps bold /12px serif':
 
+Font for 'italic small-caps small-caps 12px serif':
+
+Font for 'italic italic small-caps bold 12px serif':
+
+Font for '12px/italic serif':
+
 
index fc0823d..0f3bb98 100644 (file)
@@ -41,6 +41,9 @@ testFontValue("italic small-caps bold 14px/28px Arial, sans-serif");
 testFontValue("italic small-caps bold 12px/24px");
 testFontValue("italic small-caps bold 12px");
 testFontValue("italic small-caps bold /12px serif");
+testFontValue("italic small-caps small-caps 12px serif");
+testFontValue("italic italic small-caps bold 12px serif");
+testFontValue("12px/italic serif");
 </script>
 </body>
 </html>
index 36f92b9..50b594f 100644 (file)
@@ -14,10 +14,10 @@ margin-right: 1em (original property was margin and property was implicitly set.
 margin-bottom: 1em (original property was margin and property was implicitly set.)
 margin-left: 1em (original property was margin and property was implicitly set.)
 color: white
-font-family: 'Lucida Grande' (original property was font)
-font-size: 24px (original property was font)
 font-style: normal (original property was font and property was implicitly set.)
 font-variant: normal (original property was font and property was implicitly set.)
 font-weight: normal (original property was font and property was implicitly set.)
+font-size: 24px (original property was font)
 line-height: normal (original property was font and property was implicitly set.)
+font-family: 'Lucida Grande' (original property was font)
 
index 5b266fc..eddd55e 100644 (file)
@@ -1,3 +1,30 @@
+2012-04-23  Alexis Menard  <alexis.menard@openbossa.org>
+
+        Simplify CSSParser::parseFont.
+        https://bugs.webkit.org/show_bug.cgi?id=78698
+
+        Reviewed by Antti Koivisto.
+
+        Simplify parseFont by sharing the code we have for
+        the longhands of the font property.
+
+        No new tests : Extend the existing font shorthand test and modify expected files
+        as now the order of the longhands added in the property list of the style
+        has changed. It's very unlikely that some code is relying on this order though. It will
+        also match the way the spec order them http://www.w3.org/TR/css3-fonts/#font-prop
+        even though the order is arbitrary for some values.
+
+        * css/CSSParser.cpp:
+        (WebCore::CSSParser::parseValue):
+        (WebCore::CSSParser::parseFont):
+        (WebCore::CSSParser::parseLineHeight):
+        (WebCore):
+        (WebCore::CSSParser::parseFontSize):
+        (WebCore::CSSParser::parseFontWeight):  Fix a bug discovered while using parseFontWeight from
+        the parseFont (case font: 0/0, Arial, sans-serif; in a layout test), we should return true only
+        when we add something in the property list.
+        * css/CSSParser.h:
+
 2012-04-23  Yury Semikhatsky  <yurys@chromium.org>
 
         Web Inspector: improve the way heap snapshot diff is calculated
index afd1bd3..09cf536 100644 (file)
@@ -1525,9 +1525,11 @@ bool CSSParser::parseValue(CSSPropertyID propId, bool important)
             validPrimitive = true;
         break;
 
-    case CSSPropertyFontWeight:  // normal | bold | bolder | lighter | 100 | 200 | 300 | 400 | 500 | 600 | 700 | 800 | 900 | inherit
+    case CSSPropertyFontWeight:  { // normal | bold | bolder | lighter | 100 | 200 | 300 | 400 | 500 | 600 | 700 | 800 | 900 | inherit
+        if (m_valueList->size() != 1)
+            return false;
         return parseFontWeight(important);
-
+    }
     case CSSPropertyBorderSpacing: {
         if (num == 1) {
             ShorthandScope scope(this, CSSPropertyBorderSpacing);
@@ -1777,12 +1779,7 @@ bool CSSParser::parseValue(CSSPropertyID propId, bool important)
         break;
 
     case CSSPropertyFontSize:
-        // <absolute-size> | <relative-size> | <length> | <percentage> | inherit
-        if (id >= CSSValueXxSmall && id <= CSSValueLarger)
-            validPrimitive = true;
-        else
-            validPrimitive = (validUnit(value, FLength | FPercent | FNonNeg));
-        break;
+        return parseFontSize(important);
 
     case CSSPropertyFontVariant:         // normal | small-caps | inherit
         return parseFontVariant(important);
@@ -1838,12 +1835,8 @@ bool CSSParser::parseValue(CSSPropertyID propId, bool important)
         validPrimitive = (!id && validUnit(value, FInteger, CSSQuirksMode));
         break;
 
-    case CSSPropertyLineHeight:          // normal | <number> | <length> | <percentage> | inherit
-        if (id == CSSValueNormal)
-            validPrimitive = true;
-        else
-            validPrimitive = (!id && validUnit(value, FNumber | FLength | FPercent | FNonNeg));
-        break;
+    case CSSPropertyLineHeight:
+        return parseLineHeight(important);
     case CSSPropertyCounterIncrement:    // [ <identifier> <integer>? ]+ | none | inherit
         if (id != CSSValueNone)
             return parseCounter(propId, 1, important);
@@ -4330,111 +4323,50 @@ bool CSSParser::parseExclusionShape(bool shapeInside, bool important)
 // [ 'font-style' || 'font-variant' || 'font-weight' ]? 'font-size' [ / 'line-height' ]? 'font-family'
 bool CSSParser::parseFont(bool important)
 {
-    bool valid = true;
-    bool styleImplicit = true;
-    bool variantImplicit = true;
-    bool weightImplicit = true;
-    bool lineHeightImplicit = true;
-    int valueOrdinal = 0;
-
-    CSSParserValue *value = m_valueList->current();
-    RefPtr<FontValue> font = FontValue::create();
-    // Optional font-style, font-variant and font-weight.
-    while (value) {
-        int id = value->id;
-        if (id == CSSValueInitial || id == CSSValueInherit)
+    // Let's check if there is an inherit or initial somewhere in the shorthand.
+    for (unsigned i = 0; i < m_valueList->size(); ++i) {
+        if (m_valueList->valueAt(i)->id == CSSValueInherit || m_valueList->valueAt(i)->id == CSSValueInitial)
             return false;
-        if (id) {
-            if (id == CSSValueNormal) {
-                // It's the initial value for all three, so mark the corresponding longhand as explicit.
-                switch (valueOrdinal) {
-                case 0:
-                    styleImplicit = false;
-                    break;
-                case 1:
-                    variantImplicit = false;
-                    break;
-                case 2:
-                    weightImplicit = false;
-                    break;
-                }
-            } else if (id == CSSValueItalic || id == CSSValueOblique) {
-                if (font->style)
-                    return false;
-                font->style = cssValuePool().createIdentifierValue(id);
-                styleImplicit = false;
-            } else if (id == CSSValueSmallCaps) {
-                if (font->variant)
-                    return false;
-                font->variant = cssValuePool().createIdentifierValue(id);
-                variantImplicit = false;
-            } else if (id >= CSSValueBold && id <= CSSValueLighter) {
-                if (font->weight)
-                    return false;
-                font->weight = cssValuePool().createIdentifierValue(id);
-                weightImplicit = false;
-            } else
-                valid = false;
-        } else if (!font->weight && validUnit(value, FInteger | FNonNeg, CSSStrictMode)) {
-            int weight = static_cast<int>(value->fValue);
-            int val = 0;
-            if (weight == 100)
-                val = CSSValue100;
-            else if (weight == 200)
-                val = CSSValue200;
-            else if (weight == 300)
-                val = CSSValue300;
-            else if (weight == 400)
-                val = CSSValue400;
-            else if (weight == 500)
-                val = CSSValue500;
-            else if (weight == 600)
-                val = CSSValue600;
-            else if (weight == 700)
-                val = CSSValue700;
-            else if (weight == 800)
-                val = CSSValue800;
-            else if (weight == 900)
-                val = CSSValue900;
+    }
 
-            if (val) {
-                font->weight = cssValuePool().createIdentifierValue(val);
-                weightImplicit = false;
-            } else
-                valid = false;
-        } else {
-            valid = false;
-        }
-        if (!valid)
+    ShorthandScope scope(this, CSSPropertyFont);
+    // Optional font-style, font-variant and font-weight.
+    bool fontStyleParsed = false;
+    bool fontVariantParsed = false;
+    bool fontWeightParsed = false;
+    CSSParserValue* value;
+    while ((value = m_valueList->current())) {
+        if (!fontStyleParsed && isValidKeywordPropertyAndValue(CSSPropertyFontStyle, value->id)) {
+            addProperty(CSSPropertyFontStyle, cssValuePool().createIdentifierValue(value->id), important);
+            fontStyleParsed = true;
+        } else if (!fontVariantParsed && (value->id == CSSValueNormal || value->id == CSSValueSmallCaps)) {
+            // Font variant in the shorthand is particular, it only accepts normal or small-caps.
+            addProperty(CSSPropertyFontVariant, cssValuePool().createIdentifierValue(value->id), important);
+            fontVariantParsed = true;
+        } else if (!fontWeightParsed && parseFontWeight(important))
+            fontWeightParsed = true;
+        else
             break;
-        value = m_valueList->next();
-        ++valueOrdinal;
+        m_valueList->next();
     }
-    if (!value)
-        return false;
 
-    if (value->id == CSSValueInitial || value->id == CSSValueInherit)
+    if (!value)
         return false;
 
-    // Set undefined values to default.
-    if (!font->style)
-        font->style = cssValuePool().createIdentifierValue(CSSValueNormal);
-    if (!font->variant)
-        font->variant = cssValuePool().createIdentifierValue(CSSValueNormal);
-    if (!font->weight)
-        font->weight = cssValuePool().createIdentifierValue(CSSValueNormal);
+    if (!fontStyleParsed)
+        addProperty(CSSPropertyFontStyle, cssValuePool().createIdentifierValue(CSSValueNormal), important, true);
+    if (!fontVariantParsed)
+        addProperty(CSSPropertyFontVariant, cssValuePool().createIdentifierValue(CSSValueNormal), important, true);
+    if (!fontWeightParsed)
+        addProperty(CSSPropertyFontWeight, cssValuePool().createIdentifierValue(CSSValueNormal), important, true);
 
     // Now a font size _must_ come.
     // <absolute-size> | <relative-size> | <length> | <percentage> | inherit
-    if (value->id >= CSSValueXxSmall && value->id <= CSSValueLarger)
-        font->size = cssValuePool().createIdentifierValue(value->id);
-    else if (validUnit(value, FLength | FPercent | FNonNeg))
-        font->size = createPrimitiveNumericValue(value);
-    value = m_valueList->next();
-    if (!font->size || !value)
+    if (!parseFontSize(important))
         return false;
 
-    if (value->id == CSSValueInitial || value->id == CSSValueInherit)
+    value = m_valueList->current();
+    if (!value)
         return false;
 
     if (value->unit == CSSParserValue::Operator && value->iValue == '/') {
@@ -4442,42 +4374,23 @@ bool CSSParser::parseFont(bool important)
         value = m_valueList->next();
         if (!value)
             return false;
-        if (value->id == CSSValueNormal) {
-            // Default value, just mark the property as explicit.
-            lineHeightImplicit = false;
-        } else if (validUnit(value, FNumber | FLength | FPercent | FNonNeg)) {
-            font->lineHeight = createPrimitiveNumericValue(value);
-            lineHeightImplicit = false;
-        } else
-            return false;
-        value = m_valueList->next();
-        if (!value)
+        if (!parseLineHeight(important))
             return false;
-    }
-
-    if (value->id == CSSValueInitial || value->id == CSSValueInherit)
-        return false;
-
-    if (!font->lineHeight)
-        font->lineHeight = cssValuePool().createIdentifierValue(CSSValueNormal);
+    } else
+        addProperty(CSSPropertyLineHeight, cssValuePool().createIdentifierValue(CSSValueNormal), important, true);
 
     // Font family must come now.
-    font->family = parseFontFamily();
-
-    if (m_valueList->current() || !font->family)
+    RefPtr<CSSValue> parsedFamilyValue = parseFontFamily();
+    if (!parsedFamilyValue)
         return false;
 
-    ShorthandScope scope(this, CSSPropertyFont);
-    addProperty(CSSPropertyFontFamily, font->family, important);
-    addProperty(CSSPropertyFontSize, font->size, important);
-    addProperty(CSSPropertyFontStyle, font->style, important, styleImplicit);
-    addProperty(CSSPropertyFontVariant, font->variant, important, variantImplicit);
-    addProperty(CSSPropertyFontWeight, font->weight, important, weightImplicit);
-    addProperty(CSSPropertyLineHeight, font->lineHeight, important, lineHeightImplicit);
+    addProperty(CSSPropertyFontFamily, parsedFamilyValue.release(), important);
 
     // FIXME: http://www.w3.org/TR/2011/WD-css3-fonts-20110324/#font-prop requires that
     // "font-stretch", "font-size-adjust", and "font-kerning" be reset to their initial values
     // but we don't seem to support them at the moment. They should also be added here once implemented.
+    if (m_valueList->current())
+        return false;
 
     return true;
 }
@@ -4576,6 +4489,36 @@ PassRefPtr<CSSValueList> CSSParser::parseFontFamily()
     return list.release();
 }
 
+bool CSSParser::parseLineHeight(bool important)
+{
+    CSSParserValue* value = m_valueList->current();
+    int id = value->id;
+    bool validPrimitive = false;
+    // normal | <number> | <length> | <percentage> | inherit
+    if (id == CSSValueNormal)
+        validPrimitive = true;
+    else
+        validPrimitive = (!id && validUnit(value, FNumber | FLength | FPercent | FNonNeg));
+    if (validPrimitive && (!m_valueList->next() || inShorthand()))
+        addProperty(CSSPropertyLineHeight, parseValidPrimitive(id, value), important);
+    return validPrimitive;
+}
+
+bool CSSParser::parseFontSize(bool important)
+{
+    CSSParserValue* value = m_valueList->current();
+    int id = value->id;
+    bool validPrimitive = false;
+    // <absolute-size> | <relative-size> | <length> | <percentage> | inherit
+    if (id >= CSSValueXxSmall && id <= CSSValueLarger)
+        validPrimitive = true;
+    else
+        validPrimitive = validUnit(value, FLength | FPercent | FNonNeg);
+    if (validPrimitive && (!m_valueList->next() || inShorthand()))
+        addProperty(CSSPropertyFontSize, parseValidPrimitive(id, value), important);
+    return validPrimitive;
+}
+
 bool CSSParser::parseFontVariant(bool important)
 {
     RefPtr<CSSValueList> values;
@@ -4625,9 +4568,6 @@ bool CSSParser::parseFontVariant(bool important)
 
 bool CSSParser::parseFontWeight(bool important)
 {
-    if (m_valueList->size() != 1)
-        return false;
-
     CSSParserValue* value = m_valueList->current();
     if ((value->id >= CSSValueNormal) && (value->id <= CSSValue900)) {
         addProperty(CSSPropertyFontWeight, cssValuePool().createIdentifierValue(value->id), important);
@@ -4635,9 +4575,10 @@ bool CSSParser::parseFontWeight(bool important)
     }
     if (validUnit(value, FInteger | FNonNeg, CSSQuirksMode)) {
         int weight = static_cast<int>(value->fValue);
-        if (!(weight % 100) && weight >= 100 && weight <= 900)
+        if (!(weight % 100) && weight >= 100 && weight <= 900) {
             addProperty(CSSPropertyFontWeight, cssValuePool().createIdentifierValue(CSSValue100 + weight / 100 - 1), important);
-        return true;
+            return true;
+        }
     }
     return false;
 }
index 393885b..2c5a27a 100644 (file)
@@ -157,6 +157,8 @@ public:
 
     static bool fastParseColor(RGBA32&, const String&, bool strict);
 
+    bool parseLineHeight(bool important);
+    bool parseFontSize(bool important);
     bool parseFontVariant(bool important);
     bool parseFontWeight(bool important);
     bool parseFontFaceSrc();