[CSS Parser] Eliminate in-place lowercasing in the parser.
authorhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 5 Dec 2016 00:35:34 +0000 (00:35 +0000)
committerhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 5 Dec 2016 00:35:34 +0000 (00:35 +0000)
https://bugs.webkit.org/show_bug.cgi?id=165368

Reviewed by Darin Adler.

Source/WebCore:

Replace the in-place lowercasing that the parser does with new
mechanisms. In-place lowercasing ruins serialization and doesn't
work on CSS parsed from static strings. It also has the side effect
of mutating strings passed in from JavaScript like for querySelectorAll.

For class/id selectors, we now check if the string is lowercase or not.
If it contains uppercase ASCII characters, then we allocate the RareData
for the selector. RareData now has two fields instead of one for the value,
a matching value (all lowercase in quirks mode), and a serializing value (the
original string). Because this is done at the CSSSelector level, the old
parser has been patched as well for these cases.

In addition, in-place lowercasing was done for pseudo-elements, for
media query features, and for attr(). In all of these cases we do
lowercase converting by first checking if it's needed. Serialization will
not retain the original string in these cases, so we may want to revisit
these cases in the future and apply a solution similar to what we did for
selectors.

* css/CSSGrammar.y.in:
* css/CSSSelector.cpp:
(WebCore::CSSSelector::createRareData):
(WebCore::CSSSelector::selectorText):
(WebCore::CSSSelector::RareData::RareData):
(WebCore::CSSSelector::RareData::~RareData):
* css/CSSSelector.h:
(WebCore::CSSSelector::RareData::create):
(WebCore::CSSSelector::setValue):
(WebCore::CSSSelector::value):
(WebCore::CSSSelector::serializingValue):
* css/MediaQueryExp.cpp:
(WebCore::MediaQueryExpression::MediaQueryExpression):
* css/parser/CSSParserToken.cpp:
(WebCore::convertToASCIILowercaseInPlace): Deleted.
(WebCore::CSSParserToken::convertToASCIILowercaseInPlace): Deleted.
* css/parser/CSSParserToken.h:
* css/parser/CSSParserValues.h:
(WebCore::CSSParserSelector::setValue):
* css/parser/CSSPropertyParser.cpp:
(WebCore::consumeAttr):
* css/parser/CSSSelectorParser.cpp:
(WebCore::CSSSelectorParser::consumeId):
(WebCore::CSSSelectorParser::consumeClass):
(WebCore::CSSSelectorParser::consumePseudo):
* css/parser/MediaQueryParser.cpp:
(WebCore::MediaQueryParser::readFeature):

LayoutTests:

* fast/media/mq-pointer-expected.txt:

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

13 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/media/mq-pointer-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/css/CSSGrammar.y.in
Source/WebCore/css/CSSSelector.cpp
Source/WebCore/css/CSSSelector.h
Source/WebCore/css/MediaQueryExp.cpp
Source/WebCore/css/parser/CSSParserToken.cpp
Source/WebCore/css/parser/CSSParserToken.h
Source/WebCore/css/parser/CSSParserValues.h
Source/WebCore/css/parser/CSSPropertyParser.cpp
Source/WebCore/css/parser/CSSSelectorParser.cpp
Source/WebCore/css/parser/MediaQueryParser.cpp

index 3f64830..53b734f 100644 (file)
@@ -1,3 +1,12 @@
+2016-12-04  Dave Hyatt  <hyatt@apple.com>
+
+        [CSS Parser] Eliminate in-place lowercasing in the parser.
+        https://bugs.webkit.org/show_bug.cgi?id=165368
+
+        Reviewed by Darin Adler.
+
+        * fast/media/mq-pointer-expected.txt:
+
 2016-12-04  Matt Baker  <mattbaker@apple.com>
 
         Web Inspector: Assertion Failures breakpoint should respect global Breakpoints enabled setting
index a8a3612..e871aaa 100644 (file)
@@ -1,7 +1,7 @@
 Test the (pointer) and (hover) media features. See Bug 87403 for details.
 
 Query "(pointer)": true
-Query "(pointer)": true
+Query "(Pointer)": true
 Query "(pointer:none)": false
 Query "(pointer:coarse)": false
 Query "(pointer:coARse)": false
