Simplify CSS variables resolution logic [2 of 2]

third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp

Index: third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp
diff --git a/third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp b/third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp
index df0d37926dc10401145263a7ea7bebc327f6e193..525740c44685bb63ed15fcea82c411073617a2a0 100644
--- a/third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp
+++ b/third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp
@@ -28,74 +28,70 @@ bool CSSVariableResolver::resolveFallback(CSSParserTokenRange range, Vector<CSSP
         return false;
     ASSERT(range.peek().type() == CommaToken);
     range.consume();
-    return resolveVariableReferencesFromTokens(range, result);
+    return resolveTokenRange(range, result);
 }
 
-bool CSSVariableResolver::resolveVariableTokensRecursive(CSSParserTokenRange range,
-    Vector<CSSParserToken>& result)
+CSSVariableData* CSSVariableResolver::valueForCustomProperty(AtomicString name)
+{
+    if (m_variablesSeen.contains(name)) {
+        m_cycleStartPoints.add(name);
+        return nullptr;
+    }
+
+    if (!m_styleVariableData)
+        return nullptr;
+    CSSVariableData* variableData = m_styleVariableData->getVariable(name);
+    if (!variableData)
+        return nullptr;
+    if (!variableData->needsVariableResolution())
+        return variableData;
+    RefPtr<CSSVariableData> newVariableData = resolveCustomProperty(name, *variableData);
+    m_styleVariableData->setVariable(name, newVariableData);
+    return newVariableData.get();
+}
+
+PassRefPtr<CSSVariableData> CSSVariableResolver::resolveCustomProperty(AtomicString name, const CSSVariableData& variableData)
+{
+    Vector<CSSParserToken> tokens;
+    m_variablesSeen.add(name);
+    bool success = resolveTokenRange(variableData.tokens(), tokens);
+    m_variablesSeen.remove(name);
+
+    // The old variable data holds onto the backing string the new resolved CSSVariableData
+    // relies on. Ensure it will live beyond us overwriting the RefPtr in StyleVariableData.
+    ASSERT(variableData.refCount() > 1);
+
+    if (!success || !m_cycleStartPoints.isEmpty()) {
+        m_cycleStartPoints.remove(name);
+        return nullptr;
+    }
+    return CSSVariableData::createResolved(tokens, variableData);
+}
+
+bool CSSVariableResolver::resolveVariableReference(CSSParserTokenRange range, Vector<CSSParserToken>& result)
 {
-    Vector<CSSParserToken> trash;
     range.consumeWhitespace();
     ASSERT(range.peek().type() == IdentToken);
     AtomicString variableName = range.consumeIncludingWhitespace().value();
     ASSERT(range.atEnd() || (range.peek().type() == CommaToken));
 
-    // Cycle detection.
-    if (m_variablesSeen.contains(variableName)) {
-        m_cycleStartPoints.add(variableName);
-        resolveFallback(range, trash);
-        return false;
-    }
-
-    CSSVariableData* variableData = m_styleVariableData ? m_styleVariableData->getVariable(variableName) : nullptr;
-    if (variableData) {
-        Vector<CSSParserToken> tokens;
-        if (variableData->needsVariableResolution()) {
-            m_variablesSeen.add(variableName);
-            bool referenceValid = resolveVariableReferencesFromTokens(variableData->tokens(), tokens);
-            m_variablesSeen.remove(variableName);
-
-            // The old variable data holds onto the backing string the new resolved CSSVariableData
-            // relies on. Ensure it will live beyond us overwriting the RefPtr in StyleVariableData.
-            ASSERT(variableData->refCount() > 1);
-
-            if (!referenceValid || !m_cycleStartPoints.isEmpty()) {
-                m_styleVariableData->setVariable(variableName, nullptr);
-                m_cycleStartPoints.remove(variableName);
-                if (!m_cycleStartPoints.isEmpty()) {
-                    resolveFallback(range, trash);
-                    return false;
-                }
-                return resolveFallback(range, result);
-            }
-
-            m_styleVariableData->setVariable(variableName, CSSVariableData::createResolved(tokens, variableData));
-        } else {
-            tokens = variableData->tokens();
-        }
-
-        ASSERT(!tokens.isEmpty());
-        // Check that loops are not induced by the fallback.
-        resolveFallback(range, trash);
-        if (m_cycleStartPoints.isEmpty()) {
-            // It's OK if the fallback fails to resolve - we're not actually taking it.
-            result.appendVector(tokens);
-            return true;
-        }
-        m_cycleStartPoints.remove(variableName);
-        return false;
-    }
+    CSSVariableData* variableData = valueForCustomProperty(variableName);
+    if (!variableData)
+        return resolveFallback(range, result);
 
-    return resolveFallback(range, result);
+    result.appendVector(variableData->tokens());
+    Vector<CSSParserToken> trash;
+    resolveFallback(range, trash);
+    return true;
 }
 
