From 466df01e712ef2640d5bbf749850f892a67a88d6 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Mon, 22 Aug 2016 21:06:58 +0530 Subject: [PATCH] Review fixups --- components/layout/display_list_builder.rs | 40 +++---- .../helpers/animated_properties.mako.rs | 13 +-- .../properties/shorthand/background.mako.rs | 105 ++++++++---------- tests/unit/style/stylesheets.rs | 35 ++++-- 4 files changed, 99 insertions(+), 94 deletions(-) diff --git a/components/layout/display_list_builder.rs b/components/layout/display_list_builder.rs index 8b967d54ec1..ce3df3f4c47 100644 --- a/components/layout/display_list_builder.rs +++ b/components/layout/display_list_builder.rs @@ -516,31 +516,31 @@ impl FragmentDisplayListBuilding for Fragment { // Use 'background-origin' to get the origin value. let origin = get_cyclic(&background.background_origin.0, index); let (mut origin_x, mut origin_y) = match *origin { - background_origin::single_value::T::padding_box => { - (Au(0), Au(0)) - } - background_origin::single_value::T::border_box => { - (-border.left, -border.top) - } - background_origin::single_value::T::content_box => { - let border_padding = self.border_padding.to_physical(self.style.writing_mode); - (border_padding.left - border.left, border_padding.top - border.top) - } + background_origin::single_value::T::padding_box => { + (Au(0), Au(0)) + } + background_origin::single_value::T::border_box => { + (-border.left, -border.top) + } + background_origin::single_value::T::content_box => { + let border_padding = self.border_padding.to_physical(self.style.writing_mode); + (border_padding.left - border.left, border_padding.top - border.top) + } }; // Use `background-attachment` to get the initial virtual origin let attachment = get_cyclic(&background.background_attachment.0, index); let (virtual_origin_x, virtual_origin_y) = match *attachment { - background_attachment::single_value::T::scroll => { - (absolute_bounds.origin.x, absolute_bounds.origin.y) - } - background_attachment::single_value::T::fixed => { - // If the ‘background-attachment’ value for this image is ‘fixed’, then - // 'background-origin' has no effect. - origin_x = Au(0); - origin_y = Au(0); - (Au(0), Au(0)) - } + background_attachment::single_value::T::scroll => { + (absolute_bounds.origin.x, absolute_bounds.origin.y) + } + background_attachment::single_value::T::fixed => { + // If the ‘background-attachment’ value for this image is ‘fixed’, then + // 'background-origin' has no effect. + origin_x = Au(0); + origin_y = Au(0); + (Au(0), Au(0)) + } }; let position = *get_cyclic(&background.background_position.0, index); diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 6eb79de97e3..e3d0a1a3db5 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -162,16 +162,9 @@ impl Interpolate for Vec { fn interpolate(&self, other: &Self, time: f64) -> Result { use num_integer::lcm; let len = lcm(self.len(), other.len()); - let ret = self.iter().cycle().zip(other.iter().cycle()) - .take(len) - .filter_map(|(ref me, ref you)| { - me.interpolate(you, time).ok() - }).collect::(); - if ret.len() == len { - Ok(ret) - } else { - Err(()) - } + self.iter().cycle().zip(other.iter().cycle()).take(len).map(|(me, you)| { + me.interpolate(you, time) + }).collect() } } /// https://drafts.csswg.org/css-transitions/#animtype-number diff --git a/components/style/properties/shorthand/background.mako.rs b/components/style/properties/shorthand/background.mako.rs index 31cd555ab23..f507af9ec56 100644 --- a/components/style/properties/shorthand/background.mako.rs +++ b/components/style/properties/shorthand/background.mako.rs @@ -12,16 +12,15 @@ use properties::longhands::{background_image, background_size, background_origin, background_clip}; pub fn parse_value(context: &ParserContext, input: &mut Parser) -> Result { - % for name in "image position repeat size attachment origin clip".split(): - let mut any_${name} = false; - % endfor - let mut background_color = None; - let vec = try!(input.parse_comma_separated(|input| { + + % for name in "image position repeat size attachment origin clip".split(): + let mut background_${name} = background_${name}::SpecifiedValue(Vec::new()); + % endfor + try!(input.parse_comma_separated(|input| { % for name in "image position repeat size attachment origin clip".split(): let mut ${name} = None; % endfor - loop { if let Ok(value) = input.try(|input| background_color::parse(context, input)) { if background_color.is_none() { @@ -33,71 +32,59 @@ } } if position.is_none() { - if let Ok(value) = input.try(|input| background_position::single_value::parse(context, input)) { + if let Ok(value) = input.try(|input| background_position::single_value + ::parse(context, input)) { position = Some(value); - any_position = true; // Parse background size, if applicable. size = input.try(|input| { try!(input.expect_delim('/')); background_size::single_value::parse(context, input) }).ok(); - if let Some(_) = size { - any_size = true; - } continue } } % for name in "image repeat attachment origin clip".split(): if ${name}.is_none() { - if let Ok(value) = input.try(|input| background_${name}::single_value::parse(context, input)) { + if let Ok(value) = input.try(|input| background_${name}::single_value + ::parse(context, input)) { ${name} = Some(value); - any_${name} = true; continue } } % endfor break } - Ok((image, position, repeat, size, attachment, origin, clip)) - })); - - if !(any_image || any_position || any_repeat || any_size || - any_attachment || any_origin || any_clip || - background_color.is_some()) { - return Err(()) - } - - % for name in "image position repeat size attachment origin clip".split(): - let mut background_${name} = if any_${name} { - Some(background_${name}::SpecifiedValue(Vec::new())) - } else { - None - }; - % endfor - - for i in vec { - % for i,name in enumerate("image position repeat size attachment origin clip".split()): - if let Some(ref mut buf) = background_${name} { - if let Some(bg_${name}) = i.${i} { - buf.0.push(bg_${name}) - } else { - buf.0.push(background_${name}::single_value::get_initial_specified_value()) - } - } + let mut any = false; + % for name in "image position repeat size attachment origin clip".split(): + any = any || ${name}.is_some(); % endfor - } + any = any || background_color.is_some(); + if any { + % for name in "image position repeat size attachment origin clip".split(): + if let Some(bg_${name}) = ${name} { + background_${name}.0.push(bg_${name}); + } else { + background_${name}.0.push(background_${name}::single_value + ::get_initial_specified_value()); + } + % endfor + Ok(()) + } else { + Err(()) + } + })); Ok(Longhands { background_color: background_color, - background_image: background_image, - background_position: background_position, - background_repeat: background_repeat, - background_attachment: background_attachment, - background_size: background_size, - background_origin: background_origin, - background_clip: background_clip, + background_image: Some(background_image), + background_position: Some(background_position), + background_repeat: Some(background_repeat), + background_attachment: Some(background_attachment), + background_size: Some(background_size), + background_origin: Some(background_origin), + background_clip: Some(background_clip), }) } @@ -111,7 +98,6 @@ } } use std::cmp; - use std::iter::{once, repeat}; let mut len = 0; % for name in "image position repeat size attachment origin clip".split(): len = cmp::max(len, extract_value(self.background_${name}).map(|i| i.0.len()) @@ -123,15 +109,21 @@ return dest.write_str("") } - let iter = repeat(None).take(len - 1).chain(once(Some(self.background_color))) - % for name in "image position repeat size attachment origin clip".split(): - .zip(extract_value(self.background_${name}).into_iter() - .flat_map(|x| x.0.iter()) - .map(Some).chain(repeat(None))) - % endfor - ; let mut first = true; - for (((((((color, image), position), repeat), size), attachment), origin), clip) in iter { + for i in 0..len { + % for name in "image position repeat size attachment origin clip".split(): + let ${name} = if let DeclaredValue::Value(ref arr) = *self.background_${name} { + arr.0.get(i % arr.0.len()) + } else { + None + }; + % endfor + let color = if i == len - 1 { + Some(self.background_color) + } else { + None + }; + if first { first = false; } else { @@ -144,7 +136,6 @@ }, Some(_) => { try!(write!(dest, "transparent ")); - try!(write!(dest, " ")); } // Not yet the last one None => () diff --git a/tests/unit/style/stylesheets.rs b/tests/unit/style/stylesheets.rs index 68e0b8e9577..528dcd67084 100644 --- a/tests/unit/style/stylesheets.rs +++ b/tests/unit/style/stylesheets.rs @@ -191,19 +191,40 @@ fn test_parse_stylesheet() { } )), Importance::Normal), - (PropertyDeclaration::BackgroundPosition(DeclaredValue::Initial), + (PropertyDeclaration::BackgroundPosition(DeclaredValue::Value( + longhands::background_position::SpecifiedValue( + vec![longhands::background_position::single_value + ::get_initial_specified_value()]))), Importance::Normal), - (PropertyDeclaration::BackgroundRepeat(DeclaredValue::Initial), + (PropertyDeclaration::BackgroundRepeat(DeclaredValue::Value( + longhands::background_repeat::SpecifiedValue( + vec![longhands::background_repeat::single_value + ::get_initial_specified_value()]))), Importance::Normal), - (PropertyDeclaration::BackgroundAttachment(DeclaredValue::Initial), + (PropertyDeclaration::BackgroundAttachment(DeclaredValue::Value( + longhands::background_attachment::SpecifiedValue( + vec![longhands::background_attachment::single_value + ::get_initial_specified_value()]))), Importance::Normal), - (PropertyDeclaration::BackgroundImage(DeclaredValue::Initial), + (PropertyDeclaration::BackgroundImage(DeclaredValue::Value( + longhands::background_image::SpecifiedValue( + vec![longhands::background_image::single_value + ::get_initial_specified_value()]))), Importance::Normal), - (PropertyDeclaration::BackgroundSize(DeclaredValue::Initial), + (PropertyDeclaration::BackgroundSize(DeclaredValue::Value( + longhands::background_size::SpecifiedValue( + vec![longhands::background_size::single_value + ::get_initial_specified_value()]))), Importance::Normal), - (PropertyDeclaration::BackgroundOrigin(DeclaredValue::Initial), + (PropertyDeclaration::BackgroundOrigin(DeclaredValue::Value( + longhands::background_origin::SpecifiedValue( + vec![longhands::background_origin::single_value + ::get_initial_specified_value()]))), Importance::Normal), - (PropertyDeclaration::BackgroundClip(DeclaredValue::Initial), + (PropertyDeclaration::BackgroundClip(DeclaredValue::Value( + longhands::background_clip::SpecifiedValue( + vec![longhands::background_clip::single_value + ::get_initial_specified_value()]))), Importance::Normal), ]), important_count: 0,