A thread-safe Media Query Parser

Source/core/css/parser/MediaQueryParser.cpp

Index: Source/core/css/parser/MediaQueryParser.cpp
diff --git a/Source/core/css/parser/MediaQueryParser.cpp b/Source/core/css/parser/MediaQueryParser.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..312ce42a72511536ea6bd3b373cb14ab976719a4
--- /dev/null
+++ b/Source/core/css/parser/MediaQueryParser.cpp
@@ -0,0 +1,226 @@
+// 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(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(String queryString)

[eseidel, 2014/03/11 20:14:19]

I think String is normally passed const String&?  It's basically just
RefPtr<StringImpl> :)

+    : m_state(&MediaQueryParser::readRestrictor)
+{
+    MediaQueryTokenizer::tokenize(queryString, m_tokens);
+}
+
+// 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 = &MediaQueryParser::readFeature;

[eseidel, 2014/03/11 20:14:19]

Intersting that you're using function pointers as state.  I might try making
this more readable by using some helper private consts like:

const StateFunction ReadFeature = &readFeature;

Then you can write:

m_state = ReadFeature;

or

m_state = kReadFeature;

or 

m_state = ReadFeatureState;

and it might read easier, since you type these a lot.  The cost is obviously
some boiler-plate in the header or top of the .cpp file, but I suspect it will
be easier on the eyes than the current code. :)

[Yoav Weiss, 2014/03/12 20:39:00]

I like that :)

+    } else if (type == IdentToken) {
+        if (m_state == &MediaQueryParser::readRestrictor && token->value().lower() == "not") {

[eseidel, 2014/03/11 20:14:19]

We have case-insensitive comparison functions.

equalIgnoringCase(token->value(), "not");

You can even make your own helper which takes a token if you type this a lot:

equalIgnoringCase(token, "not");

Or even:
token->hasValue("not")

+            m_mediaQueryData.setRestrictor(MediaQuery::Not);

[eseidel, 2014/03/11 20:14:19]

If these are often tied, you might add a helper method:

setStateAndRestrict(ReadMediaTypeState, MediaQuery::Not);

+            m_state = &MediaQueryParser::readMediaType;
+        } else if (m_state == &MediaQueryParser::readRestrictor && token->value().lower() == "only") {
+            m_mediaQueryData.setRestrictor(MediaQuery::Only);
+            m_state = &MediaQueryParser::readMediaType;
+        } else {
+            m_mediaQueryData.setMediaType(token->value());
+            m_state = &MediaQueryParser::readAnd;
+        }
+    } else if (type == EOFToken && (!m_mediaQueryData.querySetSize() || m_state != &MediaQueryParser::readRestrictor)) {
+        m_state = &MediaQueryParser::done;
+    } else {
+        if (type == CommaToken)
+            --token;
+        m_state = &MediaQueryParser::skipUntilComma;
+    }
+}
+
+void MediaQueryParser::readAnd(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == IdentToken && token->value().lower() == "and") {
+        m_state = &MediaQueryParser::readFeatureStart;
+    } else if (type == CommaToken) {
+        m_mediaQueryData.addCurrentMediaQuery();
+        m_state = &MediaQueryParser::readRestrictor;
+    } else if (type == EOFToken) {
+        m_state = &MediaQueryParser::done;
+    } else {
+        m_state = &MediaQueryParser::skipUntilComma;
+    }
+}
+
+void MediaQueryParser::readFeatureStart(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == LeftParenthesisToken)
+        m_state = &MediaQueryParser::readFeature;
+    else
+        m_state = &MediaQueryParser::skipUntilComma;
+}
+
+void MediaQueryParser::readFeature(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == IdentToken) {
+        m_mediaQueryData.setMediaFeature(token->value());
+        m_state = &MediaQueryParser::readFeatureColon;
+    } else {
+        m_state = &MediaQueryParser::skipUntilComma;
+    }
+}
+
+void MediaQueryParser::readFeatureColon(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == ColonToken) {
+        m_state = &MediaQueryParser::readFeatureValue;
+    } else if (type == RightParenthesisToken || type == EOFToken) {
+        --token;
+        m_state = &MediaQueryParser::readFeatureEnd;
+    } else {
+        m_state = &MediaQueryParser::skipUntilParenthesis;
+    }
+}
+
+void MediaQueryParser::readFeatureValue(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == DimensionToken && token->unitType() == CSSPrimitiveValue::CSS_UNKNOWN) {
+        m_state = &MediaQueryParser::skipUntilComma;
+    } else {
+        m_mediaQueryData.addParserValue(type, token);
+        m_state = &MediaQueryParser::readFeatureEnd;
+    }
+}
+
+void MediaQueryParser::readFeatureEnd(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == RightParenthesisToken || type == EOFToken) {
+        if (m_mediaQueryData.addExpression())
+            m_state = &MediaQueryParser::readAnd;
+        else
+            m_state = &MediaQueryParser::skipUntilComma;
+    } else if (type == DelimiterToken && token->delimiter() == '/') {
+        m_mediaQueryData.addParserValue(type, token);
+        m_state = &MediaQueryParser::readFeatureValue;
+    } else {
+        m_state = &MediaQueryParser::skipUntilParenthesis;
+    }
+}
+
+void MediaQueryParser::skipUntilComma(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == CommaToken || type == EOFToken) {
+        m_state = &MediaQueryParser::readRestrictor;
+        m_mediaQueryData.resetCurrentQuery();
+        m_mediaQueryData.addNotAllMediaQuery();
+    }
+}
+
+void MediaQueryParser::skipUntilParenthesis(MediaQueryTokenType type, TokenIterator& token)
+{
+    if (type == RightParenthesisToken)
+        m_state = &MediaQueryParser::skipUntilComma;
+}
+
+void MediaQueryParser::done(MediaQueryTokenType type, TokenIterator& token) { }
+
+// The state machine loop
+PassRefPtrWillBeRawPtr<MediaQuerySet> MediaQueryParser::parseImpl()
+{
+    MediaQueryTokenType type;
+
+    for (Vector<MediaQueryToken>::iterator token = m_tokens.begin(); token != m_tokens.end(); ++token) {
+        type = token->type();
+        if (type == WhitespaceToken)
+            continue;
+
+        // Call the function that handles current state
+        ((this)->*(m_state))(type, token);

[eseidel, 2014/03/11 20:14:19]

I might have moved this loop body into a helper function:

handleToken(token) or processToken(token);

+    }
+
+    if (m_state != &MediaQueryParser::readAnd && m_state != &MediaQueryParser::readRestrictor && m_state != &MediaQueryParser::done)
+        m_mediaQueryData.addNotAllMediaQuery();
+    else if (m_mediaQueryData.currentMediaQueryChanged())
+        m_mediaQueryData.addCurrentMediaQuery();
+
+    return m_mediaQueryData.querySet();
+}
+
+MediaQueryData::MediaQueryData()
+    : m_querySet(MediaQuerySet::create())
+    , m_restrictor(MediaQuery::None)
+    , m_mediaType(MediaTypeNames::all)
+    , m_expressions(adoptPtr(new ExpressionHeapVector()))

