From 7fcf0d7415c179833347a1126c7abed139b06001 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 19 Dec 2023 11:02:39 +0000 Subject: [PATCH] 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 --- layout/style/nsCSSPropertyID.h.in | 1 - layout/style/nsStyleStruct.cpp | 2 +- layout/style/nsTransitionManager.cpp | 17 ++----- .../components/style/properties/gecko.mako.rs | 10 ++-- .../helpers/animated_properties.mako.rs | 47 +++++++------------ servo/components/style/properties/mod.rs | 12 ++++- .../style/values/specified/animation.rs | 40 ++++------------ 7 files changed, 47 insertions(+), 82 deletions(-) diff --git a/layout/style/nsCSSPropertyID.h.in b/layout/style/nsCSSPropertyID.h.in index 418c3ce622fc1..7bd987b0f3449 100644 --- a/layout/style/nsCSSPropertyID.h.in +++ b/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, diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp index 9a78917d649af..08b01a780eced 100644 --- a/layout/style/nsStyleStruct.cpp +++ b/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 { diff --git a/layout/style/nsTransitionManager.cpp b/layout/style/nsTransitionManager.cpp index ef1b9bcb52ccf..93d6eb2855300 100644 --- a/layout/style/nsTransitionManager.cpp +++ b/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;) { diff --git a/servo/components/style/properties/gecko.mako.rs b/servo/components/style/properties/gecko.mako.rs index 8e8800a1bed91..a7f6aad6d7f92 100644 --- a/servo/components/style/properties/gecko.mako.rs +++ b/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; } diff --git a/servo/components/style/properties/helpers/animated_properties.mako.rs b/servo/components/style/properties/helpers/animated_properties.mako.rs index 0e30022e7e1a2..0ba4b176b4ee5 100644 --- a/servo/components/style/properties/helpers/animated_properties.mako.rs +++ b/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 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(..) => {} } } diff --git a/servo/components/style/properties/mod.rs b/servo/components/style/properties/mod.rs index e8df125effe45..40f1c228f1e79 100644 --- a/servo/components/style/properties/mod.rs +++ b/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(&self, dest: &mut CssWriter) -> 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 { diff --git a/servo/components/style/values/specified/animation.rs b/servo/components/style/values/specified/animation.rs index ad4fbc587c139..6edff654b9e9f 100644 --- a/servo/components/style/values/specified/animation.rs +++ b/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 { - 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)) } }