[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;
 };
 
+StringView findVariableName(CSSParserTokenRange& range)
+{
+    range.consumeWhitespace();
+    return range.consume().value();
+}
+
+CSSStyleVariableReferenceValue* variableReferenceValue(StringView variableName, CSSTokenStreamValue* tokenStreamValue)
+{
+    return CSSStyleVariableReferenceValue::create(variableName.toString(), tokenStreamValue);

[meade_UTC10, 2016/08/26 01:29:22]

It's not really worth creating a new function that just contains a single line.
May as well just put this back inside the other function in this case. 

FWIW, I thought it was tidier when the recursing function only dealt with
tokens, and didn't instantiate any objects, so the functions were

StringOrCSSVariableReferenceValue variableReferenceValue(const StringView& name,
HeapVector<StringOrCSSVariableReferenceValue> tokens)
{
  // Instantiates CSSTokenStreamValue value if tokens not empty
  // Instantiates CSSVariableReferenceValue
  // Wraps CSSVariableReferenceValue in StringOr
}

HeapVector<StringOrCSSVariableReferenceValue>
parserTokenRangeToFragments(CSSParserTokenRange range)
{
  // while !atEnd
    // if var
      // find variable name
      // fragments.append(variableReferenceValue(variableName,
parserTokenRangeToFragments(block));
// etc
}

...

For the fromCSSValue case, I think it's fine to return an empty
CSSTokenStreamValue.

[anthonyhkf, 2016/08/26 03:10:28]

Done.

+}
+
+CSSTokenStreamValue* parserTokenRangeToTokenStreamValue(CSSParserTokenRange range)
+{
+    HeapVector<StringOrCSSVariableReferenceValue> fragments;
+    StringBuilder builder;
+    while (!range.atEnd()) {
+        if (range.peek().functionId() == CSSValueVar) {
+            if (!builder.isEmpty()) {
+                fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString()));
+                builder.clear();
+            }
+            CSSParserTokenRange current = range.consumeBlock();

[meade_UTC10, 2016/08/26 01:29:22]

Current is not a good variable name, as it is too general. Variable names should
describe what they are, in this case "block" could be a good name (because it
came from consumeBlock, and "range" is already taken).

[anthonyhkf, 2016/08/26 03:10:28]

Done.

+            StringView variableName = findVariableName(current);
+            while (!current.atEnd() && current.consume().type() != CSSParserTokenType::CommaToken) { }

[meade_UTC10, 2016/08/26 01:29:22]

This will do the wrong thing if there isn't a comma. Why not just do

consumeWhitespace()
if (peek().type() == Comma)
  consume()

?

[anthonyhkf, 2016/08/26 03:10:28]

Done.

+            CSSTokenStreamValue* tokenStreamValue = parserTokenRangeToTokenStreamValue(current);
+            fragments.append(StringOrCSSVariableReferenceValue::fromCSSVariableReferenceValue(variableReferenceValue(variableName, tokenStreamValue)));
+        } else {
+            range.consume().serialize(builder);
+        }
+    }
+    if (!builder.isEmpty())
+        fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString()));
+    // returns undefined/null if there is no value following the variable's name

[meade_UTC10, 2016/08/26 01:29:22]

We aren't in a var(--foo, fallback) block, so this comment doesn't make sense.
It's also not really useful to just state what the code is doing (which is
returning null if no fragments were found)

[anthonyhkf, 2016/08/26 03:10:29]

Done.

+    if (fragments.size() == 0)
+        return nullptr;
+    return CSSTokenStreamValue::create(fragments);
+}
+
 } // namespace
 
 ValueIterable<StringOrCSSVariableReferenceValue>::IterationSource* CSSTokenStreamValue::startIteration(ScriptState*, ExceptionState&)
 {
     return new TokenStreamValueIterationSource(this);
 }
 
+CSSTokenStreamValue* CSSTokenStreamValue::fromCSSValue(const CSSVariableReferenceValue& cssVariableReferenceValue)
+{
+    return parserTokenRangeToTokenStreamValue(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