Bug 1864736 - Use ComputedRegisteredValue for custom property maps. r=firefox-style-system-reviewers,emilio

Unparsed custom properties are the
ComputedRegisteredValueInner::Universal variant.

Although the inner value is the ComputedRegisteredValueInner type,
custom property maps hold only the
ComputedRegisteredValueInner::Universal variant to keep behavior
unchanged for now.

D203360 also removes Arc<> from the CustomPropertiesMap value.

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

servo/components/style/custom_properties.rs

@@ -15,7 +15,10 @@ use crate::properties::{
 };
 use crate::properties_and_values::{
     registry::PropertyRegistrationData,
-    value::{AllowComputationallyDependent, SpecifiedValue as SpecifiedRegisteredValue},
+    value::{
+        AllowComputationallyDependent, ComputedValue as ComputedRegisteredValue,
+        SpecifiedValue as SpecifiedRegisteredValue,
+    },
 };
 use crate::selector_map::{PrecomputedHashMap, PrecomputedHashSet};
 use crate::stylesheets::UrlExtraData;
@@ -250,7 +253,7 @@ impl ComputedCustomProperties {
     }
 
     /// Return the name and value of the property at specified index, if any.
-    pub fn property_at(&self, index: usize) -> Option<(&Name, &Option<Arc<VariableValue>>)> {
+    pub fn property_at(&self, index: usize) -> Option<(&Name, &Option<ComputedRegisteredValue>)> {
         // Just expose the custom property items from custom_properties.inherited, followed
         // by custom property items from custom_properties.non_inherited.
         self.inherited
@@ -264,7 +267,7 @@ impl ComputedCustomProperties {
         &mut self,
         registration: &PropertyRegistrationData,
         name: &Name,
-        value: Arc<VariableValue>,
+        value: ComputedRegisteredValue,
     ) {
         self.map_mut(registration).insert(name, value)
     }
@@ -293,7 +296,7 @@ impl ComputedCustomProperties {
         &self,
         registration: &PropertyRegistrationData,
         name: &Name,
-    ) -> Option<&Arc<VariableValue>> {
+    ) -> Option<&ComputedRegisteredValue> {
         if registration.inherits() {
             self.inherited.get(name)
         } else {
@@ -957,8 +960,8 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
 
         let map = &mut self.custom_properties;
         let registration = self.stylist.get_custom_property_registration(&name);
-        match *value {
+        match value {
-            CustomDeclarationValue::Value(ref unparsed_value) => {
+            CustomDeclarationValue::Value(unparsed_value) => {
                 let has_custom_property_references = unparsed_value.references.any_var;
                 let registered_length_property =
                     registration.syntax.may_reference_font_relative_length();
@@ -984,21 +987,19 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
                         self.computed_context,
                     );
                 }
-                map.insert(registration, name, Arc::clone(unparsed_value));
+                let value = ComputedRegisteredValue::universal(Arc::clone(unparsed_value));
+                map.insert(registration, name, value);
             },
             CustomDeclarationValue::CSSWideKeyword(keyword) => match keyword {
                 CSSWideKeyword::RevertLayer | CSSWideKeyword::Revert => {
-                    let origin_revert = keyword == CSSWideKeyword::Revert;
+                    let origin_revert = matches!(keyword, CSSWideKeyword::Revert);
                     self.seen.remove(name);
                     self.reverted.insert(name, (priority, origin_revert));
                 },
                 CSSWideKeyword::Initial => {
                     // For non-inherited custom properties, 'initial' was handled in value_may_affect_style.
                     debug_assert!(registration.inherits(), "Should've been handled earlier");
-                    map.remove(registration, name);
+                    remove_and_insert_initial_value(name, registration, map);
-                    if let Some(ref initial_value) = registration.initial_value {
-                        map.insert(registration, name, initial_value.clone());
-                    }
                 },
                 CSSWideKeyword::Inherit => {
                     // For inherited custom properties, 'inherit' was handled in value_may_affect_style.
@@ -1124,8 +1125,12 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
                 debug_assert!(registration.inherits(), "Should've been handled earlier");
                 // Don't bother overwriting an existing value with the initial value specified in
                 // the registration.
-                if Some(existing_value) == registration.initial_value.as_ref() {
+                if let Some(initial_value) = self
-                    return false;
+                    .stylist
+                    .get_custom_property_initial_values()
+                    .get(registration, name)
+                {
+                    return existing_value != initial_value;
                 }
             },
             (Some(_), &CustomDeclarationValue::CSSWideKeyword(CSSWideKeyword::Inherit)) => {
@@ -1146,8 +1151,16 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
             (Some(existing_value), &CustomDeclarationValue::Value(ref value)) => {
                 // Don't bother overwriting an existing value with the same
                 // specified value.
-                if existing_value == value {
+                if let Some(existing_value) = existing_value.as_universal() {
-                    return false;
+                    return existing_value != value;
+                }
+                if let Ok(value) = compute_value(
+                    &value.css,
+                    &value.url_data,
+                    registration,
+                    self.computed_context,
+                ) {
+                    return existing_value.v != value.v;
                 }
             },
             _ => {},
@@ -1245,6 +1258,9 @@ impl<'a, 'b: 'a> CustomPropertiesBuilder<'a, 'b> {
                 // have to worry about resolving in a wrong order.
                 for (k, v) in deferred.iter() {
                     let Some(v) = v else { continue };
+                    let Some(v) = v.as_universal() else {
+                        unreachable!("Computing should have been deferred!")
+                    };
                     substitute_references_if_needed_and_apply(
                         k,
                         v,
@@ -1368,6 +1384,7 @@ fn substitute_all(
             VarType::Custom(ref name) => {
                 let registration = context.stylist.get_custom_property_registration(name);
                 let value = context.map.get(registration, name)?;
+                let value = value.as_universal()?;
 
                 let non_custom_references = value
                     .references
@@ -1513,13 +1530,7 @@ fn substitute_all(
                     .insert(LonghandId::LineHeight);
             }
             // This variable is in loop. Resolve to invalid.
-            handle_invalid_at_computed_value_time(
+            handle_invalid_at_computed_value_time(name, context.map, context.computed_context);
-                name,
-                context.map,
-                context.computed_context.inherited_custom_properties(),
-                context.stylist,
-                context.computed_context.is_root_element(),
-            );
         };
         loop {
             let var_index = context
@@ -1562,7 +1573,7 @@ fn substitute_all(
             return None;
         }
 
-        if let Some(ref v) = value.as_ref() {
+        if let Some(ref v) = value {
             let registration = context.stylist.get_custom_property_registration(&name);
             let registered_length_property =
                 registration.syntax.may_reference_font_relative_length();
@@ -1570,7 +1581,8 @@ fn substitute_all(
             if !context.non_custom_references.is_empty() && registered_length_property {
                 if let Some(deferred) = &mut context.deferred_properties {
                     // This property directly depends on a non-custom property, defer resolving it.
-                    deferred.insert(registration, &name, (*v).clone());
+                    let deferred_property = ComputedRegisteredValue::universal(Arc::clone(v));
+                    deferred.insert(registration, &name, deferred_property);
                     context.map.remove(registration, &name);
                     defer = true;
                 }
@@ -1586,7 +1598,9 @@ fn substitute_all(
                             .get_custom_property_registration(&reference.name);
                         if deferred.get(registration, &reference.name).is_some() {
                             // This property depends on a custom property that depends on a non-custom property, defer.
-                            deferred.insert(registration, &name, Arc::clone(v));
+                            let deferred_property =
+                                ComputedRegisteredValue::universal(Arc::clone(v));
+                            deferred.insert(registration, &name, deferred_property);
                             context.map.remove(registration, &name);
                             defer = true;
                             break;
@@ -1639,22 +1653,27 @@ fn substitute_all(
 fn handle_invalid_at_computed_value_time(
     name: &Name,
     custom_properties: &mut ComputedCustomProperties,
-    inherited: &ComputedCustomProperties,
+    computed_context: &computed::Context,
-    stylist: &Stylist,
-    is_root_element: bool,
 ) {
+    let stylist = computed_context.style().stylist.unwrap();
     let registration = stylist.get_custom_property_registration(&name);
     if !registration.syntax.is_universal() {
         // For the root element, inherited maps are empty. We should just
         // use the initial value if any, rather than removing the name.
-        if registration.inherits() && !is_root_element {
+        if registration.inherits() && !computed_context.is_root_element() {
+            let inherited = computed_context.inherited_custom_properties();
             if let Some(value) = inherited.get(registration, name) {
-                custom_properties.insert(registration, name, Arc::clone(value));
+                custom_properties.insert(registration, name, value.clone());
                 return;
             }
-        } else {
+        } else if let Some(ref initial_value) = registration.initial_value {
-            if let Some(ref initial_value) = registration.initial_value {
+            if let Ok(initial_value) = compute_value(
-                custom_properties.insert(registration, name, Arc::clone(initial_value));
+                &initial_value.css,
+                &initial_value.url_data,
+                registration,
+                computed_context,
+            ) {
+                custom_properties.insert(registration, name, initial_value);
                 return;
             }
         }
@@ -1673,11 +1692,13 @@ fn substitute_references_if_needed_and_apply(
     let registration = stylist.get_custom_property_registration(&name);
     if !value.has_references() && registration.syntax.is_universal() {
         // Trivial path: no references and no need to compute the value, just apply it directly.
-        custom_properties.insert(registration, name, Arc::clone(value));
+        let computed_value = ComputedRegisteredValue::universal(Arc::clone(value));
+        custom_properties.insert(registration, name, computed_value);
         return;
     }
 
     let inherited = computed_context.inherited_custom_properties();
+    let url_data = &value.url_data;
     let value = match substitute_internal(
         value,
         custom_properties,
@@ -1687,21 +1708,16 @@ fn substitute_references_if_needed_and_apply(
     ) {
         Ok(v) => v,
         Err(..) => {
-            handle_invalid_at_computed_value_time(
+            handle_invalid_at_computed_value_time(name, custom_properties, computed_context);
-                name,
-                custom_properties,
-                inherited,
-                stylist,
-                computed_context.is_root_element(),
-            );
             return;
         },
     }
-    .into_value(&value.url_data);
+    .into_value(url_data);
 
     // If variable fallback results in a wide keyword, deal with it now.
     {
-        let mut input = ParserInput::new(&value.css);
+        let css = value.to_variable_value().css;
+        let mut input = ParserInput::new(&css);
         let mut input = Parser::new(&mut input);
 
         if let Ok(kw) = input.try_parse(CSSWideKeyword::parse) {
@@ -1721,10 +1737,7 @@ fn substitute_references_if_needed_and_apply(
                 (CSSWideKeyword::RevertLayer, true, true) |
                 (CSSWideKeyword::Unset, true, true) |
                 (CSSWideKeyword::Inherit, _, true) => {
-                    custom_properties.remove(registration, name);
+                    remove_and_insert_initial_value(name, registration, custom_properties);
-                    if let Some(ref initial_value) = registration.initial_value {
-                        custom_properties.insert(registration, name, Arc::clone(initial_value));
-                    }
                 },
                 (CSSWideKeyword::Revert, true, false) |
                 (CSSWideKeyword::RevertLayer, true, false) |
@@ -1732,7 +1745,7 @@ fn substitute_references_if_needed_and_apply(
                 (CSSWideKeyword::Unset, true, false) => {
                     match inherited.get(registration, name) {
                         Some(value) => {
-                            custom_properties.insert(registration, name, Arc::clone(value));
+                            custom_properties.insert(registration, name, value.clone());
                         },
                         None => {
                             custom_properties.remove(registration, name);
@@ -1744,48 +1757,81 @@ fn substitute_references_if_needed_and_apply(
         }
     }
 
-    custom_properties.insert(registration, name, Arc::new(value));
+    custom_properties.insert(registration, name, value);
+}
+
+enum Substitution<'a> {
+    Universal(UniversalSubstitution<'a>),
+    Computed(ComputedRegisteredValue),
+}
+
+impl<'a> Default for Substitution<'a> {
+    fn default() -> Self {
+        Self::Universal(UniversalSubstitution::default())
+    }
 }
 
 #[derive(Default)]
-struct Substitution<'a> {
+struct UniversalSubstitution<'a> {
     css: Cow<'a, str>,
     first_token_type: TokenSerializationType,
     last_token_type: TokenSerializationType,
 }
 
+impl<'a> UniversalSubstitution<'a> {
+    fn from_value(v: VariableValue) -> Self {
+        UniversalSubstitution {
+            css: Cow::from(v.css),
+            first_token_type: v.first_token_type,
+            last_token_type: v.last_token_type,
+        }
+    }
+}
+
 impl<'a> Substitution<'a> {
     fn new(
         css: &'a str,
         first_token_type: TokenSerializationType,
         last_token_type: TokenSerializationType,
     ) -> Self {
-        Self {
+        Self::Universal(UniversalSubstitution {
             css: Cow::Borrowed(css),
             first_token_type,
             last_token_type,
+        })
+    }
+
+    fn into_universal(self) -> UniversalSubstitution<'a> {
+        match self {
+            Substitution::Universal(substitution) => substitution,
+            Substitution::Computed(computed) => {
+                UniversalSubstitution::from_value(computed.to_variable_value())
+            },
         }
     }
 
-    fn from_value(v: VariableValue) -> Substitution<'static> {
+    fn from_value(v: VariableValue) -> Self {
         debug_assert!(
             !v.has_references(),
             "Computed values shouldn't have references"
         );
-        Substitution {
+        let substitution = UniversalSubstitution::from_value(v);
-            css: Cow::from(v.css),
+        Self::Universal(substitution)
-            first_token_type: v.first_token_type,
-            last_token_type: v.last_token_type,
-        }
     }
 
-    fn into_value(self, url_data: &UrlExtraData) -> VariableValue {
+    fn into_value(self, url_data: &UrlExtraData) -> ComputedRegisteredValue {
-        VariableValue {
+        match self {
-            css: self.css.into_owned(),
+            Substitution::Universal(substitution) => {
-            first_token_type: self.first_token_type,
+                let value = Arc::new(VariableValue {
-            last_token_type: self.last_token_type,
+                    css: substitution.css.into_owned(),
-            url_data: url_data.clone(),
+                    first_token_type: substitution.first_token_type,
-            references: Default::default(),
+                    last_token_type: substitution.last_token_type,
+                    url_data: url_data.clone(),
+                    references: Default::default(),
+                });
+                ComputedRegisteredValue::universal(value)
+            },
+            Substitution::Computed(computed) => computed,
         }
     }
 }
@@ -1795,7 +1841,7 @@ fn compute_value(
     url_data: &UrlExtraData,
     registration: &PropertyRegistrationData,
     computed_context: &computed::Context,
-) -> Result<Substitution<'static>, ()> {
+) -> Result<ComputedRegisteredValue, ()> {
     debug_assert!(!registration.syntax.is_universal());
 
     let mut input = ParserInput::new(&css);
@@ -1808,7 +1854,20 @@ fn compute_value(
         computed_context,
         AllowComputationallyDependent::Yes,
     )?;
-    Ok(Substitution::from_value(value))
+    Ok(ComputedRegisteredValue::universal(Arc::new(value)))
+}
+
+/// Removes the named registered custom property and inserts its uncomputed initial value.
+fn remove_and_insert_initial_value(
+    name: &Name,
+    registration: &PropertyRegistrationData,
+    custom_properties: &mut ComputedCustomProperties,
+) {
+    custom_properties.remove(registration, name);
+    if let Some(ref initial_value) = registration.initial_value {
+        let value = ComputedRegisteredValue::universal(Arc::clone(initial_value));
+        custom_properties.insert(registration, name, value);
+    }
 }
 
 fn do_substitute_chunk<'a>(
@@ -1835,7 +1894,8 @@ fn do_substitute_chunk<'a>(
     {
         let result = &css[start..end];
         if !registration.syntax.is_universal() {
-            return compute_value(result, url_data, registration, computed_context);
+            let computed_value = compute_value(result, url_data, registration, computed_context)?;
+            return Ok(Substitution::Computed(computed_value));
         }
         return Ok(Substitution::new(result, first_token_type, last_token_type));
     }
@@ -1867,6 +1927,7 @@ fn do_substitute_chunk<'a>(
             return Ok(substitution);
         }
 
+        let substitution = substitution.into_universal();
         substituted.push(
             &substitution.css,
             substitution.first_token_type,
@@ -1880,7 +1941,9 @@ fn do_substitute_chunk<'a>(
         substituted.push(&css[cur_pos..end], next_token_type, last_token_type)?;
     }
     if !registration.syntax.is_universal() {
-        return compute_value(&substituted.css, url_data, registration, computed_context);
+        let computed_value =
+            compute_value(&substituted.css, url_data, registration, computed_context)?;
+        return Ok(Substitution::Computed(computed_value));
     }
     Ok(Substitution::from_value(substituted))
 }
@@ -1898,7 +1961,6 @@ fn substitute_one_reference<'a>(
     if reference.is_var {
         registration = stylist.get_custom_property_registration(&reference.name);
         if let Some(v) = custom_properties.get(registration, &reference.name) {
-            debug_assert!(!v.has_references(), "Should be already computed");
             if registration.syntax.is_universal() {
                 // Skip references that are inside the outer variable (in fallback for example).
                 while references
@@ -1924,11 +1986,7 @@ fn substitute_one_reference<'a>(
                     )?;
                 }
             }
-            return Ok(Substitution {
+            return Ok(Substitution::Computed(v.clone()));
-                css: Cow::from(&v.css),
-                first_token_type: v.first_token_type,
-                last_token_type: v.last_token_type,
-            });
         }
     } else {
         registration = PropertyRegistrationData::unregistered();
@@ -2000,5 +2058,6 @@ pub fn substitute<'a>(
         PropertyRegistrationData::unregistered(),
         computed_context,
     )?;
+    let v = v.into_universal();
     Ok(v.css)
 }

servo/components/style/custom_properties_map.rs

@@ -4,7 +4,8 @@
 
 //! The structure that contains the custom properties of a given element.
 
-use crate::custom_properties::{Name, VariableValue};
+use crate::custom_properties::Name;
+use crate::properties_and_values::value::ComputedValue as ComputedRegisteredValue;
 use crate::selector_map::PrecomputedHasher;
 use indexmap::IndexMap;
 use servo_arc::Arc;
@@ -22,7 +23,8 @@ impl Default for CustomPropertiesMap {
 }
 
 /// We use None in the value to represent a removed entry.
-type OwnMap = IndexMap<Name, Option<Arc<VariableValue>>, BuildHasherDefault<PrecomputedHasher>>;
+type OwnMap =
+    IndexMap<Name, Option<ComputedRegisteredValue>, BuildHasherDefault<PrecomputedHasher>>;
 
 // IndexMap equality doesn't consider ordering, which we want to account for. Also, for the same
 // reason, IndexMap equality comparisons are slower than needed.
@@ -69,12 +71,12 @@ const ANCESTOR_COUNT_LIMIT: usize = 4;
 /// An iterator over the custom properties.
 pub struct Iter<'a> {
     current: &'a Inner,
-    current_iter: indexmap::map::Iter<'a, Name, Option<Arc<VariableValue>>>,
+    current_iter: indexmap::map::Iter<'a, Name, Option<ComputedRegisteredValue>>,
     descendants: smallvec::SmallVec<[&'a Inner; ANCESTOR_COUNT_LIMIT]>,
 }
 
 impl<'a> Iterator for Iter<'a> {
-    type Item = (&'a Name, &'a Option<Arc<VariableValue>>);
+    type Item = (&'a Name, &'a Option<ComputedRegisteredValue>);
 
     fn next(&mut self) -> Option<Self::Item> {
         loop {
@@ -141,14 +143,14 @@ impl Inner {
         self.len
     }
 
-    fn get(&self, name: &Name) -> Option<&Arc<VariableValue>> {
+    fn get(&self, name: &Name) -> Option<&ComputedRegisteredValue> {
         if let Some(p) = self.own_properties.get(name) {
             return p.as_ref();
         }
         self.parent.as_ref()?.get(name)
     }
 
-    fn insert(&mut self, name: &Name, value: Option<Arc<VariableValue>>) {
+    fn insert(&mut self, name: &Name, value: Option<ComputedRegisteredValue>) {
         let new = self.own_properties.insert(name.clone(), value).is_none();
         if new && self.parent.as_ref().map_or(true, |p| p.get(name).is_none()) {
             self.len += 1;
@@ -177,7 +179,7 @@ impl CustomPropertiesMap {
     }
 
     /// Returns the property name and value at a given index.
-    pub fn get_index(&self, index: usize) -> Option<(&Name, &Option<Arc<VariableValue>>)> {
+    pub fn get_index(&self, index: usize) -> Option<(&Name, &Option<ComputedRegisteredValue>)> {
         if index >= self.len() {
             return None;
         }
@@ -186,11 +188,11 @@ impl CustomPropertiesMap {
     }
 
     /// Returns a given property value by name.
-    pub fn get(&self, name: &Name) -> Option<&Arc<VariableValue>> {
+    pub fn get(&self, name: &Name) -> Option<&ComputedRegisteredValue> {
         self.0.get(name)
     }
 
-    fn do_insert(&mut self, name: &Name, value: Option<Arc<VariableValue>>) {
+    fn do_insert(&mut self, name: &Name, value: Option<ComputedRegisteredValue>) {
         if let Some(inner) = Arc::get_mut(&mut self.0) {
             return inner.insert(name, value);
         }
@@ -214,7 +216,7 @@ impl CustomPropertiesMap {
     }
 
     /// Inserts an element in the map.
-    pub fn insert(&mut self, name: &Name, value: Arc<VariableValue>) {
+    pub fn insert(&mut self, name: &Name, value: ComputedRegisteredValue) {
         self.do_insert(name, Some(value))
     }
 

servo/components/style/properties_and_values/rule.rs

@@ -9,7 +9,10 @@
 use super::{
     registry::{PropertyRegistration, PropertyRegistrationData},
     syntax::Descriptor,
-    value::{AllowComputationallyDependent, SpecifiedValue as SpecifiedRegisteredValue},
+    value::{
+        AllowComputationallyDependent, ComputedValue as ComputedRegisteredValue,
+        SpecifiedValue as SpecifiedRegisteredValue,
+    },
 };
 use crate::custom_properties::{Name as CustomPropertyName, SpecifiedValue};
 use crate::error_reporting::ContextualParseError;
@@ -216,13 +219,13 @@ impl PropertyRegistration {
     pub fn compute_initial_value(
         &self,
         computed_context: &computed::Context,
-    ) -> Result<InitialValue, ()> {
+    ) -> Result<ComputedRegisteredValue, ()> {
         let Some(ref initial) = self.data.initial_value else {
             return Err(());
         };
 
         if self.data.syntax.is_universal() {
-            return Ok(Arc::clone(initial));
+            return Ok(ComputedRegisteredValue::universal(Arc::clone(initial)));
         }
 
         let mut input = ParserInput::new(initial.css_text());
@@ -236,7 +239,7 @@ impl PropertyRegistration {
             computed_context,
             AllowComputationallyDependent::No,
         ) {
-            Ok(computed) => Ok(Arc::new(computed)),
+            Ok(computed) => Ok(ComputedRegisteredValue::universal(Arc::new(computed))),
             Err(_) => Err(()),
         }
     }

servo/components/style/properties_and_values/value.rs

@@ -221,6 +221,13 @@ impl<Component> Value<Component> {
     pub fn new(v: ValueInner<Component>, url_data: UrlExtraData) -> Self {
         Self { v, url_data }
     }
+
+    /// Creates a new registered custom property value presumed to have universal syntax.
+    pub fn universal(var: Arc<ComputedPropertyValue>) -> Self {
+        let url_data = var.url_data.clone();
+        let v = ValueInner::Universal(var);
+        Self { v, url_data }
+    }
 }
 
 /// A specified registered custom property value.
@@ -281,7 +288,7 @@ impl SpecifiedValue {
 
     /// Convert a registered custom property to a Computed custom property value, given input and a
     /// property registration.
-    fn get_computed_value<'i, 't>(
+    pub fn get_computed_value<'i, 't>(
         input: &mut CSSParser<'i, 't>,
         registration: &PropertyRegistrationData,
         url_data: &UrlExtraData,
@@ -357,13 +364,17 @@ impl ComputedValue {
         Arc::new(self.to_variable_value())
     }
 
-    fn to_variable_value(&self) -> ComputedPropertyValue {
+    /// Returns the contained variable value if it exists, otherwise `None`.
-        debug_assert!(
+    pub fn as_universal(&self) -> Option<&Arc<ComputedPropertyValue>> {
-            !matches!(self.v, ValueInner::Universal(..)),
+        if let ValueInner::Universal(ref var) = self.v {
-            "Shouldn't be needed"
+            Some(var)
-        );
+        } else {
-        // TODO(zrhoffman, 1864736): Preserve the computed type instead of converting back to a
+            None
-        // string.
+        }
+    }
+
+    /// Convert to an untyped variable value.
+    pub fn to_variable_value(&self) -> ComputedPropertyValue {
         if let ValueInner::Universal(ref value) = self.v {
             return (**value).clone();
         }
@@ -617,16 +628,11 @@ impl Animate for CustomAnimatedValue {
 impl CustomAnimatedValue {
     pub(crate) fn from_computed(
         name: &crate::custom_properties::Name,
-        value: &Arc<ComputedPropertyValue>,
+        value: &ComputedValue,
     ) -> Self {
-        let value = ComputedValue {
-            // FIXME: Should probably preserve type-ness in ComputedPropertyValue.
-            v: ValueInner::Universal(value.clone()),
-            url_data: value.url_data.clone(),
-        };
         Self {
             name: name.clone(),
-            value,
+            value: value.clone(),
         }
     }