Fixing inconsistency with Media Query append/deleteMedium.

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 17 matching lines @@
 #include "core/dom/Document.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/WebCoreMemoryInstrumentation.h"
 #include "core/page/DOMWindow.h"
 #include "wtf/MemoryInstrumentationVector.h"
 #include "wtf/text/StringBuilder.h"
 
 namespace WebCore {
 
 /* MediaList is used to store 3 types of media related entities which mean the same:
+ *
  * Media Queries, Media Types and Media Descriptors.
- * Currently MediaList always tries to parse media queries and if parsing fails,
- * tries to fallback to Media Descriptors if m_fallbackToDescriptor flag is set.
+ *
  * Slight problem with syntax error handling:
  * CSS 2.1 Spec (http://www.w3.org/TR/CSS21/media.html)
  * specifies that failing media type parsing is a syntax error
  * CSS 3 Media Queries Spec (http://www.w3.org/TR/css3-mediaqueries/)
  * specifies that failing media query is a syntax error
  * HTML 4.01 spec (http://www.w3.org/TR/REC-html40/present/styles.html#adef-media)
  * specifies that Media Descriptors should be parsed with forward-compatible syntax
  * DOM Level 2 Style Sheet spec (http://www.w3.org/TR/DOM-Level-2-Style/)
  * talks about MediaList.mediaText and refers
  *   -  to Media Descriptors of HTML 4.0 in context of StyleSheet
  *   -  to Media Types of CSS 2.0 in context of CSSMediaRule and CSSImportRule
  *
  * These facts create situation where same (illegal) media specification may result in
  * different parses depending on whether it is media attr of style element or part of
  * css @media rule.
  * <style media="screen and resolution > 40dpi"> ..</style> will be enabled on screen devices where as
  * @media screen and resolution > 40dpi {..} will not.
  * This gets more counter-intuitive in JavaScript:
  * document.styleSheets[0].media.mediaText = "screen and resolution > 40dpi" will be ok and
  * enabled, while
  * document.styleSheets[0].cssRules[0].media.mediaText = "screen and resolution > 40dpi" will
  * throw SYNTAX_ERR exception.
  */
 
 MediaQuerySet::MediaQuerySet()
-    : m_fallbackToDescriptor(false)
+    : m_parserMode(MediaQueryNormalMode)
     , m_lastLine(0)
 {
 }
 
-MediaQuerySet::MediaQuerySet(const String& mediaString, bool fallbackToDescriptor)
-    : m_fallbackToDescriptor(fallbackToDescriptor)
+MediaQuerySet::MediaQuerySet(const String& mediaString, MediaQueryParserMode mode)
+    : m_parserMode(mode)
     , 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)
     : RefCounted<MediaQuerySet>()
-    , m_fallbackToDescriptor(o.m_fallbackToDescriptor)
+    , m_parserMode(o.m_parserMode)
     , m_lastLine(o.m_lastLine)
     , m_queries(o.m_queries.size())
 {
     for (unsigned i = 0; i < m_queries.size(); ++i)
         m_queries[i] = o.m_queries[i]->copy();
 }
 
 MediaQuerySet::~MediaQuerySet()
 {
 }
 
 static String parseMediaDescriptor(const String& string)
 {
     // http://www.w3.org/TR/REC-html40/types.html#type-media-descriptors
     // "Each entry is truncated just before the first character that isn't a
     // US ASCII letter [a-zA-Z] (ISO 10646 hex 41-5a, 61-7a), digit [0-9] (hex 30-39),
     // or hyphen (hex 2d)."
     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)
+PassOwnPtr<MediaQuery> MediaQuerySet::parseMediaQuery(const String& queryString, MediaQueryParserMode mode)
 {
     CSSParser parser(CSSStrictMode);
     OwnPtr<MediaQuery> parsedQuery = parser.parseMediaQuery(queryString);
 
     if (parsedQuery)
         return parsedQuery.release();
 
-    if (m_fallbackToDescriptor) {
+    switch (mode) {
+    case MediaQueryForwardCompatibleSyntaxMode: {
         String medium = parseMediaDescriptor(queryString);
         if (!medium.isNull())
             return adoptPtr(new MediaQuery(MediaQuery::None, medium, nullptr));

[apavlov, 2013/05/29 15:44:36]

Should you add something like
// Fall through.
after this line?

[kenneth.r.christiansen, 2013/05/29 15:52:56]

I actually had that :-) Let me readd that

     }
-
-    return adoptPtr(new MediaQuery(MediaQuery::None, "not all", nullptr));
+    case MediaQueryNormalMode:
+        return adoptPtr(new MediaQuery(MediaQuery::None, "not all", nullptr));
+    case MediaQueryStrictMode:
+    default:

[apavlov, 2013/05/29 15:44:36]

Should we guard ourselves against the default-case by ASSERT_NOT_REACHED() ?

[kenneth.r.christiansen, 2013/05/29 15:52:56]

Fine with me

+        return nullptr;
+    }
 }
 