index 9d10250..fafc87c 100644 (file)
@@ -1,3 +1,57 @@
+2016-12-04  Dave Hyatt  <hyatt@apple.com>
+
+        [CSS Parser] Eliminate in-place lowercasing in the parser.
+        https://bugs.webkit.org/show_bug.cgi?id=165368
+
+        Reviewed by Darin Adler.
+
+        Replace the in-place lowercasing that the parser does with new
+        mechanisms. In-place lowercasing ruins serialization and doesn't
+        work on CSS parsed from static strings. It also has the side effect
+        of mutating strings passed in from JavaScript like for querySelectorAll.
+
+        For class/id selectors, we now check if the string is lowercase or not.
+        If it contains uppercase ASCII characters, then we allocate the RareData
+        for the selector. RareData now has two fields instead of one for the value,
+        a matching value (all lowercase in quirks mode), and a serializing value (the
+        original string). Because this is done at the CSSSelector level, the old
+        parser has been patched as well for these cases.
+
+        In addition, in-place lowercasing was done for pseudo-elements, for
+        media query features, and for attr(). In all of these cases we do
+        lowercase converting by first checking if it's needed. Serialization will
+        not retain the original string in these cases, so we may want to revisit
+        these cases in the future and apply a solution similar to what we did for
+        selectors.
+
+        * css/CSSGrammar.y.in:
+        * css/CSSSelector.cpp:
+        (WebCore::CSSSelector::createRareData):
+        (WebCore::CSSSelector::selectorText):
+        (WebCore::CSSSelector::RareData::RareData):
+        (WebCore::CSSSelector::RareData::~RareData):
+        * css/CSSSelector.h:
+        (WebCore::CSSSelector::RareData::create):
+        (WebCore::CSSSelector::setValue):
+        (WebCore::CSSSelector::value):
+        (WebCore::CSSSelector::serializingValue):
+        * css/MediaQueryExp.cpp:
+        (WebCore::MediaQueryExpression::MediaQueryExpression):
+        * css/parser/CSSParserToken.cpp:
+        (WebCore::convertToASCIILowercaseInPlace): Deleted.
+        (WebCore::CSSParserToken::convertToASCIILowercaseInPlace): Deleted.
+        * css/parser/CSSParserToken.h:
+        * css/parser/CSSParserValues.h:
+        (WebCore::CSSParserSelector::setValue):
+        * css/parser/CSSPropertyParser.cpp:
+        (WebCore::consumeAttr):
+        * css/parser/CSSSelectorParser.cpp:
+        (WebCore::CSSSelectorParser::consumeId):
+        (WebCore::CSSSelectorParser::consumeClass):
+        (WebCore::CSSSelectorParser::consumePseudo):
+        * css/parser/MediaQueryParser.cpp:
+        (WebCore::MediaQueryParser::readFeature):
+
 2016-12-04  Gyuyoung Kim  <gyuyoung.kim@webkit.org>
 
         Fix a build break on EFL since r209303.
index 4fff3d5..f2ee93a 100644 (file)
@@ -1214,9 +1214,7 @@ specifier:
     IDSEL {
         $$ = new CSSParserSelector;
         $$->setMatch(CSSSelector::Id);
-        if (parser->m_context.mode == HTMLQuirksMode)
-            $1.convertToASCIILowercaseInPlace();
-        $$->setValue($1);
+        $$->setValue($1, parser->m_context.mode == HTMLQuirksMode);
     }
   | HEX {
         if ($1[0] >= '0' && $1[0] <= '9')
@@ -1224,9 +1222,7 @@ specifier:
         else {
             $$ = new CSSParserSelector;
             $$->setMatch(CSSSelector::Id);
-            if (parser->m_context.mode == HTMLQuirksMode)
-                $1.convertToASCIILowercaseInPlace();
-            $$->setValue($1);
+            $$->setValue($1, parser->m_context.mode == HTMLQuirksMode);
         }
     }
   | class
@@ -1238,9 +1234,7 @@ class:
     '.' IDENT {
         $$ = new CSSParserSelector;
         $$->setMatch(CSSSelector::Class);
-        if (parser->m_context.mode == HTMLQuirksMode)
-            $2.convertToASCIILowercaseInPlace();
-        $$->setValue($2);
+        $$->setValue($2, parser->m_context.mode == HTMLQuirksMode);
     }
   ;
 
index f2499a4..2388a79 100644 (file)
@@ -84,7 +84,8 @@ void CSSSelector::createRareData()
     if (m_hasRareData)
         return;
     // Move the value to the rare data stucture.
-    m_data.m_rareData = &RareData::create(adoptRef(m_data.m_value)).leakRef();
+    AtomicString value { adoptRef(m_data.m_value) };
+    m_data.m_rareData = &RareData::create(WTFMove(value)).leakRef();
     m_hasRareData = true;
 }
 
