Bug 1840478 - Introduce ComputedCustomProperties struct to handle inherited/non-inherited properties. r=emilio

The CSS Properties and Values API allows to register CSS properties that
can be either inherited or non-inherited, and which can also have
initial values specified.

Stylo currently implements efficient inheritance of classical CSS
variables (inherited, guaranteed-invalid initial value) by using
a ref-counted CustomPropertiesMap in the cascade. Such a map
contains computed values differing from the parent (or default)
style. For non-inherited properties, the computed value is
instead generally the initial value and only inherits the
computed value from the parent if explicitly specified on the node.

This patch converts the computed value for custom properties into a
pair of maps CustomPropertiesMap, one map which is ref-counted
(containing inherited custom properties) and one which is not
(containing non-inherited properties). The code is refactored to
continue to deal with the `inherited` map of the pair and behavior
is unchanged. A follow-up patch will modify the code to deal the
`non-inherited` map too.

Additionally, two different structs are used to represent the pair
of maps. A mutable version MutableCustomProperties used when
building the computed style and an immutable version
ComputedCustomProperties used during subsequent substitutions of
var/env in other properties. As an improvement to the current code,
the ref-counted map is actually represented as a UniqueArc for
the MutableCustomProperties instead of an Arc.

[1] https://drafts.css-houdini.org/css-properties-values-api-1/

Differential Revision: https://phabricator.services.mozilla.com/D188727

layout/style/ServoBindings.toml

