[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..fa1f930f599e72b0fed3ffec00be9ff3f9176394 100644
--- a/third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp
+++ b/third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp
@@ -4,7 +4,6 @@
 
 #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"
@@ -38,6 +37,71 @@ private:
     const Member<CSSTokenStreamValue> m_tokenStreamValue;
 };
 
+void parseTokenRangeToFragments(CSSParserTokenRange, HeapVector<StringOrCSSVariableReferenceValue>&, StringBuilder&, bool);
+
+CSSTokenStreamValue* fromParserTokenRangeToCSSTokenStreamValue(CSSParserTokenRange range, StringBuilder& builder, bool isInsideVariable)
+{
+    HeapVector<StringOrCSSVariableReferenceValue> fragments;
+    parseTokenRangeToFragments(range, fragments, builder, isInsideVariable);
+
+    // return undefined if there is no value following the variable's name
+    if (fragments.size() == 0)
+        return nullptr;
+    return CSSTokenStreamValue::create(fragments);
+}
+
+void handleVariableBlock(CSSParserTokenRange range, HeapVector<StringOrCSSVariableReferenceValue>& fragments, StringBuilder& builder)

[Timothy Loh, 2016/08/22 06:30:59]

handle -> parse?

I think this function should be structured as:

input: CSSParserTokenRange
output: CSSVariableReferenceValue

This function should be responsible for everything specific to var() blocks,
instead of splitting it up.

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

how about calling this void variableReferenceValue(const StringView& name, const
HeapVector<StringOrCSSVariableReferenceValue>& fragments)?

As Tim says, it should only do stuff relevant to var() blocks, and if you get
the recursion right, you shouldn't need to pass in a builder or
CSSParserTokenRange.

[anthonyhkf, 2016/08/24 04:47:39]

Done.

+{
+    if (!builder.isEmpty()) {
+        // we only append the string before "var" if it is not an empty string
+        fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString()));

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

If you move this bit to the caller of this function, you can avoid having to
clear the builder inside this function, allowing you to create the new one.

[anthonyhkf, 2016/08/24 04:47:39]

Done.

+        builder.clear();
+    }
+
+    CSSTokenStreamValue* tokenStreamValue = fromParserTokenRangeToCSSTokenStreamValue(range, builder, true);
+    CSSStyleVariableReferenceValue* variableReferenceValue = CSSStyleVariableReferenceValue::create(builder.toString(), tokenStreamValue);
+
+    fragments.append(StringOrCSSVariableReferenceValue::fromCSSVariableReferenceValue(variableReferenceValue));
+    builder.clear();
+}
+
+void parseTokenRangeToFragments(CSSParserTokenRange range, HeapVector<StringOrCSSVariableReferenceValue>& fragments, StringBuilder& builder, bool isInsideVariable)

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

nit: Was this supposed to be parserTokenRangeToFragments? (parse -> parser)

[anthonyhkf, 2016/08/24 04:47:39]

Done.

+{
+    // if isInsideVariable, we need to store the variable's name
+    StringView variableName;
+    while (!range.atEnd()) {

[Timothy Loh, 2016/08/22 06:30:59]

I think it'd be better (overall much simpler) to structure this as something
like:

input: CSSParserTokenRange
output: CSSTokenStreamValue

do {
  const CSSParserToken* start = &range.peek();
  while (!atEnd() && !var) range.consume();
  if (start != &range.peek())
    append range.makeSubRange(start, &range.peek()).serialize();
  if var, call other function
} while (!atEnd())

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

I agree, but I think it works out slightly better to have the function return a
HeapVector<StringOrCSSVariableReferenceValue>, so like this:

HeapVector<StringOrCSSVariableReferenceValue>
parserTokenRangeToFragments2(CSSParserTokenRange range)
{
  HeapVector<...> fragments;
  StringBuilder builder;

  while (!range.atEnd()) {
    if (range.peek().functionId() == CSSValueVar) {
      // Do var stuff
    } else {
      range.consume().serialize(builder);
    }
  }

  // Append builder stuff to fragments
  return fragments;
}

If you get this right, you don't need to pass any HeapVectors, StringBuilders,
or booleans into this function :)

[anthonyhkf, 2016/08/24 04:47:39]

Done.

+        if (range.peek().getBlockType() == CSSParserToken::BlockStart) {
+            if (range.peek().functionId() == CSSValueVar) {
+                handleVariableBlock(range.consumeBlock(), fragments, builder);
+            } else {
+                range.peek().serialize(builder);
+                parseTokenRangeToFragments(range.consumeBlock(), fragments, builder, false);
+                builder.append(')');
+            }
+        } else {
+            CSSParserToken token = range.consume();
+            if (token.type() == CSSParserTokenType::CommaToken) {
+                if (!isInsideVariable)

[Timothy Loh, 2016/08/22 06:30:59]

BTW this logic is wrong because you can write "var(--foo, a, b)" which makes the
fallback " a, b" but this would output " a b" and the variable name "b".

[anthonyhkf, 2016/08/24 04:47:39]

Oh, sorry. Before I thought that it is only possible to have at most one value.
This will return fallback as " a b" but the variable name is still "--foo".

+                    builder.append(',');
+            } else if (token.type() == CSSParserTokenType::IdentToken) {
+                variableName = token.value();
+            } else {
+                token.serialize(builder);
+            }
+        }
+    }
+    if (isInsideVariable) {
+        if (!builder.isEmpty()) {

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

You do this line regardless of isInsideVariable, so why not move it out of the
if statements?

if (!builder.isEmpty()) {

fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString()));
}

+            fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString()));
+            builder.clear();
+        }
+        // if it is inside variable block, we should keep the variable's name
+        builder.append(variableName);
+    } else if (!builder.isEmpty()) {
+        fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString()));
+    }
+}
+
 } // namespace
 
 ValueIterable<StringOrCSSVariableReferenceValue>::IterationSource* CSSTokenStreamValue::startIteration(ScriptState*, ExceptionState&)
@@ -45,6 +109,12 @@ ValueIterable<StringOrCSSVariableReferenceValue>::IterationSource* CSSTokenStrea
     return new TokenStreamValueIterationSource(this);
 }
 
+CSSTokenStreamValue* CSSTokenStreamValue::fromCSSVariableReferenceValue(const CSSVariableReferenceValue& cssVariableReferenceValue)

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

You can still call this fromCSSValue for consistency with the other Typed OM
types though :)

[anthonyhkf, 2016/08/24 04:47:39]

Done.

+{
+    StringBuilder builder;
+    return fromParserTokenRangeToCSSTokenStreamValue(cssVariableReferenceValue.variableDataValue()->tokenRange(), builder, false);
+}
+
 CSSValue* CSSTokenStreamValue::toCSSValue() const
 {
     StringBuilder tokens;