From deb1b0aea67e570463b53f4817b517c8dc8a3751 Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Fri, 3 Mar 2017 10:23:25 +1100 Subject: [PATCH 1/6] Replace assert! in animation code with debug_assert! --- components/style/properties/gecko.mako.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 1c26c4a8f5a..d1f06601bb5 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -1324,7 +1324,7 @@ fn static_assert() { <%def name="impl_animation_time_value(ident, gecko_ffi_name)"> #[allow(non_snake_case)] pub fn set_animation_${ident}(&mut self, v: longhands::animation_${ident}::computed_value::T) { - assert!(v.0.len() > 0); + debug_assert!(!v.0.is_empty()); unsafe { self.gecko.mAnimations.ensure_len(v.0.len()) }; self.gecko.mAnimation${gecko_ffi_name}Count = v.0.len() as u32; @@ -1348,7 +1348,7 @@ fn static_assert() { use properties::longhands::animation_${ident}::single_value::computed_value::T as Keyword; use gecko_bindings::structs; - assert!(v.0.len() > 0); + debug_assert!(!v.0.is_empty()); unsafe { self.gecko.mAnimations.ensure_len(v.0.len()) }; self.gecko.mAnimation${gecko_ffi_name}Count = v.0.len() as u32; @@ -1773,7 +1773,7 @@ fn static_assert() { use std::f32; use properties::longhands::animation_iteration_count::single_value::SpecifiedValue as AnimationIterationCount; - assert!(v.0.len() > 0); + debug_assert!(!v.0.is_empty()); unsafe { self.gecko.mAnimations.ensure_len(v.0.len()) }; self.gecko.mAnimationIterationCountCount = v.0.len() as u32; @@ -1799,7 +1799,7 @@ fn static_assert() { ${impl_copy_animation_value('iteration_count', 'IterationCount')} pub fn set_animation_timing_function(&mut self, v: longhands::animation_timing_function::computed_value::T) { - assert!(v.0.len() > 0); + debug_assert!(!v.0.is_empty()); unsafe { self.gecko.mAnimations.ensure_len(v.0.len()) }; self.gecko.mAnimationTimingFunctionCount = v.0.len() as u32; From 550c64cbf26ee8cb037005f3de0820219b799e7f Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Thu, 2 Mar 2017 17:42:45 +1100 Subject: [PATCH 2/6] Make animation-name parse none --- components/style/matching.rs | 4 +++- components/style/properties/gecko.mako.rs | 13 +++++------ .../style/properties/longhand/box.mako.rs | 23 ++++++++++++++++--- tests/unit/style/lib.rs | 2 +- tests/unit/style/parsing/animation.rs | 16 +++++++++++++ 5 files changed, 46 insertions(+), 12 deletions(-) diff --git a/components/style/matching.rs b/components/style/matching.rs index 129b4884cbe..4680bed1bdf 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -398,7 +398,9 @@ impl StyleSharingCandidateCache { return; } - if box_style.animation_name_count() > 0 { + let animation_count = box_style.animation_name_count(); + debug_assert!(animation_count > 0); + if animation_count > 1 || box_style.animation_name_at(0).0 != atom!("") { debug!("Failing to insert to the cache: animations"); return; } diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index d1f06601bb5..240a0926799 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -1727,15 +1727,14 @@ fn static_assert() { pub fn set_animation_name(&mut self, v: longhands::animation_name::computed_value::T) { use nsstring::nsCString; + + debug_assert!(!v.0.is_empty()); unsafe { self.gecko.mAnimations.ensure_len(v.0.len()) }; - if v.0.len() > 0 { - self.gecko.mAnimationNameCount = v.0.len() as u32; - for (servo, gecko) in v.0.into_iter().zip(self.gecko.mAnimations.iter_mut()) { - gecko.mName.assign_utf8(&nsCString::from(servo.0.to_string())); - } - } else { - unsafe { self.gecko.mAnimations[0].mName.truncate(); } + self.gecko.mAnimationNameCount = v.0.len() as u32; + for (servo, gecko) in v.0.into_iter().zip(self.gecko.mAnimations.iter_mut()) { + // TODO This is inefficient. We should fix this in bug 1329169. + gecko.mName.assign_utf8(&nsCString::from(servo.0.to_string())); } } pub fn animation_name_at(&self, index: usize) diff --git a/components/style/properties/longhand/box.mako.rs b/components/style/properties/longhand/box.mako.rs index 696d9038353..c342b9ab123 100644 --- a/components/style/properties/longhand/box.mako.rs +++ b/components/style/properties/longhand/box.mako.rs @@ -776,7 +776,6 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto", <%helpers:vector_longhand name="animation-name" - allow_empty="True" need_index="True" animatable="False", extra_prefixes="moz webkit" @@ -797,6 +796,16 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto", #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub struct SpecifiedValue(pub Atom); + #[inline] + pub fn get_initial_value() -> computed_value::T { + get_initial_specified_value() + } + + #[inline] + pub fn get_initial_specified_value() -> SpecifiedValue { + SpecifiedValue(atom!("")) + } + impl fmt::Display for SpecifiedValue { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { self.0.fmt(f) @@ -805,7 +814,11 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto", impl ToCss for SpecifiedValue { fn to_css(&self, dest: &mut W) -> fmt::Result where W: fmt::Write { - dest.write_str(&*self.0.to_string()) + if self.0 == atom!("") { + dest.write_str("none") + } else { + dest.write_str(&*self.0.to_string()) + } } } @@ -813,7 +826,11 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto", fn parse(_context: &ParserContext, input: &mut ::cssparser::Parser) -> Result { use cssparser::Token; Ok(match input.next() { - Ok(Token::Ident(ref value)) if value != "none" => SpecifiedValue(Atom::from(&**value)), + Ok(Token::Ident(ref value)) => SpecifiedValue(if value == "none" { + atom!("") + } else { + Atom::from(&**value) + }), Ok(Token::QuotedString(value)) => SpecifiedValue(Atom::from(&*value)), _ => return Err(()), }) diff --git a/tests/unit/style/lib.rs b/tests/unit/style/lib.rs index 184ff4e1579..bb88a538a73 100644 --- a/tests/unit/style/lib.rs +++ b/tests/unit/style/lib.rs @@ -14,7 +14,7 @@ extern crate parking_lot; extern crate rayon; extern crate rustc_serialize; extern crate selectors; -extern crate servo_atoms; +#[macro_use] extern crate servo_atoms; extern crate servo_config; extern crate servo_url; extern crate style; diff --git a/tests/unit/style/parsing/animation.rs b/tests/unit/style/parsing/animation.rs index ed746790920..bf9f3b3e2ee 100644 --- a/tests/unit/style/parsing/animation.rs +++ b/tests/unit/style/parsing/animation.rs @@ -5,11 +5,27 @@ use cssparser::Parser; use media_queries::CSSErrorReporterTest; use parsing::parse; +use servo_atoms::Atom; use style::parser::{Parse, ParserContext}; use style::properties::longhands::animation_iteration_count::single_value::computed_value::T as AnimationIterationCount; +use style::properties::longhands::animation_name; use style::stylesheets::Origin; use style_traits::ToCss; +#[test] +fn test_animation_name() { + use self::animation_name::single_value::SpecifiedValue as SingleValue; + let other_name = Atom::from("other-name"); + assert_eq!(parse_longhand!(animation_name, "none"), + animation_name::SpecifiedValue(vec![SingleValue(atom!(""))])); + assert_eq!(parse_longhand!(animation_name, "other-name, none, 'other-name', \"other-name\""), + animation_name::SpecifiedValue( + vec![SingleValue(other_name.clone()), + SingleValue(atom!("")), + SingleValue(other_name.clone()), + SingleValue(other_name.clone())])); +} + #[test] fn test_animation_iteration() { assert_roundtrip_with_context!(AnimationIterationCount::parse, "0", "0"); From 88772f9cb94a18c7e00a534503ed8800504c8069 Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Thu, 2 Mar 2017 17:52:57 +1100 Subject: [PATCH 3/6] Add / fix get_initial_specified_value for animation longhands --- components/style/properties/longhand/box.mako.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/components/style/properties/longhand/box.mako.rs b/components/style/properties/longhand/box.mako.rs index c342b9ab123..ae749fcde16 100644 --- a/components/style/properties/longhand/box.mako.rs +++ b/components/style/properties/longhand/box.mako.rs @@ -460,6 +460,11 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto", Time(0.0) } + #[inline] + pub fn get_initial_specified_value() -> SpecifiedValue { + SpecifiedValue(0.0) + } + pub fn parse(context: &ParserContext, input: &mut Parser) -> Result { Time::parse(context, input) } @@ -727,7 +732,7 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto", #[inline] pub fn get_initial_specified_value() -> SpecifiedValue { - ToComputedValue::from_computed_value(&ease()) + SpecifiedValue::Keyword(FunctionKeyword::Ease) } pub fn parse(context: &ParserContext, input: &mut Parser) -> Result { @@ -852,6 +857,7 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto", spec="https://drafts.csswg.org/css-animations/#propdef-animation-duration", allowed_in_keyframe_block="False"> pub use properties::longhands::transition_duration::single_value::computed_value; + pub use properties::longhands::transition_duration::single_value::get_initial_specified_value; pub use properties::longhands::transition_duration::single_value::{get_initial_value, parse}; pub use properties::longhands::transition_duration::single_value::SpecifiedValue; @@ -920,7 +926,12 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto", #[inline] pub fn get_initial_value() -> computed_value::T { - computed_value::T::Number(1.0) + get_initial_specified_value() + } + + #[inline] + pub fn get_initial_specified_value() -> SpecifiedValue { + SpecifiedValue::Number(1.0) } #[inline] @@ -972,6 +983,7 @@ ${helpers.single_keyword("animation-fill-mode", spec="https://drafts.csswg.org/css-animations/#propdef-animation-delay", allowed_in_keyframe_block="False"> pub use properties::longhands::transition_duration::single_value::computed_value; + pub use properties::longhands::transition_duration::single_value::get_initial_specified_value; pub use properties::longhands::transition_duration::single_value::{get_initial_value, parse}; pub use properties::longhands::transition_duration::single_value::SpecifiedValue; From a06b3f2039fa004eb7db12ff127f496498d9b65a Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Thu, 2 Mar 2017 17:59:27 +1100 Subject: [PATCH 4/6] Allow animation-name to be omitted from shorthand --- .../style/properties/shorthand/box.mako.rs | 41 +++++++++---------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/components/style/properties/shorthand/box.mako.rs b/components/style/properties/shorthand/box.mako.rs index 3bb821fe737..0fa958952cd 100644 --- a/components/style/properties/shorthand/box.mako.rs +++ b/components/style/properties/shorthand/box.mako.rs @@ -198,28 +198,25 @@ macro_rules! try_parse_one { break } - if let Some(name) = name { - Ok(SingleAnimation { - animation_name: name, - animation_duration: - duration.unwrap_or_else(animation_duration::single_value::get_initial_value), - animation_timing_function: - timing_function.unwrap_or_else(animation_timing_function::single_value - ::get_initial_specified_value), - animation_delay: - delay.unwrap_or_else(animation_delay::single_value::get_initial_value), - animation_iteration_count: - iteration_count.unwrap_or_else(animation_iteration_count::single_value::get_initial_value), - animation_direction: - direction.unwrap_or_else(animation_direction::single_value::get_initial_value), - animation_fill_mode: - fill_mode.unwrap_or_else(animation_fill_mode::single_value::get_initial_value), - animation_play_state: - play_state.unwrap_or_else(animation_play_state::single_value::get_initial_value), - }) - } else { - Err(()) - } + Ok(SingleAnimation { + animation_name: + name.unwrap_or_else(animation_name::single_value::get_initial_specified_value), + animation_duration: + duration.unwrap_or_else(animation_duration::single_value::get_initial_value), + animation_timing_function: + timing_function.unwrap_or_else(animation_timing_function::single_value + ::get_initial_specified_value), + animation_delay: + delay.unwrap_or_else(animation_delay::single_value::get_initial_value), + animation_iteration_count: + iteration_count.unwrap_or_else(animation_iteration_count::single_value::get_initial_value), + animation_direction: + direction.unwrap_or_else(animation_direction::single_value::get_initial_value), + animation_fill_mode: + fill_mode.unwrap_or_else(animation_fill_mode::single_value::get_initial_value), + animation_play_state: + play_state.unwrap_or_else(animation_play_state::single_value::get_initial_value), + }) } let mut names = vec![]; From 1bb1eeeb53254554184499b8c538de9fdf961d6b Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Thu, 2 Mar 2017 18:07:10 +1100 Subject: [PATCH 5/6] Simplify animation shorthand parsing code --- .../style/properties/shorthand/box.mako.rs | 95 ++++++------------- tests/unit/style/properties/serialization.rs | 6 +- 2 files changed, 31 insertions(+), 70 deletions(-) diff --git a/components/style/properties/shorthand/box.mako.rs b/components/style/properties/shorthand/box.mako.rs index 0fa958952cd..1639dd475f3 100644 --- a/components/style/properties/shorthand/box.mako.rs +++ b/components/style/properties/shorthand/box.mako.rs @@ -153,32 +153,26 @@ macro_rules! try_parse_one { animation-fill-mode animation-play-state" allowed_in_keyframe_block="False" spec="https://drafts.csswg.org/css-animations/#propdef-animation"> + <% + props = "name duration timing_function delay iteration_count \ + direction fill_mode play_state".split() + %> use parser::Parse; - use properties::longhands::{animation_name, animation_duration, animation_timing_function}; - use properties::longhands::{animation_delay, animation_iteration_count, animation_direction}; - use properties::longhands::{animation_fill_mode, animation_play_state}; + % for prop in props: + use properties::longhands::animation_${prop}; + % endfor pub fn parse_value(context: &ParserContext, input: &mut Parser) -> Result { struct SingleAnimation { - animation_name: animation_name::SingleSpecifiedValue, - animation_duration: animation_duration::SingleSpecifiedValue, - animation_timing_function: animation_timing_function::SingleSpecifiedValue, - animation_delay: animation_delay::SingleSpecifiedValue, - animation_iteration_count: animation_iteration_count::SingleSpecifiedValue, - animation_direction: animation_direction::SingleSpecifiedValue, - animation_fill_mode: animation_fill_mode::SingleSpecifiedValue, - animation_play_state: animation_play_state::SingleSpecifiedValue, + % for prop in props: + animation_${prop}: animation_${prop}::SingleSpecifiedValue, + % endfor } fn parse_one_animation(context: &ParserContext, input: &mut Parser) -> Result { - let mut duration = None; - let mut timing_function = None; - let mut delay = None; - let mut iteration_count = None; - let mut direction = None; - let mut fill_mode = None; - let mut play_state = None; - let mut name = None; + % for prop in props: + let mut ${prop} = None; + % endfor // NB: Name must be the last one here so that keywords valid for other // longhands are not interpreted as names. @@ -199,58 +193,30 @@ macro_rules! try_parse_one { } Ok(SingleAnimation { - animation_name: - name.unwrap_or_else(animation_name::single_value::get_initial_specified_value), - animation_duration: - duration.unwrap_or_else(animation_duration::single_value::get_initial_value), - animation_timing_function: - timing_function.unwrap_or_else(animation_timing_function::single_value - ::get_initial_specified_value), - animation_delay: - delay.unwrap_or_else(animation_delay::single_value::get_initial_value), - animation_iteration_count: - iteration_count.unwrap_or_else(animation_iteration_count::single_value::get_initial_value), - animation_direction: - direction.unwrap_or_else(animation_direction::single_value::get_initial_value), - animation_fill_mode: - fill_mode.unwrap_or_else(animation_fill_mode::single_value::get_initial_value), - animation_play_state: - play_state.unwrap_or_else(animation_play_state::single_value::get_initial_value), + % for prop in props: + animation_${prop}: ${prop}.unwrap_or_else(animation_${prop}::single_value + ::get_initial_specified_value), + % endfor }) } - let mut names = vec![]; - let mut durations = vec![]; - let mut timing_functions = vec![]; - let mut delays = vec![]; - let mut iteration_counts = vec![]; - let mut directions = vec![]; - let mut fill_modes = vec![]; - let mut play_states = vec![]; + % for prop in props: + let mut ${prop}s = vec![]; + % endfor if input.try(|input| input.expect_ident_matching("none")).is_err() { let results = try!(input.parse_comma_separated(|i| parse_one_animation(context, i))); for result in results.into_iter() { - names.push(result.animation_name); - durations.push(result.animation_duration); - timing_functions.push(result.animation_timing_function); - delays.push(result.animation_delay); - iteration_counts.push(result.animation_iteration_count); - directions.push(result.animation_direction); - fill_modes.push(result.animation_fill_mode); - play_states.push(result.animation_play_state); + % for prop in props: + ${prop}s.push(result.animation_${prop}); + % endfor } } Ok(Longhands { - animation_name: animation_name::SpecifiedValue(names), - animation_duration: animation_duration::SpecifiedValue(durations), - animation_timing_function: animation_timing_function::SpecifiedValue(timing_functions), - animation_delay: animation_delay::SpecifiedValue(delays), - animation_iteration_count: animation_iteration_count::SpecifiedValue(iteration_counts), - animation_direction: animation_direction::SpecifiedValue(directions), - animation_fill_mode: animation_fill_mode::SpecifiedValue(fill_modes), - animation_play_state: animation_play_state::SpecifiedValue(play_states), + % for prop in props: + animation_${prop}: animation_${prop}::SpecifiedValue(${prop}s), + % endfor }) } @@ -262,14 +228,9 @@ macro_rules! try_parse_one { return Ok(()); } - <% - subproperties = "duration timing_function delay direction \ - fill_mode iteration_count play_state".split() - %> - // If any value list length is differs then we don't do a shorthand serialization // either. - % for name in subproperties: + % for name in props[1:]: if len != self.animation_${name}.0.len() { return Ok(()) } @@ -280,7 +241,7 @@ macro_rules! try_parse_one { try!(write!(dest, ", ")); } - % for name in subproperties: + % for name in props[1:]: self.animation_${name}.0[i].to_css(dest)?; dest.write_str(" ")?; % endfor diff --git a/tests/unit/style/properties/serialization.rs b/tests/unit/style/properties/serialization.rs index 9e99169c54d..9433d97ed3d 100644 --- a/tests/unit/style/properties/serialization.rs +++ b/tests/unit/style/properties/serialization.rs @@ -983,7 +983,7 @@ mod shorthand_serialization { let serialization = block.to_css_string(); - assert_eq!(serialization, "animation: 1s ease-in 0s normal forwards infinite paused bounce;") + assert_eq!(serialization, "animation: 1s ease-in 0s infinite normal forwards paused bounce;") } #[test] @@ -1001,8 +1001,8 @@ mod shorthand_serialization { let serialization = block.to_css_string(); assert_eq!(serialization, - "animation: 1s ease-in 0s normal forwards infinite paused bounce, \ - 0.2s linear 1s reverse backwards 2 running roll;"); + "animation: 1s ease-in 0s infinite normal forwards paused bounce, \ + 0.2s linear 1s 2 reverse backwards running roll;"); } #[test] From 53027723ae78f3b2196025859b03408471459107 Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Thu, 2 Mar 2017 18:20:10 +1100 Subject: [PATCH 6/6] Parse none as just a normal animation item --- components/style/properties/shorthand/box.mako.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/components/style/properties/shorthand/box.mako.rs b/components/style/properties/shorthand/box.mako.rs index 1639dd475f3..c951c3833ad 100644 --- a/components/style/properties/shorthand/box.mako.rs +++ b/components/style/properties/shorthand/box.mako.rs @@ -204,13 +204,11 @@ macro_rules! try_parse_one { let mut ${prop}s = vec![]; % endfor - if input.try(|input| input.expect_ident_matching("none")).is_err() { - let results = try!(input.parse_comma_separated(|i| parse_one_animation(context, i))); - for result in results.into_iter() { - % for prop in props: - ${prop}s.push(result.animation_${prop}); - % endfor - } + let results = try!(input.parse_comma_separated(|i| parse_one_animation(context, i))); + for result in results.into_iter() { + % for prop in props: + ${prop}s.push(result.animation_${prop}); + % endfor } Ok(Longhands {