Bug 1419695: Make the transition-property code make more sense. r?xidorn draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 01 Jun 2018 16:34:08 +0200
changeset 802878 9d71758952380e760ffa3f192b7d4861e18405b6
parent 802877 09f68b93adf3e560769e1fd67630a14c8ab790a1
child 802879 ea33ab2045db0625d23006b40ad55d0a343cfa98
push id111985
push userbmo:emilio@crisal.io
push dateFri, 01 Jun 2018 15:50:16 +0000
reviewersxidorn
bugs1419695
milestone62.0a1
Bug 1419695: Make the transition-property code make more sense. r?xidorn We were working around the lack of alias support during parsing in TransitionProperty by doing a Gecko lookup. That's a hack and is now gone. MozReview-Commit-ID: EptUvJNTrZr
layout/style/ServoBindings.cpp
servo/components/style/animation.rs
servo/components/style/gecko/wrapper.rs
servo/components/style/properties/gecko.mako.rs
servo/components/style/properties/helpers/animated_properties.mako.rs
servo/components/style/properties/longhand/box.mako.rs
servo/components/style/properties/shorthand/box.mako.rs
--- a/layout/style/ServoBindings.cpp
+++ b/layout/style/ServoBindings.cpp
@@ -769,30 +769,16 @@ Gecko_AnimationGetBaseStyle(void* aBaseS
 {
   auto base =
     static_cast<nsRefPtrHashtable<nsUint32HashKey, RawServoAnimationValue>*>
       (aBaseStyles);
   return base->GetWeak(aProperty);
 }
 
 void
-Gecko_StyleTransition_SetUnsupportedProperty(StyleTransition* aTransition,
-                                             nsAtom* aAtom)
-{
-  nsCSSPropertyID id =
-    nsCSSProps::LookupProperty(nsDependentAtomString(aAtom),
-                               CSSEnabledState::eForAllContent);
-  if (id == eCSSProperty_UNKNOWN || id == eCSSPropertyExtra_variable) {
-    aTransition->SetUnknownProperty(id, aAtom);
-  } else {
-    aTransition->SetProperty(id);
-  }
-}
-
-void
 Gecko_FillAllImageLayers(nsStyleImageLayers* aLayers, uint32_t aMaxLen)
 {
   aLayers->FillAllLayers(aMaxLen);
 }
 
 bool
 Gecko_IsDocumentBody(RawGeckoElementBorrowed aElement)
 {
--- a/servo/components/style/animation.rs
+++ b/servo/components/style/animation.rs
@@ -298,17 +298,18 @@ impl PropertyAnimation {
     ) -> Vec<PropertyAnimation> {
         let mut result = vec![];
         let box_style = new_style.get_box();
         let transition_property = box_style.transition_property_at(transition_index);
         let timing_function = box_style.transition_timing_function_mod(transition_index);
         let duration = box_style.transition_duration_mod(transition_index);
 
         match transition_property {
-            TransitionProperty::Unsupported(_) => result,
+            TransitionProperty::Custom(..) |
+            TransitionProperty::Unsupported(..) => result,
             TransitionProperty::Shorthand(ref shorthand_id) => shorthand_id
                 .longhands()
                 .filter_map(|longhand| {
                     PropertyAnimation::from_longhand(
                         longhand,
                         timing_function,
                         duration,
                         old_style,
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -1625,16 +1625,17 @@ impl<'le> TElement for GeckoElement<'le>
                     combined_duration,
                     before_change_style,
                     after_change_style,
                     &existing_transitions,
                 )
             };
 
             match transition_property {
+                TransitionProperty::Custom(..) |
                 TransitionProperty::Unsupported(..) => {},
                 TransitionProperty::Shorthand(ref shorthand) => {
                     if shorthand.longhands().any(property_check_helper) {
                         return true;
                     }
                 },
                 TransitionProperty::Longhand(longhand_id) => {
                     if property_check_helper(longhand_id) {
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -23,17 +23,16 @@ use gecko_bindings::bindings::Gecko_Dest
 use gecko_bindings::bindings::Gecko_CopyCounterStyle;
 use gecko_bindings::bindings::Gecko_CopyCursorArrayFrom;
 use gecko_bindings::bindings::Gecko_CopyFontFamilyFrom;
 use gecko_bindings::bindings::Gecko_CopyImageValueFrom;
 use gecko_bindings::bindings::Gecko_CopyListStyleImageFrom;
 use gecko_bindings::bindings::Gecko_EnsureImageLayersLength;
 use gecko_bindings::bindings::Gecko_SetCursorArrayLength;
 use gecko_bindings::bindings::Gecko_SetCursorImageValue;
-use gecko_bindings::bindings::Gecko_StyleTransition_SetUnsupportedProperty;
 use gecko_bindings::bindings::Gecko_NewCSSShadowArray;
 use gecko_bindings::bindings::Gecko_nsStyleFont_SetLang;
 use gecko_bindings::bindings::Gecko_nsStyleFont_CopyLangFrom;
 use gecko_bindings::bindings::Gecko_SetListStyleImageNone;
 use gecko_bindings::bindings::Gecko_SetListStyleImageImageValue;
 use gecko_bindings::bindings::Gecko_SetNullImageValue;
 use gecko_bindings::bindings::{Gecko_ResetFilters, Gecko_CopyFiltersFrom};
 use gecko_bindings::bindings::RawGeckoPresContextBorrowed;
@@ -3262,27 +3261,39 @@ fn static_assert() {
             + self.gecko.mTransitions[index % self.gecko.mTransitionDelayCount as usize].mDelay
     }
 
     pub fn set_transition_property<I>(&mut self, v: I)
         where I: IntoIterator<Item = longhands::transition_property::computed_value::single_value::T>,
               I::IntoIter: ExactSizeIterator
     {
         use gecko_bindings::structs::nsCSSPropertyID::eCSSPropertyExtra_no_properties;
+        use gecko_bindings::structs::nsCSSPropertyID::eCSSPropertyExtra_variable;
+        use gecko_bindings::structs::nsCSSPropertyID::eCSSProperty_UNKNOWN;
 
         let v = v.into_iter();
 
         if v.len() != 0 {
             self.gecko.mTransitions.ensure_len(v.len());
             self.gecko.mTransitionPropertyCount = v.len() as u32;
             for (servo, gecko) in v.zip(self.gecko.mTransitions.iter_mut()) {
+                if !gecko.mUnknownProperty.mRawPtr.is_null() {
+                    unsafe { Atom::from_addrefed(gecko.mUnknownProperty.mRawPtr) };
+                    gecko.mUnknownProperty.mRawPtr = ptr::null_mut();
+                }
+
                 match servo {
-                    TransitionProperty::Unsupported(ref ident) => unsafe {
-                        Gecko_StyleTransition_SetUnsupportedProperty(gecko, ident.0.as_ptr())
+                    TransitionProperty::Unsupported(ident) => {
+                        gecko.mProperty = eCSSProperty_UNKNOWN;
+                        gecko.mUnknownProperty.mRawPtr = ident.0.into_addrefed();
                     },
+                    TransitionProperty::Custom(name) => {
+                        gecko.mProperty = eCSSPropertyExtra_variable;
+                        gecko.mUnknownProperty.mRawPtr = name.into_addrefed();
+                    }
                     _ => gecko.mProperty = servo.to_nscsspropertyid().unwrap(),
                 }
             }
         } else {
             // In gecko |none| is represented by eCSSPropertyExtra_no_properties.
             self.gecko.mTransitionPropertyCount = 1;
             self.gecko.mTransitions[0].mProperty = eCSSPropertyExtra_no_properties;
         }
@@ -3302,25 +3313,34 @@ fn static_assert() {
 
     pub fn transition_property_at(&self, index: usize)
         -> longhands::transition_property::computed_value::SingleComputedValue {
         use gecko_bindings::structs::nsCSSPropertyID::eCSSPropertyExtra_no_properties;
         use gecko_bindings::structs::nsCSSPropertyID::eCSSPropertyExtra_variable;
         use gecko_bindings::structs::nsCSSPropertyID::eCSSProperty_UNKNOWN;
 
         let property = self.gecko.mTransitions[index].mProperty;
-        if property == eCSSProperty_UNKNOWN || property == eCSSPropertyExtra_variable {
+        if property == eCSSProperty_UNKNOWN {
             let atom = self.gecko.mTransitions[index].mUnknownProperty.mRawPtr;
             debug_assert!(!atom.is_null());
             TransitionProperty::Unsupported(CustomIdent(unsafe{
                 Atom::from_raw(atom)
             }))
+        } else if property == eCSSPropertyExtra_variable {
+            let atom = self.gecko.mTransitions[index].mUnknownProperty.mRawPtr;
+            debug_assert!(!atom.is_null());
+            TransitionProperty::Custom(unsafe{
+                Atom::from_raw(atom)
+            })
         } else if property == eCSSPropertyExtra_no_properties {
-            // Actually, we don't expect TransitionProperty::Unsupported also represents "none",
-            // but if the caller wants to convert it, it is fine. Please use it carefully.
+            // Actually, we don't expect TransitionProperty::Unsupported also
+            // represents "none", but if the caller wants to convert it, it is
+            // fine. Please use it carefully.
+            //
+            // FIXME(emilio): This is a hack, is this reachable?
             TransitionProperty::Unsupported(CustomIdent(atom!("none")))
         } else {
             property.into()
         }
     }
 
     pub fn transition_nscsspropertyid_at(&self, index: usize) -> nsCSSPropertyID {
         self.gecko.mTransitions[index].mProperty
@@ -3331,21 +3351,25 @@ fn static_assert() {
         use gecko_bindings::structs::nsCSSPropertyID::eCSSProperty_UNKNOWN;
         self.gecko.mTransitions.ensure_len(other.gecko.mTransitions.len());
 
         let count = other.gecko.mTransitionPropertyCount;
         self.gecko.mTransitionPropertyCount = count;
 
         for (index, transition) in self.gecko.mTransitions.iter_mut().enumerate().take(count as usize) {
             transition.mProperty = other.gecko.mTransitions[index].mProperty;
+            if !transition.mUnknownProperty.mRawPtr.is_null() {
+                unsafe { Atom::from_addrefed(transition.mUnknownProperty.mRawPtr) };
+                transition.mUnknownProperty.mRawPtr = ptr::null_mut();
+            }
             if transition.mProperty == eCSSProperty_UNKNOWN ||
                transition.mProperty == eCSSPropertyExtra_variable {
                 let atom = other.gecko.mTransitions[index].mUnknownProperty.mRawPtr;
                 debug_assert!(!atom.is_null());
-                unsafe { Gecko_StyleTransition_SetUnsupportedProperty(transition, atom) };
+                transition.mUnknownProperty.mRawPtr = unsafe { Atom::from_raw(atom) }.into_addrefed();
             }
         }
     }
 
     pub fn reset_transition_property(&mut self, other: &Self) {
         self.copy_transition_property_from(other)
     }
 
--- a/servo/components/style/properties/helpers/animated_properties.mako.rs
+++ b/servo/components/style/properties/helpers/animated_properties.mako.rs
@@ -4,35 +4,38 @@
 
 <%namespace name="helpers" file="/helpers.mako.rs" />
 
 <%
     from data import to_idl_name, SYSTEM_FONT_LONGHANDS
     from itertools import groupby
 %>
 
+use Atom;
 use cssparser::Parser;
 #[cfg(feature = "gecko")] use gecko_bindings::bindings::RawServoAnimationValueMap;
 #[cfg(feature = "gecko")] use gecko_bindings::structs::RawGeckoGfxMatrix4x4;
 #[cfg(feature = "gecko")] use gecko_bindings::structs::nsCSSPropertyID;
 #[cfg(feature = "gecko")] use gecko_bindings::sugar::ownership::{HasFFI, HasSimpleFFI};
 use itertools::{EitherOrBoth, Itertools};
 use num_traits::Zero;
-use properties::{CSSWideKeyword, PropertyDeclaration};
+use parser::ParserContext;
+use properties::{CSSWideKeyword, PropertyDeclaration, PropertyDeclarationId};
 use properties::longhands;
 use properties::longhands::font_weight::computed_value::T as FontWeight;
 use properties::longhands::visibility::computed_value::T as Visibility;
 use properties::PropertyId;
 use properties::{LonghandId, ShorthandId};
 use servo_arc::Arc;
 use smallvec::SmallVec;
 use std::{cmp, ptr};
+use std::fmt::{self, Write};
 use std::mem::{self, ManuallyDrop};
 #[cfg(feature = "gecko")] use hash::FnvHashMap;
-use style_traits::{KeywordsCollectFn, ParseError, SpecifiedValueInfo};
+use style_traits::{KeywordsCollectFn, ParseError, SpecifiedValueInfo, ToCss, CssWriter};
 use super::ComputedValues;
 use values::{CSSFloat, CustomIdent};
 use values::animated::{Animate, Procedure, ToAnimatedValue, ToAnimatedZero};
 use values::animated::color::RGBA as AnimatedRGBA;
 use values::animated::effects::Filter as AnimatedFilter;
 use values::computed::{Angle, CalcLengthOrPercentage};
 use values::computed::{ClipRect, Context};
 use values::computed::{Length, LengthOrPercentage, LengthOrPercentageOrAuto};
@@ -69,79 +72,92 @@ pub fn nscsspropertyid_is_animatable(pro
         _ => false
     }
 }
 
 /// A given transition property, that is either `All`, a transitionable longhand property,
 /// a shorthand with at least one transitionable longhand component, or an unsupported property.
 // NB: This needs to be here because it needs all the longhands generated
 // beforehand.
-#[derive(Clone, Debug, Eq, Hash, MallocSizeOf, PartialEq, ToComputedValue, ToCss)]
+#[derive(Clone, Debug, Eq, Hash, MallocSizeOf, PartialEq, ToComputedValue)]
 pub enum TransitionProperty {
     /// A shorthand.
     Shorthand(ShorthandId),
     /// A longhand transitionable property.
     Longhand(LonghandId),
+    /// A custom property.
+    Custom(Atom),
     /// Unrecognized property which could be any non-transitionable, custom property, or
     /// unknown property.
     Unsupported(CustomIdent),
 }
 
+impl ToCss for TransitionProperty {
+    fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
+    where
+        W: Write,
+    {
+        use values::serialize_atom_identifier;
+        match *self {
+            TransitionProperty::Shorthand(ref s) => s.to_css(dest),
+            TransitionProperty::Longhand(ref l) => l.to_css(dest),
+            TransitionProperty::Custom(ref name) => {
+                dest.write_str("--")?;
+                serialize_atom_identifier(name, dest)
+            }
+            TransitionProperty::Unsupported(ref i) => i.to_css(dest),
+        }
+    }
+}
+
 impl TransitionProperty {
     /// Returns `all`.
     #[inline]
     pub fn all() -> Self {
         TransitionProperty::Shorthand(ShorthandId::All)
     }
 
     /// Parse a transition-property value.
-    pub fn parse<'i, 't>(input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {
-        // FIXME(https://github.com/rust-lang/rust/issues/33156): remove this
-        // enum and use PropertyId when stable Rust allows destructors in
-        // statics.
-        //
-        // FIXME: This should handle aliases too.
-        pub enum StaticId {
-            Longhand(LonghandId),
-            Shorthand(ShorthandId),
-        }
-        ascii_case_insensitive_phf_map! {
-            static_id -> StaticId = {
-                % for prop in data.shorthands:
-                "${prop.name}" => StaticId::Shorthand(ShorthandId::${prop.camel_case}),
-                % endfor
-                % for prop in data.longhands:
-                "${prop.name}" => StaticId::Longhand(LonghandId::${prop.camel_case}),
-                % endfor
-            }
-        }
-
+    pub fn parse<'i, 't>(
+        context: &ParserContext,
+        input: &mut Parser<'i, 't>,
+    ) -> Result<Self, ParseError<'i>> {
         let location = input.current_source_location();
         let ident = input.expect_ident()?;
 
-        Ok(match static_id(&ident) {
-            Some(&StaticId::Longhand(id)) => TransitionProperty::Longhand(id),
-            Some(&StaticId::Shorthand(id)) => TransitionProperty::Shorthand(id),
-            None => {
-                TransitionProperty::Unsupported(
-                    CustomIdent::from_ident(location, ident, &["none"])?,
-                )
-            },
+        let id = match PropertyId::parse(&ident, context) {
+            Ok(id) => id,
+            Err(..) => return Ok(TransitionProperty::Unsupported(
+                CustomIdent::from_ident(location, ident, &["none"])?,
+            )),
+        };
+
+        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())
+                    }
+                }
+            }
         })
     }
 
     /// Convert TransitionProperty to nsCSSPropertyID.
     #[cfg(feature = "gecko")]
     pub fn to_nscsspropertyid(&self) -> Result<nsCSSPropertyID, ()> {
         Ok(match *self {
             TransitionProperty::Shorthand(ShorthandId::All) => {
                 nsCSSPropertyID::eCSSPropertyExtra_all_properties
             }
             TransitionProperty::Shorthand(ref id) => id.to_nscsspropertyid(),
             TransitionProperty::Longhand(ref id) => id.to_nscsspropertyid(),
+            TransitionProperty::Custom(..) |
             TransitionProperty::Unsupported(..) => return Err(()),
         })
     }
 }
 
 /// Convert nsCSSPropertyID to TransitionProperty
 #[cfg(feature = "gecko")]
 #[allow(non_upper_case_globals)]
--- a/servo/components/style/properties/longhand/box.mako.rs
+++ b/servo/components/style/properties/longhand/box.mako.rs
@@ -252,17 +252,16 @@
 ${helpers.predefined_type(
     "transition-property",
     "TransitionProperty",
     "computed::TransitionProperty::all()",
     initial_specified_value="specified::TransitionProperty::all()",
     vector=True,
     allow_empty="NotInitial",
     need_index=True,
-    needs_context=False,
     animation_value_type="none",
     extra_prefixes=transition_extra_prefixes,
     spec="https://drafts.csswg.org/css-transitions/#propdef-transition-property",
 )}
 
 ${helpers.predefined_type("transition-delay",
                           "Time",
                           "computed::Time::zero()",
--- a/servo/components/style/properties/shorthand/box.mako.rs
+++ b/servo/components/style/properties/shorthand/box.mako.rs
@@ -153,20 +153,22 @@ macro_rules! try_parse_one {
                 parsed += 1;
 
                 try_parse_one!(context, input, duration, transition_duration);
                 try_parse_one!(context, input, timing_function, transition_timing_function);
                 try_parse_one!(context, input, delay, transition_delay);
                 // Must check 'transition-property' after 'transition-timing-function' since
                 // 'transition-property' accepts any keyword.
                 if property.is_none() {
-                    if let Ok(value) = input.try(|i| transition_property::SingleSpecifiedValue::parse(i)) {
+                    if let Ok(value) = input.try(|i| transition_property::SingleSpecifiedValue::parse(context, i)) {
                         property = Some(Some(value));
                         continue;
-                    } else if input.try(|i| i.expect_ident_matching("none")).is_ok() {
+                    }
+
+                    if input.try(|i| i.expect_ident_matching("none")).is_ok() {
                         // 'none' is not a valid value for <single-transition-property>,
                         // so it's not acceptable in the function above.
                         property = Some(None);
                         continue;
                     }
                 }
 
                 parsed -= 1;