A thread-safe Media Query Parser

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 "MediaTypeNames.h"
+#include "core/css/parser/CSSPropertyParser.h"
+#include "core/css/parser/MediaQueryTokenizer.h"
+
+namespace WebCore {
+
+PassRefPtrWillBeRawPtr<MediaQuerySet> MediaQueryParser::parse(const String& queryString)
+{
+    MediaQueryParser parser(queryString);
+    return parser.parseImpl();
+}
+
+// FIXME: Replace the MediaQueryTokenizer with a generic CSSTokenizer, once there is one,
+// or better yet, replace the MediaQueryParser with a generic thread-safe CSS parser.
+MediaQueryParser::MediaQueryParser(const String& queryString)
+    : m_state(&MediaQueryParser::readRestrictor)
+    , m_querySet(MediaQuerySet::create())
+    , ReadRestrictor(&MediaQueryParser::readRestrictor)

[eseidel, 2014/03/13 17:58:45]

I'm surprised these can't just be static?  Either statics in this file, or on
the class?  Obviously we won't have many MediaQueryParsers, but surprised they
need to carry these as instance variables.

+    , ReadMediaType(&MediaQueryParser::readMediaType)
+    , ReadAnd(&MediaQueryParser::readAnd)
+    , ReadFeatureStart(&MediaQueryParser::readFeatureStart)
+    , ReadFeature(&MediaQueryParser::readFeature)
+    , ReadFeatureColon(&MediaQueryParser::readFeatureColon)
+    , ReadFeatureValue(&MediaQueryParser::readFeatureValue)
+    , ReadFeatureEnd(&MediaQueryParser::readFeatureEnd)
+    , SkipUntilComma(&MediaQueryParser::skipUntilComma)
+    , SkipUntilParenthesis(&MediaQueryParser::skipUntilParenthesis)
+    , Done(&MediaQueryParser::done)
+
+{
+    MediaQueryTokenizer::tokenize(queryString, m_tokens);
+}
+
+void MediaQueryParser::setStateAndRestrict(State state, MediaQuery::Restrictor restrictor)
+{
+    m_mediaQueryData.setRestrictor(restrictor);
+    m_state = state;
+}
+
+// State machine member functions start here
+void MediaQueryParser::readRestrictor(MediaQueryTokenType type, TokenIterator& token)
+{
+    readMediaType(type, token);
+}
+
+void MediaQueryParser::readMediaType(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == LeftParenthesisToken) {
+        m_state = ReadFeature;
+    } else if (type == IdentToken) {
+        if (m_state == ReadRestrictor && equalIgnoringCase(token->value(), "not")) {

[eseidel, 2014/03/13 17:58:45]

I see, so -> dereferences the iterator's current value?

[Yoav Weiss, 2014/03/13 22:19:16]

Yeah

+            setStateAndRestrict(ReadMediaType, MediaQuery::Not);
+        } else if (m_state == ReadRestrictor && equalIgnoringCase(token->value(), "only")) {
+            setStateAndRestrict(ReadMediaType, MediaQuery::Only);
+        } else {
+            m_mediaQueryData.setMediaType(token->value());
+            m_state = ReadAnd;
+        }
+    } else if (type == EOFToken && (!m_querySet->queryVector().size() || m_state != ReadRestrictor)) {
+        m_state = Done;
+    } else {
+        if (type == CommaToken)
+            --token;
+        m_state = SkipUntilComma;
+    }
+}
+
+void MediaQueryParser::readAnd(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == IdentToken && equalIgnoringCase(token->value(), "and")) {
+        m_state = ReadFeatureStart;
+    } else if (type == CommaToken) {
+        m_querySet->addMediaQuery(m_mediaQueryData.takeMediaQueryAndClear());
+        m_state = ReadRestrictor;
+    } else if (type == EOFToken) {
+        m_state = Done;
+    } else {
+        m_state = SkipUntilComma;
+    }
+}
+
+void MediaQueryParser::readFeatureStart(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == LeftParenthesisToken)
+        m_state = ReadFeature;
+    else
+        m_state = SkipUntilComma;
+}
+
+void MediaQueryParser::readFeature(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == IdentToken) {
+        m_mediaQueryData.setMediaFeature(token->value());
+        m_state = ReadFeatureColon;
+    } else {
+        m_state = SkipUntilComma;
+    }
+}
+
+void MediaQueryParser::readFeatureColon(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == ColonToken) {
+        m_state = ReadFeatureValue;
+    } else if (type == RightParenthesisToken || type == EOFToken) {
+        --token;
+        m_state = ReadFeatureEnd;
+    } else {
+        m_state = SkipUntilParenthesis;
+    }
+}
+
+void MediaQueryParser::readFeatureValue(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == DimensionToken && token->unitType() == CSSPrimitiveValue::CSS_UNKNOWN) {
+        m_state = SkipUntilComma;
+    } else {
+        m_mediaQueryData.addParserValue(type, *token);
+        m_state = ReadFeatureEnd;
+    }
+}
+
+void MediaQueryParser::readFeatureEnd(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == RightParenthesisToken || type == EOFToken) {
+        if (m_mediaQueryData.addExpression())
+            m_state = ReadAnd;
+        else
+            m_state = SkipUntilComma;
+    } else if (type == DelimiterToken && token->delimiter() == '/') {
+        m_mediaQueryData.addParserValue(type, *token);
+        m_state = ReadFeatureValue;
+    } else {
+        m_state = SkipUntilParenthesis;
+    }
+}
+
+void MediaQueryParser::skipUntilComma(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == CommaToken || type == EOFToken) {
+        m_state = ReadRestrictor;
+        m_mediaQueryData.clear();
+        m_querySet->addMediaQuery(MediaQuery::createNotAll());
+    }
+}
+
+void MediaQueryParser::skipUntilParenthesis(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == RightParenthesisToken)
+        m_state = SkipUntilComma;
+}
+
+void MediaQueryParser::done(MediaQueryTokenType type, TokenIterator& token) { }
+
+void MediaQueryParser::processToken(TokenIterator& token)
+{
+    MediaQueryTokenType type = token->type();
+
+    // Call the function that handles current state
+    if (type != WhitespaceToken)
+        ((this)->*(m_state))(type, token);
+}
+
+// The state machine loop
+PassRefPtrWillBeRawPtr<MediaQuerySet> MediaQueryParser::parseImpl()
+{
+    for (Vector<MediaQueryToken>::iterator token = m_tokens.begin(); token != m_tokens.end(); ++token)
+        processToken(token);
+
+    if (m_state != ReadAnd && m_state != ReadRestrictor && m_state != Done)
+        m_querySet->addMediaQuery(MediaQuery::createNotAll());
+    else if (m_mediaQueryData.currentMediaQueryChanged())
+        m_querySet->addMediaQuery(m_mediaQueryData.takeMediaQueryAndClear());
+
+    return m_querySet;
+}
+
+MediaQueryData::MediaQueryData()
+    : m_restrictor(MediaQuery::None)
+    , m_mediaType(MediaTypeNames::all)
+    , m_expressions(adoptPtr(new ExpressionHeapVector))

