Some invalid media queries are parsed as valid

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 "core/MediaTypeNames.h"
 #include "core/css/parser/CSSPropertyParser.h"
 #include "core/css/parser/CSSTokenizer.h"
@@ skipping 61 matching lines @@
 {
     if (type == IdentToken && equalIgnoringCase(token.value(), "not"))
         setStateAndRestrict(ReadFeatureStart, MediaQuery::Not);
     else
         readFeatureStart(type, token);
 }
 
 void MediaQueryParser::readMediaType(CSSParserTokenType type, const CSSParserToken& token)
 {
     if (type == LeftParenthesisToken) {
-        m_state = ReadFeature;
+        if (m_mediaQueryData.restrictor() != MediaQuery::None)
+            m_state = SkipUntilComma;
+        else
+            m_state = ReadFeature;
     } else if (type == IdentToken) {
         if (m_state == ReadRestrictor && equalIgnoringCase(token.value(), "not")) {
             setStateAndRestrict(ReadMediaType, MediaQuery::Not);
         } else if (m_state == ReadRestrictor && equalIgnoringCase(token.value(), "only")) {
             setStateAndRestrict(ReadMediaType, MediaQuery::Only);
         } else {

[Yoav Weiss, 2015/01/30 08:31:51]

Nit: Can you make that an "else if"?

-            m_mediaQueryData.setMediaType(token.value());
-            m_state = ReadAnd;
+            if (m_mediaQueryData.restrictor() != MediaQuery::None && (equalIgnoringCase(token.value(), "not")
+                || equalIgnoringCase(token.value(), "and")
+                || equalIgnoringCase(token.value(), "or")
+                || equalIgnoringCase(token.value(), "only"))) {

[Yoav Weiss, 2015/01/30 08:31:51]

Can you break out the "equqlIgnoringCase"s into their own helper function?
"isRestrictorOrLogicalOperator" (or a better name :) )?

Also - I don't know if this is a performance bottleneck in any way, but we're
starting to lowercase that token a few times too many... Can you add a FIXME in
that spirit, so we'd later move the lowercasing up the chain.

+                m_state = SkipUntilComma;
+            } else {
+                m_mediaQueryData.setMediaType(token.value());
+                m_state = ReadAnd;
+            }
         }
     } else if (type == EOFToken && (!m_querySet->queryVector().size() || m_state != ReadRestrictor)) {
         m_state = Done;
     } else {
         m_state = SkipUntilComma;
         if (type == CommaToken)
             skipUntilComma(type, token);
     }
 }
 
@@ skipping 179 matching lines @@
     return true;
 }
 
 void MediaQueryData::setMediaType(const String& mediaType)
 {
     m_mediaType = mediaType;
     m_mediaTypeSet = true;
 }
 
 } // namespace blink