[Typed-OM] Get CSSTokenStreamValue from StyleMap

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

Index: third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp
diff --git a/third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp b/third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp
index fdb62cc2379dda0c5debdeedf8735653af398397..ea652e28a0b8c9437d7f1bf5c3df657292ebb598 100644
--- a/third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp
+++ b/third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp
@@ -45,6 +45,62 @@ ValueIterable<StringOrCSSVariableReferenceValue>::IterationSource* CSSTokenStrea
     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;