servo: Merge #15683 - Fill missing property in keyframe (from hiikezoe:fill-missing-property-in-keyframe); r=emilio
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Wed, 22 Feb 2017 08:17:18 -0800
changeset 373294 887ae49f0e2fa2a26b2215348bc25932d0590851
parent 373293 5bf71e06cef7e76eff9b9265608ab63c46cff538
child 373295 981fefe750ae561574db37420eddc9b978cdd8c3
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
milestone54.0a1
servo: Merge #15683 - Fill missing property in keyframe (from hiikezoe:fill-missing-property-in-keyframe); r=emilio <!-- Please describe your changes on the following line: --> This is a PR of https://bugzilla.mozilla.org/show_bug.cgi?id=1340961 All patches has been reviewed by @emilio. Thanks! --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [X] There are tests for these changes <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 4f7e422054237c8ba0a8e521a615a6012b90eab4
servo/components/style/keyframes.rs
servo/components/style/properties/helpers/animated_properties.mako.rs
servo/components/style/properties/properties.mako.rs
servo/ports/geckolib/glue.rs
servo/tests/unit/style/keyframes.rs
--- a/servo/components/style/keyframes.rs
+++ b/servo/components/style/keyframes.rs
@@ -10,16 +10,17 @@ use cssparser::{AtRuleParser, Parser, Qu
 use cssparser::{DeclarationListParser, DeclarationParser, parse_one_rule};
 use parking_lot::RwLock;
 use parser::{ParserContext, ParserContextExtraData, log_css_error};
 use properties::{Importance, PropertyDeclaration, PropertyDeclarationBlock, PropertyId};
 use properties::{PropertyDeclarationId, LonghandId, DeclaredValue};
 use properties::PropertyDeclarationParseResult;
 use properties::animated_properties::TransitionProperty;
 use properties::longhands::transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction;
+use properties::property_bit_field::PropertyBitField;
 use std::fmt;
 use std::sync::Arc;
 use style_traits::ToCss;
 use stylesheets::{MemoryHoleReporter, Stylesheet};
 
 /// A number from 0 to 1, indicating the percentage of the animation when this
 /// keyframe should run.
 #[derive(Debug, Copy, Clone, PartialEq, PartialOrd)]
@@ -239,31 +240,33 @@ impl KeyframesStep {
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub struct KeyframesAnimation {
     /// The difference steps of the animation.
     pub steps: Vec<KeyframesStep>,
     /// The properties that change in this animation.
     pub properties_changed: Vec<TransitionProperty>,
 }
 
-/// Get all the animated properties in a keyframes animation. Note that it's not
-/// defined what happens when a property is not on a keyframe, so we only peek
-/// the props of the first one.
-///
-/// In practice, browsers seem to try to do their best job at it, so we might
-/// want to go through all the actual keyframes and deduplicate properties.
-fn get_animated_properties(keyframe: &Keyframe) -> Vec<TransitionProperty> {
+/// Get all the animated properties in a keyframes animation.
+fn get_animated_properties(keyframes: &[Arc<RwLock<Keyframe>>]) -> Vec<TransitionProperty> {
     let mut ret = vec![];
+    let mut seen = PropertyBitField::new();
     // NB: declarations are already deduplicated, so we don't have to check for
     // it here.
-    for &(ref declaration, importance) in keyframe.block.read().declarations.iter() {
-        assert!(!importance.important());
+    for keyframe in keyframes {
+        let keyframe = keyframe.read();
+        for &(ref declaration, importance) in keyframe.block.read().declarations.iter() {
+            assert!(!importance.important());
 
-        if let Some(property) = TransitionProperty::from_declaration(declaration) {
-            ret.push(property);
+            if let Some(property) = TransitionProperty::from_declaration(declaration) {
+                if !seen.has_transition_property_bit(&property) {
+                    ret.push(property);
+                    seen.set_transition_property_bit(&property);
+                }
+            }
         }
     }
 
     ret
 }
 
 impl KeyframesAnimation {
     /// Create a keyframes animation from a given list of keyframes.
@@ -279,17 +282,17 @@ impl KeyframesAnimation {
             steps: vec![],
             properties_changed: vec![],
         };
 
         if keyframes.is_empty() {
             return result;
         }
 
-        result.properties_changed = get_animated_properties(&keyframes[0].read());
+        result.properties_changed = get_animated_properties(keyframes);
         if result.properties_changed.is_empty() {
             return result;
         }
 
         for keyframe in keyframes {
             let keyframe = keyframe.read();
             for percentage in keyframe.selector.0.iter() {
                 result.steps.push(KeyframesStep::new(*percentage, KeyframesStepValue::Declarations {
--- a/servo/components/style/properties/helpers/animated_properties.mako.rs
+++ b/servo/components/style/properties/helpers/animated_properties.mako.rs
@@ -38,17 +38,17 @@ use values::computed::ToComputedValue;
 use values::specified::Angle as SpecifiedAngle;
 
 
 
 /// A given transition property, that is either `All`, or an animatable
 /// property.
 // NB: This needs to be here because it needs all the longhands generated
 // beforehand.
-#[derive(Copy, Clone, Debug, PartialEq)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub enum TransitionProperty {
     /// All, any animatable property changing should generate a transition.
     All,
     % for prop in data.longhands:
         % if prop.animatable:
             /// ${prop.name}
             ${prop.camel_case},
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -176,18 +176,20 @@ pub mod shorthands {
 /// given they populate the global data.
 pub mod animated_properties {
     <%include file="/helpers/animated_properties.mako.rs" />
 }
 
 
 // TODO(SimonSapin): Convert this to a syntax extension rather than a Mako template.
 // Maybe submit for inclusion in libstd?
-mod property_bit_field {
+#[allow(missing_docs)]
+pub mod property_bit_field {
     use logical_geometry::WritingMode;
+    use properties::animated_properties::TransitionProperty;
 
     /// A bitfield for all longhand properties, in order to quickly test whether
     /// we've seen one of them.
     pub struct PropertyBitField {
         storage: [u32; (${len(data.longhands)} - 1 + 32) / 32]
     }
 
     impl PropertyBitField {
@@ -208,17 +210,17 @@ mod property_bit_field {
         }
         % for i, property in enumerate(data.longhands):
             % if not property.derived_from:
                 #[allow(non_snake_case, missing_docs)]
                 #[inline]
                 pub fn get_${property.ident}(&self) -> bool {
                     self.get(${i})
                 }
-                #[allow(non_snake_case)]
+                #[allow(non_snake_case, missing_docs)]
                 #[inline]
                 pub fn set_${property.ident}(&mut self) {
                     self.set(${i})
                 }
             % endif
             % if property.logical:
                 #[allow(non_snake_case, missing_docs)]
                 pub fn get_physical_${property.ident}(&self, wm: WritingMode) -> bool {
@@ -233,16 +235,42 @@ mod property_bit_field {
                     <%helpers:logical_setter_helper name="${property.name}">
                         <%def name="inner(physical_ident)">
                             self.set_${physical_ident}()
                         </%def>
                     </%helpers:logical_setter_helper>
                 }
             % endif
         % endfor
+
+        /// Set the corresponding bit of TransitionProperty.
+        /// This function will panic if TransitionProperty::All is given.
+        pub fn set_transition_property_bit(&mut self, property: &TransitionProperty) {
+            match *property {
+                % for i, prop in enumerate(data.longhands):
+                    % if prop.animatable:
+                        TransitionProperty::${prop.camel_case} => self.set(${i}),
+                    % endif
+                % endfor
+                TransitionProperty::All => unreachable!("Tried to set TransitionProperty::All in a PropertyBitfield"),
+            }
+        }
+
+        /// Return true if the corresponding bit of TransitionProperty is set.
+        /// This function will panic if TransitionProperty::All is given.
+        pub fn has_transition_property_bit(&self, property: &TransitionProperty) -> bool {
+            match *property {
+                % for i, prop in enumerate(data.longhands):
+                    % if prop.animatable:
+                        TransitionProperty::${prop.camel_case} => self.get(${i}),
+                    % endif
+                % endfor
+                TransitionProperty::All => unreachable!("Tried to get TransitionProperty::All in a PropertyBitfield"),
+            }
+        }
     }
 }
 
 % for property in data.longhands:
     % if not property.derived_from:
         /// Perform CSS variable substitution if needed, and execute `f` with
         /// the resulting declared value.
         #[allow(non_snake_case)]
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -1430,16 +1430,19 @@ pub extern "C" fn Servo_AssertTreeIsClea
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_StyleSet_FillKeyframesForName(raw_data: RawServoStyleSetBorrowed,
                                                       name: *const nsACString,
                                                       timing_function: *const nsTimingFunction,
                                                       style: ServoComputedValuesBorrowed,
                                                       keyframes: RawGeckoKeyframeListBorrowedMut) -> bool {
+    use style::gecko_bindings::structs::Keyframe;
+    use style::properties::property_bit_field::PropertyBitField;
+
     let data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
     let name = unsafe { Atom::from(name.as_ref().unwrap().as_str_unchecked()) };
     let style_timing_function = unsafe { timing_function.as_ref().unwrap() };
     let style = ComputedValues::as_arc(&style);
 
     if let Some(ref animation) = data.stylist.animations().get(&name) {
        for step in &animation.steps {
           // Override timing_function if the keyframe has animation-timing-function.
@@ -1450,47 +1453,73 @@ pub extern "C" fn Servo_StyleSet_FillKey
           };
 
           let keyframe = unsafe {
                 Gecko_AnimationAppendKeyframe(keyframes,
                                               step.start_percentage.0 as f32,
                                               &timing_function)
           };
 
+          fn add_computed_property_value(keyframe: *mut Keyframe,
+                                         index: usize,
+                                         style: &ComputedValues,
+                                         property: &TransitionProperty) {
+              let block = style.to_declaration_block(property.clone().into());
+              unsafe {
+                  (*keyframe).mPropertyValues.set_len((index + 1) as u32);
+                  (*keyframe).mPropertyValues[index].mProperty = property.clone().into();
+                  // FIXME. Do not set computed values once we handles missing keyframes
+                  // with additive composition.
+                  (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky(
+                      Arc::new(RwLock::new(block)));
+              }
+          }
+
           match step.value {
               KeyframesStepValue::ComputedValues => {
                   for (index, property) in animation.properties_changed.iter().enumerate() {
-                      let block = style.to_declaration_block(property.clone().into());
-                      unsafe {
-                          (*keyframe).mPropertyValues.set_len((index + 1) as u32);
-                          (*keyframe).mPropertyValues[index].mProperty = property.clone().into();
-                          (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky(
-                              Arc::new(RwLock::new(block)));
-                      }
+                      add_computed_property_value(keyframe, index, style, property);
                   }
               },
               KeyframesStepValue::Declarations { ref block } => {
                   let guard = block.read();
                   // Filter out non-animatable properties.
                   let animatable =
                       guard.declarations
                            .iter()
                            .filter(|&&(ref declaration, _)| {
                                declaration.is_animatable()
                            });
+
+                  let mut seen = PropertyBitField::new();
+
                   for (index, &(ref declaration, _)) in animatable.enumerate() {
                       unsafe {
+                          let property = TransitionProperty::from_declaration(declaration).unwrap();
                           (*keyframe).mPropertyValues.set_len((index + 1) as u32);
-                          (*keyframe).mPropertyValues[index].mProperty =
-                              TransitionProperty::from_declaration(declaration).unwrap().into();
+                          (*keyframe).mPropertyValues[index].mProperty = property.into();
                           (*keyframe).mPropertyValues[index].mServoDeclarationBlock.set_arc_leaky(
                               Arc::new(RwLock::new(
                                   PropertyDeclarationBlock { declarations: vec![ (declaration.clone(),
                                                                                   Importance::Normal) ],
                                                              important_count: 0 })));
+                          if step.start_percentage.0 == 0. ||
+                             step.start_percentage.0 == 1. {
+                              seen.set_transition_property_bit(&property);
+                          }
+                      }
+                  }
+
+                  // Append missing property values in the initial or the finial keyframes.
+                  if step.start_percentage.0 == 0. ||
+                     step.start_percentage.0 == 1. {
+                      for (index, property) in animation.properties_changed.iter().enumerate() {
+                          if !seen.has_transition_property_bit(&property) {
+                              add_computed_property_value(keyframe, index, style, property);
+                          }
                       }
                   }
               },
           }
        }
        return true
     }
     false
--- a/servo/tests/unit/style/keyframes.rs
+++ b/servo/tests/unit/style/keyframes.rs
@@ -1,16 +1,19 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use parking_lot::RwLock;
 use std::sync::Arc;
 use style::keyframes::{Keyframe, KeyframesAnimation, KeyframePercentage,  KeyframeSelector};
-use style::properties::PropertyDeclarationBlock;
+use style::keyframes::{KeyframesStep, KeyframesStepValue};
+use style::properties::{DeclaredValue, PropertyDeclaration, PropertyDeclarationBlock, Importance};
+use style::properties::animated_properties::TransitionProperty;
+use style::values::specified::{LengthOrPercentageOrAuto, NoCalcLength};
 
 #[test]
 fn test_empty_keyframe() {
     let keyframes = vec![];
     let animation = KeyframesAnimation::from_keyframes(&keyframes);
     let expected = KeyframesAnimation {
         steps: vec![],
         properties_changed: vec![],
@@ -33,8 +36,182 @@ fn test_no_property_in_keyframe() {
     let animation = KeyframesAnimation::from_keyframes(&keyframes);
     let expected = KeyframesAnimation {
         steps: vec![],
         properties_changed: vec![],
     };
 
     assert_eq!(format!("{:#?}", animation), format!("{:#?}", expected));
 }
+
+#[test]
+fn test_missing_property_in_initial_keyframe() {
+    let declarations_on_initial_keyframe =
+        Arc::new(RwLock::new(PropertyDeclarationBlock {
+            declarations: vec![
+                (PropertyDeclaration::Width(
+                    DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+            ],
+            important_count: 0,
+        }));
+
+    let declarations_on_final_keyframe =
+        Arc::new(RwLock::new(PropertyDeclarationBlock {
+            declarations: vec![
+                (PropertyDeclaration::Width(
+                    DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+
+                (PropertyDeclaration::Height(
+                    DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+            ],
+            important_count: 0,
+        }));
+
+    let keyframes = vec![
+        Arc::new(RwLock::new(Keyframe {
+            selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.)]),
+            block: declarations_on_initial_keyframe.clone(),
+        })),
+
+        Arc::new(RwLock::new(Keyframe {
+            selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(1.)]),
+            block: declarations_on_final_keyframe.clone(),
+        })),
+    ];
+    let animation = KeyframesAnimation::from_keyframes(&keyframes);
+    let expected = KeyframesAnimation {
+        steps: vec![
+            KeyframesStep {
+                start_percentage: KeyframePercentage(0.),
+                value: KeyframesStepValue::Declarations { block: declarations_on_initial_keyframe },
+                declared_timing_function: false,
+            },
+            KeyframesStep {
+                start_percentage: KeyframePercentage(1.),
+                value: KeyframesStepValue::Declarations { block: declarations_on_final_keyframe },
+                declared_timing_function: false,
+            },
+        ],
+        properties_changed: vec![TransitionProperty::Width, TransitionProperty::Height],
+    };
+
+    assert_eq!(format!("{:#?}", animation), format!("{:#?}", expected));
+}
+
+#[test]
+fn test_missing_property_in_final_keyframe() {
+    let declarations_on_initial_keyframe =
+        Arc::new(RwLock::new(PropertyDeclarationBlock {
+            declarations: vec![
+                (PropertyDeclaration::Width(
+                    DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+
+                (PropertyDeclaration::Height(
+                    DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+            ],
+            important_count: 0,
+        }));
+
+    let declarations_on_final_keyframe =
+        Arc::new(RwLock::new(PropertyDeclarationBlock {
+            declarations: vec![
+                (PropertyDeclaration::Height(
+                    DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+            ],
+            important_count: 0,
+        }));
+
+    let keyframes = vec![
+        Arc::new(RwLock::new(Keyframe {
+            selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.)]),
+            block: declarations_on_initial_keyframe.clone(),
+        })),
+
+        Arc::new(RwLock::new(Keyframe {
+            selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(1.)]),
+            block: declarations_on_final_keyframe.clone(),
+        })),
+    ];
+    let animation = KeyframesAnimation::from_keyframes(&keyframes);
+    let expected = KeyframesAnimation {
+        steps: vec![
+            KeyframesStep {
+                start_percentage: KeyframePercentage(0.),
+                value: KeyframesStepValue::Declarations { block: declarations_on_initial_keyframe },
+                declared_timing_function: false,
+            },
+            KeyframesStep {
+                start_percentage: KeyframePercentage(1.),
+                value: KeyframesStepValue::Declarations { block: declarations_on_final_keyframe },
+                declared_timing_function: false,
+            },
+        ],
+        properties_changed: vec![TransitionProperty::Width, TransitionProperty::Height],
+    };
+
+    assert_eq!(format!("{:#?}", animation), format!("{:#?}", expected));
+}
+
+#[test]
+fn test_missing_keyframe_in_both_of_initial_and_final_keyframe() {
+    let declarations =
+        Arc::new(RwLock::new(PropertyDeclarationBlock {
+            declarations: vec![
+                (PropertyDeclaration::Width(
+                        DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                 Importance::Normal),
+
+                (PropertyDeclaration::Height(
+                        DeclaredValue::Value(LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32)))),
+                Importance::Normal),
+            ],
+            important_count: 0,
+        }));
+
+    let keyframes = vec![
+        Arc::new(RwLock::new(Keyframe {
+            selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.)]),
+            block: Arc::new(RwLock::new(PropertyDeclarationBlock {
+                declarations: vec![],
+                important_count: 0,
+            }))
+        })),
+        Arc::new(RwLock::new(Keyframe {
+            selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.5)]),
+            block: declarations.clone(),
+        })),
+    ];
+    let animation = KeyframesAnimation::from_keyframes(&keyframes);
+    let expected = KeyframesAnimation {
+        steps: vec![
+            KeyframesStep {
+                start_percentage: KeyframePercentage(0.),
+                value: KeyframesStepValue::Declarations {
+                    block: Arc::new(RwLock::new(PropertyDeclarationBlock {
+                        // XXX: Should we use ComputedValues in this case?
+                        declarations: vec![],
+                        important_count: 0,
+                    }))
+                },
+                declared_timing_function: false,
+            },
+            KeyframesStep {
+                start_percentage: KeyframePercentage(0.5),
+                value: KeyframesStepValue::Declarations { block: declarations },
+                declared_timing_function: false,
+            },
+            KeyframesStep {
+                start_percentage: KeyframePercentage(1.),
+                value: KeyframesStepValue::ComputedValues,
+                declared_timing_function: false,
+            }
+        ],
+        properties_changed: vec![TransitionProperty::Width, TransitionProperty::Height],
+    };
+
+    assert_eq!(format!("{:#?}", animation), format!("{:#?}", expected));
+}