Cache complex CSS variable values

https://bugs.webkit.org/show_bug.cgi?id=264250 rdar://117996144 Reviewed by Alan Baradlay. Cache the final CSSValue for variable references in all cases. * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/css/CSSPendingSubstitutionValue.cpp: Added. (WebCore::CSSPendingSubstitutionValue::resolveValue const): For shortcut references cache each individual longhand. * Source/WebCore/css/CSSPendingSubstitutionValue.h: * Source/WebCore/css/CSSVariableReferenceValue.cpp: (WebCore::CSSVariableReferenceValue::cacheSimpleReference): (WebCore::CSSVariableReferenceValue::resolveSingleValue const): (WebCore::CSSVariableReferenceValue::resolveAndCacheValue const): Deleted. (WebCore::CSSVariableReferenceValue::resolveSubstitutionValue const): Deleted. * Source/WebCore/css/CSSVariableReferenceValue.h: (WebCore::CSSVariableReferenceValue::resolveAndCacheValue const): Cache also in non-simple reference case. In these cases we still resolve the tokens and use their equaivalence to test the validity of the cached value. * Source/WebCore/style/StyleBuilder.cpp: (WebCore::Style::Builder::resolveVariableReferences): Canonical link: https://commits.webkit.org/270376@main
WebKit · Nov 8, 2023 · 72e9a6a · 72e9a6a
1 parent a4e3641
commit 72e9a6a
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 56 deletions.
diff --git a/Source/WebCore/Sources.txt b/Source/WebCore/Sources.txt
@@ -859,6 +859,7 @@ css/CSSNamespaceRule.cpp
 css/CSSOffsetRotateValue.cpp
 css/CSSPageRule.cpp
 css/CSSPaintImageValue.cpp
+css/CSSPendingSubstitutionValue.cpp
 css/CSSPrimitiveValue.cpp
 css/CSSProperty.cpp
 css/CSSPropertyRule.cpp

diff --git a/Source/WebCore/WebCore.xcodeproj/project.pbxproj b/Source/WebCore/WebCore.xcodeproj/project.pbxproj
@@ -19650,6 +19650,7 @@
 		E4F819C526FB4EBF0094E162 /* InlineBoxPainter.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = InlineBoxPainter.h; sourceTree = "<group>"; };
 		E4F9EEF0156D84C400D23E7E /* StyleSheetContents.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = StyleSheetContents.cpp; sourceTree = "<group>"; };
 		E4F9EEF1156D84C400D23E7E /* StyleSheetContents.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = StyleSheetContents.h; sourceTree = "<group>"; };
+		E4FA70FF2AF8EEB700D32FEA /* CSSPendingSubstitutionValue.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = CSSPendingSubstitutionValue.cpp; sourceTree = "<group>"; };
 		E4FB4B35239BEB10003C336A /* LayoutIntegrationInlineContent.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = LayoutIntegrationInlineContent.cpp; sourceTree = "<group>"; };
 		E4FFCEB82760AC0000A68B03 /* CSSLayerStatementRule.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CSSLayerStatementRule.h; sourceTree = "<group>"; };
 		E4FFCEB92760AC0000A68B03 /* CSSLayerStatementRule.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = CSSLayerStatementRule.cpp; sourceTree = "<group>"; };
@@ -35444,6 +35445,7 @@
 				4B6E87682176D69200420E5E /* CSSPaintImageValue.h */,
 				4BAFD0CA2190EBD600C0AB64 /* CSSPaintSize.h */,
 				4BAFD0CD2190EBE900C0AB64 /* CSSPaintSize.idl */,
+				E4FA70FF2AF8EEB700D32FEA /* CSSPendingSubstitutionValue.cpp */,
 				9418278D1D8CAE9500492764 /* CSSPendingSubstitutionValue.h */,
 				A80E6CDB0A1989CA007FB8C5 /* CSSPrimitiveValue.cpp */,
 				A80E6CBC0A1989CA007FB8C5 /* CSSPrimitiveValue.h */,

