Bug 1870676 - Clean up transition-property handling, and remove eCSSPropertyExtra_all_properties. r=firefox-style-system-reviewers,zrhoffman

While we're at it, let's fix this long-standing TODO.

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

Author: emilio

layout/style/nsCSSPropertyID.h.in

@@ -29,7 +29,6 @@ $property_ids
   // Extra values for use in the values of the 'transition-property'
   // property.
   eCSSPropertyExtra_no_properties,
-  eCSSPropertyExtra_all_properties,
 
   // Extra value to represent custom properties (--*).
   eCSSPropertyExtra_variable,

layout/style/nsStyleStruct.cpp

@@ -2031,7 +2031,7 @@ void StyleTransition::SetInitialValues() {
       StyleComputedTimingFunction::Keyword(StyleTimingKeyword::Ease);
   mDuration = {0.0};
   mDelay = {0.0};
-  mProperty = eCSSPropertyExtra_all_properties;
+  mProperty = eCSSProperty_all;
 }
 
 bool StyleTransition::operator==(const StyleTransition& aOther) const {

layout/style/nsTransitionManager.cpp

@@ -76,18 +76,7 @@ static void ExpandTransitionProperty(nsCSSPropertyID aProperty,
     return;
   }
 
-  // FIXME(emilio): This should probably just use the "all" shorthand id, and we
-  // should probably remove eCSSPropertyExtra_all_properties.
-  if (aProperty == eCSSPropertyExtra_all_properties) {
-    for (nsCSSPropertyID p = nsCSSPropertyID(0);
-         p < eCSSProperty_COUNT_no_shorthands; p = nsCSSPropertyID(p + 1)) {
-      if (!nsCSSProps::IsEnabled(p, CSSEnabledState::ForAllContent)) {
-        continue;
-      }
-      AnimatedPropertyID property(p);
-      aHandler(property);
-    }
-  } else if (nsCSSProps::IsShorthand(aProperty)) {
+  if (nsCSSProps::IsShorthand(aProperty)) {
     CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(subprop, aProperty,
                                          CSSEnabledState::ForAllContent) {
       AnimatedPropertyID property(*subprop);
@@ -138,8 +127,8 @@ bool nsTransitionManager::DoUpdateTransitions(
   // transition.  This can happen delay and duration are both zero, or
   // because the new value is not interpolable.
   if (aElementTransitions) {
-    bool checkProperties =
-        aStyle.GetTransitionProperty(0) != eCSSPropertyExtra_all_properties;
+    const bool checkProperties =
+        aStyle.GetTransitionProperty(0) != eCSSProperty_all;
     AnimatedPropertyIDSet allTransitionProperties;
     if (checkProperties) {
       for (uint32_t i = aStyle.mTransitionPropertyCount; i-- != 0;) {

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

@@ -1761,8 +1761,10 @@ mask-mode mask-repeat mask-clip mask-origin mask-composite mask-position-x mask-
                     TransitionProperty::Custom(name) => {
                         gecko.mProperty = eCSSPropertyExtra_variable;
                         gecko.mUnknownProperty.mRawPtr = name.into_addrefed();
-                    }
-                    _ => gecko.mProperty = servo.to_nscsspropertyid().unwrap(),
+                    },
+                    TransitionProperty::NonCustom(id) => {
+                        gecko.mProperty = id.to_nscsspropertyid();
+                    },
                 }
             }
         } else {
@@ -1774,9 +1776,9 @@ mask-mode mask-repeat mask-clip mask-origin mask-composite mask-position-x mask-
 
     /// Returns whether there are any transitions specified.
     pub fn specifies_transitions(&self) -> bool {
-        use crate::gecko_bindings::structs::nsCSSPropertyID::eCSSPropertyExtra_all_properties;
+        use crate::gecko_bindings::structs::nsCSSPropertyID::eCSSProperty_all;
         if self.mTransitionPropertyCount == 1 &&
-            self.mTransitions[0].mProperty == eCSSPropertyExtra_all_properties &&
+            self.mTransitions[0].mProperty == eCSSProperty_all &&
             self.transition_combined_duration_at(0).seconds() <= 0.0f32 {
             return false;
         }

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

@@ -16,7 +16,7 @@ use crate::properties::{
         self, content_visibility::computed_value::T as ContentVisibility,
         visibility::computed_value::T as Visibility,
     },
-    CSSWideKeyword, CustomDeclaration, CustomDeclarationValue, LonghandId,
+    CSSWideKeyword, CustomDeclaration, CustomDeclarationValue, NonCustomPropertyId, LonghandId,
     NonCustomPropertyIterator, PropertyDeclaration, PropertyDeclarationId,
 };
 use std::ptr;
@@ -38,25 +38,7 @@ use void::{self, Void};
 #[allow(non_upper_case_globals)]
 impl From<nsCSSPropertyID> for TransitionProperty {
     fn from(property: nsCSSPropertyID) -> TransitionProperty {
-        use crate::properties::ShorthandId;
-        match property {
-            % for prop in data.longhands:
-            ${prop.nscsspropertyid()} => {
-                TransitionProperty::Longhand(LonghandId::${prop.camel_case})
-            }
-            % endfor
-            % for prop in data.shorthands_except_all():
-            ${prop.nscsspropertyid()} => {
-                TransitionProperty::Shorthand(ShorthandId::${prop.camel_case})
-            }
-            % endfor
-            nsCSSPropertyID::eCSSPropertyExtra_all_properties => {
-                TransitionProperty::Shorthand(ShorthandId::All)
-            }
-            _ => {
-                panic!("non-convertible nsCSSPropertyID")
-            }
-        }
+        TransitionProperty::NonCustom(NonCustomPropertyId::from_nscsspropertyid(property).unwrap())
     }
 }
 
@@ -791,17 +773,22 @@ impl<'a> Iterator for TransitionPropertyIterator<'a> {
 
             let index = self.index_range.next()?;
             match self.style.get_ui().transition_property_at(index) {
-                TransitionProperty::Longhand(longhand_id) => {
-                    return Some(TransitionPropertyIteration {
-                        longhand_id,
-                        index,
-                    })
+                TransitionProperty::NonCustom(id) => {
+                    match id.longhand_or_shorthand() {
+                        Ok(longhand_id) => {
+                            return Some(TransitionPropertyIteration {
+                                longhand_id,
+                                index,
+                            });
+                        },
+                        Err(shorthand_id) => {
+                            // In the other cases, we set up our state so that we are ready to
+                            // compute the next value of the iterator and then loop (equivalent
+                            // to calling self.next()).
+                            self.longhand_iterator = Some(shorthand_id.longhands());
+                        },
+                    }
                 }
-                // In the other cases, we set up our state so that we are ready to
-                // compute the next value of the iterator and then loop (equivalent
-                // to calling self.next()).
-                TransitionProperty::Shorthand(ref shorthand_id) =>
-                    self.longhand_iterator = Some(shorthand_id.longhands()),
                 TransitionProperty::Custom(..) | TransitionProperty::Unsupported(..) => {}
             }
         }

servo/components/style/properties/mod.rs

@@ -191,9 +191,19 @@ impl fmt::Debug for PropertyDeclaration {
 }
 
 /// A longhand or shorthand property.
-#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, ToComputedValue, ToResolvedValue, ToShmem, MallocSizeOf)]
 pub struct NonCustomPropertyId(u16);
 
+impl ToCss for NonCustomPropertyId {
+    #[inline]
+    fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
+    where
+        W: Write,
+    {
+        dest.write_str(self.name())
+    }
+}
+
 impl NonCustomPropertyId {
     /// Returns the underlying index, used for use counter.
     pub fn bit(self) -> usize {

servo/components/style/values/specified/animation.rs

@@ -4,9 +4,8 @@
 
 //! Specified types for properties related to animations and transitions.
 
-use crate::custom_properties::Name as CustomPropertyName;
 use crate::parser::{Parse, ParserContext};
-use crate::properties::{LonghandId, PropertyDeclarationId, PropertyId, ShorthandId};
+use crate::properties::{NonCustomPropertyId, PropertyId, ShorthandId};
 use crate::values::generics::animation as generics;
 use crate::values::specified::{LengthPercentage, NonNegativeNumber};
 use crate::values::{CustomIdent, KeyframesName, TimelineName};
@@ -23,12 +22,10 @@ use style_traits::{
     Clone, Debug, Eq, Hash, MallocSizeOf, PartialEq, ToComputedValue, ToResolvedValue, ToShmem,
 )]
 pub enum TransitionProperty {
-    /// A shorthand.
-    Shorthand(ShorthandId),
-    /// A longhand transitionable property.
-    Longhand(LonghandId),
+    /// A non-custom property.
+    NonCustom(NonCustomPropertyId),
     /// A custom property.
-    Custom(CustomPropertyName),
+    Custom(Atom),
     /// Unrecognized property which could be any non-transitionable, custom property, or
     /// unknown property.
     Unsupported(CustomIdent),
@@ -41,8 +38,7 @@ impl ToCss for TransitionProperty {
     {
         use crate::values::serialize_atom_name;
         match *self {
-            TransitionProperty::Shorthand(ref s) => s.to_css(dest),
-            TransitionProperty::Longhand(ref l) => l.to_css(dest),
+            TransitionProperty::NonCustom(ref id) => id.to_css(dest),
             TransitionProperty::Custom(ref name) => {
                 dest.write_str("--")?;
                 serialize_atom_name(name, dest)
@@ -71,12 +67,9 @@ impl Parse for TransitionProperty {
             },
         };
 
-        Ok(match id.as_shorthand() {
-            Ok(s) => TransitionProperty::Shorthand(s),
-            Err(longhand_or_custom) => match longhand_or_custom {
-                PropertyDeclarationId::Longhand(id) => TransitionProperty::Longhand(id),
-                PropertyDeclarationId::Custom(custom) => TransitionProperty::Custom(custom.clone()),
-            },
+        Ok(match id {
+            PropertyId::NonCustom(id) => TransitionProperty::NonCustom(id.unaliased()),
+            PropertyId::Custom(name) => TransitionProperty::Custom(name),
         })
     }
 }
@@ -94,22 +87,7 @@ impl TransitionProperty {
     /// Returns `all`.
     #[inline]
     pub fn all() -> Self {
-        TransitionProperty::Shorthand(ShorthandId::All)
-    }
-
-    /// Convert TransitionProperty to nsCSSPropertyID.
-    #[cfg(feature = "gecko")]
-    pub fn to_nscsspropertyid(
-        &self,
-    ) -> Result<crate::gecko_bindings::structs::nsCSSPropertyID, ()> {
-        Ok(match *self {
-            TransitionProperty::Shorthand(ShorthandId::All) => {
-                crate::gecko_bindings::structs::nsCSSPropertyID::eCSSPropertyExtra_all_properties
-            },
-            TransitionProperty::Shorthand(ref id) => id.to_nscsspropertyid(),
-            TransitionProperty::Longhand(ref id) => id.to_nscsspropertyid(),
-            TransitionProperty::Custom(..) | TransitionProperty::Unsupported(..) => return Err(()),
-        })
+        TransitionProperty::NonCustom(NonCustomPropertyId::from_shorthand(ShorthandId::All))
     }
 }