@@ -665,7 +665,7 @@ mapped-generic-types = [
     { generic = true, gecko = "mozilla::RustCell", servo = "::std::cell::Cell" },
     { generic = false, gecko = "ServoNodeData", servo = "atomic_refcell::AtomicRefCell<crate::data::ElementData>" },
     { generic = false, gecko = "mozilla::ServoWritingMode", servo = "crate::logical_geometry::WritingMode" },
-    { generic = false, gecko = "mozilla::ServoCustomPropertiesMap", servo = "Option<servo_arc::Arc<crate::custom_properties::CustomPropertiesMap>>" },
+    { generic = false, gecko = "mozilla::ServoComputedCustomProperties", servo = "crate::custom_properties::ComputedCustomProperties" },
     { generic = false, gecko = "mozilla::ServoRuleNode", servo = "Option<crate::rule_tree::StrongRuleNode>" },
     { generic = false, gecko = "nsACString", servo = "nsstring::nsACString" },
     { generic = false, gecko = "nsAString", servo = "nsstring::nsAString" },

layout/style/ServoComputedData.h

@@ -21,8 +21,9 @@ struct ServoWritingMode {
   uint8_t mBits;
 };
 
-struct ServoCustomPropertiesMap {
-  uintptr_t mPtr;
+struct ServoComputedCustomProperties {
+  uintptr_t mInherited;
+  uintptr_t mNonInherited;
 };
 
 struct ServoRuleNode {
@@ -72,7 +73,7 @@ class ServoComputedData {
   mozilla::ServoWritingMode WritingMode() const { return writing_mode; }
 
  private:
-  mozilla::ServoCustomPropertiesMap custom_properties;
+  mozilla::ServoComputedCustomProperties custom_properties;
   mozilla::ServoWritingMode writing_mode;
   mozilla::StyleComputedValueFlags flags;
   /// The rule node representing the ordered list of rules matched for this

servo/components/style/custom_properties.rs

@@ -10,15 +10,15 @@ use crate::applicable_declarations::CascadePriority;
 use crate::media_queries::Device;
 use crate::properties::{CSSWideKeyword, CustomDeclaration, CustomDeclarationValue};
 use crate::properties_and_values::value::ComputedValue as ComputedRegisteredValue;
-use crate::stylist::Stylist;
 use crate::selector_map::{PrecomputedHashMap, PrecomputedHashSet, PrecomputedHasher};
+use crate::stylist::Stylist;
 use crate::Atom;
 use cssparser::{
     CowRcStr, Delimiter, Parser, ParserInput, SourcePosition, Token, TokenSerializationType,
 };
 use indexmap::IndexMap;
 use selectors::parser::SelectorParseErrorKind;
-use servo_arc::Arc;
+use servo_arc::{Arc, UniqueArc};
 use smallvec::SmallVec;
 use std::borrow::Cow;
 use std::cmp;
@@ -207,6 +207,113 @@ impl ToCss for SpecifiedValue {
 pub type CustomPropertiesMap =
     IndexMap<Name, Arc<VariableValue>, BuildHasherDefault<PrecomputedHasher>>;
 
+/// A pair of separate CustomPropertiesMaps, split between custom properties
+/// that have the inherit flag set and those with the flag unset.
+#[repr(C)]
+#[derive(Clone, Debug, Default)]
+pub struct ComputedCustomProperties {
+    /// Map for custom properties with inherit flag set, including classical CSS
+    /// variables. Defined as ref-counted for cheap copy.
+    pub inherited: Option<Arc<CustomPropertiesMap>>,
+    /// Map for custom properties with inherit flag unset.
+    pub non_inherited: Option<Box<CustomPropertiesMap>>,
+}
+
+impl ComputedCustomProperties {
+    /// Return whether the inherited and non_inherited maps are none.
+    pub fn is_empty(&self) -> bool {
+        self.inherited.is_none() && self.non_inherited.is_none()
+    }
+
+    fn read(&self) -> ReadOnlyCustomProperties {
+        ReadOnlyCustomProperties {
+            inherited: self.inherited.as_deref(),
+            non_inherited: self.non_inherited.as_deref(),
+        }
+    }
+}
+
+impl PartialEq for ComputedCustomProperties {
+    fn eq(&self, other: &Self) -> bool {
+        // IndexMap equality doesn't consider ordering, which we have to account for.
+        // Also, for the same reason, IndexMap equality comparisons are slower than needed.
+        //
+        // See https://github.com/bluss/indexmap/issues/153
+        // TODO(bug 1840478): Handle non-inherited properties.
+        match (&self.inherited, &other.inherited) {
+            (Some(l), Some(r)) => {
+                l.len() == r.len() &&
+                    l.iter()
+                        .zip(r.iter())
+                        .all(|((k1, v1), (k2, v2))| k1 == k2 && v1 == v2)
+            },
+            (None, None) => true,
+            _ => false,
+        }
+    }
+}
+
+/// A mutable CustomPropertiesMapPair, using UniqueArc for inherited properties.
+#[derive(Default)]
+struct MutableCustomProperties {
+    inherited: Option<UniqueArc<CustomPropertiesMap>>,
+    non_inherited: Option<Box<CustomPropertiesMap>>,
+}
+
+impl MutableCustomProperties {
+    /// Insert a custom property in the corresponding inherited/non_inherited
+    /// map, depending on whether the inherit flag is set or unset.
+    fn insert(
+        &mut self,
+        name: Name,
+        value: Arc<VariableValue>,
+    ) -> Option<Arc<VariableValue>> {
+        // TODO(bug 1840478): Handle non-inherited properties.
+        let map = self
+            .inherited
+            .get_or_insert_with(|| UniqueArc::new(CustomPropertiesMap::default()));
+        map.insert(name, value)
+    }
+
+    /// Remove a custom property from the corresponding inherited/non_inherited
+    /// map, depending on whether the inherit flag is set or unset.
+    fn remove(&mut self, name: &Name) -> Option<Arc<VariableValue>> {
+        // TODO(bug 1840478): Handle non-inherited properties.
+        self.inherited.as_mut()?.remove(name)
+    }
+
+    /// Shrink the capacity of the inherited/non_inherited maps as much as
+    /// possible.
+    fn shrink_to_fit(&mut self) {
+        if let Some(ref mut map) = self.inherited {
+            map.shrink_to_fit();
+        }
+        if let Some(ref mut map) = self.non_inherited {
+            map.shrink_to_fit();
+        }
+    }
+
+    fn read(&self) -> ReadOnlyCustomProperties {
+        ReadOnlyCustomProperties {
+            inherited: self.inherited.as_deref(),
+            non_inherited: self.non_inherited.as_deref(),
+        }
+    }
+}
+
+#[derive(Copy, Clone, Default)]
+struct ReadOnlyCustomProperties<'a> {
+    inherited: Option<&'a CustomPropertiesMap>,
+    non_inherited: Option<&'a CustomPropertiesMap>,
+}
+
+impl<'a> ReadOnlyCustomProperties<'a> {
+    fn get(&self, name: &Name) -> Option<&Arc<VariableValue>> {
+        // TODO(bug 1840478): Handle non-inherited properties.
+        self.inherited?.get(name)
+    }
+}
+
 /// Both specified and computed values are VariableValues, the difference is
 /// whether var() functions are expanded.
 pub type SpecifiedValue = VariableValue;
@@ -587,9 +694,10 @@ fn parse_fallback<'i, 't>(input: &mut Parser<'i, 't>) -> Result<(), ParseError<'
 
 fn parse_and_substitute_fallback<'i>(
     input: &mut Parser<'i, '_>,
-    custom_properties: &CustomPropertiesMap,
+    custom_properties: ReadOnlyCustomProperties,
     stylist: &Stylist,
 ) -> Result<ComputedValue, ParseError<'i>> {
+    debug_assert!(custom_properties.non_inherited.is_none());
     input.skip_whitespace();
     let after_comma = input.state();
     let first_token_type = input
@@ -650,20 +758,20 @@ fn parse_env_function<'i, 't>(
 pub struct CustomPropertiesBuilder<'a> {
     seen: PrecomputedHashSet<&'a Name>,
     may_have_cycles: bool,
-    custom_properties: Option<CustomPropertiesMap>,
-    inherited: Option<&'a Arc<CustomPropertiesMap>>,
+    custom_properties: MutableCustomProperties,
+    inherited: &'a ComputedCustomProperties,
     reverted: PrecomputedHashMap<&'a Name, (CascadePriority, bool)>,
     stylist: &'a Stylist,
 }
 
 impl<'a> CustomPropertiesBuilder<'a> {
     /// Create a new builder, inheriting from a given custom properties map.
-    pub fn new(inherited: Option<&'a Arc<CustomPropertiesMap>>, stylist: &'a Stylist) -> Self {
+    pub fn new(inherited: &'a ComputedCustomProperties, stylist: &'a Stylist) -> Self {
         Self {
             seen: PrecomputedHashSet::default(),
             reverted: Default::default(),
             may_have_cycles: false,
-            custom_properties: None,
+            custom_properties: MutableCustomProperties::default(),
             inherited,
             stylist,
         }
@@ -691,14 +799,15 @@ impl<'a> CustomPropertiesBuilder<'a> {
             return;
         }
 
-        if self.custom_properties.is_none() {
-            self.custom_properties = Some(match self.inherited {
-                Some(inherited) => (**inherited).clone(),
-                None => CustomPropertiesMap::default(),
+        // TODO(bug 1840478): Handle non-inherited properties.
+        if self.custom_properties.inherited.is_none() {
+            self.custom_properties.inherited = Some(match &self.inherited.inherited {
+                Some(inherited) => UniqueArc::new((**inherited).clone()),
+                None => UniqueArc::new(CustomPropertiesMap::default()),
             });
         }
 
-        let map = self.custom_properties.as_mut().unwrap();
+        let map = &mut self.custom_properties;
         match *value {
             CustomDeclarationValue::Value(ref unparsed_value) => {
                 let has_custom_property_references =
@@ -714,7 +823,7 @@ impl<'a> CustomPropertiesBuilder<'a> {
                             name,
                             unparsed_value,
                             map,
-                            self.inherited.map(|m| &**m),
+                            self.inherited,
                             self.stylist,
                         );
                         return;
@@ -758,11 +867,13 @@ impl<'a> CustomPropertiesBuilder<'a> {
             _ => {},
         }
 
+        // TODO(bug 1840478): Handle non-inherited properties.
         let existing_value = self
             .custom_properties
+            .inherited
             .as_ref()
             .and_then(|m| m.get(name))
-            .or_else(|| self.inherited.and_then(|m| m.get(name)));
+            .or_else(|| self.inherited.inherited.as_ref().and_then(|m| m.get(name)));
 
         match (existing_value, value) {
             (None, &CustomDeclarationValue::CSSWideKeyword(CSSWideKeyword::Initial)) => {
@@ -783,10 +894,12 @@ impl<'a> CustomPropertiesBuilder<'a> {
         true
     }
 
-    fn inherited_properties_match(&self, map: &CustomPropertiesMap) -> bool {
-        let inherited = match self.inherited {
-            Some(inherited) => inherited,
-            None => return false,
+    fn inherited_properties_match(&self, custom_properties: &MutableCustomProperties) -> bool {
+        // TODO(bug 1840478): Handle non-inherited properties.
+        let (inherited, map) = match (&self.inherited.inherited, &custom_properties.inherited) {
+            (Some(inherited), Some(map)) => (inherited, map),
+            (None, None) => return true,
+            _ => return false,
         };
         if inherited.len() != map.len() {
             return false;
@@ -805,16 +918,18 @@ impl<'a> CustomPropertiesBuilder<'a> {
     /// need to remove any potential cycles, and wrap it in an arc.
     ///
     /// Otherwise, just use the inherited custom properties map.
-    pub fn build(mut self) -> Option<Arc<CustomPropertiesMap>> {
-        let mut map = match self.custom_properties.take() {
-            Some(m) => m,
-            None => return self.inherited.cloned(),
-        };
+    pub fn build(mut self) -> ComputedCustomProperties {
+        // TODO(bug 1840478): Handle non-inherited properties.
+        debug_assert!(self.custom_properties.non_inherited.is_none());
+        debug_assert!(self.inherited.non_inherited.is_none());
+        if self.custom_properties.inherited.is_none() {
+            return self.inherited.clone();
+        }
 
         if self.may_have_cycles {
             substitute_all(
-                &mut map,
-                self.inherited.map(|m| &**m),
+                &mut self.custom_properties,
+                self.inherited,
                 &self.seen,
                 self.stylist,
             );
@@ -824,12 +939,19 @@ impl<'a> CustomPropertiesBuilder<'a> {
         // bug 1758974 comment 5. Try to detect the case where the values
         // haven't really changed, and save some memory by reusing the inherited
         // map in that case.
-        if self.inherited_properties_match(&map) {
-            return self.inherited.cloned();
+        if self.inherited_properties_match(&self.custom_properties) {
+            return self.inherited.clone();
         }
 
-        map.shrink_to_fit();
-        Some(Arc::new(map))
+        self.custom_properties.shrink_to_fit();
+        ComputedCustomProperties {
+            inherited: self
+                .custom_properties
+                .inherited
+                .take()
+                .map(|m| m.shareable()),
+            non_inherited: self.custom_properties.non_inherited.take(),
+        }
     }
 }
 
@@ -838,8 +960,8 @@ impl<'a> CustomPropertiesBuilder<'a> {
 ///
 /// It does cycle dependencies removal at the same time as substitution.
 fn substitute_all(
-    custom_properties_map: &mut CustomPropertiesMap,
-    inherited: Option<&CustomPropertiesMap>,
+    custom_properties_map: &mut MutableCustomProperties,
+    inherited: &ComputedCustomProperties,
     seen: &PrecomputedHashSet<&Name>,
     stylist: &Stylist,
 ) {
@@ -876,9 +998,9 @@ fn substitute_all(
         /// The stack of order index of visited variables. It contains
         /// all unfinished strong connected components.
         stack: SmallVec<[usize; 5]>,
-        map: &'a mut CustomPropertiesMap,
+        map: &'a mut MutableCustomProperties,
         /// The inherited custom properties to handle wide keywords.
-        inherited: Option<&'a CustomPropertiesMap>,
+        inherited: &'a ComputedCustomProperties,
         /// The stylist is used to get registered properties, and to resolve the environment to
         /// substitute `env()` variables.
         stylist: &'a Stylist,
@@ -905,7 +1027,8 @@ fn substitute_all(
     fn traverse<'a>(name: &Name, context: &mut Context<'a>) -> Option<usize> {
         // Some shortcut checks.
         let (name, value) = {
-            let value = context.map.get(name)?;
+            let props = context.map.read();
+            let value = props.get(name)?;
 
             // Nothing to resolve.
             if value.references.custom_properties.is_empty() {
@@ -1048,8 +1171,8 @@ fn substitute_all(
 fn substitute_references_in_value_and_apply(
     name: &Name,
     value: &VariableValue,
-    custom_properties: &mut CustomPropertiesMap,
-    inherited: Option<&CustomPropertiesMap>,
+    custom_properties: &mut MutableCustomProperties,
+    inherited: &ComputedCustomProperties,
     stylist: &Stylist,
 ) {
     debug_assert!(value.has_references());
@@ -1065,7 +1188,7 @@ fn substitute_references_in_value_and_apply(
             &mut input,
             &mut position,
             &mut computed_value,
-            custom_properties,
+            custom_properties.read(),
             stylist,
         );
 
@@ -1104,7 +1227,8 @@ fn substitute_references_in_value_and_apply(
                     // TODO: It's unclear what this should do for revert / revert-layer, see
                     // https://github.com/w3c/csswg-drafts/issues/9131. For now treating as unset
                     // seems fine?
-                    match inherited.and_then(|map| map.get(name)) {
+                    // TODO(bug 1840478): Handle non-inherited properties.
+                    match inherited.inherited.as_ref().and_then(|map| map.get(name)) {
                         Some(value) => {
                             custom_properties.insert(name.clone(), Arc::clone(value));
                         },
@@ -1145,9 +1269,10 @@ fn substitute_block<'i>(
     input: &mut Parser<'i, '_>,
     position: &mut (SourcePosition, TokenSerializationType),
     partial_computed_value: &mut ComputedValue,
-    custom_properties: &CustomPropertiesMap,
+    custom_properties: ReadOnlyCustomProperties,
     stylist: &Stylist,
 ) -> Result<TokenSerializationType, ParseError<'i>> {
+    debug_assert!(custom_properties.non_inherited.is_none());
     let mut last_token_type = TokenSerializationType::nothing();
     let mut set_position_at_next_iteration = false;
     loop {
@@ -1203,6 +1328,7 @@ fn substitute_block<'i>(
                             None
                         }
                     } else {
+                        // TODO(bug 1840478): Handle non-inherited properties.
                         registration = stylist.get_custom_property_registration(&name);
                         custom_properties.get(&name).map(|v| &**v)
                     };
@@ -1293,23 +1419,18 @@ fn substitute_block<'i>(
 pub fn substitute<'i>(
     input: &'i str,
     first_token_type: TokenSerializationType,
-    computed_values_map: Option<&Arc<CustomPropertiesMap>>,
+    custom_properties: &ComputedCustomProperties,
     stylist: &Stylist,
 ) -> Result<String, ParseError<'i>> {
     let mut substituted = ComputedValue::empty();
     let mut input = ParserInput::new(input);
     let mut input = Parser::new(&mut input);
     let mut position = (input.position(), first_token_type);
-    let empty_map = CustomPropertiesMap::default();
-    let custom_properties = match computed_values_map {
-        Some(m) => &**m,
-        None => &empty_map,
-    };
     let last_token_type = substitute_block(
         &mut input,
         &mut position,
         &mut substituted,
-        &custom_properties,
+        custom_properties.read(),
         stylist,
     )?;
     substituted.push_from(&input, position, last_token_type)?;

servo/components/style/properties/declaration_block.rs

@@ -17,13 +17,13 @@ use crate::custom_properties::{self, CustomPropertiesBuilder};
 use crate::error_reporting::{ContextualParseError, ParseErrorReporter};
 use crate::parser::ParserContext;
 use crate::properties::animated_properties::{AnimationValue, AnimationValueMap};
-use crate::stylist::Stylist;
 use crate::rule_tree::CascadeLevel;
 use crate::selector_map::PrecomputedHashSet;
 use crate::selector_parser::SelectorImpl;
 use crate::shared_lock::Locked;
 use crate::str::{CssString, CssStringWriter};
 use crate::stylesheets::{layer_rule::LayerOrder, CssRuleType, Origin, UrlExtraData};
+use crate::stylist::Stylist;
 use crate::values::computed::Context;
 use cssparser::{
     parse_important, AtRuleParser, CowRcStr, DeclarationParser, Delimiter, ParseErrorKind, Parser,
@@ -233,7 +233,7 @@ pub struct AnimationValueIterator<'a, 'cx, 'cx_a: 'cx> {
     context: &'cx mut Context<'cx_a>,
     default_values: &'a ComputedValues,
     /// Custom properties in a keyframe if exists.
-    extra_custom_properties: Option<&'a Arc<crate::custom_properties::CustomPropertiesMap>>,
+    extra_custom_properties: Option<&'a crate::custom_properties::ComputedCustomProperties>,
 }
 
 impl<'a, 'cx, 'cx_a: 'cx> AnimationValueIterator<'a, 'cx, 'cx_a> {
@@ -241,7 +241,7 @@ impl<'a, 'cx, 'cx_a: 'cx> AnimationValueIterator<'a, 'cx, 'cx_a> {
         declarations: &'a PropertyDeclarationBlock,
         context: &'cx mut Context<'cx_a>,
         default_values: &'a ComputedValues,
-        extra_custom_properties: Option<&'a Arc<crate::custom_properties::CustomPropertiesMap>>,
+        extra_custom_properties: Option<&'a crate::custom_properties::ComputedCustomProperties>,
     ) -> AnimationValueIterator<'a, 'cx, 'cx_a> {
         AnimationValueIterator {
             iter: declarations.declaration_importance_iter(),
@@ -353,7 +353,7 @@ impl PropertyDeclarationBlock {
         &'a self,
         context: &'cx mut Context<'cx_a>,
         default_values: &'a ComputedValues,
-        extra_custom_properties: Option<&'a Arc<crate::custom_properties::CustomPropertiesMap>>,
+        extra_custom_properties: Option<&'a crate::custom_properties::ComputedCustomProperties>,
     ) -> AnimationValueIterator<'a, 'cx, 'cx_a> {
         AnimationValueIterator::new(self, context, default_values, extra_custom_properties)
     }
@@ -866,10 +866,10 @@ impl PropertyDeclarationBlock {
                 // feels like a hack anyway...
                 block.cascade_custom_properties(cv.custom_properties(), stylist)
             } else {
-                cv.custom_properties().cloned()
+                cv.custom_properties().clone()
             }
         } else {
-            None
+            crate::custom_properties::ComputedCustomProperties::default()
         };
 
         match (declaration, computed_values) {
@@ -886,7 +886,7 @@ impl PropertyDeclarationBlock {
                     .substitute_variables(
                         declaration.id,
                         computed_values.writing_mode,
-                        custom_properties.as_ref(),
+                        &custom_properties,
                         QuirksMode::NoQuirks,
                         stylist,
                         &mut Default::default(),
@@ -934,7 +934,7 @@ impl PropertyDeclarationBlock {
     pub fn cascade_custom_properties_with_context(
         &self,
         context: &Context,
-    ) -> Option<Arc<crate::custom_properties::CustomPropertiesMap>> {
+    ) -> crate::custom_properties::ComputedCustomProperties {
         self.cascade_custom_properties(context.style().custom_properties(), context.style().stylist.unwrap())
     }
 
@@ -943,9 +943,9 @@ impl PropertyDeclarationBlock {
     /// properties.
     fn cascade_custom_properties(
         &self,
-        inherited_custom_properties: Option<&Arc<crate::custom_properties::CustomPropertiesMap>>,
+        inherited_custom_properties: &crate::custom_properties::ComputedCustomProperties,
         stylist: &Stylist,
-    ) -> Option<Arc<crate::custom_properties::CustomPropertiesMap>> {
+    ) -> crate::custom_properties::ComputedCustomProperties {
         let mut builder = CustomPropertiesBuilder::new(inherited_custom_properties, stylist);
 
         for declaration in self.normal_declaration_iter() {

servo/components/style/properties/gecko.mako.rs

@@ -13,7 +13,7 @@
 use crate::Atom;
 use app_units::Au;
 use crate::computed_value_flags::*;
-use crate::custom_properties::CustomPropertiesMap;
+use crate::custom_properties::ComputedCustomProperties;
 use crate::gecko_bindings::bindings;
 % for style_struct in data.style_structs:
 use crate::gecko_bindings::bindings::Gecko_Construct_Default_${style_struct.gecko_ffi_name};
@@ -65,7 +65,7 @@ impl ComputedValues {
 
     pub fn new(
         pseudo: Option<<&PseudoElement>,
-        custom_properties: Option<Arc<CustomPropertiesMap>>,
+        custom_properties: ComputedCustomProperties,
         writing_mode: WritingMode,
         flags: ComputedValueFlags,
         rules: Option<StrongRuleNode>,
@@ -88,7 +88,7 @@ impl ComputedValues {
 
     pub fn default_values(doc: &structs::Document) -> Arc<Self> {
         ComputedValuesInner::new(
-            /* custom_properties = */ None,
+            crate::custom_properties::ComputedCustomProperties::default(),
             /* writing_mode = */ WritingMode::empty(), // FIXME(bz): This seems dubious
             ComputedValueFlags::empty(),
             /* rules = */ None,
@@ -195,7 +195,7 @@ impl Drop for ComputedValuesInner {
 
 impl ComputedValuesInner {
     pub fn new(
-        custom_properties: Option<Arc<CustomPropertiesMap>>,
+        custom_properties: ComputedCustomProperties,
         writing_mode: WritingMode,
         flags: ComputedValueFlags,
         rules: Option<StrongRuleNode>,

servo/components/style/properties/helpers/animated_properties.mako.rs

@@ -14,7 +14,6 @@ use crate::properties::{CSSWideKeyword, PropertyDeclaration, NonCustomPropertyIt
 use crate::properties::longhands;
 use crate::properties::longhands::visibility::computed_value::T as Visibility;
 use crate::properties::LonghandId;
-use servo_arc::Arc;
 use std::ptr;
 use std::mem;
 use fxhash::FxHashMap;
@@ -245,7 +244,7 @@ impl AnimationValue {
     pub fn from_declaration(
         decl: &PropertyDeclaration,
         context: &mut Context,
-        extra_custom_properties: Option<<&Arc<crate::custom_properties::CustomPropertiesMap>>,
+        extra_custom_properties: Option< &crate::custom_properties::ComputedCustomProperties>,
         initial: &ComputedValues,
     ) -> Option<Self> {
         use super::PropertyDeclarationVariantRepr;
@@ -365,8 +364,7 @@ impl AnimationValue {
             PropertyDeclaration::WithVariables(ref declaration) => {
                 let mut cache = Default::default();
                 let substituted = {
-                    let custom_properties =
-                        extra_custom_properties.or_else(|| context.style().custom_properties());
+                    let custom_properties = extra_custom_properties.unwrap_or(&context.style().custom_properties());
 
                     debug_assert!(
                         context.builder.stylist.is_some(),

servo/components/style/properties/properties.mako.rs

@@ -1708,7 +1708,7 @@ impl UnparsedValue {
         &self,
         longhand_id: LonghandId,
         writing_mode: WritingMode,
-        custom_properties: Option<<&Arc<crate::custom_properties::CustomPropertiesMap>>,
+        custom_properties: &crate::custom_properties::ComputedCustomProperties,
         quirks_mode: QuirksMode,
         stylist: &Stylist,
         shorthand_cache: &'cache mut ShorthandsWithPropertyReferencesCache,
@@ -2998,7 +2998,7 @@ pub struct ComputedValuesInner {
     % for style_struct in data.active_style_structs():
         ${style_struct.ident}: Arc<style_structs::${style_struct.name}>,
     % endfor
-    custom_properties: Option<Arc<crate::custom_properties::CustomPropertiesMap>>,
+    custom_properties: crate::custom_properties::ComputedCustomProperties,
 
     /// The writing mode of this computed values struct.
     pub writing_mode: WritingMode,
@@ -3068,29 +3068,13 @@ impl ComputedValues {
     }
 
     /// Gets a reference to the custom properties map (if one exists).
-    pub fn custom_properties(&self) -> Option<<&Arc<crate::custom_properties::CustomPropertiesMap>> {
-        self.custom_properties.as_ref()
+    pub fn custom_properties(&self) -> &crate::custom_properties::ComputedCustomProperties {
+        &self.custom_properties
     }
 
     /// Returns whether we have the same custom properties as another style.
-    ///
-    /// This should effectively be just:
-    ///
-    ///   self.custom_properties() == other.custom_properties()
-    ///
-    /// But that's not really the case because IndexMap equality doesn't
-    /// consider ordering, which we have to account for. Also, for the same
-    /// reason, IndexMap equality comparisons are slower than needed.
-    ///
-    /// See https://github.com/bluss/indexmap/issues/153
     pub fn custom_properties_equal(&self, other: &Self) -> bool {
-        match (self.custom_properties(), other.custom_properties()) {
-            (Some(l), Some(r)) => {
-                l.len() == r.len() && l.iter().zip(r.iter()).all(|((k1, v1), (k2, v2))| k1 == k2 && v1 == v2)
-            },
-            (None, None) => true,
-            _ => false,
-        }
+      self.custom_properties() == other.custom_properties()
     }
 
 % for prop in data.longhands:
@@ -3227,7 +3211,7 @@ impl ComputedValues {
     /// Create a new refcounted `ComputedValues`
     pub fn new(
         pseudo: Option<<&PseudoElement>,
-        custom_properties: Option<Arc<crate::custom_properties::CustomPropertiesMap>>,
+        custom_properties: crate::custom_properties::ComputedCustomProperties,
         writing_mode: WritingMode,
         flags: ComputedValueFlags,
         rules: Option<StrongRuleNode>,
@@ -3266,8 +3250,9 @@ impl ComputedValues {
                 s
             }
             PropertyDeclarationId::Custom(name) => {
-                self.custom_properties
-                    .as_ref()
+              // TODO(bug 1840478): Handle non-inherited properties.
+              self.custom_properties
+                    .as_ref().inherited.unwrap()
                     .and_then(|map| map.get(name))
                     .map_or(String::new(), |value| value.to_css_string())
             }
@@ -3637,7 +3622,7 @@ pub struct StyleBuilder<'a> {
     pub rules: Option<StrongRuleNode>,
 
     /// The computed custom properties.
-    pub custom_properties: Option<Arc<crate::custom_properties::CustomPropertiesMap>>,
+    pub custom_properties: crate::custom_properties::ComputedCustomProperties,
 
     /// The pseudo-element this style will represent.
     pub pseudo: Option<<&'a PseudoElement>,
@@ -3691,7 +3676,7 @@ impl<'a> StyleBuilder<'a> {
             rules,
             modified_reset: false,
             is_root_element,
-            custom_properties: None,
+            custom_properties: crate::custom_properties::ComputedCustomProperties::default(),
             writing_mode: inherited_style.writing_mode,
             flags: Cell::new(flags),
             visited_style: None,
@@ -3728,7 +3713,7 @@ impl<'a> StyleBuilder<'a> {
             modified_reset: false,
             is_root_element: false,
             rules: None,
-            custom_properties: style_to_derive_from.custom_properties().cloned(),
+            custom_properties: style_to_derive_from.custom_properties().clone(),
             writing_mode: style_to_derive_from.writing_mode,
             flags: Cell::new(style_to_derive_from.flags),
             visited_style: None,
@@ -3848,7 +3833,7 @@ impl<'a> StyleBuilder<'a> {
                 ).build()
             })
         });
-        let custom_properties = parent.and_then(|p| p.custom_properties().cloned());
+        let custom_properties = if let Some(p) = parent { p.custom_properties().clone() } else { crate::custom_properties::ComputedCustomProperties::default() };
         let mut ret = Self::new(
             device,
             stylist,
@@ -3991,8 +3976,8 @@ impl<'a> StyleBuilder<'a> {
     }
 
     /// Get the custom properties map if necessary.
-    pub fn custom_properties(&self) -> Option<<&Arc<crate::custom_properties::CustomPropertiesMap>> {
-        self.custom_properties.as_ref()
+    pub fn custom_properties(&self) -> &crate::custom_properties::ComputedCustomProperties {
+        &self.custom_properties
     }
 
     /// Access to various information about our inherited styles.  We don't
@@ -4059,7 +4044,7 @@ mod lazy_static_module {
                         % endif
                     }),
                 % endfor
-                custom_properties: None,
+                custom_properties,
                 writing_mode: WritingMode::empty(),
                 rules: None,
                 visited_style: None,
@@ -4186,7 +4171,7 @@ macro_rules! longhand_properties_idents {
 }
 
 // Large pages generate tens of thousands of ComputedValues.
-size_of_test!(ComputedValues, 232);
+size_of_test!(ComputedValues, 240);
 // FFI relies on this.
 size_of_test!(Option<Arc<ComputedValues>>, 8);
 

servo/ports/geckolib/glue.rs

@@ -26,6 +26,7 @@ use style::color::{AbsoluteColor, ColorSpace};
 use style::context::ThreadLocalStyleContext;
 use style::context::{CascadeInputs, QuirksMode, SharedStyleContext, StyleContext};
 use style::counter_style;
+use style::custom_properties::ComputedCustomProperties;
 use style::data::{self, ElementStyles};
 use style::dom::{ShowSubtreeData, TDocument, TElement, TNode};
 use style::driver;
@@ -6069,7 +6070,7 @@ pub extern "C" fn Servo_GetComputedKeyframeValues(
 
     let mut raw_custom_properties_block; // To make the raw block alive in the scope.
     for (index, keyframe) in keyframes.iter().enumerate() {
-        let mut custom_properties = None;
+        let mut custom_properties = ComputedCustomProperties::default();
         for property in keyframe.mPropertyValues.iter() {
             // Find the block for custom properties first.
             if property.mProperty == nsCSSPropertyID::eCSSPropertyExtra_variable {
@@ -6142,7 +6143,11 @@ pub extern "C" fn Servo_GetComputedKeyframeValues(
             let iter = guard.to_animation_value_iter(
                 &mut context,
                 &default_values,
-                custom_properties.as_ref(),
+                if custom_properties.is_empty() {
+                    None
+                } else {
+                    Some(&custom_properties)
+                }
             );
 
             for value in iter {
@@ -7393,13 +7398,14 @@ pub unsafe extern "C" fn Servo_GetCustomPropertyValue(
     name: &nsACString,
     value: &mut nsACString,
 ) -> bool {
-    let custom_properties = match computed_values.custom_properties() {
+    // TODO(bug 1840478): Handle non-inherited properties.
+    let inherited = match &computed_values.custom_properties.inherited {
         Some(p) => p,
         None => return false,
     };
 
     let name = Atom::from(name.as_str_unchecked());
-    let computed_value = match custom_properties.get(&name) {
+    let computed_value = match inherited.get(&name) {
         Some(v) => v,
         None => return false,
     };
@@ -7410,8 +7416,9 @@ pub unsafe extern "C" fn Servo_GetCustomPropertyValue(
 
 #[no_mangle]
 pub extern "C" fn Servo_GetCustomPropertiesCount(computed_values: &ComputedValues) -> u32 {
-    match computed_values.custom_properties() {
-        Some(p) => p.len() as u32,
+    // TODO(bug 1840478): Handle non-inherited properties.
+    match &computed_values.custom_properties().inherited {
+        Some(m) => m.len() as u32,
         None => 0,
     }
 }
@@ -7421,12 +7428,13 @@ pub extern "C" fn Servo_GetCustomPropertyNameAt(
     computed_values: &ComputedValues,
     index: u32,
 ) -> *mut nsAtom {
-    let custom_properties = match computed_values.custom_properties() {
+    // TODO(bug 1840478): Handle non-inherited properties.
+    let inherited = match &computed_values.custom_properties.inherited {
         Some(p) => p,
         None => return ptr::null_mut(),
     };
 
-    let property_name = match custom_properties.get_index(index as usize) {
+    let property_name = match inherited.get_index(index as usize) {
         Some((key, _value)) => key,
         None => return ptr::null_mut(),
     };