Refactor handling of surrogates in StyleCascade (again)

This CL does some refactoring necessary to reduce the noise in
subsequent CLs.

Instead of returning a value from ResolveIfSurrogate indicating
whether the surrogate should be skipped or not, we first determine if
the property is a surrogate, and if so, call ApplySurrogate, and deal
with any skip/apply logic inside that function.

This CL should have no externally observable behavior change, though it
does change one detail internally in the StyleCascade: the now-removed
ResolveSurrogate function called LookupAndApply on the original property
as a way to ensure that this property was marked as applied. The new
function (ApplySurrogate) simply marks it as such, effectively skipping
it. For symmetry this also adds MarkUnapplied, although this function
is unused at the moment. (It will be needed as part of the work on
'revert').

Bug: 579788
Change-Id: Ic10b341f9d6df3cfaa983e10b67cad0cace48a5d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2128073
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756019}

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

@@ -7,6 +7,7 @@
 #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/resolver/cascade_priority.h"
 
 namespace blink {
 
@@ -29,6 +30,16 @@ bool CascadeResolver::AllowSubstitution(CSSVariableData* data) const {
   return true;
 }
 
+void CascadeResolver::MarkUnapplied(CascadePriority* priority) const {
+  DCHECK(priority);
+  *priority = CascadePriority(*priority, 0);
+}
+
+void CascadeResolver::MarkApplied(CascadePriority* priority) const {
+  DCHECK(priority);
+  *priority = CascadePriority(*priority, generation_);
+}
+
 bool CascadeResolver::DetectCycle(const CSSProperty& property) {
   wtf_size_t index = stack_.Find(property.GetCSSPropertyName());
   if (index == kNotFound)

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

@@ -13,8 +13,9 @@
 
 namespace blink {
 
-class CSSVariableData;
+class CascadePriority;
 class CSSProperty;
+class CSSVariableData;
 
 namespace cssvalue {
 
@@ -44,6 +45,16 @@ class CORE_EXPORT CascadeResolver {
   // https://drafts.csswg.org/css-variables/#animation-tainted
   bool AllowSubstitution(CSSVariableData*) const;
 
+  // Sets the generation of the priority to zero, which has the effect of
+  // marking it as unapplied. (I.e. this can be used to force re-application of
+  // a declaration).
+  void MarkUnapplied(CascadePriority*) const;
+
+  // Sets the generation of the priority to the current generation,
+  // which has the effect of marking it as already applied. (I.e. this can be
+  // used to skip application of a declaration).
+  void MarkApplied(CascadePriority*) const;
+
   // Automatically locks and unlocks the given property. (See
   // CascadeResolver::IsLocked).
   class CORE_EXPORT AutoLock {

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

@@ -144,20 +144,6 @@ void StyleCascade::Reset() {
   generation_ = 0;
 }
 
-StyleCascade::Surrogate StyleCascade::ResolveSurrogate(
-    const CSSProperty& surrogate,
-    CascadePriority priority,
-    CascadeResolver& resolver) {
-  DCHECK(surrogate.IsSurrogate());
-  const CSSProperty* original = surrogate.SurrogateFor(
-      state_.Style()->Direction(), state_.Style()->GetWritingMode());
-  DCHECK(original && original->IsLonghand());
-  LookupAndApply(*original, resolver);
-  if (map_.At(original->GetCSSPropertyName()) < priority)
-    return Surrogate::kApply;
-  return Surrogate::kSkip;
-}
-
 const CSSValue* StyleCascade::Resolve(const CSSPropertyName& name,
                                       const CSSValue& value,
                                       CascadeResolver& resolver) {
@@ -282,8 +268,10 @@ void StyleCascade::ApplyMatchResult(CascadeResolver& resolver) {
         continue;
       *p = priority;
       const CSSProperty& property = e.Property();
-      if (ResolveIfSurrogate(property, priority, resolver) == Surrogate::kSkip)
+      if (property.IsSurrogate()) {
+        ApplySurrogate(property, priority, resolver);
         continue;
+      }
       const CSSValue* value = Resolve(property, e.Value(), resolver);
       StyleBuilder::ApplyProperty(property, state_, *value);
     }
@@ -322,8 +310,10 @@ void StyleCascade::ApplyInterpolationMap(const ActiveInterpolationsMap& map,
     }
     *p = priority;
 
-    if (ResolveIfSurrogate(property, priority, resolver) == Surrogate::kSkip)
+    if (property.IsSurrogate()) {
+      ApplySurrogate(property, priority, resolver);
       continue;
+    }
 
     ApplyInterpolation(property, priority, entry.value, resolver);
   }
@@ -368,6 +358,29 @@ void StyleCascade::ApplyInterpolation(
   }
 }
 
+void StyleCascade::ApplySurrogate(const CSSProperty& surrogate,
+                                  CascadePriority surrogate_priority,
+                                  CascadeResolver& resolver) {
+  DCHECK(surrogate.IsSurrogate());
+
+  const CSSProperty& original = SurrogateFor(surrogate);
+  CascadePriority* original_priority = map_.Find(original.GetCSSPropertyName());
+
+  if (original_priority) {
+    if (surrogate_priority < *original_priority) {
+      // The original has a higher priority, so skip the surrogate property.
+      return;
+    }
+
+    // The surrogate has a higher priority, so skip the original property.
+    // The original might have been applied already, but that doesn't matter,
+    // as we're about to overwrite it.
+    resolver.MarkApplied(original_priority);
+  }
+
+  LookupAndApplyValue(surrogate, surrogate_priority, resolver);
+}
+
 void StyleCascade::LookupAndApply(const CSSPropertyName& name,
                                   CascadeResolver& resolver) {
   CSSPropertyRef ref(name, state_.GetDocument());
@@ -390,9 +403,17 @@ void StyleCascade::LookupAndApply(const CSSProperty& property,
 
   if (resolver.filter_.Rejects(property))
     return;
-  if (ResolveIfSurrogate(property, priority, resolver) == Surrogate::kSkip)
+  if (property.IsSurrogate()) {
+    ApplySurrogate(property, priority, resolver);
     return;
+  }
 
+  LookupAndApplyValue(property, priority, resolver);
+}
+
+void StyleCascade::LookupAndApplyValue(const CSSProperty& property,
+                                       CascadePriority priority,
+                                       CascadeResolver& resolver) {
   if (priority.GetOrigin() < CascadeOrigin::kAnimation)
     LookupAndApplyDeclaration(property, priority, resolver);
   else if (priority.GetOrigin() >= CascadeOrigin::kAnimation)
@@ -781,6 +802,15 @@ const Document& StyleCascade::GetDocument() const {
   return state_.GetDocument();
 }
 
+const CSSProperty& StyleCascade::SurrogateFor(
+    const CSSProperty& surrogate) const {
+  DCHECK(surrogate.IsSurrogate());
+  const CSSProperty* original = surrogate.SurrogateFor(
+      state_.Style()->Direction(), state_.Style()->GetWritingMode());
+  DCHECK(original);
+  return *original;
+}
+
 bool StyleCascade::HasAuthorDeclaration(const CSSProperty& property) const {
   return map_.At(property.GetCSSPropertyName()).GetOrigin() ==
          CascadeOrigin::kAuthor;

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

@@ -154,10 +154,20 @@ class CORE_EXPORT StyleCascade {
                           CascadePriority,
                           const ActiveInterpolations&,
                           CascadeResolver&);
+  // Surrogates (such as css-logical properties) require special handling, since
+  // both the surrogate and the original property exist in the cascade at the
+  // same time. For example, 'inline-size' and 'width' may both exist in the
+  // CascadeMap, and the winner must be determined Apply-time, since we don't
+  // know which physical property 'inline-size' corresponds to before
+  // 'writing-mode' and 'direction' have been applied.
+  void ApplySurrogate(const CSSProperty&, CascadePriority, CascadeResolver&);
 
   // Looks up a value with random access, and applies it.
   void LookupAndApply(const CSSPropertyName&, CascadeResolver&);
   void LookupAndApply(const CSSProperty&, CascadeResolver&);
+  void LookupAndApplyValue(const CSSProperty&,
+                           CascadePriority,
+                           CascadeResolver&);
   void LookupAndApplyDeclaration(const CSSProperty&,
                                  CascadePriority,
                                  CascadeResolver&);
@@ -216,53 +226,6 @@ class CORE_EXPORT StyleCascade {
     WTF::TextEncoding charset_;
   };
 
-  enum class Surrogate { kSkip, kApply, kNoSurrogate };
-
-  // A property can be skipped apply-time if it's a surrogate property.
-  //
-  // A surrogate property is an alias-like property that shares a storage
-  // location on ComputedStyle with the property it's a surrogate for. Unlike
-  // aliases, they are not resolved parse-time, but exist alongside the original
-  // property in the parsed rule.
-  //
-  // For example, consider a rule:
-  //
-  // div {
-  //   inline-size: 20px;
-  //   width: 10px;
-  // }
-  //
-  // The inline-size property is a surrogate of width in this case. When we
-  // encounter inline-size during Apply, we check the CascadePriority of the
-  // the original property (width). Since width has a higher priority (it
-  // appears later), we skip applying inline-size.
-  //
-  // Had the order been reversed:
-  //
-  //  div {
-  //    width: 10px;
-  //    inline-size: 20px;
-  //  }
-  //
-  // we would first apply width (it's unaware that it has surrogates pointing to
-  // it), and then later also apply inline-size (we check the CascadePriority
-  // of width and find that it's lower than inline-size's).
-  //
-  // Surrogate properties should be skipped (i.e. not applied after all) if
-  // the corresponding original property has a higher priority.
-  //
-  Surrogate ResolveSurrogate(const CSSProperty&,
-                             CascadePriority,
-                             CascadeResolver&);
-
-  inline Surrogate ResolveIfSurrogate(const CSSProperty& property,
-                                      CascadePriority priority,
-                                      CascadeResolver& resolver) {
-    if (!property.IsSurrogate())
-      return Surrogate::kNoSurrogate;
-    return ResolveSurrogate(property, priority, resolver);
-  }
-
   // Resolving Values
   //
   // *Resolving* a value, means looking at the dependencies for a given
@@ -330,6 +293,7 @@ class CORE_EXPORT StyleCascade {
   void MarkHasVariableReference(const CSSProperty&);
 
   const Document& GetDocument() const;
+  const CSSProperty& SurrogateFor(const CSSProperty& surrogate) const;
 
   bool HasAuthorDeclaration(const CSSProperty&) const;
   bool HasAuthorBorder() const;

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

@@ -208,8 +208,8 @@ class TestCascadeResolver {
   STACK_ALLOCATED();
 
  public:
-  explicit TestCascadeResolver(Document& document)
-      : document_(document), resolver_(CascadeFilter(), 0) {}
+  explicit TestCascadeResolver(Document& document, uint8_t generation = 0)
+      : document_(document), resolver_(CascadeFilter(), generation) {}
   bool InCycle() const { return resolver_.InCycle(); }
   bool DetectCycle(String name) {
     CSSPropertyRef ref(name, document_);
@@ -218,6 +218,13 @@ class TestCascadeResolver {
     return resolver_.DetectCycle(property);
   }
   wtf_size_t CycleDepth() const { return resolver_.cycle_depth_; }
+  void MarkApplied(CascadePriority* priority) {
+    resolver_.MarkApplied(priority);
+  }
+  void MarkUnapplied(CascadePriority* priority) {
+    resolver_.MarkUnapplied(priority);
+  }
+  uint8_t GetGeneration() { return resolver_.generation_; }
 
  private:
   friend class TestCascadeAutoLock;
@@ -710,6 +717,37 @@ TEST_F(StyleCascadeTest, ResolverDetectMultiCycleReverse) {
   EXPECT_FALSE(resolver.InCycle());
 }
 
+TEST_F(StyleCascadeTest, ResolverMarkApplied) {
+  TestCascadeResolver resolver(GetDocument(), 2);
+
+  CascadePriority priority(CascadeOrigin::kAuthor);
+  EXPECT_EQ(0, priority.GetGeneration());
+
+  resolver.MarkApplied(&priority);
+  EXPECT_EQ(2, priority.GetGeneration());
+
+  // Mark a second time to verify observation of the same generation.
+  resolver.MarkApplied(&priority);
+  EXPECT_EQ(2, priority.GetGeneration());
+}
+
+TEST_F(StyleCascadeTest, ResolverMarkUnapplied) {
+  TestCascadeResolver resolver(GetDocument(), 7);
+
+  CascadePriority priority(CascadeOrigin::kAuthor);
+  EXPECT_EQ(0, priority.GetGeneration());
+
+  resolver.MarkApplied(&priority);
+  EXPECT_EQ(7, priority.GetGeneration());
+
+  resolver.MarkUnapplied(&priority);
+  EXPECT_EQ(0, priority.GetGeneration());
+
+  // Mark a second time to verify observation of the same generation.
+  resolver.MarkUnapplied(&priority);
+  EXPECT_EQ(0, priority.GetGeneration());
+}
+
 TEST_F(StyleCascadeTest, BasicCycle) {
   TestCascade cascade(GetDocument());
   cascade.Add("--a", "foo");