Get Media Query parser to handle parens, brackets and braces blocks correctly

Source/core/css/parser/MediaQueryParser.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "config.h"
 #include "core/css/parser/MediaQueryParser.h"
 
 #include "MediaTypeNames.h"
 #include "core/css/parser/CSSPropertyParser.h"
 #include "core/css/parser/MediaQueryTokenizer.h"
 
 namespace WebCore {
 
 PassRefPtrWillBeRawPtr<MediaQuerySet> MediaQueryParser::parse(const String& queryString)
 {
     return MediaQueryParser(queryString).parseImpl();
 }
 
 const MediaQueryParser::State MediaQueryParser::ReadRestrictor = &MediaQueryParser::readRestrictor;
 const MediaQueryParser::State MediaQueryParser::ReadMediaType = &MediaQueryParser::readMediaType;
 const MediaQueryParser::State MediaQueryParser::ReadAnd = &MediaQueryParser::readAnd;
 const MediaQueryParser::State MediaQueryParser::ReadFeatureStart = &MediaQueryParser::readFeatureStart;
 const MediaQueryParser::State MediaQueryParser::ReadFeature = &MediaQueryParser::readFeature;
 const MediaQueryParser::State MediaQueryParser::ReadFeatureColon = &MediaQueryParser::readFeatureColon;
 const MediaQueryParser::State MediaQueryParser::ReadFeatureValue = &MediaQueryParser::readFeatureValue;
 const MediaQueryParser::State MediaQueryParser::ReadFeatureEnd = &MediaQueryParser::readFeatureEnd;
 const MediaQueryParser::State MediaQueryParser::SkipUntilComma = &MediaQueryParser::skipUntilComma;
-const MediaQueryParser::State MediaQueryParser::SkipUntilParenthesis = &MediaQueryParser::skipUntilParenthesis;
+const MediaQueryParser::State MediaQueryParser::SkipUntilBlockEnd = &MediaQueryParser::skipUntilBlockEnd;
 const MediaQueryParser::State MediaQueryParser::Done = &MediaQueryParser::done;
 
 // FIXME: Replace the MediaQueryTokenizer with a generic CSSTokenizer, once there is one,
 // or better yet, replace the MediaQueryParser with a generic thread-safe CSS parser.
 MediaQueryParser::MediaQueryParser(const String& queryString)
     : m_state(&MediaQueryParser::readRestrictor)
     , m_querySet(MediaQuerySet::create())
 {
     MediaQueryTokenizer::tokenize(queryString, m_tokens);
 }
@@ skipping 65 matching lines @@
 }
 
 void MediaQueryParser::readFeatureColon(MediaQueryTokenType type, TokenIterator& token)
 {
     if (type == ColonToken) {
         m_state = ReadFeatureValue;
     } else if (type == RightParenthesisToken || type == EOFToken) {
         --token;
         m_state = ReadFeatureEnd;
     } else {
-        m_state = SkipUntilParenthesis;
+        m_state = SkipUntilBlockEnd;
     }
 }
 
 void MediaQueryParser::readFeatureValue(MediaQueryTokenType type, TokenIterator& token)
 {
     if (type == DimensionToken && token->unitType() == CSSPrimitiveValue::CSS_UNKNOWN) {
         m_state = SkipUntilComma;
     } else {
         m_mediaQueryData.addParserValue(type, *token);
         m_state = ReadFeatureEnd;
     }
 }
 
 void MediaQueryParser::readFeatureEnd(MediaQueryTokenType type, TokenIterator& token)
 {
     if (type == RightParenthesisToken || type == EOFToken) {
         if (m_mediaQueryData.addExpression())
             m_state = ReadAnd;
         else
             m_state = SkipUntilComma;
     } else if (type == DelimiterToken && token->delimiter() == '/') {
         m_mediaQueryData.addParserValue(type, *token);
         m_state = ReadFeatureValue;
     } else {
-        m_state = SkipUntilParenthesis;
+        m_state = SkipUntilBlockEnd;
     }
 }
 
 void MediaQueryParser::skipUntilComma(MediaQueryTokenType type, TokenIterator& token)
 {
     if (type == CommaToken || type == EOFToken) {
         m_state = ReadRestrictor;
         m_mediaQueryData.clear();
         m_querySet->addMediaQuery(MediaQuery::createNotAll());
     }
 }
 
-void MediaQueryParser::skipUntilParenthesis(MediaQueryTokenType type, TokenIterator& token)
+void MediaQueryParser::skipUntilBlockEnd(MediaQueryTokenType type, TokenIterator& token)
 {
-    if (type == RightParenthesisToken)
+    if (m_blockStack.isEmpty())
         m_state = SkipUntilComma;
 }
 
 void MediaQueryParser::done(MediaQueryTokenType type, TokenIterator& token) { }
 
+void MediaQueryParser::observeBlocks(MediaQueryTokenType type)
+{
+    if (type == LeftParenthesisToken) {
+        m_blockStack.append(ParenthesisBlock);
+    } else if (type == RightParenthesisToken) {
+        if (!m_blockStack.isEmpty() && m_blockStack.last() == ParenthesisBlock)

[eseidel, 2014/03/31 16:05:34]

These ifs are the same for each block type.  I would probably ahve used a helper
to make thsi more generic.

switch(tokenType)
{
case leftParen:
    return push(parenBlock);
case rightParen:
    return pop(parenBlock);
...
default:
ASSERT_NOT_REACHED();
}

Or:

if (type == LeftParen)
   push(ParenethsisBlock)
else if (type == RightParen)
     pop(ParenethsisBlock)
...

Or you could even just use something like this:
BlockType blockType; // paren, braces, brackets.
BlockDirection direction;  // push, pop?  or left/right?
blockTypeAndDirection(tokenType, blockType, blockDirection);
if (blockDirection == left)
    push(blockType)
else
   pop(blockType).


Any of those are fine, so whatever you chose.  The current code is rather
verbose.

+            m_blockStack.removeLast();
+    } else if (type == LeftBracketToken) {
+        m_state = SkipUntilBlockEnd;
+        m_blockStack.append(BracketsBlock);
+    } else if (type == RightBracketToken) {
+        if (!m_blockStack.isEmpty() && m_blockStack.last() == BracketsBlock)
+            m_blockStack.removeLast();
+    } else if (type == LeftBraceToken) {
+        m_state = SkipUntilBlockEnd;
+        m_blockStack.append(BracesBlock);
+    } else if (type == RightBraceToken) {
+        if (!m_blockStack.isEmpty() && m_blockStack.last() == BracesBlock)
+            m_blockStack.removeLast();
+    }
+}
+
 void MediaQueryParser::processToken(TokenIterator& token)
 {
     MediaQueryTokenType type = token->type();
 
+    observeBlocks(type);

[eseidel, 2014/03/31 16:05:34]

Why have it be a separate function?  Why not just have it be part of your normal
token-type switch?

+
     // Call the function that handles current state
     if (type != WhitespaceToken && type != CommentToken)
         ((this)->*(m_state))(type, token);
 }
 
 // The state machine loop
 PassRefPtrWillBeRawPtr<MediaQuerySet> MediaQueryParser::parseImpl()
 {
     for (Vector<MediaQueryToken>::iterator token = m_tokens.begin(); token != m_tokens.end(); ++token)
         processToken(token);
@@ skipping 60 matching lines @@
     m_valueList.addValue(value);
 }
 
 void MediaQueryData::setMediaType(const String& mediaType)
 {
     m_mediaType = mediaType;
     m_mediaTypeSet = true;
 }
 
 } // namespace WebCore