[mpc] Use CSSProperty* in CascadeResolver's cycle detection stack

A subsequent CL needs to get the currently applying CSSProperty* from
the CascadeResolver, and it's more efficient to get the pointer
directly, rather than looking it up via the CSSPropertyName.

This is a refactor: there is no behavior change in this CL. (Except
one addition unit test is added).

Bug: 1057072
Change-Id: I7532f2c23f9da915c132cb42abef7b804d3a5732
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2247821
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779735}

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

@@ -7,25 +7,21 @@
 #include "third_party/blink/renderer/core/animation/css/css_animations.h"
 #include "third_party/blink/renderer/core/css/css_variable_data.h"
 #include "third_party/blink/renderer/core/css/properties/css_property.h"
+#include "third_party/blink/renderer/core/css/properties/longhands/custom_property.h"
 #include "third_party/blink/renderer/core/css/resolver/cascade_priority.h"
 
 namespace blink {
 
 bool CascadeResolver::IsLocked(const CSSProperty& property) const {
-  return IsLocked(property.GetCSSPropertyName());
-}
-
-bool CascadeResolver::IsLocked(const CSSPropertyName& name) const {
-  return stack_.Contains(name);
+  return Find(property) != kNotFound;
 }
 
 bool CascadeResolver::AllowSubstitution(CSSVariableData* data) const {
   if (data && data->IsAnimationTainted() && stack_.size()) {
-    const CSSPropertyName& name = stack_.back();
-    if (name.IsCustomProperty())
+    const CSSProperty* property = stack_.back();
+    if (IsA<CustomProperty>(*property))
       return true;
-    const CSSProperty& property = CSSProperty::Get(name.Id());
-    return !CSSAnimations::IsAnimationAffectingProperty(property);
+    return !CSSAnimations::IsAnimationAffectingProperty(*property);
   }
   return true;
 }
@@ -41,7 +37,7 @@ void CascadeResolver::MarkApplied(CascadePriority* priority) const {
 }
 
 bool CascadeResolver::DetectCycle(const CSSProperty& property) {
-  wtf_size_t index = stack_.Find(property.GetCSSPropertyName());
+  wtf_size_t index = Find(property);
   if (index == kNotFound)
     return false;
   cycle_depth_ = std::min(cycle_depth_, index);
@@ -52,15 +48,21 @@ bool CascadeResolver::InCycle() const {
   return cycle_depth_ != kNotFound;
 }
 
-CascadeResolver::AutoLock::AutoLock(const CSSProperty& property,
-                                    CascadeResolver& resolver)
-    : AutoLock(property.GetCSSPropertyName(), resolver) {}
+wtf_size_t CascadeResolver::Find(const CSSProperty& property) const {
+  wtf_size_t index = 0;
+  for (const CSSProperty* p : stack_) {
+    if (p->GetCSSPropertyName() == property.GetCSSPropertyName())
+      return index;
+    ++index;
+  }
+  return kNotFound;
+}
 
-CascadeResolver::AutoLock::AutoLock(const CSSPropertyName& name,
+CascadeResolver::AutoLock::AutoLock(const CSSProperty& property,
                                     CascadeResolver& resolver)
     : resolver_(resolver) {
-  DCHECK(!resolver.IsLocked(name));
-  resolver_.stack_.push_back(name);
+  DCHECK(!resolver.IsLocked(property));
+  resolver_.stack_.push_back(&property);
 }
 
 CascadeResolver::AutoLock::~AutoLock() {

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

@@ -34,13 +34,12 @@ class CORE_EXPORT CascadeResolver {
 
  public:
   // TODO(crbug.com/985047): Probably use a HashMap for this.
-  using NameStack = Vector<CSSPropertyName, 8>;
+  using PropertyStack = Vector<const CSSProperty*, 8>;
 
   // A 'locked' property is a property we are in the process of applying.
   // In other words, once a property is locked, locking it again would form
   // a cycle, and is therefore an error.
   bool IsLocked(const CSSProperty&) const;
-  bool IsLocked(const CSSPropertyName&) const;
 
   // We do not allow substitution of animation-tainted values into
   // an animation-affecting property.
@@ -73,7 +72,6 @@ class CORE_EXPORT CascadeResolver {
 
    public:
     AutoLock(const CSSProperty&, CascadeResolver&);
-    AutoLock(const CSSPropertyName&, CascadeResolver&);
     ~AutoLock();
 
    private:
@@ -102,8 +100,12 @@ class CORE_EXPORT CascadeResolver {
   // Returns true whenever the CascadeResolver is in a cycle state.
   // This DOES NOT detect cycles; the caller must call DetectCycle first.
   bool InCycle() const;
+  // Returns the index of the given property (compared using the property's
+  // CSSPropertyName), or kNotFound if the property (name) is not present in
+  // stack_.
+  wtf_size_t Find(const CSSProperty&) const;
 
-  NameStack stack_;
+  PropertyStack stack_;
   wtf_size_t cycle_depth_ = kNotFound;
   CascadeFilter filter_;
   const uint8_t generation_ = 0;

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

@@ -422,7 +422,7 @@ void StyleCascade::LookupAndApply(const CSSProperty& property,
   DCHECK(!property.IsSurrogate());
 
   CSSPropertyName name = property.GetCSSPropertyName();
-  DCHECK(!resolver.IsLocked(name));
+  DCHECK(!resolver.IsLocked(property));
 
   CascadePriority* p = map_.Find(name);
   if (!p)

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

@@ -56,13 +56,10 @@ class TestCascadeResolver {
   STACK_ALLOCATED();
 
  public:
-  explicit TestCascadeResolver(Document& document, uint8_t generation = 0)
-      : document_(document), resolver_(CascadeFilter(), generation) {}
+  explicit TestCascadeResolver(uint8_t generation = 0)
+      : resolver_(CascadeFilter(), generation) {}
   bool InCycle() const { return resolver_.InCycle(); }
-  bool DetectCycle(String name) {
-    CSSPropertyRef ref(name, document_);
-    DCHECK(ref.IsValid());
-    const CSSProperty& property = ref.GetProperty();
+  bool DetectCycle(const CSSProperty& property) {
     return resolver_.DetectCycle(property);
   }
   wtf_size_t CycleDepth() const { return resolver_.cycle_depth_; }
@@ -78,7 +75,6 @@ class TestCascadeResolver {
  private:
   friend class TestCascadeAutoLock;
 
-  Document& document_;
   CascadeResolver resolver_;
 };
 
@@ -132,7 +128,7 @@ class TestCascade {
   void ApplySingle(const CSSProperty& property) {
     EnsureAtLeast(CascadeOrigin::kAuthor);
     cascade_.AnalyzeIfNeeded();
-    TestCascadeResolver resolver(GetDocument(), ++cascade_.generation_);
+    TestCascadeResolver resolver(++cascade_.generation_);
     cascade_.LookupAndApply(property, resolver.InnerResolver());
   }
 
@@ -261,9 +257,9 @@ class TestCascadeAutoLock {
   STACK_ALLOCATED();
 
  public:
-  TestCascadeAutoLock(const CSSPropertyName& name,
+  TestCascadeAutoLock(const CSSProperty& property,
                       TestCascadeResolver& resolver)
-      : lock_(name, resolver.resolver_) {}
+      : lock_(property, resolver.resolver_) {}
 
  private:
   CascadeResolver::AutoLock lock_;
@@ -627,21 +623,45 @@ TEST_F(StyleCascadeTest, PendingSubstitutionInLogicalShorthand) {
   EXPECT_EQ("10px", cascade.ComputedValue("margin-right"));
 }
 
+TEST_F(StyleCascadeTest, DetectCycleByName) {
+  TestCascade cascade(GetDocument());
+  TestCascadeResolver resolver;
+
+  // Two different CustomProperty instances with the same name:
+  CustomProperty a1("--a", GetDocument());
+  CustomProperty a2("--a", GetDocument());
+
+  {
+    TestCascadeAutoLock lock(a1, resolver);
+    EXPECT_FALSE(resolver.InCycle());
+
+    // This should still be detected as a cycle, even though it's not the same
+    // CustomProperty instance.
+    EXPECT_TRUE(resolver.DetectCycle(a2));
+    EXPECT_TRUE(resolver.InCycle());
+  }
+  EXPECT_FALSE(resolver.InCycle());
+}
+
 TEST_F(StyleCascadeTest, ResolverDetectCycle) {
   TestCascade cascade(GetDocument());
-  TestCascadeResolver resolver(GetDocument());
+  TestCascadeResolver resolver;
+
+  CustomProperty a("--a", GetDocument());
+  CustomProperty b("--b", GetDocument());
+  CustomProperty c("--c", GetDocument());
 
   {
-    TestCascadeAutoLock lock(CSSPropertyName("--a"), resolver);
+    TestCascadeAutoLock lock(a, resolver);
     EXPECT_FALSE(resolver.InCycle());
     {
-      TestCascadeAutoLock lock(CSSPropertyName("--b"), resolver);
+      TestCascadeAutoLock lock(b, resolver);
       EXPECT_FALSE(resolver.InCycle());
       {
-        TestCascadeAutoLock lock(CSSPropertyName("--c"), resolver);
+        TestCascadeAutoLock lock(c, resolver);
         EXPECT_FALSE(resolver.InCycle());
 
-        EXPECT_TRUE(resolver.DetectCycle("--a"));
+        EXPECT_TRUE(resolver.DetectCycle(a));
         EXPECT_TRUE(resolver.InCycle());
       }
       EXPECT_TRUE(resolver.InCycle());
@@ -653,19 +673,24 @@ TEST_F(StyleCascadeTest, ResolverDetectCycle) {
 
 TEST_F(StyleCascadeTest, ResolverDetectNoCycle) {
   TestCascade cascade(GetDocument());
-  TestCascadeResolver resolver(GetDocument());
+  TestCascadeResolver resolver;
+
+  CustomProperty a("--a", GetDocument());
+  CustomProperty b("--b", GetDocument());
+  CustomProperty c("--c", GetDocument());
+  CustomProperty x("--x", GetDocument());
 
   {
-    TestCascadeAutoLock lock(CSSPropertyName("--a"), resolver);
+    TestCascadeAutoLock lock(a, resolver);
     EXPECT_FALSE(resolver.InCycle());
     {
-      TestCascadeAutoLock lock(CSSPropertyName("--b"), resolver);
+      TestCascadeAutoLock lock(b, resolver);
       EXPECT_FALSE(resolver.InCycle());
       {
-        TestCascadeAutoLock lock(CSSPropertyName("--c"), resolver);
+        TestCascadeAutoLock lock(c, resolver);
         EXPECT_FALSE(resolver.InCycle());
 
-        EXPECT_FALSE(resolver.DetectCycle("--x"));
+        EXPECT_FALSE(resolver.DetectCycle(x));
         EXPECT_FALSE(resolver.InCycle());
       }
       EXPECT_FALSE(resolver.InCycle());
@@ -677,13 +702,15 @@ TEST_F(StyleCascadeTest, ResolverDetectNoCycle) {
 
 TEST_F(StyleCascadeTest, ResolverDetectCycleSelf) {
   TestCascade cascade(GetDocument());
-  TestCascadeResolver resolver(GetDocument());
+  TestCascadeResolver resolver;
+
+  CustomProperty a("--a", GetDocument());
 
   {
-    TestCascadeAutoLock lock(CSSPropertyName("--a"), resolver);
+    TestCascadeAutoLock lock(a, resolver);
     EXPECT_FALSE(resolver.InCycle());
 
-    EXPECT_TRUE(resolver.DetectCycle("--a"));
+    EXPECT_TRUE(resolver.DetectCycle(a));
     EXPECT_TRUE(resolver.InCycle());
   }
   EXPECT_FALSE(resolver.InCycle());
@@ -693,28 +720,33 @@ TEST_F(StyleCascadeTest, ResolverDetectMultiCycle) {
   using AutoLock = TestCascadeAutoLock;
 
   TestCascade cascade(GetDocument());
-  TestCascadeResolver resolver(GetDocument());
+  TestCascadeResolver resolver;
+
+  CustomProperty a("--a", GetDocument());
+  CustomProperty b("--b", GetDocument());
+  CustomProperty c("--c", GetDocument());
+  CustomProperty d("--d", GetDocument());
 
   {
-    AutoLock lock(CSSPropertyName("--a"), resolver);
+    AutoLock lock(a, resolver);
     EXPECT_FALSE(resolver.InCycle());
     {
-      AutoLock lock(CSSPropertyName("--b"), resolver);
+      AutoLock lock(b, resolver);
       EXPECT_FALSE(resolver.InCycle());
       {
-        AutoLock lock(CSSPropertyName("--c"), resolver);
+        AutoLock lock(c, resolver);
         EXPECT_FALSE(resolver.InCycle());
         {
-          AutoLock lock(CSSPropertyName("--d"), resolver);
+          AutoLock lock(d, resolver);
           EXPECT_FALSE(resolver.InCycle());
 
           // Cycle 1 (big cycle):
-          EXPECT_TRUE(resolver.DetectCycle("--b"));
+          EXPECT_TRUE(resolver.DetectCycle(b));
           EXPECT_TRUE(resolver.InCycle());
           EXPECT_EQ(1u, resolver.CycleDepth());
 
           // Cycle 2 (small cycle):
-          EXPECT_TRUE(resolver.DetectCycle("--c"));
+          EXPECT_TRUE(resolver.DetectCycle(c));
           EXPECT_TRUE(resolver.InCycle());
           EXPECT_EQ(1u, resolver.CycleDepth());
         }
@@ -730,28 +762,33 @@ TEST_F(StyleCascadeTest, ResolverDetectMultiCycleReverse) {
   using AutoLock = TestCascadeAutoLock;
 
   TestCascade cascade(GetDocument());
-  TestCascadeResolver resolver(GetDocument());
+  TestCascadeResolver resolver;
+
+  CustomProperty a("--a", GetDocument());
+  CustomProperty b("--b", GetDocument());
+  CustomProperty c("--c", GetDocument());
+  CustomProperty d("--d", GetDocument());
 
   {
-    AutoLock lock(CSSPropertyName("--a"), resolver);
+    AutoLock lock(a, resolver);
     EXPECT_FALSE(resolver.InCycle());
     {
-      AutoLock lock(CSSPropertyName("--b"), resolver);
+      AutoLock lock(b, resolver);
       EXPECT_FALSE(resolver.InCycle());
       {
-        AutoLock lock(CSSPropertyName("--c"), resolver);
+        AutoLock lock(c, resolver);
         EXPECT_FALSE(resolver.InCycle());
         {
-          AutoLock lock(CSSPropertyName("--d"), resolver);
+          AutoLock lock(d, resolver);
           EXPECT_FALSE(resolver.InCycle());
 
           // Cycle 1 (small cycle):
-          EXPECT_TRUE(resolver.DetectCycle("--c"));
+          EXPECT_TRUE(resolver.DetectCycle(c));
           EXPECT_TRUE(resolver.InCycle());
           EXPECT_EQ(2u, resolver.CycleDepth());
 
           // Cycle 2 (big cycle):
-          EXPECT_TRUE(resolver.DetectCycle("--b"));
+          EXPECT_TRUE(resolver.DetectCycle(b));
           EXPECT_TRUE(resolver.InCycle());
           EXPECT_EQ(1u, resolver.CycleDepth());
         }
@@ -764,7 +801,7 @@ TEST_F(StyleCascadeTest, ResolverDetectMultiCycleReverse) {
 }
 
 TEST_F(StyleCascadeTest, ResolverMarkApplied) {
-  TestCascadeResolver resolver(GetDocument(), 2);
+  TestCascadeResolver resolver(2);
 
   CascadePriority priority(CascadeOrigin::kAuthor);
   EXPECT_EQ(0, priority.GetGeneration());
@@ -778,7 +815,7 @@ TEST_F(StyleCascadeTest, ResolverMarkApplied) {
 }
 
 TEST_F(StyleCascadeTest, ResolverMarkUnapplied) {
-  TestCascadeResolver resolver(GetDocument(), 7);
+  TestCascadeResolver resolver(7);
 
   CascadePriority priority(CascadeOrigin::kAuthor);
   EXPECT_EQ(0, priority.GetGeneration());