-bool CSSVariableResolver::resolveVariableReferencesFromTokens(CSSParserTokenRange range,
+bool CSSVariableResolver::resolveTokenRange(CSSParserTokenRange range,
     Vector<CSSParserToken>& result)
 {
     bool success = true;
     while (!range.atEnd()) {
         if (range.peek().functionId() == CSSValueVar) {
-            success &= resolveVariableTokensRecursive(range.consumeBlock(), result);
+            success &= resolveVariableReference(range.consumeBlock(), result);
         } else {
             result.append(range.consume());
         }
@@ -109,7 +105,7 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSVariableResolver::resolveVariableReferences(
 
     CSSVariableResolver resolver(styleVariableData);
     Vector<CSSParserToken> tokens;
-    if (!resolver.resolveVariableReferencesFromTokens(value.variableDataValue()->tokens(), tokens))
+    if (!resolver.resolveTokenRange(value.variableDataValue()->tokens(), tokens))
         return cssValuePool().createUnsetValue();
 
     CSSParserContext context(HTMLStandardMode, nullptr);
@@ -123,12 +119,10 @@ PassRefPtrWillBeRawPtr<CSSValue> CSSVariableResolver::resolveVariableReferences(
 
 void CSSVariableResolver::resolveAndApplyVariableReferences(StyleResolverState& state, CSSPropertyID id, const CSSVariableReferenceValue& value)
 {
-
-    // TODO(leviw): This should be a stack

[Timothy Loh, 2016/02/16 03:34:56]

I assume this means to unroll the recursion into iteration and use an explicit
stack. I don't think it'd be worthwhile to consider this unless lots of websites
started having custom property dependency chains which were thousands of custom
properties long...

     CSSVariableResolver resolver(state.style()->variables());
 
     Vector<CSSParserToken> tokens;
-    if (resolver.resolveVariableReferencesFromTokens(value.variableDataValue()->tokens(), tokens)) {
+    if (resolver.resolveTokenRange(value.variableDataValue()->tokens(), tokens)) {
         CSSParserContext context(HTMLStandardMode, 0);
 
         WillBeHeapVector<CSSProperty, 256> parsedProperties;
@@ -157,16 +151,10 @@ void CSSVariableResolver::resolveVariableDefinitions(StyleVariableData* variable
     if (!variables)
         return;
 
+    CSSVariableResolver resolver(variables);
     for (auto& variable : variables->m_data) {
-        if (!variable.value || !variable.value->needsVariableResolution())
-            continue;
-        Vector<CSSParserToken> resolvedTokens;
-
-        CSSVariableResolver resolver(variables, variable.key);
-        if (resolver.resolveVariableReferencesFromTokens(variable.value->tokens(), resolvedTokens))
-            variable.value = CSSVariableData::createResolved(resolvedTokens, variable.value);
-        else
-            variable.value = nullptr;
+        if (variable.value && variable.value->needsVariableResolution())
+            variable.value = resolver.resolveCustomProperty(variable.key, *variable.value);
     }
 }
 
@@ -175,10 +163,4 @@ CSSVariableResolver::CSSVariableResolver(StyleVariableData* styleVariableData)
 {
 }
 
-CSSVariableResolver::CSSVariableResolver(StyleVariableData* styleVariableData, AtomicString& variable)
-    : m_styleVariableData(styleVariableData)
-{
-    m_variablesSeen.add(variable);
-}
-
 } // namespace blink