@@ -406,10 +407,10 @@ String CSSSelector::selectorText(const String& rightSide) const
     while (true) {
         if (cs->match() == CSSSelector::Id) {
             str.append('#');
-            serializeIdentifier(cs->value(), str);
+            serializeIdentifier(cs->serializingValue(), str);
         } else if (cs->match() == CSSSelector::Class) {
             str.append('.');
-            serializeIdentifier(cs->value(), str);
+            serializeIdentifier(cs->serializingValue(), str);
         } else if (cs->match() == CSSSelector::PseudoClass) {
             switch (cs->pseudoClassType()) {
 #if ENABLE(FULLSCREEN_API)
@@ -659,7 +660,7 @@ String CSSSelector::selectorText(const String& rightSide) const
                 break;
             default:
                 str.appendLiteral("::");
-                str.append(cs->value());
+                str.append(cs->serializingValue());
             }
         } else if (cs->isAttributeSelector()) {
             str.append('[');
@@ -696,7 +697,7 @@ String CSSSelector::selectorText(const String& rightSide) const
                     break;
             }
             if (cs->match() != CSSSelector::Set) {
-                serializeString(cs->value(), str);
+                serializeString(cs->serializingValue(), str);
                 if (cs->attributeValueMatchingIsCaseInsensitive())
                     str.appendLiteral(" i]");
                 else
@@ -820,8 +821,9 @@ int CSSSelector::nthB() const
     return m_data.m_rareData->m_b;
 }
 
-CSSSelector::RareData::RareData(PassRefPtr<AtomicStringImpl> value)
-    : m_value(value.leakRef())
+CSSSelector::RareData::RareData(AtomicString&& value)
+    : m_matchingValue(value)
+    , m_serializingValue(value)
     , m_a(0)
     , m_b(0)
     , m_attribute(anyQName())
@@ -831,8 +833,6 @@ CSSSelector::RareData::RareData(PassRefPtr<AtomicStringImpl> value)
 
 CSSSelector::RareData::~RareData()
 {
-    if (m_value)
-        m_value->deref();
 }
 
 // a helper function for parsing nth-arguments
index a301590..665a5f9 100644 (file)
@@ -233,6 +233,7 @@ namespace WebCore {
         const AtomicString& tagLowercaseLocalName() const;
 
         const AtomicString& value() const;
+        const AtomicString& serializingValue() const;
         const QualifiedName& attribute() const;
         const AtomicString& attributeCanonicalLocalName() const;
         const AtomicString& argument() const { return m_hasRareData ? m_data.m_rareData->m_argument : nullAtom; }
@@ -240,7 +241,7 @@ namespace WebCore {
         const Vector<AtomicString>* langArgumentList() const { return m_hasRareData ? m_data.m_rareData->m_langArgumentList.get() : nullptr; }
         const CSSSelectorList* selectorList() const { return m_hasRareData ? m_data.m_rareData->m_selectorList.get() : nullptr; }
 
-        void setValue(const AtomicString&);
+        void setValue(const AtomicString&, bool matchLowerCase = false);
         
         // FIXME-NEWPARSER: These two methods can go away once the old parser is gone.
         void setAttribute(const QualifiedName&, bool);
@@ -348,13 +349,18 @@ namespace WebCore {
         CSSSelector& operator=(const CSSSelector&);
 
         struct RareData : public RefCounted<RareData> {
-            static Ref<RareData> create(PassRefPtr<AtomicStringImpl> value) { return adoptRef(*new RareData(value)); }
+            static Ref<RareData> create(AtomicString&& value) { return adoptRef(*new RareData(WTFMove(value))); }
             ~RareData();
 
             bool parseNth();
             bool matchNth(int count);
 
-            AtomicStringImpl* m_value; // Plain pointer to keep things uniform with the union.
+            // For quirks mode, class and id are case-insensitive. In the case where uppercase
+            // letters are used in quirks mode, |m_matchingValue| holds the lowercase class/id
+            // and |m_serializingValue| holds the original string.
+            AtomicString m_matchingValue;
+            AtomicString m_serializingValue;
+            
             int m_a; // Used for :nth-*
             int m_b; // Used for :nth-*
             QualifiedName m_attribute; // used for attribute selector
@@ -364,7 +370,7 @@ namespace WebCore {
             std::unique_ptr<CSSSelectorList> m_selectorList; // Used for :matches() and :not().
         
         private:
-            RareData(PassRefPtr<AtomicStringImpl> value);
+            RareData(AtomicString&& value);
         };
         void createRareData();
 
@@ -459,21 +465,24 @@ inline bool CSSSelector::isAttributeSelector() const
         || match() == CSSSelector::End;
 }
 
-inline void CSSSelector::setValue(const AtomicString& value)
+inline void CSSSelector::setValue(const AtomicString& value, bool matchLowerCase)
 {
     ASSERT(match() != Tag);
+    AtomicString matchingValue = matchLowerCase ? value.convertToASCIILowercase() : value;
+    if (!m_hasRareData && matchingValue != value)
+        createRareData();
+    
     // Need to do ref counting manually for the union.
-    if (m_hasRareData) {
-        if (m_data.m_rareData->m_value)
-            m_data.m_rareData->m_value->deref();
-        m_data.m_rareData->m_value = value.impl();
-        m_data.m_rareData->m_value->ref();
+    if (!m_hasRareData) {
+        if (m_data.m_value)
+            m_data.m_value->deref();
+        m_data.m_value = value.impl();
+        m_data.m_value->ref();
         return;
     }
-    if (m_data.m_value)
-        m_data.m_value->deref();
-    m_data.m_value = value.impl();
-    m_data.m_value->ref();
+
+    m_data.m_rareData->m_matchingValue = WTFMove(matchingValue);
+    m_data.m_rareData->m_serializingValue = value;
 }
 
 inline CSSSelector::CSSSelector()
@@ -567,9 +576,21 @@ inline const AtomicString& CSSSelector::tagLowercaseLocalName() const
 inline const AtomicString& CSSSelector::value() const
 {
     ASSERT(match() != Tag);
+    if (m_hasRareData)
+        return m_data.m_rareData->m_matchingValue;
+
+    // AtomicString is really just an AtomicStringImpl* so the cast below is safe.
+    return *reinterpret_cast<const AtomicString*>(&m_data.m_value);
+}
+
+inline const AtomicString& CSSSelector::serializingValue() const
+{
+    ASSERT(match() != Tag);
+    if (m_hasRareData)
+        return m_data.m_rareData->m_serializingValue;
+    
     // AtomicString is really just an AtomicStringImpl* so the cast below is safe.
-    // FIXME: Perhaps call sites could be changed to accept AtomicStringImpl?
-    return *reinterpret_cast<const AtomicString*>(m_hasRareData ? &m_data.m_rareData->m_value : &m_data.m_value);
+    return *reinterpret_cast<const AtomicString*>(&m_data.m_value);
 }
 
 inline void CSSSelector::setAttributeValueMatchingIsCaseInsensitive(bool isCaseInsensitive)
index 787b662..329ddd1 100644 (file)
@@ -289,7 +289,7 @@ MediaQueryExpression::MediaQueryExpression(const AtomicString& mediaFeature, CSS
 }
 
 MediaQueryExpression::MediaQueryExpression(const String& feature, const Vector<CSSParserToken, 4>& tokenList)
-    : m_mediaFeature(feature)
+    : m_mediaFeature(feature.convertToASCIILowercase())
     , m_isValid(false)
 {
     // Create value for media query expression that must have 1 or more values.
index 721d7ce..d597568 100644 (file)
@@ -476,23 +476,4 @@ void CSSParserToken::serialize(StringBuilder& builder) const
     }
 }
 
-template<typename CharacterType> ALWAYS_INLINE static void convertToASCIILowercaseInPlace(CharacterType* characters, unsigned length)
-{
-    for (unsigned i = 0; i < length; ++i)
-        characters[i] = toASCIILower(characters[i]);
-}
-
-// FIXME-NEWPARSER: Would like to get rid of this operation. Blink uses HTMLParser static lowercase
-// string hashing, but we don't have that code in our HTMLParser.
-void CSSParserToken::convertToASCIILowercaseInPlace()
-{
-    if (!hasStringBacking())
-        return;
-
-    if (m_valueIs8Bit)
-        WebCore::convertToASCIILowercaseInPlace(static_cast<LChar*>(m_valueDataCharRaw), m_valueLength);
-    else
-        WebCore::convertToASCIILowercaseInPlace(static_cast<UChar*>(m_valueDataCharRaw), m_valueLength);
-}
-
 } // namespace WebCore
index 6eb121a..e819755 100644 (file)
@@ -121,8 +121,6 @@ public:
         return StringView(static_cast<const UChar*>(m_valueDataCharRaw), m_valueLength);
     }
 
-    void convertToASCIILowercaseInPlace();
-
     UChar delimiter() const;
     NumericSign numericSign() const;
     NumericValueType numericValueType() const;
index 9097017..823fc4a 100644 (file)
@@ -218,7 +218,7 @@ public:
 
     std::unique_ptr<CSSSelector> releaseSelector() { return WTFMove(m_selector); }
 
-    void setValue(const AtomicString& value) { m_selector->setValue(value); }
+    void setValue(const AtomicString& value, bool matchLowerCase = false) { m_selector->setValue(value, matchLowerCase); }
     
     // FIXME-NEWPARSER: These two methods can go away once old parser is gone.
     void setAttribute(const QualifiedName& value, bool isCaseInsensitive) { m_selector->setAttribute(value, isCaseInsensitive); }
index 179f6b2..c2e4e44 100644 (file)
@@ -2122,10 +2122,10 @@ static RefPtr<CSSValue> consumeAttr(CSSParserTokenRange args, CSSParserContext c
         return nullptr;
     
     CSSParserToken token = args.consumeIncludingWhitespace();
+    auto attrName = token.value().toAtomicString();
     if (context.isHTMLDocument)
-        token.convertToASCIILowercaseInPlace();
+        attrName = attrName.convertToASCIILowercase();
 
-    String attrName = token.value().toString();
     if (!args.atEnd())
         return nullptr;
 
index 09e76ff..2480327 100644 (file)
@@ -389,10 +389,7 @@ std::unique_ptr<CSSParserSelector> CSSSelectorParser::consumeId(CSSParserTokenRa
     // FIXME-NEWPARSER: Avoid having to do this, but the old parser does and we need
     // to be compatible for now.
     CSSParserToken token = range.consume();
-    if (m_context.mode == HTMLQuirksMode)
-        token.convertToASCIILowercaseInPlace();
-    selector->setValue(token.value().toAtomicString());
-
+    selector->setValue(token.value().toAtomicString(), m_context.mode == HTMLQuirksMode);
     return selector;
 }
 
@@ -409,9 +406,7 @@ std::unique_ptr<CSSParserSelector> CSSSelectorParser::consumeClass(CSSParserToke
     // FIXME-NEWPARSER: Avoid having to do this, but the old parser does and we need
     // to be compatible for now.
     CSSParserToken token = range.consume();
-    if (m_context.mode == HTMLQuirksMode)
-        token.convertToASCIILowercaseInPlace();
-    selector->setValue(token.value().toAtomicString());
+    selector->setValue(token.value().toAtomicString(), m_context.mode == HTMLQuirksMode);
 
     return selector;
 }
@@ -512,19 +507,17 @@ std::unique_ptr<CSSParserSelector> CSSSelectorParser::consumePseudo(CSSParserTok
 
     std::unique_ptr<CSSParserSelector> selector;
     
-    // FIXME-NEWPARSER: Would like to eliminate this.
-    const_cast<CSSParserToken&>(token).convertToASCIILowercaseInPlace();
-    
-    StringView value = token.value();
+    auto lowercasedValue = token.value().toString().convertToASCIILowercase();
+    auto value = StringView { lowercasedValue };
     
     // FIXME-NEWPARSER: We can't change the pseudoclass/element maps that the old parser
     // uses without breaking it; this hack allows function selectors to work. When the new
     // parser turns on, we can patch the map and remove this code.
     String newValue;
     if (token.type() == FunctionToken && colons == 1) {
-        String tokenString = value.toString();
-        if (!tokenString.startsWithIgnoringASCIICase("host")) {
-            newValue = value.toString() + "(";
+        auto tokenString = value.toString();
+        if (!value.startsWithIgnoringASCIICase(StringView { "host" })) {
+            newValue = makeString(value, '(');
             value = newValue;
         }
     }
index d4a7224..e2ee697 100644 (file)
@@ -166,8 +166,6 @@ void MediaQueryParser::readFeatureStart(CSSParserTokenType type, const CSSParser
 void MediaQueryParser::readFeature(CSSParserTokenType type, const CSSParserToken& token)
 {
     if (type == IdentToken) {
-        // FIXME-NEWPARSER: Find a way to avoid this.
-        const_cast<CSSParserToken&>(token).convertToASCIILowercaseInPlace();
         m_mediaQueryData.setMediaFeature(token.value().toString());
         m_state = ReadFeatureColon;
     } else