[Typed-OM] Get CSSTokenStreamValue from StyleMap

third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp

 // Copyright 2016 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 "core/css/cssom/CSSTokenStreamValue.h"
 
-#include "core/css/CSSVariableReferenceValue.h"
 #include "core/css/cssom/CSSStyleVariableReferenceValue.h"
 #include "core/css/parser/CSSTokenizer.h"
 #include "wtf/text/StringBuilder.h"
 
 namespace blink {
 
 namespace {
 
 class TokenStreamValueIterationSource final : public ValueIterable<StringOrCSSVariableReferenceValue>::IterationSource {
 public:
@@ skipping 13 matching lines @@
     DEFINE_INLINE_VIRTUAL_TRACE()
     {
         visitor->trace(m_tokenStreamValue);
         ValueIterable<StringOrCSSVariableReferenceValue>::IterationSource::trace(visitor);
     }
 
 private:
     const Member<CSSTokenStreamValue> m_tokenStreamValue;
 };
 
+HeapVector<StringOrCSSVariableReferenceValue> parserTokenRangeToFragments(CSSParserTokenRange);

[meade_UTC10, 2016/08/25 01:00:01]

You shouldn't need to have so complicated a function calling chain that this is
necessary.

+
+CSSTokenStreamValue* fromParserTokenRangeToCSSTokenStreamValue(const CSSParserTokenRange& range)

[meade_UTC10, 2016/08/25 01:00:02]

Still don't need to have "from" in the method name for conversion functions of
the type classXToClassY.

In any case, this function is unnecessary (and your code simplified) if you keep
CSSParserToken handling strictly inside parserTokenRangeToFragments.

+{
+    const HeapVector<StringOrCSSVariableReferenceValue>& fragments = parserTokenRangeToFragments(range);
+    // return undefined if there is no value following the variable's name

[meade_UTC10, 2016/08/25 01:00:01]

s/undefined/null

[anthonyhkf, 2016/08/25 03:48:31]

Done.

+    if (fragments.size() == 0)
+        return nullptr;
+    return CSSTokenStreamValue::create(fragments);
+}
+
+StringView findVariableName(CSSParserTokenRange& range)
+{
+    while (range.peek().type() != CSSParserTokenType::IdentToken)

[meade_UTC10, 2016/08/25 01:00:01]

You shouldn't need to do this. At most you'd need to consumeWhitespace(). 
Remember that the token stream is already validated by the time it gets here,
and anyway if it wasn't, this would potentially return the wrong thing.

[anthonyhkf, 2016/08/25 03:48:31]

Done.

+        range.consume();
+    return range.consume().value();
+}
+
+CSSStyleVariableReferenceValue* variableReferenceValue(CSSParserTokenRange range)

[meade_UTC10, 2016/08/25 01:00:01]

It would be clearer if this function took (StringView name, HeapVector
fragments) instead of doing more parsing. That way you can simplify your
recursion in parserTokenRangeToFragments.

+{
+    StringView variableName = findVariableName(range);
+    // skip the first comma token
+    while (!range.atEnd() && range.consume().type() != CSSParserTokenType::CommaToken) { }

[meade_UTC10, 2016/08/25 01:00:01]

If there's no comma, this will just skip a bunch of stuff... (e.g. var(--foo))

[anthonyhkf, 2016/08/25 03:48:31]

Hmm.. I think if there's no comma, it will go to range.atEnd(), and then it will
stop (?)

+    CSSTokenStreamValue* tokenStreamValue = fromParserTokenRangeToCSSTokenStreamValue(range);
+    return CSSStyleVariableReferenceValue::create(variableName.toString(), tokenStreamValue);
+}
+
+HeapVector<StringOrCSSVariableReferenceValue> parserTokenRangeToFragments(CSSParserTokenRange range)
+{
+    HeapVector<StringOrCSSVariableReferenceValue> fragments;
+    do {
+        const CSSParserToken* start = &range.peek();
+        while (!range.atEnd() && (range.peek().functionId() != CSSValueVar))
+            range.consume();
+        if (start != &range.peek()) {
+            fragments.append(StringOrCSSVariableReferenceValue::fromString(range.makeSubRange(start, &range.peek()).serialize()));
+        } else if (!range.atEnd()) {
+            if (range.peek().functionId() == CSSValueVar)
+                fragments.append(StringOrCSSVariableReferenceValue::fromCSSVariableReferenceValue(variableReferenceValue(range.consumeBlock())));
+            else
+                NOTREACHED();
+        }
+    } while (!range.atEnd());

[meade_UTC10, 2016/08/25 01:00:01]

This doesn't need to be a do/while loop, nor does it need to have nested loops.
It's easier to see that 

while(!range.atEnd()) {
  //dostuff
}

will just get skipped and return an empty Vector, as opposed to

do {
  while (!range.atEnd()) { ...}
  if (start != &range.peek()) {
    //do stuff
  } else if (!range.atEnd()) {
    // do other stuff
  }
} while (!range.atEnd())

Which also does nothing if the range is empty.



You can simplify to

while(!range.atEnd()) {
  if (functionId == CSSValueVar) {
    // Serialize and clear your StringBuilder
    // Consume block
    // Extract variable name
    // Remove comma if necessary
    // Create CSSVariableReferenceValue, recursing tokenToFragments on consumed
block
  } else {
    // Handle non-variable case
  }
}

[anthonyhkf, 2016/08/25 03:48:31]

Done.

+    return fragments;
+}
+
 } // namespace
 
 ValueIterable<StringOrCSSVariableReferenceValue>::IterationSource* CSSTokenStreamValue::startIteration(ScriptState*, ExceptionState&)
 {
     return new TokenStreamValueIterationSource(this);
 }
 
+CSSTokenStreamValue* CSSTokenStreamValue::fromCSSValue(const CSSVariableReferenceValue& cssVariableReferenceValue)
+{
+    return fromParserTokenRangeToCSSTokenStreamValue(cssVariableReferenceValue.variableDataValue()->tokenRange());
+}
+
 CSSValue* CSSTokenStreamValue::toCSSValue() const
 {
     StringBuilder tokens;
 
     for (unsigned i = 0; i < m_fragments.size(); i++) {
         if (i)
             tokens.append("/**/");
         if (m_fragments[i].isString())
             tokens.append(m_fragments[i].getAsString());
         else if (m_fragments[i].isCSSVariableReferenceValue())
             tokens.append(m_fragments[i].getAsCSSVariableReferenceValue()->variable());
         else
             NOTREACHED();
     }
 
     CSSTokenizer::Scope scope(tokens.toString());
 
     return CSSVariableReferenceValue::create(CSSVariableData::create(scope.tokenRange()));
 }
 
 } // namespace blink