[CSS Parser] Consolidate string/ident/url serialization functions
authorhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 8 Dec 2016 00:34:02 +0000 (00:34 +0000)
committerhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 8 Dec 2016 00:34:02 +0000 (00:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=165552

Reviewed by Zalan Bujtas.

Source/WebCore:

Right now CSSParser has string, ident and url serialization functions
called quoteCSStringIfNeeded (which actually serializes both strings and
identifiers), as well as quoteCSSURLIfNeeded.

CSSMarkup already has serialization functions that exist outside of the
CSSParser and that handle serialization of strings, idents and URLs. This
patch eliminates the CSSParser functions and consolidates all of the
serialization to use CSSMarkup's functions.

Note that we are not spec-compliant at all here, and so I had to amend
the functions to support our non-spec-compliant serialization. The goal
of this patch is consolidation and not to fix our broken serialization.

Notable changes include parameterizing string serialization so that
both single and double quotes are supported, since in the existing code
we're sometimes spec-compliant (CSSSelectors) and sometimes not
(CSSPrimitiveValue).

We also overload CSS_STRING primitive value type and have it act as both
a string and a custom identifier. This is lame, since the parser should
have made two different types of objects instead, but since our parser
doesn't do that yet, I added a serializeAsStringOrCustomIdent that
preserves our old behavior of "quote the string only if needed." In this
case what that really meant was "Try to guess that we were originally a
custom ident and leave off quotes if so." This function will go away
once we properly create CSSStringValues and CSSCustomIdentValues instead
of turning the latter into strings.

* css/CSSBasicShapes.cpp:
(WebCore::buildPathString):
* css/CSSImageValue.cpp:
(WebCore::CSSImageValue::customCSSText):
* css/CSSMarkup.cpp:
(WebCore::isCSSTokenizerURL):
(WebCore::serializeString):
(WebCore::serializeURL):
(WebCore::serializeAsStringOrCustomIdent):
(WebCore::serializeURI): Deleted.
* css/CSSMarkup.h:
* css/CSSPrimitiveValue.cpp:
(WebCore::CSSPrimitiveValue::formatNumberForCustomCSSText):
* css/CSSSelector.cpp:
(WebCore::CSSSelector::selectorText):
* css/parser/CSSParser.cpp:
(WebCore::isCSSTokenizerIdent): Deleted.
(WebCore::isCSSTokenizerURL): Deleted.
(WebCore::quoteCSSStringInternal): Deleted.
(WebCore::quoteCSSString): Deleted.
(WebCore::quoteCSSStringIfNeeded): Deleted.
(WebCore::quoteCSSURLIfNeeded): Deleted.
* css/parser/CSSParser.h:
* html/HTMLElement.cpp:
(WebCore::HTMLElement::mapLanguageAttributeToLocale):

LayoutTests:

* fast/css/content-language-only-whitespace-expected.txt:
* fast/css/content-language-with-whitespace-expected.txt:

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

13 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/css/content-language-only-whitespace-expected.txt
LayoutTests/fast/css/content-language-with-whitespace-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/css/CSSBasicShapes.cpp
Source/WebCore/css/CSSImageValue.cpp
Source/WebCore/css/CSSMarkup.cpp
Source/WebCore/css/CSSMarkup.h
Source/WebCore/css/CSSPrimitiveValue.cpp
Source/WebCore/css/CSSSelector.cpp
Source/WebCore/css/parser/CSSParser.cpp
Source/WebCore/css/parser/CSSParser.h
Source/WebCore/html/HTMLElement.cpp

index 419de06..a91435e 100644 (file)
@@ -1,3 +1,13 @@
+2016-12-07  Dave Hyatt  <hyatt@apple.com>
+
+        [CSS Parser] Consolidate string/ident/url serialization functions
+        https://bugs.webkit.org/show_bug.cgi?id=165552
+
+        Reviewed by Zalan Bujtas.
+
+        * fast/css/content-language-only-whitespace-expected.txt:
+        * fast/css/content-language-with-whitespace-expected.txt:
+
 2016-12-07  Ryan Haddad  <ryanhaddad@apple.com>
 
         Marking imported/mathml-in-html5/mathml/presentation-markup/fractions/frac-parameters-1.html as flaky on El Capitan WK2.
index 5d14508..1331701 100644 (file)
@@ -1,6 +1,6 @@
 Test for bug 76701: map HTTP-EQUIV content-language to -webkit-locale. This particular test tests that a content-language with whitespace-only content is ignored. This expectation may change, see bug. HTML5 decrees that the meta element be ignored in case of whitespace only content. It's unclear what other browsers do.
 
-FAIL languageOfNode('x') should be auto. Was '  \9\a  '.
+FAIL languageOfNode('x') should be auto. Was '  \9 \a  '.
 PASS languageOfNode('y') is "ar"
 PASS successfullyParsed is true
 
index ba01c72..477580e 100644 (file)
@@ -1,6 +1,6 @@
 Test for bug 76701: map HTTP-EQUIV content-language to -webkit-locale. This particular test tests that the the pragma-set default language is set to the first sequence of non-whitespace characters of the content-language content. This expectation may change, see bug. This expectation is as per the HTML 5 spec. It appears that Firefox does not exactly do this, but trims the leading and trailing whitespace. It's unclear what IE does.
 
-FAIL languageOfNode('x') should be ja_JP. Was '  \a\9\9ja-JP   \9  zh_CN \9\a\a\9\9'.
+FAIL languageOfNode('x') should be ja_JP. Was '  \a \9 \9 ja-JP   \9  zh_CN \9 \a \a \9 \9 '.
 PASS languageOfNode('y') is "ar"
 PASS successfullyParsed is true
 
index af9f5fe..b243ee1 100644 (file)
@@ -1,3 +1,64 @@
+2016-12-07  Dave Hyatt  <hyatt@apple.com>
+
+        [CSS Parser] Consolidate string/ident/url serialization functions
+        https://bugs.webkit.org/show_bug.cgi?id=165552
+
+        Reviewed by Zalan Bujtas.
+
+        Right now CSSParser has string, ident and url serialization functions
+        called quoteCSStringIfNeeded (which actually serializes both strings and
+        identifiers), as well as quoteCSSURLIfNeeded.
+
+        CSSMarkup already has serialization functions that exist outside of the
+        CSSParser and that handle serialization of strings, idents and URLs. This
+        patch eliminates the CSSParser functions and consolidates all of the
+        serialization to use CSSMarkup's functions.
+
+        Note that we are not spec-compliant at all here, and so I had to amend
+        the functions to support our non-spec-compliant serialization. The goal
+        of this patch is consolidation and not to fix our broken serialization.
+
+        Notable changes include parameterizing string serialization so that
+        both single and double quotes are supported, since in the existing code
+        we're sometimes spec-compliant (CSSSelectors) and sometimes not
+        (CSSPrimitiveValue).
+
+        We also overload CSS_STRING primitive value type and have it act as both
+        a string and a custom identifier. This is lame, since the parser should
+        have made two different types of objects instead, but since our parser
+        doesn't do that yet, I added a serializeAsStringOrCustomIdent that
+        preserves our old behavior of "quote the string only if needed." In this
+        case what that really meant was "Try to guess that we were originally a
+        custom ident and leave off quotes if so." This function will go away
+        once we properly create CSSStringValues and CSSCustomIdentValues instead
+        of turning the latter into strings.
+
+        * css/CSSBasicShapes.cpp:
+        (WebCore::buildPathString):
+        * css/CSSImageValue.cpp:
+        (WebCore::CSSImageValue::customCSSText):
+        * css/CSSMarkup.cpp:
+        (WebCore::isCSSTokenizerURL):
+        (WebCore::serializeString):
+        (WebCore::serializeURL):
+        (WebCore::serializeAsStringOrCustomIdent):
+        (WebCore::serializeURI): Deleted.
+        * css/CSSMarkup.h:
+        * css/CSSPrimitiveValue.cpp:
+        (WebCore::CSSPrimitiveValue::formatNumberForCustomCSSText):
+        * css/CSSSelector.cpp:
+        (WebCore::CSSSelector::selectorText):
+        * css/parser/CSSParser.cpp:
+        (WebCore::isCSSTokenizerIdent): Deleted.
+        (WebCore::isCSSTokenizerURL): Deleted.
+        (WebCore::quoteCSSStringInternal): Deleted.
+        (WebCore::quoteCSSString): Deleted.
+        (WebCore::quoteCSSStringIfNeeded): Deleted.
+        (WebCore::quoteCSSURLIfNeeded): Deleted.
+        * css/parser/CSSParser.h:
+        * html/HTMLElement.cpp:
+        (WebCore::HTMLElement::mapLanguageAttributeToLocale):
+
 2016-12-07  Dean Jackson  <dino@apple.com>
 
         Expose internal API to detect media documents
index 6f67620..dd8f9c7 100644 (file)
@@ -31,7 +31,7 @@
 
 #include "CSSBasicShapes.h"
 
-#include "CSSParser.h"
+#include "CSSMarkup.h"
 #include "CSSPrimitiveValueMappings.h"
 #include "CSSValuePool.h"
 #include "Pair.h"
@@ -216,7 +216,7 @@ static String buildPathString(const WindRule& windRule, const String& path, cons
     else
         result.appendLiteral("path(");
 
-    result.append(quoteCSSString(path));
+    serializeString(path, result);
     result.append(')');
 
     if (box.length()) {
index 2f2a8dc..a00fcbf 100644 (file)
@@ -22,7 +22,8 @@
 #include "CSSImageValue.h"
 
 #include "CSSCursorImageValue.h"
-#include "CSSParser.h"
+#include "CSSMarkup.h"
+#include "CSSPrimitiveValue.h"
 #include "CSSValueKeywords.h"
 #include "CachedImage.h"
 #include "CachedResourceLoader.h"
@@ -94,7 +95,7 @@ bool CSSImageValue::equals(const CSSImageValue& other) const
 
 String CSSImageValue::customCSSText() const
 {
-    return "url(" + quoteCSSURLIfNeeded(m_url) + ')';
+    return serializeURL(m_url);
 }
 
 Ref<CSSValue> CSSImageValue::cloneForCSSOM() const
index 2c578d9..3abff12 100644 (file)
@@ -118,9 +118,53 @@ void serializeIdentifier(const String& identifier, StringBuilder& appendTo, bool
     }
 }
 
-void serializeString(const String& string, StringBuilder& appendTo)
+template <typename CharacterType>
+static inline bool isCSSTokenizerURL(const CharacterType* characters, unsigned length)
+{
+    const CharacterType* end = characters + length;
+    
+    for (; characters != end; ++characters) {
+        CharacterType c = characters[0];
+        switch (c) {
+        case '!':
+        case '#':
+        case '$':
+        case '%':
+        case '&':
+            break;
+        default:
+            if (c < '*')
+                return false;
+            if (c <= '~')
+                break;
+            if (c < 128)
+                return false;
+        }
+    }
+    
+    return true;
+}
+
+// "url" from the CSS tokenizer, minus backslash-escape sequences
+static bool isCSSTokenizerURL(const String& string)
 {
-    appendTo.append('\"');
+    unsigned length = string.length();
+    
+    if (!length)
+        return true;
+    
+    if (string.is8Bit())
+        return isCSSTokenizerURL(string.characters8(), length);
+    return isCSSTokenizerURL(string.characters16(), length);
+}
+
+void serializeString(const String& string, StringBuilder& appendTo, bool useDoubleQuotes)
+{
+    // FIXME: From the CSS OM draft:
+    // To serialize a string means to create a string represented by '"' (U+0022).
+    // We need to switch to using " instead of ', but this involves patching a large
+    // number of tests and changing editing code to not get confused by double quotes.
+    appendTo.append(useDoubleQuotes ? '\"' : '\'');
 
     unsigned index = 0;
     while (index < string.length()) {
@@ -135,21 +179,36 @@ void serializeString(const String& string, StringBuilder& appendTo)
             appendTo.append(c);
     }
 
-    appendTo.append('\"');
+    appendTo.append(useDoubleQuotes ? '\"' : '\'');
 }
 
-String serializeString(const String& string)
+String serializeString(const String& string, bool useDoubleQuotes)
 {
     StringBuilder builder;
-    serializeString(string, builder);
+    serializeString(string, builder, useDoubleQuotes);
     return builder.toString();
 }
 
-String serializeURI(const String& string)
+String serializeURL(const String& string)
 {
-    return "url(" + serializeString(string) + ")";
+    // FIXME: URLS must always be double quoted. From the CSS OM draft:
+    // To serialize a URL means to create a string represented by "url(", followed by
+    // the serialization of the URL as a string, followed by ")".
+    // To keep backwards compatibility with existing tests, for now we only quote if needed and
+    // we use a single quote.
+    return "url(" + (isCSSTokenizerURL(string) ? string : serializeString(string)) + ")";
 }
 
+String serializeAsStringOrCustomIdent(const String& string)
+{
+    if (isCSSTokenizerIdentifier(string)) {
+        StringBuilder builder;
+        serializeIdentifier(string, builder);
+        return builder.toString();
+    }
+    return serializeString(string);
+}
+    
 String serializeFontFamily(const String& string)
 {
     return isCSSTokenizerIdentifier(string) ? string : serializeString(string);
index 4ddafe5..de638eb 100644 (file)
@@ -30,9 +30,13 @@ namespace WebCore {
 
 // Common serializing methods. See: http://dev.w3.org/csswg/cssom/#common-serializing-idioms
 void serializeIdentifier(const String& identifier, StringBuilder& appendTo, bool skipStartChecks = false);
-void serializeString(const String&, StringBuilder& appendTo);
-String serializeString(const String&);
-String serializeURI(const String&);
+void serializeString(const String&, StringBuilder& appendTo, bool useDoubleQuotes = false);
+String serializeString(const String&, bool useDoubleQuotes = false);
+String serializeURL(const String&);
 String serializeFontFamily(const String&);
 
+// FIXME-NEWPARSER: This hybrid "check for both string or ident" function can be removed
+// once we have enabled CSSCustomIdentValue and CSSStringValue.
+String serializeAsStringOrCustomIdent(const String&);
+
 } // namespace WebCore
index 5b0d1fb..78965e8 100644 (file)
@@ -25,7 +25,8 @@
 #include "CSSCalculationValue.h"
 #include "CSSFontFamily.h"
 #include "CSSHelper.h"
-#include "CSSParser.h"
+#include "CSSMarkup.h"
+#include "CSSParserValues.h"
 #include "CSSPrimitiveValueMappings.h"
 #include "CSSPropertyNames.h"
 #include "CSSToLengthConversionData.h"
@@ -1068,11 +1069,14 @@ ALWAYS_INLINE String CSSPrimitiveValue::formatNumberForCustomCSSText() const
         // FIXME: We currently don't handle CSS_DIMENSION properly as we don't store
         // the actual dimension, just the numeric value as a string.
     case CSS_STRING:
-        return quoteCSSStringIfNeeded(m_value.string);
+        // FIME-NEWPARSER: Once we have CSSCustomIdentValue hooked up, this can just be
+        // serializeString, since custom identifiers won't be the same value as strings
+        // any longer.
+        return serializeAsStringOrCustomIdent(m_value.string);
     case CSS_FONT_FAMILY:
-        return quoteCSSStringIfNeeded(m_value.fontFamily->familyName);
+        return serializeFontFamily(m_value.fontFamily->familyName);
     case CSS_URI:
-        return "url(" + quoteCSSURLIfNeeded(m_value.string) + ')';
+        return serializeURL(m_value.string);
     case CSS_VALUE_ID:
         return valueName(m_value.valueID);
     case CSS_PROPERTY_ID:
@@ -1099,7 +1103,7 @@ ALWAYS_INLINE String CSSPrimitiveValue::formatNumberForCustomCSSText() const
         result.append(m_value.counter->identifier());
         if (!separator.isEmpty()) {
             result.appendLiteral(", ");
-            result.append(quoteCSSStringIfNeeded(separator));
+            serializeString(separator, result);
         }
         String listStyle = m_value.counter->listStyle();
         if (!listStyle.isEmpty()) {
index 2388a79..34f3cad 100644 (file)
@@ -697,7 +697,7 @@ String CSSSelector::selectorText(const String& rightSide) const
                     break;
             }
             if (cs->match() != CSSSelector::Set) {
-                serializeString(cs->serializingValue(), str);
+                serializeString(cs->serializingValue(), str, true);
                 if (cs->attributeValueMatchingIsCaseInsensitive())
                     str.appendLiteral(" i]");
                 else
index 8350f32..a9d63c3 100644 (file)
@@ -13699,161 +13699,6 @@ CSSValueID cssValueKeywordID(const CSSParserString& string)
     return string.is8Bit() ? cssValueKeywordID(string.characters8(), length) : cssValueKeywordID(string.characters16(), length);
 }
 
-template <typename CharacterType>
-static inline bool isCSSTokenizerIdent(const CharacterType* characters, unsigned length)
-{
-    const CharacterType* end = characters + length;
-
-    // -?
-    if (characters != end && characters[0] == '-')
-        ++characters;
-
-    // {nmstart}
-    if (characters == end || !(characters[0] == '_' || characters[0] >= 128 || isASCIIAlpha(characters[0])))
-        return false;
-    ++characters;
-
-    // {nmchar}*
-    for (; characters != end; ++characters) {
-        if (!(characters[0] == '_' || characters[0] == '-' || characters[0] >= 128 || isASCIIAlphanumeric(characters[0])))
-            return false;
-    }
-
-    return true;
-}
-
-// "ident" from the CSS tokenizer, minus backslash-escape sequences
-static bool isCSSTokenizerIdent(const String& string)
-{
-    unsigned length = string.length();
-
-    if (!length)
-        return false;
-
-    if (string.is8Bit())
-        return isCSSTokenizerIdent(string.characters8(), length);
-    return isCSSTokenizerIdent(string.characters16(), length);
-}
-
-template <typename CharacterType>
-static inline bool isCSSTokenizerURL(const CharacterType* characters, unsigned length)
-{
-    const CharacterType* end = characters + length;
-    
-    for (; characters != end; ++characters) {
-        CharacterType c = characters[0];
-        switch (c) {
-            case '!':
-            case '#':
-            case '$':
-            case '%':
-            case '&':
-                break;
-            default:
-                if (c < '*')
-                    return false;
-                if (c <= '~')
-                    break;
-                if (c < 128)
-                    return false;
-        }
-    }
-    
-    return true;
-}
-
-// "url" from the CSS tokenizer, minus backslash-escape sequences
-static bool isCSSTokenizerURL(const String& string)
-{
-    unsigned length = string.length();
-
-    if (!length)
-        return true;
-
-    if (string.is8Bit())
-        return isCSSTokenizerURL(string.characters8(), length);
-    return isCSSTokenizerURL(string.characters16(), length);
-}
-
-
-template <typename CharacterType>
-static inline String quoteCSSStringInternal(const CharacterType* characters, unsigned length)
-{
-    // For efficiency, we first pre-calculate the length of the quoted string, then we build the actual one.
-    // Please see below for the actual logic.
-    unsigned quotedStringSize = 2; // Two quotes surrounding the entire string.
-    bool afterEscape = false;
-    for (unsigned i = 0; i < length; ++i) {
-        CharacterType ch = characters[i];
-        if (ch == '\\' || ch == '\'') {
-            quotedStringSize += 2;
-            afterEscape = false;
-        } else if (ch < 0x20 || ch == 0x7F) {
-            quotedStringSize += 2 + (ch >= 0x10);
-            afterEscape = true;
-        } else {
-            quotedStringSize += 1 + (afterEscape && (isASCIIHexDigit(ch) || ch == ' '));
-            afterEscape = false;
-        }
-    }
-
-    StringBuffer<CharacterType> buffer(quotedStringSize);
-    unsigned index = 0;
-    buffer[index++] = '\'';
-    afterEscape = false;
-    for (unsigned i = 0; i < length; ++i) {
-        CharacterType ch = characters[i];
-        if (ch == '\\' || ch == '\'') {
-            buffer[index++] = '\\';
-            buffer[index++] = ch;
-            afterEscape = false;
-        } else if (ch < 0x20 || ch == 0x7F) { // Control characters.
-            buffer[index++] = '\\';
-            placeByteAsHexCompressIfPossible(ch, buffer, index, Lowercase);
-            afterEscape = true;
-        } else {
-            // Space character may be required to separate backslash-escape sequence and normal characters.
-            if (afterEscape && (isASCIIHexDigit(ch) || ch == ' '))
-                buffer[index++] = ' ';
-            buffer[index++] = ch;
-            afterEscape = false;
-        }
-    }
-    buffer[index++] = '\'';
-
-    ASSERT(quotedStringSize == index);
-    return String::adopt(WTFMove(buffer));
-}
-
-// We use single quotes for now because markup.cpp uses double quotes.
-String quoteCSSString(const String& string)
-{
-    // This function expands each character to at most 3 characters ('\u0010' -> '\' '1' '0') as well as adds
-    // 2 quote characters (before and after). Make sure the resulting size (3 * length + 2) will not overflow unsigned.
-
-    unsigned length = string.length();
-
-    if (!length)
-        return ASCIILiteral("\'\'");
-
-    if (length > std::numeric_limits<unsigned>::max() / 3 - 2)
-        return emptyString();
-
-    if (string.is8Bit())
-        return quoteCSSStringInternal(string.characters8(), length);
-    return quoteCSSStringInternal(string.characters16(), length);
-}
-
-String quoteCSSStringIfNeeded(const String& string)
-{
-    return isCSSTokenizerIdent(string) ? string : quoteCSSString(string);
-}
-
-String quoteCSSURLIfNeeded(const String& string)
-{
-    return isCSSTokenizerURL(string) ? string : quoteCSSString(string);
-}
-
 bool isValidNthToken(const CSSParserString& token)
 {
     // The tokenizer checks for the construct of an+b.
index b17f14e..68b2f5a 100644 (file)
@@ -753,10 +753,6 @@ struct CSSParser::Location {
     CSSParserString token;
 };
 
-String quoteCSSString(const String&);
-String quoteCSSStringIfNeeded(const String&);
-String quoteCSSURLIfNeeded(const String&);
-
 bool isValidNthToken(const CSSParserString&);
 
 template <>
index 589ca89..f4be715 100644 (file)
@@ -25,7 +25,7 @@
 #include "config.h"
 #include "HTMLElement.h"
 
-#include "CSSParser.h"
+#include "CSSMarkup.h"
 #include "CSSPropertyNames.h"
 #include "CSSValueKeywords.h"
 #include "CSSValuePool.h"
@@ -117,7 +117,7 @@ void HTMLElement::mapLanguageAttributeToLocale(const AtomicString& value, Mutabl
 {
     if (!value.isEmpty()) {
         // Have to quote so the locale id is treated as a string instead of as a CSS keyword.
-        addPropertyToPresentationAttributeStyle(style, CSSPropertyWebkitLocale, quoteCSSString(value));
+        addPropertyToPresentationAttributeStyle(style, CSSPropertyWebkitLocale, serializeString(value));
     } else {
         // The empty string means the language is explicitly unknown.
         addPropertyToPresentationAttributeStyle(style, CSSPropertyWebkitLocale, CSSValueAuto);