Fixing inconsistency with the media query spec and other browsers

Source/core/css/MediaList.cpp

 /*
  * (C) 1999-2003 Lars Knoll (knoll@kde.org)
  * Copyright (C) 2004, 2006, 2010, 2012 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
@@ skipping 102 matching lines @@
     unsigned length = string.length();
     unsigned i = 0;
     for (; i < length; ++i) {
         unsigned short c = string[i];
         if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '1' && c <= '9') || (c == '-')))
             break;
     }
     return string.left(i);
 }
 
+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)
 {
-    CSSParser parser(CSSStrictMode);
+    if (mediaString.isEmpty()) {
+        m_queries.clear();
+        return true;
+    }
+
+    Vector<String> 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;
-    Vector<String> list;
-    mediaString.split(',', list);
+    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());
+        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());
     }
-    // ",,,," falls straight through, but is not valid unless fallback
-    if (!m_fallbackToDescriptor && list.isEmpty()) {
-        String strippedMediaString = mediaString.stripWhiteSpace();
-        if (!strippedMediaString.isEmpty())
-            return false;
-    }
+
     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;
 
     for (size_t i = 0; i < m_queries.size(); ++i) {
         MediaQuery* query = m_queries[i].get();
         if (*query == *parsedQuery) {
             m_queries.remove(i);
             return true;
         }
     }
@@ skipping 162 matching lines @@
                     CSSPrimitiveValue* primitiveValue = toCSSPrimitiveValue(cssValue);
                     if (primitiveValue->isDotsPerInch() || primitiveValue->isDotsPerCentimeter())
                         addResolutionWarningMessageToConsole(document, mediaQuerySet->mediaText(), primitiveValue);
                 }
             }
         }
     }
 }
 
 }