Fold tree scopes into CSSValue and prepare to remove ScopedCSSValue

(See [2] for the full design) We currently support tree-scoped names and references with ScopedCSSValue. However, this only allows us to associate an entire property value with a tree scope. With anchor position, it's possible that we need to deal with a property value whose different parts are associated with different tree scopes (e.g., during a transition, see test cases in the follow up patch [1]). So instead of creating a temporary ScopedCSSValue, we need to actually store tree scopes in CSSValues. See [2] for the full design. This patch lays the foundation of turning from ScopedCSSValue to the new tree-scoped CSSValue: - Add fields into CSSValue to indicate whether it's populated with a tree scope, and add functions to create tree-scope-populated versions of the same value - StyleCascade passes tree-scope-populated CSSValues to StyleBuilder - Also converts the implementation of `list-style-type` property to use the new CSSValue instead of ScopedCSSValue. This doesn't cause any behavior change, and verifies the correctness of the design Follow up patches will convert all properties that are currently using ScopedCSSValue to use the new tree-scope-populated CSSValue, and then remove ScopedCSSValue. [1] crrev.com/c/4185495 [2] crbug.com/1395026#c12 Bug: 1395026 Change-Id: I0dabae063d3901e2605dd8e65692fd1f7d6283f8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4167268 Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org> Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org> Cr-Commit-Position: refs/heads/main@{#1096450}
chromium · Jan 24, 2023 · 87ec449 · 87ec449
1 parent cd56fa3
commit 87ec449
Show file tree

Hide file tree