[eseidel, 2014/03/11 20:14:19]

ExpressionHeapVector::create() is preferred if possible.

[Yoav Weiss, 2014/03/12 20:39:00]

ExpressionHeapVector is a typedef for a Vector. Should I add "create" on Vector?

+    , m_mediaTypeSet(false)
+{
+}
+
+void MediaQueryData::resetCurrentQuery()

[eseidel, 2014/03/11 20:14:19]

Does this store more than the "current query"?  Should the "current query" be a
separate object which we just do:

m_currentQuery = FooBar();

to clear?

[Yoav Weiss, 2014/03/12 20:39:00]

MediaQueryData also contains m_querySet that we don't want to clear here. I can
add a "current query" object here if needed. Or maybe take querySet out of
MediaQueryData, so that the rest can be easily cleared.

+{
+    m_mediaType = MediaTypeNames::all;
+    m_mediaTypeSet = false;
+    m_mediaFeature = "";
+    m_restrictor = MediaQuery::None;
+    m_valueList.clear();
+    m_expressions = adoptPtr(new ExpressionHeapVector());
+}
+
+void MediaQueryData::addCurrentMediaQuery()
+{
+    m_querySet->addMediaQuery(adoptPtr(new MediaQuery(m_restrictor, m_mediaType, m_expressions.release())));

[eseidel, 2014/03/11 20:14:19]

::create() is better when possible, for hiding the adoptPtr stuff.

+    resetCurrentQuery();

[eseidel, 2014/03/11 20:14:19]

Should this instead be:

m_querySet->addMediaQuery(takeCurrentQuery());?

"take" is a common name for "copy out" in this sense.

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

[eseidel, 2014/03/11 20:14:19]

I don't think you need hte inner ()

+    bool retValue = (expression.get());

[eseidel, 2014/03/11 20:14:19]

Or here. !! works too.

bool hadExpression = !!expression;

Except... this can't ever be null?  Unless create can return null!?  Which would
not be normal for create() to do!

[Yoav Weiss, 2014/03/12 20:39:00]

MediaQueryExp::create returns nullptr when the expression is not a valid MQ
expression

+    m_expressions->append(expression.release());
+    m_valueList.clear();
+    return retValue;
+}
+
+void MediaQueryData::addParserValue(MediaQueryTokenType type, MediaQueryToken* token)

[eseidel, 2014/03/11 20:14:19]

If Token can't ever be null in these functions it should be a reference.

+{
+    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/11 20:14:19]

Please use OwnPtr.

[Yoav Weiss, 2014/03/12 20:39:00]

The owner of the CSSParserFunction needs to be the CSSParserValue. I'm not sure
what's the best way to do that, but I think that OwnPtr here would cause later
references to CSSParserValue to reference a destructed pointer.

+        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