-bool MediaQuerySet::parse(const String& mediaString)
+void MediaQuerySet::parseMediaQueryList(const String& mediaString, MediaQueryParserMode mode, Vector<OwnPtr<MediaQuery> >& result)
 {
     if (mediaString.isEmpty()) {
-        m_queries.clear();
-        return true;
+        result.clear();
+        return;
     }
 
     Vector<String> list;
     // FIXME: This is too simple as it shouldn't split when the ',' is inside
     // 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))
+        OwnPtr<MediaQuery> parsedQuery = parseMediaQuery(queryString, mode);
+        if (parsedQuery)
             result.uncheckedAppend(parsedQuery.release());
     }
+}
 
+bool MediaQuerySet::set(const String& mediaString)
+{
+    Vector<OwnPtr<MediaQuery> > result;
+    parseMediaQueryList(mediaString, m_parserMode, result);
     m_queries.swap(result);
     return true;
 }
 
 bool MediaQuerySet::add(const String& queryString)
 {
-    if (OwnPtr<MediaQuery> parsedQuery = parseMediaQuery(queryString)) {
-        m_queries.append(parsedQuery.release());
+    // 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

[apavlov, 2013/05/29 15:44:36]

Should the "parse a media query list" be quoted so that the sentence structure
would become a bit more clear? (I've been trying to attribute "a" to "steps",
which is wrong, while "parse a media query list" sounds like an algorithm name
from the respective spec.)

[kenneth.r.christiansen, 2013/05/29 15:52:56]

Just quotes around that? That is fine with me.

+    // is returned or a media query if a single media query is returned.
+    Vector<OwnPtr<MediaQuery> > queries;
+    parseMediaQueryList(queryString, MediaQueryStrictMode, queries);
+
+    // If "null", terminate these steps.

[apavlov, 2013/05/29 15:44:36]

Can it be more than 1, as opposed to 0?

[kenneth.r.christiansen, 2013/05/29 15:52:56]

No, it cannot according to the spec. It has to be exactly one. The spec
considers the result equal to "null" if it is not exacltly one

+    if (queries.size() != 1)
         return true;
+
+    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)
-        return false;
+    // To parse a media query for a given string means to follow the parse

[apavlov, 2013/05/29 15:44:36]

Ditto for quoting

+    // 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, MediaQueryStrictMode, queries);
 
+    // If "null", terminate these steps.

[apavlov, 2013/05/29 15:44:36]

Ditto for the comment

[kenneth.r.christiansen, 2013/05/29 15:52:56]

Suggestions for the comments?

+    if (queries.size() != 1)
+        return true;
+
+    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.

[apavlov, 2013/05/29 15:44:36]

What is "m"?

[kenneth.r.christiansen, 2013/05/29 15:52:56]

Copy paste from the algorithm, forgot to remove the 'm'

+    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)
 {
     m_queries.append(mediaQuery);
 }
 
 String MediaQuerySet::mediaText() const
 {
     StringBuilder text;
@@ skipping 26 matching lines @@
     : m_mediaQueries(mediaQueries)
     , m_parentStyleSheet(0)
     , m_parentRule(parentRule)
 {
 }
 
 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();
 }
 
 String MediaList::item(unsigned index) const
 {
     const Vector<OwnPtr<MediaQuery> >& queries = m_mediaQueries->queryVector();
     if (index < queries.size())
         return queries[index]->cssText();
     return String();
@@ skipping 11 matching lines @@
     if (m_parentStyleSheet)
         m_parentStyleSheet->didMutate();
 }
 
 void MediaList::appendMedium(const String& medium, ExceptionCode& ec)
 {
     CSSStyleSheet::RuleMutationScope mutationScope(m_parentRule);
 
     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();
 }
 
 void MediaList::reattach(MediaQuerySet* mediaQueries)
 {
     ASSERT(mediaQueries);
     m_mediaQueries = mediaQueries;
 }
 
@@ skipping 61 matching lines @@
                     CSSPrimitiveValue* primitiveValue = toCSSPrimitiveValue(cssValue);
                     if (primitiveValue->isDotsPerInch() || primitiveValue->isDotsPerCentimeter())
                         addResolutionWarningMessageToConsole(document, mediaQuerySet->mediaText(), primitiveValue);
                 }
             }
         }
     }
 }
 
 }