Showing 9 changed files with 95 additions and 26 deletions.
diff --git a/third_party/blink/renderer/core/animation/css_default_interpolation_type.cc b/third_party/blink/renderer/core/animation/css_default_interpolation_type.cc
@@ -46,11 +46,14 @@ void CSSDefaultInterpolationType::Apply(
     InterpolationEnvironment& environment) const {
   DCHECK(
       To<CSSDefaultNonInterpolableValue>(non_interpolable_value)->CssValue());
+  // TODO(crbug.com/1395026): Populate the correct tree scope here, and stop
+  // using ScopedCSSValue.
   StyleBuilder::ApplyProperty(
       GetProperty().GetCSSPropertyName(),
       To<CSSInterpolationEnvironment>(environment).GetState(),
-      ScopedCSSValue(*To<CSSDefaultNonInterpolableValue>(non_interpolable_value)
-                          ->CssValue(),
+      ScopedCSSValue(To<CSSDefaultNonInterpolableValue>(non_interpolable_value)
+                         ->CssValue()
+                         ->EnsureScopedValue(nullptr),
                      nullptr));
 }
 

diff --git a/third_party/blink/renderer/core/css/css_custom_ident_value.cc b/third_party/blink/renderer/core/css/css_custom_ident_value.cc
@@ -6,6 +6,7 @@
 
 #include "third_party/blink/renderer/core/css/css_markup.h"
 #include "third_party/blink/renderer/core/css/properties/css_unresolved_property.h"
+#include "third_party/blink/renderer/core/dom/tree_scope.h"
 #include "third_party/blink/renderer/platform/wtf/text/string_builder.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
 
@@ -14,7 +15,9 @@ namespace blink {
 CSSCustomIdentValue::CSSCustomIdentValue(const AtomicString& str)
     : CSSValue(kCustomIdentClass),
       string_(str),
-      property_id_(CSSPropertyID::kInvalid) {}
+      property_id_(CSSPropertyID::kInvalid) {
+  needs_tree_scope_population_ = true;
+}
 
 CSSCustomIdentValue::CSSCustomIdentValue(CSSPropertyID id)
     : CSSValue(kCustomIdentClass), string_(), property_id_(id) {
@@ -31,7 +34,18 @@ String CSSCustomIdentValue::CustomCSSText() const {
   return builder.ReleaseString();
 }
 
+const CSSCustomIdentValue& CSSCustomIdentValue::PopulateWithTreeScope(
+    const TreeScope* tree_scope) const {
+  DCHECK(this->needs_tree_scope_population_);
+  CSSCustomIdentValue* populated =
+      MakeGarbageCollected<CSSCustomIdentValue>(*this);
+  populated->tree_scope_ = tree_scope;
+  populated->needs_tree_scope_population_ = false;
+  return *populated;
+}
+
 void CSSCustomIdentValue::TraceAfterDispatch(blink::Visitor* visitor) const {
+  visitor->Trace(tree_scope_);
   CSSValue::TraceAfterDispatch(visitor);
 }
 

diff --git a/third_party/blink/renderer/core/css/css_custom_ident_value.h b/third_party/blink/renderer/core/css/css_custom_ident_value.h
@@ -13,11 +13,14 @@
 
 namespace blink {
 
+class TreeScope;
+
 class CORE_EXPORT CSSCustomIdentValue : public CSSValue {
  public:
   explicit CSSCustomIdentValue(const AtomicString&);
   explicit CSSCustomIdentValue(CSSPropertyID);
 
+  const TreeScope* GetTreeScope() const { return tree_scope_; }
   const AtomicString& Value() const {
     DCHECK(!IsKnownPropertyID());
     return string_;
@@ -32,14 +35,20 @@ class CORE_EXPORT CSSCustomIdentValue : public CSSValue {
 
   String CustomCSSText() const;
 
+  const CSSCustomIdentValue& PopulateWithTreeScope(const TreeScope*) const;
+
   bool Equals(const CSSCustomIdentValue& other) const {
-    return IsKnownPropertyID() ? property_id_ == other.property_id_
-                               : string_ == other.string_;
+    if (IsKnownPropertyID()) {
+      return property_id_ == other.property_id_;
+    }
+    return IsScopedValue() == other.IsScopedValue() &&
+           tree_scope_ == other.tree_scope_ && string_ == other.string_;
   }
 
   void TraceAfterDispatch(blink::Visitor*) const;
 
  private:
+  Member<const TreeScope> tree_scope_;
   AtomicString string_;
   CSSPropertyID property_id_;
 };

diff --git a/third_party/blink/renderer/core/css/css_properties.json5 b/third_party/blink/renderer/core/css/css_properties.json5
@@ -3221,7 +3221,6 @@
         "decimal", "none"
       ],
       style_builder_custom_functions: ["value"],
-      tree_scoped_value: true,
     },
     {
       name: "margin-bottom",

diff --git a/third_party/blink/renderer/core/css/css_value.cc b/third_party/blink/renderer/core/css/css_value.cc
@@ -459,6 +459,17 @@ String CSSValue::CssText() const {
   return String();
 }
 
+const CSSValue& CSSValue::PopulateWithTreeScope(
+    const TreeScope* tree_scope) const {
+  switch (GetClassType()) {
+    case kCustomIdentClass:
+      return To<CSSCustomIdentValue>(this)->PopulateWithTreeScope(tree_scope);
+    default:
+      NOTREACHED();
+      return *this;
+  }
+}
+
 void CSSValue::Trace(Visitor* visitor) const {
   switch (GetClassType()) {
     case kAxisClass:

diff --git a/third_party/blink/renderer/core/css/css_value.h b/third_party/blink/renderer/core/css/css_value.h
@@ -31,6 +31,7 @@ namespace blink {
 
 class Document;
 class Length;
+class TreeScope;
 
 class CORE_EXPORT CSSValue : public GarbageCollected<CSSValue> {
  public:
@@ -195,6 +196,16 @@ class CORE_EXPORT CSSValue : public GarbageCollected<CSSValue> {
   bool operator==(const CSSValue&) const;
   bool operator!=(const CSSValue& o) const { return !(*this == o); }
 
+  // Returns the same CSS value, but populated with the given tree scope for
+  // tree-scoped names and references.
+  const CSSValue& EnsureScopedValue(const TreeScope* tree_scope) const {
+    if (!needs_tree_scope_population_) {
+      return *this;
+    }
+    return PopulateWithTreeScope(tree_scope);
+  }
+  bool IsScopedValue() const { return !needs_tree_scope_population_; }
+
   void TraceAfterDispatch(blink::Visitor* visitor) const {}
   void Trace(Visitor*) const;
 
@@ -290,7 +301,12 @@ class CORE_EXPORT CSSValue : public GarbageCollected<CSSValue> {
 
   ClassType GetClassType() const { return static_cast<ClassType>(class_type_); }
 
-  explicit CSSValue(ClassType class_type) : class_type_(class_type) {}
+  const CSSValue& PopulateWithTreeScope(const TreeScope*) const;
+
+  explicit CSSValue(ClassType class_type)
+      : allows_negative_percentage_reference_(false),
+        needs_tree_scope_population_(false),
+        class_type_(class_type) {}
 
   // NOTE: This class is non-virtual for memory and performance reasons.
   // Don't go making it virtual again unless you know exactly what you're doing!
@@ -310,7 +326,15 @@ class CORE_EXPORT CSSValue : public GarbageCollected<CSSValue> {
   uint8_t value_list_separator_ = kSpaceSeparator;
 
   // CSSMathFunctionValue:
-  bool allows_negative_percentage_reference_ = false;
+  uint8_t allows_negative_percentage_reference_ : 1;  // NOLINT
+
+  // Any CSS value that defines/references a global name should be tree-scoped.
+  // However, to allow sharing StyleSheetContents, we don't directly populate
+  // CSS values with tree scope in parsed results, but wait until resolving an
+  // element's style.
+  // The flag is true if the value contains such references but hasn't been
+  // populated with a tree scope.
+  uint8_t needs_tree_scope_population_ : 1;  // NOLINT
 
  private:
   const uint8_t class_type_;  // ClassType

diff --git a/third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc b/third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
@@ -4852,24 +4852,20 @@ const CSSValue* ListStyleType::CSSValueFromComputedStyleInternal(
   if (!style.ListStyleType()) {
     return CSSIdentifierValue::Create(CSSValueID::kNone);
   }
-  if (style.ListStyleType()->IsString()) {
+  const ListStyleTypeData& list_style_type = *style.ListStyleType();
+  if (list_style_type.IsString()) {
     return MakeGarbageCollected<CSSStringValue>(
-        style.ListStyleType()->GetStringValue());
+        list_style_type.GetStringValue());
   }
   // TODO(crbug.com/687225): Return a scoped CSSValue?
   return MakeGarbageCollected<CSSCustomIdentValue>(
-      style.ListStyleType()->GetCounterStyleName());
+      list_style_type.GetCounterStyleName());
 }
 
 void ListStyleType::ApplyValue(StyleResolverState& state,
                                const CSSValue& value) const {
-  NOTREACHED();
-}
-
-void ListStyleType::ApplyValue(StyleResolverState& state,
-                               const ScopedCSSValue& scoped_value) const {
+  DCHECK(value.IsScopedValue());
   ComputedStyleBuilder& builder = state.StyleBuilder();
-  const CSSValue& value = scoped_value.GetCSSValue();
   if (const auto* identifier_value = DynamicTo<CSSIdentifierValue>(value)) {
     DCHECK_EQ(CSSValueID::kNone, identifier_value->GetValueID());
     builder.SetListStyleType(nullptr);
@@ -4885,7 +4881,7 @@ void ListStyleType::ApplyValue(StyleResolverState& state,
   DCHECK(value.IsCustomIdentValue());
   const auto& custom_ident_value = To<CSSCustomIdentValue>(value);
   builder.SetListStyleType(ListStyleTypeData::CreateCounterStyle(
-      custom_ident_value.Value(), scoped_value.GetTreeScope()));
+      custom_ident_value.Value(), custom_ident_value.GetTreeScope()));
 }
 
 bool MarginBlockEnd::IsLayoutDependent(const ComputedStyle* style,

diff --git a/third_party/blink/renderer/core/css/resolver/style_cascade.cc b/third_party/blink/renderer/core/css/resolver/style_cascade.cc
@@ -704,8 +704,12 @@ void StyleCascade::LookupAndApplyDeclaration(const CSSProperty& property,
   } else if (origin == CascadeOrigin::kAuthorPresentationalHint) {
     tree_scope = &GetDocument();
   }
-  StyleBuilder::ApplyPhysicalProperty(property, state_,
-                                      ScopedCSSValue(*value, tree_scope));
+  // TODO(crbug.com/1395026): Convert properties to use CSSValues (with tree
+  // scope populated) directly instead of ScopedCSSValue, and get rid of
+  // ScopedCSSValue class when all properties are converted.
+  StyleBuilder::ApplyPhysicalProperty(
+      property, state_,
+      ScopedCSSValue(value->EnsureScopedValue(tree_scope), tree_scope));
 }
 
 void StyleCascade::LookupAndApplyInterpolation(const CSSProperty& property,

diff --git a/third_party/blink/renderer/core/css/resolver/style_resolver.cc b/third_party/blink/renderer/core/css/resolver/style_resolver.cc
@@ -1508,9 +1508,12 @@ void StyleResolver::ApplyBaseStyle(
            ++property_idx) {
         CSSPropertyValueSet::PropertyReference property =
             inline_style->PropertyAt(property_idx);
+        // TODO(crbug.com/1395026): Get rid of ScopedCSSValue when all
+        // properties are converted to use CSSValue directly.
         StyleBuilder::ApplyProperty(
             property.Name(), state,
-            ScopedCSSValue(property.Value(), &GetDocument()));
+            ScopedCSSValue(property.Value().EnsureScopedValue(&GetDocument()),
+                           &GetDocument()));
       }
     }
 
@@ -2195,8 +2198,12 @@ FilterOperations StyleResolver::ComputeFilterOperations(
 
   state.SetStyle(ComputedStyle::Clone(*parent));
 
-  StyleBuilder::ApplyProperty(GetCSSPropertyFilter(), state,
-                              ScopedCSSValue(filter_value, &GetDocument()));
+  // TODO(crbug.com/1395026): Get rid of ScopedCSSValue when all
+  // properties are converted to use CSSValue directly.
+  StyleBuilder::ApplyProperty(
+      GetCSSPropertyFilter(), state,
+      ScopedCSSValue(filter_value.EnsureScopedValue(&GetDocument()),
+                     &GetDocument()));
 
   state.LoadPendingResources();
 
@@ -2364,11 +2371,13 @@ void StyleResolver::ComputeFont(Element& element,
     // TODO(futhark): If we start supporting fonts on ShadowRoot.fonts in
     // addition to Document.fonts, we need to pass the correct TreeScope instead
     // of GetDocument() in the ScopedCSSValue below.
+    // TODO(crbug.com/1395026): Get rid of ScopedCSSValue when all
+    // properties are converted to use CSSValue directly.
     StyleBuilder::ApplyProperty(
         *property, state,
-        ScopedCSSValue(
-            *property_set.GetPropertyCSSValue(property->PropertyID()),
-            &GetDocument()));
+        ScopedCSSValue(property_set.GetPropertyCSSValue(property->PropertyID())
+                           ->EnsureScopedValue(&GetDocument()),
+                       &GetDocument()));
   }
   state.UpdateFont();
 }