[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"
@@ skipping 27 matching lines @@
     const Member<CSSTokenStreamValue> m_tokenStreamValue;
 };
 
 } // namespace
 
 ValueIterable<StringOrCSSVariableReferenceValue>::IterationSource* CSSTokenStreamValue::startIteration(ScriptState*, ExceptionState&)
 {
     return new TokenStreamValueIterationSource(this);
 }
 
+CSSTokenStreamValue* CSSTokenStreamValue::fromCSSValue(const CSSValue& cssValue)

[meade_UTC10, 2016/08/19 08:43:51]

It'd be more correct to take a CSSVariableReferenceValue here, and do the cast
before this function gets called.

[anthonyhkf, 2016/08/22 03:19:21]

Done.

+{
+    StringBuilder builder;
+    return CSSTokenStreamValue::fromParserTokenRange(toCSSVariableReferenceValue(cssValue).variableDataValue()->tokenRange(), builder, false);
+}
+
+void parseTokenRangeToFragments(CSSParserTokenRange range, HeapVector<StringOrCSSVariableReferenceValue>& fragments, StringBuilder& builder, bool isInsideVariable)

[meade_UTC10, 2016/08/19 08:43:51]

Put this helper function up in the anonymous namespace above, since it's not a
member of CSSTokenStreamValue.

[anthonyhkf, 2016/08/22 03:19:21]

Done.

+{
+    StringView variable;

[meade_UTC10, 2016/08/19 08:43:52]

This variable needs a more descriptive name - I don't understand what it's
doing.

[anthonyhkf, 2016/08/22 03:19:20]

Done.

+    while (!range.atEnd()) {
+        if (range.peek().getBlockType() == CSSParserToken::BlockStart) {
+            if (range.peek().functionId() == CSSValueVar) {
+                fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString()));
+                builder.clear();
+                CSSTokenStreamValue* tokenStreamValue = CSSTokenStreamValue::fromParserTokenRange(range.consumeBlock(), builder, true);

[meade_UTC10, 2016/08/19 08:43:51]

It would be clearer if you split out the creation of the tokenstream value into
another helper function. The level of nesting here is confusing.

Alternatively you could pull out the entire section that handles what happens if
it is a var.

[anthonyhkf, 2016/08/22 03:19:21]

Done.

+                CSSStyleVariableReferenceValue* variableReferenceValue = CSSStyleVariableReferenceValue::create(builder.toString(), tokenStreamValue);
+                fragments.append(StringOrCSSVariableReferenceValue::fromCSSVariableReferenceValue(variableReferenceValue));
+                builder.clear();
+            } else {
+                range.peek().serialize(builder);
+                parseTokenRangeToFragments(range.consumeBlock(), fragments, builder, false);

[meade_UTC10, 2016/08/19 08:43:51]

Should you be both serializing the first token, and then parsing the block? It
seems like the chars will appear twice?

[anthonyhkf, 2016/08/22 03:19:21]

I think the consumeBlock() will only return the parts inside the block.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/parse...

[meade_UTC10, 2016/08/22 07:55:47]

In that case it might be clearer to do 

builder.append('(');
parseTokenRangeToFragments(range.consumeBlock(), fragments, builder, false);
builder.append(')');

Or whatever the equivalent thing you expect to come from
range.peek().serialize().

+                builder.append(')');
+            }
+        } else {
+            if (range.peek().type() == CSSParserTokenType::CommaToken) {
+                if (!isInsideVariable)
+                    builder.append(',');
+            } else if (range.peek().type() == CSSParserTokenType::IdentToken) {
+                variable = range.peek().value();
+            } else {
+                range.peek().serialize(builder);
+            }
+            range.consume();

[meade_UTC10, 2016/08/19 08:43:51]

You should be able simplify these type of calls by instead doing

CSSParserToken& token = range.consume();
if (token.type() == CSSParserTokenType...)
..
} else if (token.type() == CSSParserTokenType...)

[anthonyhkf, 2016/08/22 03:19:21]

Done.

+        }
+    }
+    if (isInsideVariable) {
+        if (!builder.isEmpty()) {
+            fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString()));
+            builder.clear();
+        }
+        builder.append(variable);
+    }
+}
+
+CSSTokenStreamValue* CSSTokenStreamValue::fromParserTokenRange(CSSParserTokenRange range, StringBuilder& builder, bool isInsideVariable)
+{
+    HeapVector<StringOrCSSVariableReferenceValue> fragments;
+    parseTokenRangeToFragments(range, fragments, builder, isInsideVariable);
+    if (!builder.isEmpty() && !isInsideVariable) {
+        fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString()));

[meade_UTC10, 2016/08/19 08:43:51]

Can this be moved up into parseTokenFragments? It's kind of weird here.

[anthonyhkf, 2016/08/22 03:19:21]

Done.

+    }
+    if (fragments.size() == 0)
+        return nullptr;

[meade_UTC10, 2016/08/19 08:43:51]

If this is a private function, you can DCHECK this condition instead to ensure
this never happens.

[anthonyhkf, 2016/08/22 03:19:21]

It is possible to happen. This one is for the cases like "var(--a)" (without the
value of the variable), it should return the fallback as "undefined".

+    return CSSTokenStreamValue::create(fragments);
+}
+
 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