Fixing inconsistency with the media query spec and other browsers

Source/core/css/MediaList.cpp

Index: Source/core/css/MediaList.cpp
diff --git a/Source/core/css/MediaList.cpp b/Source/core/css/MediaList.cpp
index ebaa05ba9a9db25a3e98523954e854e13f573e22..6fc085c62396689aa6f306a23ef0a8c8529d499d 100644
--- a/Source/core/css/MediaList.cpp
+++ b/Source/core/css/MediaList.cpp
@@ -120,68 +120,62 @@ static String parseMediaDescriptor(const String& string)
     return string.left(i);
 }
 
-bool MediaQuerySet::parse(const String& mediaString)
+PassOwnPtr<MediaQuery> MediaQuerySet::parseMediaQuery(const String& queryString)
 {
+    ASSERT(!queryString.isEmpty());

[apavlov, 2013/05/15 12:28:36]

I cannot see any code that might guarantee the queryString is not empty
(consider the ",,,," case from the original code comment)

+
     CSSParser parser(CSSStrictMode);
+    OwnPtr<MediaQuery> parsedQuery = parser.parseMediaQuery(queryString);
+
+    if (parsedQuery)
+        return parsedQuery.release();
+
+    if (m_fallbackToDescriptor) {
+        String medium = parseMediaDescriptor(queryString);
+        if (!medium.isNull())
+            return adoptPtr(new MediaQuery(MediaQuery::None, medium, nullptr));
+    }
+
+    return adoptPtr(new MediaQuery(MediaQuery::None, "not all", nullptr));
+}
+
+bool MediaQuerySet::parse(const String& mediaString)
+{
+    if (mediaString.isEmpty()) {
+        m_queries.clear();
+        return true;
+    }
 
-    Vector<OwnPtr<MediaQuery> > result;
     Vector<String> list;
-    mediaString.split(',', list);
+    // FIXME: This is too simple as it shouldn't split when the ',' is inside
+    // other allowed matching pairs such as (), [], {}, "", and ''.

[rune, 2013/05/15 11:28:56]

Yes, ideally CSSParser should have a parseMediaQueryList() with an
@-webkit-mediaquerylist production, but not until the error recovery in the
parser is better.

+    mediaString.split(',', /* allowEmptyEntries */ true, list);
+
+    Vector<OwnPtr<MediaQuery> > result;
+    result.reserveInitialCapacity(list.size());
+
     for (unsigned i = 0; i < list.size(); ++i) {
-        String medium = list[i].stripWhiteSpace();
-        if (medium.isEmpty()) {
-            if (!m_fallbackToDescriptor)
-                return false;
-            continue;
-        }
-        OwnPtr<MediaQuery> mediaQuery = parser.parseMediaQuery(medium);
-        if (!mediaQuery) {
-            if (!m_fallbackToDescriptor)
-                return false;
-            String mediaDescriptor = parseMediaDescriptor(medium);
-            if (mediaDescriptor.isNull())
-                continue;
-            mediaQuery = adoptPtr(new MediaQuery(MediaQuery::None, mediaDescriptor, nullptr));
-        }
-        result.append(mediaQuery.release());
-    }
-    // ",,,," falls straight through, but is not valid unless fallback
-    if (!m_fallbackToDescriptor && list.isEmpty()) {
-        String strippedMediaString = mediaString.stripWhiteSpace();
-        if (!strippedMediaString.isEmpty())
-            return false;
+        String queryString = list[i].stripWhiteSpace();
+        if (OwnPtr<MediaQuery> parsedQuery = parseMediaQuery(queryString))

[apavlov, 2013/05/15 12:28:36]

Shouldn't the code check that queryString is not empty before calling this?

+            result.uncheckedAppend(parsedQuery.release());
     }
+
     m_queries.swap(result);
     return true;
 }
 
 bool MediaQuerySet::add(const String& queryString)
 {
-    CSSParser parser(CSSStrictMode);
-
-    OwnPtr<MediaQuery> parsedQuery = parser.parseMediaQuery(queryString);
-    if (!parsedQuery && m_fallbackToDescriptor) {
-        String medium = parseMediaDescriptor(queryString);
-        if (!medium.isNull())
-            parsedQuery = adoptPtr(new MediaQuery(MediaQuery::None, medium, nullptr));
+    if (OwnPtr<MediaQuery> parsedQuery = parseMediaQuery(queryString)) {
+        m_queries.append(parsedQuery.release());
+        return true;
     }
-    if (!parsedQuery)
-        return false;
-
-    m_queries.append(parsedQuery.release());
-    return true;
+    return false;
 }
 
 bool MediaQuerySet::remove(const String& queryStringToRemove)
 {
-    CSSParser parser(CSSStrictMode);
-
-    OwnPtr<MediaQuery> parsedQuery = parser.parseMediaQuery(queryStringToRemove);
-    if (!parsedQuery && m_fallbackToDescriptor) {
-        String medium = parseMediaDescriptor(queryStringToRemove);
-        if (!medium.isNull())
-            parsedQuery = adoptPtr(new MediaQuery(MediaQuery::None, medium, nullptr));
-    }
+    OwnPtr<MediaQuery> parsedQuery = parseMediaQuery(queryStringToRemove);
     if (!parsedQuery)
         return false;