diff --git a/Source/WebCore/css/CSSPendingSubstitutionValue.cpp b/Source/WebCore/css/CSSPendingSubstitutionValue.cpp
@@ -0,0 +1,56 @@
+/*
+ * Copyright (C) 2023 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+#include "CSSPendingSubstitutionValue.h"
+
+#include "CSSPropertyParser.h"
+
+namespace WebCore {
+
+RefPtr<CSSValue> CSSPendingSubstitutionValue::resolveValue(Style::BuilderState& builderState, CSSPropertyID propertyID) const
+{
+    auto cacheValue = [&](auto data) {
+        ParsedPropertyVector parsedProperties;
+        if (!CSSPropertyParser::parseValue(m_shorthandPropertyId, false, data->tokens(), data->context(), parsedProperties, StyleRuleType::Style)) {
+            m_cachedPropertyValues = { };
+            return;
+        }
+
+        m_cachedPropertyValues = parsedProperties;
+    };
+
+    if (!m_shorthandValue->resolveAndCacheValue(builderState, cacheValue))
+        return nullptr;
+
+    for (auto& property : m_cachedPropertyValues) {
+        if (property.id() == propertyID)
+            return property.value();
+    }
+
+    return nullptr;
+}
+
+}
diff --git a/Source/WebCore/css/CSSPendingSubstitutionValue.h b/Source/WebCore/css/CSSPendingSubstitutionValue.h
@@ -34,6 +34,8 @@
 
 namespace WebCore {
 
+class CSSProperty;
+
 class CSSPendingSubstitutionValue : public CSSValue {
 public:
     static Ref<CSSPendingSubstitutionValue> create(CSSPropertyID shorthandPropertyId, Ref<CSSVariableReferenceValue>&& shorthandValue)
@@ -47,6 +49,8 @@ class CSSPendingSubstitutionValue : public CSSValue {
     bool equals(const CSSPendingSubstitutionValue& other) const { return m_shorthandValue.ptr() == other.m_shorthandValue.ptr(); }
     static String customCSSText() { return emptyString(); }
 
+    RefPtr<CSSValue> resolveValue(Style::BuilderState&, CSSPropertyID) const;
+
 private:
     CSSPendingSubstitutionValue(CSSPropertyID shorthandPropertyId, Ref<CSSVariableReferenceValue>&& shorthandValue)
         : CSSValue(PendingSubstitutionValueClass)
@@ -57,6 +61,8 @@ class CSSPendingSubstitutionValue : public CSSValue {
 
     const CSSPropertyID m_shorthandPropertyId;
     Ref<CSSVariableReferenceValue> m_shorthandValue;
+
+    mutable Vector<CSSProperty> m_cachedPropertyValues;
 };
 
 } // namespace WebCore

diff --git a/Source/WebCore/css/CSSVariableReferenceValue.cpp b/Source/WebCore/css/CSSVariableReferenceValue.cpp
@@ -177,6 +177,7 @@ void CSSVariableReferenceValue::cacheSimpleReference()
     ASSERT(!m_simpleReference);
 
     auto range = m_data->tokenRange();
+
     auto functionId = range.peek().functionId();
     if (functionId != CSSValueVar && functionId != CSSValueEnv)
         return;
@@ -225,53 +226,20 @@ RefPtr<CSSVariableData> CSSVariableReferenceValue::resolveVariableReferences(Sty
     return CSSVariableData::create(*resolvedTokens, context());
 }
 
-template<typename ParseFunction>
-RefPtr<CSSValue> CSSVariableReferenceValue::resolveAndCacheValue(Style::BuilderState& builderState, CSSPropertyID propertyID, CSSPropertyID shorthandID, ParseFunction&& parseFunction) const
-{
-    if (auto data = tryResolveSimpleReference(builderState)) {
-        // FIXME: Also cache the complex case.
-        auto hasValidCachedValue = m_cachedResolvedValue
-            && arePointingToEqualData(m_cachedResolvedValue->dependencyData, data)
-            && m_cachedResolvedValue->propertyID == propertyID
-            && m_cachedResolvedValue->shorthandID == shorthandID;
-
-        if (hasValidCachedValue) {
-            // Update in case the object changed but data stayed the same.
-            m_cachedResolvedValue->dependencyData = data;
-        } else {
-            auto value = parseFunction(data->tokenRange());
-            m_cachedResolvedValue = makeUnique<ResolvedValue>(ResolvedValue { value, propertyID, shorthandID, data });
-        }
-        return m_cachedResolvedValue->value;
-    }
-
-    auto resolvedTokens = resolveTokenRange(m_data->tokenRange(), builderState);
-    if (!resolvedTokens)
-        return nullptr;
-
-    return parseFunction(CSSParserTokenRange { *resolvedTokens });
-}
-
 RefPtr<CSSValue> CSSVariableReferenceValue::resolveSingleValue(Style::BuilderState& builderState, CSSPropertyID propertyID) const
 {
-    return resolveAndCacheValue(builderState, propertyID, CSSPropertyInvalid, [&](auto tokens) -> RefPtr<CSSValue> {
-        return CSSPropertyParser::parseSingleValue(propertyID, tokens, context());
-    });
-}
+    auto cacheValue = [&](auto data) {
+        m_cachedValue = CSSPropertyParser::parseSingleValue(propertyID, data->tokens(), context());
+#if ASSERT_ENABLED
+        m_cachePropertyID = propertyID;
+#endif
+    };
 
-RefPtr<CSSValue> CSSVariableReferenceValue::resolveSubstitutionValue(Style::BuilderState& builderState, CSSPropertyID propertyID, CSSPropertyID shorthandID) const
-{
-    return resolveAndCacheValue(builderState, propertyID, shorthandID, [&](auto tokens) -> RefPtr<CSSValue> {
-        ParsedPropertyVector parsedProperties;
-        if (!CSSPropertyParser::parseValue(shorthandID, false, tokens, context(), parsedProperties, StyleRuleType::Style))
-            return nullptr;
-
-        for (auto& property : parsedProperties) {
-            if (property.id() == propertyID)
-                return property.value();
-        }
+    if (!resolveAndCacheValue(builderState, cacheValue))
         return nullptr;
-    });
+
+    ASSERT(m_cachePropertyID == propertyID);
+    return m_cachedValue;
 }
 
 } // namespace WebCore
diff --git a/Source/WebCore/css/CSSVariableReferenceValue.h b/Source/WebCore/css/CSSVariableReferenceValue.h
@@ -31,6 +31,7 @@
 
 #include "CSSValue.h"
 #include "CSSValueKeywords.h"
+#include <wtf/PointerComparison.h>
 #include <wtf/text/WTFString.h>
 
 namespace WebCore {
@@ -58,14 +59,15 @@ class CSSVariableReferenceValue : public CSSValue {
     const CSSParserContext& context() const;
 
     RefPtr<CSSValue> resolveSingleValue(Style::BuilderState&, CSSPropertyID) const;
-    RefPtr<CSSValue> resolveSubstitutionValue(Style::BuilderState&, CSSPropertyID, CSSPropertyID shorthandID) const;
 
     // The maximum number of tokens that may be produced by a var() reference or var() fallback value.
     // https://drafts.csswg.org/css-variables/#long-variables
     static constexpr size_t maxSubstitutionTokens = 65536;
 
     const CSSVariableData& data() const { return m_data.get(); }
 
+    template<typename CacheFunction> bool resolveAndCacheValue(Style::BuilderState&, CacheFunction&&) const;
+
 private:
     explicit CSSVariableReferenceValue(Ref<CSSVariableData>&&);
 
@@ -77,8 +79,6 @@ class CSSVariableReferenceValue : public CSSValue {
     void cacheSimpleReference();
     RefPtr<CSSVariableData> tryResolveSimpleReference(Style::BuilderState&) const;
 
-    template<typename ParseFunction> RefPtr<CSSValue> resolveAndCacheValue(Style::BuilderState&, CSSPropertyID, CSSPropertyID shorthandID, ParseFunction&&) const;
-
     Ref<CSSVariableData> m_data;
     mutable String m_stringValue;
 
@@ -89,17 +89,35 @@ class CSSVariableReferenceValue : public CSSValue {
     };
     std::optional<SimpleReference> m_simpleReference;
 
-    struct ResolvedValue {
-        WTF_MAKE_STRUCT_FAST_ALLOCATED;
-
-        RefPtr<CSSValue> value;
-        CSSPropertyID propertyID;
-        CSSPropertyID shorthandID;
-        RefPtr<CSSVariableData> dependencyData;
-    };
-    mutable std::unique_ptr<ResolvedValue> m_cachedResolvedValue;
+    mutable RefPtr<CSSVariableData> m_cacheDependencyData;
+    mutable RefPtr<CSSValue> m_cachedValue;
+#if ASSERT_ENABLED
+    mutable CSSPropertyID m_cachePropertyID { CSSPropertyInvalid };
+#endif
 };
 
+template<typename CacheFunction>
+bool CSSVariableReferenceValue::resolveAndCacheValue(Style::BuilderState& builderState, CacheFunction&& cacheFunction) const
+
+{
+    if (auto data = tryResolveSimpleReference(builderState)) {
+        if (!arePointingToEqualData(m_cacheDependencyData, data))
+            cacheFunction(data);
+        m_cacheDependencyData = WTFMove(data);
+        return true;
+    }
+
+    auto resolvedTokens = resolveTokenRange(m_data->tokenRange(), builderState);
+    if (!resolvedTokens)
+        return false;
+
+    if (!m_cacheDependencyData || m_cacheDependencyData->tokens() != *resolvedTokens) {
+        m_cacheDependencyData = CSSVariableData::create(*resolvedTokens, context());
+        cacheFunction(m_cacheDependencyData);
+    }
+    return true;
+}
+
 } // namespace WebCore
 
 SPECIALIZE_TYPE_TRAITS_CSS_VALUE(CSSVariableReferenceValue, isVariableReferenceValue())
diff --git a/Source/WebCore/style/StyleBuilder.cpp b/Source/WebCore/style/StyleBuilder.cpp
@@ -376,7 +376,7 @@ Ref<CSSValue> Builder::resolveVariableReferences(CSSPropertyID propertyID, CSSVa
     auto variableValue = [&]() -> RefPtr<CSSValue> {
         if (is<CSSPendingSubstitutionValue>(value)) {
             auto& substitution = downcast<CSSPendingSubstitutionValue>(value);
-            return substitution.shorthandValue().resolveSubstitutionValue(m_state, propertyID, substitution.shorthandPropertyId());
+            return substitution.resolveValue(m_state, propertyID);
         }
 
         auto& variableReferenceValue = downcast<CSSVariableReferenceValue>(value);