Don't re-resolve custom properties.

When we inherit a set of custom properties, without modifying that set,
we don't need to resolve those properties again. Also, we only need to
resolve at all if any values with var()-references have been observed.

This CL simply stores a flag which indicates whether the
Style[Non]InheritedVariables instance contains unresolved var()-
references or not. This fixes a performance issue where variables
specified on :root would be re-resolved for all elements in the tree.

Bug: 875123
Change-Id: I1f68b4c78465c1c09a2b4951207f8bf4eb855e45
Reviewed-on: https://chromium-review.googlesource.com/1180209
Commit-Queue: Anders Ruud <andruud@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584576}

third_party/blink/renderer/core/css/css_custom_property_declaration.h

@@ -12,7 +12,7 @@
 
 namespace blink {
 
-class CSSCustomPropertyDeclaration : public CSSValue {
+class CORE_EXPORT CSSCustomPropertyDeclaration : public CSSValue {
  public:
   static CSSCustomPropertyDeclaration* Create(
       const AtomicString& name,

third_party/blink/renderer/core/css/property_registration.cc

@@ -22,6 +22,16 @@
 
 namespace blink {
 
+PropertyRegistration* PropertyRegistration::Create(
+    const AtomicString& name,
+    const CSSSyntaxDescriptor& syntax,
+    bool inherits,
+    const CSSValue* initial,
+    scoped_refptr<CSSVariableData> initial_variable_data) {
+  return new PropertyRegistration(name, syntax, inherits, initial,
+                                  initial_variable_data);
+}
+
 PropertyRegistration::PropertyRegistration(
     const AtomicString& name,
     const CSSSyntaxDescriptor& syntax,

third_party/blink/renderer/core/css/property_registration.h

@@ -29,6 +29,13 @@ class CORE_EXPORT PropertyRegistration
                                const PropertyDescriptor&,
                                ExceptionState&);
 
+  static PropertyRegistration* Create(
+      const AtomicString& name,
+      const CSSSyntaxDescriptor&,
+      bool inherits,
+      const CSSValue* initial,
+      scoped_refptr<CSSVariableData> initial_variable_data);
+
   const CSSSyntaxDescriptor& Syntax() const { return syntax_; }
   bool Inherits() const { return inherits_; }
   const CSSValue* Initial() const { return initial_; }

third_party/blink/renderer/core/css/property_registry.h

@@ -11,7 +11,7 @@
 
 namespace blink {
 
-class PropertyRegistry : public GarbageCollected<PropertyRegistry> {
+class CORE_EXPORT PropertyRegistry : public GarbageCollected<PropertyRegistry> {
  public:
   static PropertyRegistry* Create() { return new PropertyRegistry(); }
 

third_party/blink/renderer/core/css/resolver/css_variable_resolver.cc

@@ -387,14 +387,16 @@ void CSSVariableResolver::ResolveVariableDefinitions() {
     return;
 
   int variable_count = 0;
-  if (inherited_variables_) {
+  if (inherited_variables_ && inherited_variables_->NeedsResolution()) {
     for (auto& variable : inherited_variables_->data_)
       ValueForCustomProperty(variable.key);
+    inherited_variables_->ClearNeedsResolution();
     variable_count += inherited_variables_->data_.size();
   }
-  if (non_inherited_variables_) {
+  if (non_inherited_variables_ && non_inherited_variables_->NeedsResolution()) {
     for (auto& variable : non_inherited_variables_->data_)
       ValueForCustomProperty(variable.key);
+    non_inherited_variables_->ClearNeedsResolution();
     variable_count += non_inherited_variables_->data_.size();
   }
   INCREMENT_STYLE_STATS_COUNTER(state_.GetDocument().GetStyleEngine(),

third_party/blink/renderer/core/css/resolver/css_variable_resolver.h

@@ -29,7 +29,7 @@ class StyleInheritedVariables;
 class StyleNonInheritedVariables;
 class StyleResolverState;
 
-class CSSVariableResolver {
+class CORE_EXPORT CSSVariableResolver {
   STACK_ALLOCATED();
 
  public:

third_party/blink/renderer/core/css/resolver/css_variable_resolver_test.cc

@@ -2,7 +2,18 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "third_party/blink/renderer/core/css/resolver/css_variable_resolver.h"
+#include "third_party/blink/renderer/core/css/css_custom_property_declaration.h"
+#include "third_party/blink/renderer/core/css/css_variable_reference_value.h"
 #include "third_party/blink/renderer/core/css/document_style_environment_variables.h"
+#include "third_party/blink/renderer/core/css/parser/css_parser_context.h"
+#include "third_party/blink/renderer/core/css/parser/css_tokenizer.h"
+#include "third_party/blink/renderer/core/css/parser/css_variable_parser.h"
+#include "third_party/blink/renderer/core/css/properties/css_property.h"
+#include "third_party/blink/renderer/core/css/properties/longhand.h"
+#include "third_party/blink/renderer/core/css/property_registration.h"
+#include "third_party/blink/renderer/core/css/property_registry.h"
+#include "third_party/blink/renderer/core/css/resolver/style_resolver_state.h"
 #include "third_party/blink/renderer/core/css/style_engine.h"
 #include "third_party/blink/renderer/core/dom/node_computed_style.h"
 #include "third_party/blink/renderer/core/html/html_element.h"
@@ -48,6 +59,19 @@ class CSSVariableResolverTest : public PageTestBase {
         "</div>");
     GetDocument().View()->UpdateAllLifecyclePhases();
   }
+
+  const CSSCustomPropertyDeclaration* CreateCustomProperty(
+      const String& value) {
+    return CreateCustomProperty("--unused", value);
+  }
+
+  const CSSCustomPropertyDeclaration* CreateCustomProperty(
+      const AtomicString& name,
+      const String& value) {
+    const auto tokens = CSSTokenizer(value).TokenizeToEOF();
+    return CSSVariableParser::ParseDeclarationValue(
+        name, tokens, false, *CSSParserContext::Create(GetDocument()));
+  }
 };
 
 TEST_F(CSSVariableResolverTest, ParseEnvVariable_Missing_NestedVar) {
@@ -116,4 +140,141 @@ TEST_F(CSSVariableResolverTest, ParseEnvVariable_WhenNested_WillFallback) {
                             GetCSSPropertyBackgroundColor()));
 }
 
+TEST_F(CSSVariableResolverTest, NoResolutionWithoutVar) {
+  scoped_refptr<StyleInheritedVariables> inherited_variables =
+      StyleInheritedVariables::Create();
+  std::unique_ptr<StyleNonInheritedVariables> non_inherited_variables =
+      StyleNonInheritedVariables::Create();
+
+  EXPECT_FALSE(inherited_variables->NeedsResolution());
+  EXPECT_FALSE(non_inherited_variables->NeedsResolution());
+
+  const auto* prop = CreateCustomProperty("#fefefe");
+
+  inherited_variables->SetVariable("--prop", prop->Value());
+  non_inherited_variables->SetVariable("--prop", prop->Value());
+
+  EXPECT_FALSE(inherited_variables->NeedsResolution());
+  EXPECT_FALSE(non_inherited_variables->NeedsResolution());
+}
+
+TEST_F(CSSVariableResolverTest, VarNeedsResolution) {
+  scoped_refptr<StyleInheritedVariables> inherited_variables =
+      StyleInheritedVariables::Create();
+  std::unique_ptr<StyleNonInheritedVariables> non_inherited_variables =
+      StyleNonInheritedVariables::Create();
+
+  EXPECT_FALSE(inherited_variables->NeedsResolution());
+  EXPECT_FALSE(non_inherited_variables->NeedsResolution());
+
+  const auto* prop1 = CreateCustomProperty("var(--prop2)");
+  const auto* prop2 = CreateCustomProperty("#fefefe");
+
+  inherited_variables->SetVariable("--prop1", prop1->Value());
+  non_inherited_variables->SetVariable("--prop1", prop1->Value());
+
+  EXPECT_TRUE(inherited_variables->NeedsResolution());
+  EXPECT_TRUE(non_inherited_variables->NeedsResolution());
+
+  // While NeedsResolution() == true, add some properties without
+  // var()-references.
+  inherited_variables->SetVariable("--prop2", prop2->Value());
+  non_inherited_variables->SetVariable("--prop2", prop2->Value());
+
+  // We should still need resolution even after adding properties that don't
+  // have var-references.
+  EXPECT_TRUE(inherited_variables->NeedsResolution());
+  EXPECT_TRUE(non_inherited_variables->NeedsResolution());
+
+  inherited_variables->ClearNeedsResolution();
+  non_inherited_variables->ClearNeedsResolution();
+
+  EXPECT_FALSE(inherited_variables->NeedsResolution());
+  EXPECT_FALSE(non_inherited_variables->NeedsResolution());
+}
+
+TEST_F(CSSVariableResolverTest, UrlNeedsResolution) {
+  scoped_refptr<StyleInheritedVariables> inherited_variables =
+      StyleInheritedVariables::Create();
+  std::unique_ptr<StyleNonInheritedVariables> non_inherited_variables =
+      StyleNonInheritedVariables::Create();
+
+  EXPECT_FALSE(inherited_variables->NeedsResolution());
+  EXPECT_FALSE(non_inherited_variables->NeedsResolution());
+
+  const auto* prop = CreateCustomProperty("url(a)");
+
+  inherited_variables->SetVariable("--prop", prop->Value());
+  non_inherited_variables->SetVariable("--prop", prop->Value());
+
+  EXPECT_TRUE(inherited_variables->NeedsResolution());
+  EXPECT_TRUE(non_inherited_variables->NeedsResolution());
+}
+
+TEST_F(CSSVariableResolverTest, CopiedVariablesRetainNeedsResolution) {
+  scoped_refptr<StyleInheritedVariables> inherited_variables =
+      StyleInheritedVariables::Create();
+  std::unique_ptr<StyleNonInheritedVariables> non_inherited_variables =
+      StyleNonInheritedVariables::Create();
+
+  const auto* prop = CreateCustomProperty("var(--x)");
+
+  inherited_variables->SetVariable("--prop", prop->Value());
+  non_inherited_variables->SetVariable("--prop", prop->Value());
+
+  EXPECT_TRUE(inherited_variables->NeedsResolution());
+  EXPECT_TRUE(non_inherited_variables->NeedsResolution());
+  EXPECT_TRUE(inherited_variables->Copy()->NeedsResolution());
+  EXPECT_TRUE(non_inherited_variables->Clone()->NeedsResolution());
+
+  inherited_variables->ClearNeedsResolution();
+  non_inherited_variables->ClearNeedsResolution();
+
+  EXPECT_FALSE(inherited_variables->NeedsResolution());
+  EXPECT_FALSE(non_inherited_variables->NeedsResolution());
+  EXPECT_FALSE(inherited_variables->Copy()->NeedsResolution());
+  EXPECT_FALSE(non_inherited_variables->Clone()->NeedsResolution());
+}
+
+TEST_F(CSSVariableResolverTest, NeedsResolutionClearedByResolver) {
+  const ComputedStyle* initial = &ComputedStyle::InitialStyle();
+  StyleResolverState state(GetDocument(), nullptr, initial, initial);
+
+  scoped_refptr<ComputedStyle> style = ComputedStyle::Create();
+  style->InheritFrom(*initial);
+  state.SetStyle(std::move(style));
+
+  const auto* prop1 = CreateCustomProperty("--prop1", "var(--prop2)");
+  const auto* prop2 = CreateCustomProperty("--prop2", "#fefefe");
+  const auto* prop3 = CreateCustomProperty("--prop3", "var(--prop2)");
+
+  // Register prop3 to make it non-inherited.
+  CSSSyntaxDescriptor token_syntax("*");
+  String initial_value_str("foo");
+  const auto tokens = CSSTokenizer(initial_value_str).TokenizeToEOF();
+  const CSSParserContext* context = CSSParserContext::Create(GetDocument());
+  const CSSValue* initial_value =
+      token_syntax.Parse(CSSParserTokenRange(tokens), context, false);
+  ASSERT_TRUE(initial_value);
+  ASSERT_TRUE(initial_value->IsVariableReferenceValue());
+  PropertyRegistration* registration = PropertyRegistration::Create(
+      "--prop3", token_syntax, false, initial_value,
+      ToCSSVariableReferenceValue(*initial_value).VariableDataValue());
+  ASSERT_TRUE(GetDocument().GetPropertyRegistry());
+  GetDocument().GetPropertyRegistry()->RegisterProperty("--prop3",
+                                                        *registration);
+
+  ToLonghand(GetCSSPropertyVariable()).ApplyValue(state, *prop1);
+  ToLonghand(GetCSSPropertyVariable()).ApplyValue(state, *prop2);
+  ToLonghand(GetCSSPropertyVariable()).ApplyValue(state, *prop3);
+
+  EXPECT_TRUE(state.Style()->InheritedVariables()->NeedsResolution());
+  EXPECT_TRUE(state.Style()->NonInheritedVariables()->NeedsResolution());
+
+  CSSVariableResolver(state).ResolveVariableDefinitions();
+
+  EXPECT_FALSE(state.Style()->InheritedVariables()->NeedsResolution());
+  EXPECT_FALSE(state.Style()->NonInheritedVariables()->NeedsResolution());
+}
+
 }  // namespace blink

third_party/blink/renderer/core/css/resolver/style_resolver.cc

@@ -1713,7 +1713,7 @@ void StyleResolver::ApplyCustomProperties(StyleResolverState& state,
   if (apply_animations == kIncludeAnimations) {
     ApplyAnimatedCustomProperties(state);
   }
-  // TODO(leviw): stop recalculating every time
+
   CSSVariableResolver(state).ResolveVariableDefinitions();
 }
 

third_party/blink/renderer/core/style/computed_style.h

@@ -287,8 +287,8 @@ class ComputedStyle : public ComputedStyleBase,
   StyleDifference VisualInvalidationDiff(const Document&,
                                          const ComputedStyle&) const;
 
-  void InheritFrom(const ComputedStyle& inherit_parent,
-                   IsAtShadowBoundary = kNotAtShadowBoundary);
+  CORE_EXPORT void InheritFrom(const ComputedStyle& inherit_parent,
+                               IsAtShadowBoundary = kNotAtShadowBoundary);
   void CopyNonInheritedFromCached(const ComputedStyle&);
 
   PseudoId StyleType() const { return static_cast<PseudoId>(StyleTypeInternal()); }
@@ -1079,8 +1079,8 @@ class ComputedStyle : public ComputedStyleBase,
   void ClearResetDirectives();
 
   // Variables.
-  StyleInheritedVariables* InheritedVariables() const;
-  StyleNonInheritedVariables* NonInheritedVariables() const;
+  CORE_EXPORT StyleInheritedVariables* InheritedVariables() const;
+  CORE_EXPORT StyleNonInheritedVariables* NonInheritedVariables() const;
 
   void SetUnresolvedInheritedVariable(const AtomicString&,
                                       scoped_refptr<CSSVariableData>);

third_party/blink/renderer/core/style/style_inherited_variables.cc

@@ -39,6 +39,7 @@ StyleInheritedVariables::StyleInheritedVariables(
     registered_data_ = other.registered_data_;
     root_ = other.root_;
   }
+  needs_resolution_ = other.needs_resolution_;
 }
 
 CSSVariableData* StyleInheritedVariables::GetVariable(

third_party/blink/renderer/core/style/style_inherited_variables.h

@@ -14,7 +14,8 @@
 
 namespace blink {
 
-class StyleInheritedVariables : public RefCounted<StyleInheritedVariables> {
+class CORE_EXPORT StyleInheritedVariables
+    : public RefCounted<StyleInheritedVariables> {
  public:
   static scoped_refptr<StyleInheritedVariables> Create() {
     return base::AdoptRef(new StyleInheritedVariables());
@@ -31,6 +32,8 @@ class StyleInheritedVariables : public RefCounted<StyleInheritedVariables> {
 
   void SetVariable(const AtomicString& name,
                    scoped_refptr<CSSVariableData> value) {
+    needs_resolution_ = needs_resolution_ || value->NeedsVariableResolution() ||
+                        value->NeedsUrlResolution();
     data_.Set(name, std::move(value));
   }
   CSSVariableData* GetVariable(const AtomicString& name) const;
@@ -44,8 +47,11 @@ class StyleInheritedVariables : public RefCounted<StyleInheritedVariables> {
   // using a fallback.
   HashSet<AtomicString> GetCustomPropertyNames() const;
 
+  bool NeedsResolution() const { return needs_resolution_; }
+  void ClearNeedsResolution() { needs_resolution_ = false; }
+
  private:
-  StyleInheritedVariables() : root_(nullptr) {}
+  StyleInheritedVariables() : root_(nullptr), needs_resolution_(false) {}
   StyleInheritedVariables(StyleInheritedVariables& other);
 
   friend class CSSVariableResolver;
@@ -53,6 +59,7 @@ class StyleInheritedVariables : public RefCounted<StyleInheritedVariables> {
   HashMap<AtomicString, scoped_refptr<CSSVariableData>> data_;
   PersistentHeapHashMap<AtomicString, Member<CSSValue>> registered_data_;
   scoped_refptr<StyleInheritedVariables> root_;
+  bool needs_resolution_;
 };
 
 }  // namespace blink

third_party/blink/renderer/core/style/style_non_inherited_variables.cc

@@ -42,6 +42,7 @@ StyleNonInheritedVariables::StyleNonInheritedVariables(
     StyleNonInheritedVariables& other) {
   data_ = other.data_;
   registered_data_ = other.registered_data_;
+  needs_resolution_ = other.needs_resolution_;
 }
 
 HashSet<AtomicString> StyleNonInheritedVariables::GetCustomPropertyNames()

third_party/blink/renderer/core/style/style_non_inherited_variables.h

@@ -17,7 +17,7 @@
 
 namespace blink {
 
-class StyleNonInheritedVariables {
+class CORE_EXPORT StyleNonInheritedVariables {
  public:
   static std::unique_ptr<StyleNonInheritedVariables> Create() {
     return base::WrapUnique(new StyleNonInheritedVariables);
@@ -34,6 +34,8 @@ class StyleNonInheritedVariables {
 
   void SetVariable(const AtomicString& name,
                    scoped_refptr<CSSVariableData> value) {
+    needs_resolution_ = needs_resolution_ || value->NeedsVariableResolution() ||
+                        value->NeedsUrlResolution();
     data_.Set(name, std::move(value));
   }
   CSSVariableData* GetVariable(const AtomicString& name) const;
@@ -46,14 +48,18 @@ class StyleNonInheritedVariables {
 
   HashSet<AtomicString> GetCustomPropertyNames() const;
 
+  bool NeedsResolution() const { return needs_resolution_; }
+  void ClearNeedsResolution() { needs_resolution_ = false; }
+
  private:
-  StyleNonInheritedVariables() = default;
+  StyleNonInheritedVariables() : needs_resolution_(false) {}
   StyleNonInheritedVariables(StyleNonInheritedVariables&);
 
   friend class CSSVariableResolver;
 
   HashMap<AtomicString, scoped_refptr<CSSVariableData>> data_;
   PersistentHeapHashMap<AtomicString, Member<CSSValue>> registered_data_;
+  bool needs_resolution_;
 };
 
 }  // namespace blink