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

Source/core/css/parser/MediaQueryParser.cpp

Index: Source/core/css/parser/MediaQueryParser.cpp
diff --git a/Source/core/css/parser/MediaQueryParser.cpp b/Source/core/css/parser/MediaQueryParser.cpp
index a4afb61bc34c592f2bb66661b740e6d19a0a0ce5..a115335275a5ac9bbc06e72be309a96c55111c5e 100644
--- a/Source/core/css/parser/MediaQueryParser.cpp
+++ b/Source/core/css/parser/MediaQueryParser.cpp
@@ -25,7 +25,7 @@ const MediaQueryParser::State MediaQueryParser::ReadFeatureColon = &MediaQueryPa
 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,
@@ -111,7 +111,7 @@ void MediaQueryParser::readFeatureColon(MediaQueryTokenType type, TokenIterator&
         --token;
         m_state = ReadFeatureEnd;
     } else {
-        m_state = SkipUntilParenthesis;
+        m_state = SkipUntilBlockEnd;
     }
 }
 
@@ -136,7 +136,7 @@ void MediaQueryParser::readFeatureEnd(MediaQueryTokenType type, TokenIterator& t
         m_mediaQueryData.addParserValue(type, *token);
         m_state = ReadFeatureValue;
     } else {
-        m_state = SkipUntilParenthesis;
+        m_state = SkipUntilBlockEnd;
     }
 }
 
@@ -149,18 +149,42 @@ void MediaQueryParser::skipUntilComma(MediaQueryTokenType type, TokenIterator& t
     }
 }
 
-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);