[eseidel, 2014/03/13 17:58:45]

I'm surprised the adoptPtr is needed?

[Yoav Weiss, 2014/03/13 22:19:16]

When doing `, m_expressions(new ExpressionHeapVector())`
I'm getting:  error: no matching constructor for initialization of
'OwnPtr<ExpressionHeapVector>'

It might be related to the lack of assignment operator for ExpressionHeapVector
I saw earlier, related to the fact that the vector contains OwnPtrs. I'm not
sure.

+    , m_mediaTypeSet(false)
+{
+}
+
+void MediaQueryData::clear()
+{
+    m_restrictor = MediaQuery::None;
+    m_mediaType = MediaTypeNames::all;
+    m_mediaTypeSet = false;
+    m_mediaFeature = String();
+    m_valueList.clear();
+    m_expressions = adoptPtr(new ExpressionHeapVector);
+}
+
+PassOwnPtr<MediaQuery> MediaQueryData::takeMediaQueryAndClear()
+{
+    MediaQuery* mediaQuery = new MediaQuery(m_restrictor, m_mediaType, m_expressions.release());

[eseidel, 2014/03/13 17:58:45]

Normally adoptPtr is on the same line as new.  check-webkit-style used to
enforce that.

Then this would read:
OwnPtr<MediaQuery> query = adoptPtr(new MediaQuery...);
clear();
return query.release();

+    clear();
+    return adoptPtr(mediaQuery);
+}
+
+bool MediaQueryData::addExpression()
+{
+    OwnPtr<MediaQueryExp> expression = MediaQueryExp::create(m_mediaFeature, &m_valueList);

[eseidel, 2014/03/13 17:58:45]

We need to rename the create() method if it can return null.  That's not
expected in the general case of create().  That can be a later patch.  But
create() should always return a value.  You can make a different
createIfPossible or something...

+    bool isValid = !!expression;
+    m_expressions->append(expression.release());
+    m_valueList.clear();
+    return isValid;
+}
+
+void MediaQueryData::addParserValue(MediaQueryTokenType type, MediaQueryToken& token)
+{
+    CSSParserValue value;
+    if (type == NumberToken || type == PercentageToken || type == DimensionToken) {
+        value.setFromNumber(token.numericValue(), token.unitType());
+        value.isInt = (token.numericValueType() == IntegerValueType);
+    } else if (type == DelimiterToken) {
+        value.unit = CSSParserValue::Operator;
+        value.iValue = token.delimiter();
+    } else {
+        CSSParserFunction* function = new CSSParserFunction;

[eseidel, 2014/03/13 17:58:45]

How is memory management dealt with here?

[Yoav Weiss, 2014/03/13 22:19:16]

function (CSSParserFunction) is added to value (CSSParserValue), which is added
to m_valueList (CSSParserValueList).
Then when MediaQueryParser is destructed, m_valueList is destructed, and its
destructor loops through the values, and deletes the contained functions. 

+        function->name.init(token.value());
+        value.setFromFunction(function);
+        CSSParserString tokenValue;
+        tokenValue.init(token.value());
+        value.id = cssValueKeywordID(tokenValue);
+    }
+    m_valueList.addValue(value);
+}
+
+void MediaQueryData::setMediaType(const String& mediaType)
+{
+    m_mediaType = mediaType;
+    m_mediaTypeSet = true;
+}
+
+} // namespace WebCore