Fixing inconsistency with Media Query append/deleteMedium.

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 9ce772ff9740d40184f407813df0074840b65229..8b4e421b88f4d8cc2be49f82e8f804b7478900fe 100644
--- a/Source/core/css/MediaList.cpp
+++ b/Source/core/css/MediaList.cpp
@@ -75,19 +75,7 @@ MediaQuerySet::MediaQuerySet(const String& mediaString, bool fallbackToDescripto
     : m_fallbackToDescriptor(fallbackToDescriptor)
     , m_lastLine(0)
 {
-    bool success = parse(mediaString);
-    // FIXME: parsing can fail. The problem with failing constructor is that
-    // we would need additional flag saying MediaList is not valid
-    // Parse can fail only when fallbackToDescriptor == false, i.e when HTML4 media descriptor
-    // forward-compatible syntax is not in use.
-    // DOMImplementationCSS seems to mandate that media descriptors are used
-    // for both html and svg, even though svg:style doesn't use media descriptors
-    // Currently the only places where parsing can fail are
-    // creating <svg:style>, creating css media / import rules from js
-
-    // FIXME: This doesn't make much sense.
-    if (!success)
-        parse("invalid");
+    set(mediaString);
 }
 
 MediaQuerySet::MediaQuerySet(const MediaQuerySet& o)
@@ -137,11 +125,11 @@ PassOwnPtr<MediaQuery> MediaQuerySet::parseMediaQuery(const String& queryString)
     return adoptPtr(new MediaQuery(MediaQuery::None, "not all", nullptr));
 }
 
-bool MediaQuerySet::parse(const String& mediaString)
+void MediaQuerySet::parseMediaQueryList(const String& mediaString, Vector<OwnPtr<MediaQuery> >& result)
 {
     if (mediaString.isEmpty()) {
-        m_queries.clear();
-        return true;
+        result.clear();
+        return;
     }
 
     Vector<String> list;
@@ -149,42 +137,79 @@ bool MediaQuerySet::parse(const String& mediaString)
     // other allowed matching pairs such as (), [], {}, "", and ''.
     mediaString.split(',', /* allowEmptyEntries */ true, list);
 
-    Vector<OwnPtr<MediaQuery> > result;
     result.reserveInitialCapacity(list.size());
 
     for (unsigned i = 0; i < list.size(); ++i) {
         String queryString = list[i].stripWhiteSpace();
-        if (OwnPtr<MediaQuery> parsedQuery = parseMediaQuery(queryString))
-            result.uncheckedAppend(parsedQuery.release());
+        OwnPtr<MediaQuery> parsedQuery = parseMediaQuery(queryString);
+        ASSERT(parsedQuery);
+        result.uncheckedAppend(parsedQuery.release());
     }
+}
 
+bool MediaQuerySet::set(const String& mediaString)
+{
+    Vector<OwnPtr<MediaQuery> > result;
+    parseMediaQueryList(mediaString, result);
     m_queries.swap(result);
     return true;
 }
 
 bool MediaQuerySet::add(const String& queryString)
 {
-    if (OwnPtr<MediaQuery> parsedQuery = parseMediaQuery(queryString)) {
-        m_queries.append(parsedQuery.release());
-        return true;
+    // To parse a media query for a given string means to follow the parse
+    // a media query list steps and return null if more than one media query
+    // is returned or a media query if a single media query is returned.
+    Vector<OwnPtr<MediaQuery> > queries;
+    parseMediaQueryList(queryString, queries);
+
+    // If "null", terminate these steps.
+    if (queries.size() != 1)
+        return false;
+
+    OwnPtr<MediaQuery> newQuery = queries[0].release();
+    ASSERT(newQuery);
+
+    // If comparing with any of the media queries in the collection of media
+    // queries returns true terminate these steps.
+    for (size_t i = 0; i < m_queries.size(); ++i) {
+        MediaQuery* query = m_queries[i].get();
+        if (*query == *newQuery)
+            return true;
     }
-    return false;
+
+    m_queries.append(newQuery.release());
+    return true;
 }
 
 bool MediaQuerySet::remove(const String& queryStringToRemove)
 {
-    OwnPtr<MediaQuery> parsedQuery = parseMediaQuery(queryStringToRemove);
-    if (!parsedQuery)
+    // To parse a media query for a given string means to follow the parse
+    // a media query list steps and return null if more than one media query
+    // is returned or a media query if a single media query is returned.
+    Vector<OwnPtr<MediaQuery> > queries;
+    parseMediaQueryList(queryStringToRemove, queries);
+
+    // If "null", terminate these steps.
+    if (queries.size() != 1)
         return false;
 
+    OwnPtr<MediaQuery> newQuery = queries[0].release();
+    ASSERT(newQuery);
+
+    // Remove any media query from the collection of media queries for which
+    // comparing the media query with m returns true.
+    bool found = false;
     for (size_t i = 0; i < m_queries.size(); ++i) {
         MediaQuery* query = m_queries[i].get();
-        if (*query == *parsedQuery) {
+        if (*query == *newQuery) {
             m_queries.remove(i);
-            return true;
+            --i;
+            found = true;
         }
     }
-    return false;
+
+    return found;
 }
 
 void MediaQuerySet::addMediaQuery(PassOwnPtr<MediaQuery> mediaQuery)
@@ -231,15 +256,12 @@ MediaList::~MediaList()
 {
 }
 
-void MediaList::setMediaText(const String& value, ExceptionCode& ec)
+void MediaList::setMediaText(const String& value)
 {
     CSSStyleSheet::RuleMutationScope mutationScope(m_parentRule);
 
-    bool success = m_mediaQueries->parse(value);
-    if (!success) {
-        ec = SYNTAX_ERR;
-        return;
-    }
+    m_mediaQueries->set(value);
+
     if (m_parentStyleSheet)
         m_parentStyleSheet->didMutate();
 }
@@ -271,10 +293,10 @@ void MediaList::appendMedium(const String& medium, ExceptionCode& ec)
 
     bool success = m_mediaQueries->add(medium);
     if (!success) {
-        // FIXME: Should this really be INVALID_CHARACTER_ERR?
         ec = INVALID_CHARACTER_ERR;
         return;
     }
+
     if (m_parentStyleSheet)
         m_parentStyleSheet->didMutate();
 }