From 1b236bf6209c18c3e1965a401054d389602411e1 Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Thu, 10 May 2018 09:56:10 +1000 Subject: [PATCH 01/24] style: Rename CaretColor to ColorOrAuto for reusing. Bug: 1460456 Reviewed-by: heycam MozReview-Commit-ID: LD6PlNI60GC --- .../sugar/style_complex_color.rs | 23 ++++++++----------- .../properties/longhand/inherited_ui.mako.rs | 4 ++-- components/style/values/computed/mod.rs | 2 +- components/style/values/computed/ui.rs | 5 ++-- components/style/values/generics/ui.rs | 11 --------- components/style/values/specified/mod.rs | 2 +- components/style/values/specified/ui.rs | 17 +++----------- 7 files changed, 19 insertions(+), 45 deletions(-) diff --git a/components/style/gecko_bindings/sugar/style_complex_color.rs b/components/style/gecko_bindings/sugar/style_complex_color.rs index 0e06b4c50ba..696eb6968b6 100644 --- a/components/style/gecko_bindings/sugar/style_complex_color.rs +++ b/components/style/gecko_bindings/sugar/style_complex_color.rs @@ -6,8 +6,9 @@ use gecko::values::{convert_nscolor_to_rgba, convert_rgba_to_nscolor}; use gecko_bindings::structs::{nscolor, StyleComplexColor}; +use values::{Auto, Either}; use values::computed::Color as ComputedColor; -use values::generics::ui::CaretColor; +use values::computed::ui::ColorOrAuto; impl From for StyleComplexColor { fn from(other: nscolor) -> Self { @@ -59,27 +60,21 @@ impl From for ComputedColor { } } -impl From> for StyleComplexColor -where - Color: Into, -{ - fn from(other: CaretColor) -> Self { +impl From for StyleComplexColor { + fn from(other: ColorOrAuto) -> Self { match other { - CaretColor::Color(color) => color.into(), - CaretColor::Auto => StyleComplexColor::auto(), + Either::First(color) => color.into(), + Either::Second(_) => StyleComplexColor::auto(), } } } -impl From for CaretColor -where - StyleComplexColor: Into, -{ +impl From for ColorOrAuto { fn from(other: StyleComplexColor) -> Self { if !other.mIsAuto { - CaretColor::Color(other.into()) + Either::First(other.into()) } else { - CaretColor::Auto + Either::Second(Auto) } } } diff --git a/components/style/properties/longhand/inherited_ui.mako.rs b/components/style/properties/longhand/inherited_ui.mako.rs index 3b516e2f1ce..b806a099123 100644 --- a/components/style/properties/longhand/inherited_ui.mako.rs +++ b/components/style/properties/longhand/inherited_ui.mako.rs @@ -43,8 +43,8 @@ ${helpers.single_keyword("-moz-user-focus", ${helpers.predefined_type( "caret-color", - "CaretColor", - "generics::ui::CaretColor::Auto", + "ColorOrAuto", + "Either::Second(Auto)", spec="https://drafts.csswg.org/css-ui/#caret-color", animation_value_type="AnimatedCaretColor", ignored_when_colors_disabled=True, diff --git a/components/style/values/computed/mod.rs b/components/style/values/computed/mod.rs index 0f27225eeb0..d6660f6a6eb 100644 --- a/components/style/values/computed/mod.rs +++ b/components/style/values/computed/mod.rs @@ -78,7 +78,7 @@ pub use self::text::{TextAlign, TextEmphasisPosition, TextEmphasisStyle, TextOve pub use self::time::Time; pub use self::transform::{Rotate, Scale, TimingFunction, Transform, TransformOperation}; pub use self::transform::{TransformOrigin, TransformStyle, Translate}; -pub use self::ui::{CaretColor, Cursor, MozForceBrokenImageIcon}; +pub use self::ui::{ColorOrAuto, Cursor, MozForceBrokenImageIcon}; #[cfg(feature = "gecko")] pub use self::ui::CursorImage; diff --git a/components/style/values/computed/ui.rs b/components/style/values/computed/ui.rs index 1313a5e5dd9..5307f79a38e 100644 --- a/components/style/values/computed/ui.rs +++ b/components/style/values/computed/ui.rs @@ -4,6 +4,7 @@ //! Computed values for UI properties +use values::{Auto, Either}; use values::computed::Number; use values::computed::color::Color; use values::computed::url::ComputedImageUrl; @@ -11,8 +12,8 @@ use values::generics::ui as generics; pub use values::specified::ui::MozForceBrokenImageIcon; -/// A computed value for the `caret-color` property. -pub type CaretColor = generics::CaretColor; +/// auto | +pub type ColorOrAuto = Either; /// A computed value for the `cursor` property. pub type Cursor = generics::Cursor; diff --git a/components/style/values/generics/ui.rs b/components/style/values/generics/ui.rs index a52ee9fcf76..9ccf1f80d53 100644 --- a/components/style/values/generics/ui.rs +++ b/components/style/values/generics/ui.rs @@ -8,17 +8,6 @@ use std::fmt::{self, Write}; use style_traits::{CssWriter, ToCss}; use style_traits::cursor::CursorKind; -/// A generic value for the `caret-color` property. -#[derive(Animate, Clone, ComputeSquaredDistance, Copy, Debug, MallocSizeOf, - PartialEq, SpecifiedValueInfo, ToAnimatedValue, ToAnimatedZero, - ToComputedValue, ToCss)] -pub enum CaretColor { - /// An explicit color. - Color(Color), - /// The keyword `auto`. - Auto, -} - /// A generic value for the `cursor` property. /// /// https://drafts.csswg.org/css-ui/#cursor diff --git a/components/style/values/specified/mod.rs b/components/style/values/specified/mod.rs index b1795acd65b..c25d58332f5 100644 --- a/components/style/values/specified/mod.rs +++ b/components/style/values/specified/mod.rs @@ -75,7 +75,7 @@ pub use self::text::{TextAlignKeyword, TextDecorationLine, TextOverflow, WordSpa pub use self::time::Time; pub use self::transform::{Rotate, Scale, TimingFunction, Transform}; pub use self::transform::{TransformOrigin, TransformStyle, Translate}; -pub use self::ui::{CaretColor, Cursor, MozForceBrokenImageIcon}; +pub use self::ui::{ColorOrAuto, Cursor, MozForceBrokenImageIcon}; #[cfg(feature = "gecko")] pub use self::ui::CursorImage; pub use super::generics::grid::GridTemplateComponent as GenericGridTemplateComponent; diff --git a/components/style/values/specified/ui.rs b/components/style/values/specified/ui.rs index d42b1e4e257..e912c24b808 100644 --- a/components/style/values/specified/ui.rs +++ b/components/style/values/specified/ui.rs @@ -9,25 +9,14 @@ use parser::{Parse, ParserContext}; use std::fmt::{self, Write}; use style_traits::{CssWriter, ParseError, StyleParseErrorKind, ToCss}; use style_traits::cursor::CursorKind; +use values::{Auto, Either}; use values::generics::ui as generics; use values::specified::Number; use values::specified::color::Color; use values::specified::url::SpecifiedImageUrl; -/// A specified value for the `caret-color` property. -pub type CaretColor = generics::CaretColor; - -impl Parse for CaretColor { - fn parse<'i, 't>( - context: &ParserContext, - input: &mut Parser<'i, 't>, - ) -> Result> { - if input.try(|i| i.expect_ident_matching("auto")).is_ok() { - return Ok(generics::CaretColor::Auto); - } - Ok(generics::CaretColor::Color(Color::parse(context, input)?)) - } -} +/// auto | +pub type ColorOrAuto = Either; /// A specified value for the `cursor` property. pub type Cursor = generics::Cursor; From 3016a7f552cbbcf28686b4349a414f0ff6b99493 Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Thu, 10 May 2018 10:40:17 +1000 Subject: [PATCH 02/24] style: Add scrollbar-{face,track}-color properties. Bug: 1460456 Reviewed-by: heycam MozReview-Commit-ID: ImNfHHfzRdM --- components/style/properties/gecko.mako.rs | 5 ++--- .../properties/longhand/inherited_ui.mako.rs | 22 +++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 3d9a8e579af..9d00239f86b 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -1436,6 +1436,7 @@ impl Clone for ${style_struct.gecko_struct_name} { # Types used with predefined_type()-defined properties that we can auto-generate. predefined_types = { "Color": impl_color, + "ColorOrAuto": impl_color, "GreaterThanOrEqualToOneNumber": impl_simple, "Integer": impl_simple, "length::LengthOrAuto": impl_style_coord, @@ -5303,7 +5304,7 @@ clip-path <%self:impl_trait style_struct_name="InheritedUI" - skip_longhands="cursor caret-color"> + skip_longhands="cursor"> pub fn set_cursor(&mut self, v: longhands::cursor::computed_value::T) { use style_traits::cursor::CursorKind; @@ -5449,8 +5450,6 @@ clip-path longhands::cursor::computed_value::T { images, keyword } } - - <%call expr="impl_color('caret_color', 'mCaretColor')"> <%self:impl_trait style_struct_name="Column" diff --git a/components/style/properties/longhand/inherited_ui.mako.rs b/components/style/properties/longhand/inherited_ui.mako.rs index b806a099123..9718e33a84d 100644 --- a/components/style/properties/longhand/inherited_ui.mako.rs +++ b/components/style/properties/longhand/inherited_ui.mako.rs @@ -50,3 +50,25 @@ ${helpers.predefined_type( ignored_when_colors_disabled=True, products="gecko", )} + +// Only scrollbar-face-color and scrollbar-track-color are added here. +// These two are the only common parts of scrollbar among Windows and +// macOS. We may or may not want to provide other properties to allow +// finer-grain control. +// +// NOTE The syntax in spec is currently `normal | `, but I think +// reusing `auto | ` as `caret-color` makes more sense. See +// https://github.com/w3c/csswg-drafts/issues/2660 +% for part in ["face", "track"]: +${helpers.predefined_type( + "scrollbar-%s-color" % part, + "ColorOrAuto", + "Either::Second(Auto)", + spec="https://drafts.csswg.org/css-scrollbars-1/#scrollbar-color-properties", + gecko_pref="layout.css.scrollbar-colors.enabled", + animation_value_type="ColorOrAuto", + ignored_when_colors_disabled=True, + enabled_in="chrome", + products="gecko", +)} +% endfor From 32c6d5b7c6c84eab51c24c9c0fa89add4e3f84de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 18 May 2018 22:06:25 +0200 Subject: [PATCH 03/24] style: Refactor vector types. This fixes clamping of mask-size and moves it out of mako while at it. Bug: 1462829 Reviewed-by: hiro,xidorn MozReview-Commit-ID: 9hiTe63odna --- components/style/properties/data.py | 2 +- components/style/properties/gecko.mako.rs | 38 ++-- components/style/properties/helpers.mako.rs | 89 +++++--- .../helpers/animated_properties.mako.rs | 210 ++++++++++-------- .../properties/longhand/background.mako.rs | 7 +- .../style/properties/longhand/effects.mako.rs | 2 + .../longhand/inherited_text.mako.rs | 1 + .../style/properties/longhand/svg.mako.rs | 33 ++- .../style/properties/properties.mako.rs | 11 +- .../style/properties/shorthand/mask.mako.rs | 9 +- components/style/values/animated/effects.rs | 124 ----------- components/style/values/animated/mod.rs | 2 + .../style/values/computed/background.rs | 66 +----- components/style/values/computed/length.rs | 4 +- .../style/values/computed/percentage.rs | 2 +- .../style/values/generics/background.rs | 3 +- components/style/values/generics/svg.rs | 3 +- .../style/values/specified/background.rs | 25 +-- 18 files changed, 259 insertions(+), 372 deletions(-) diff --git a/components/style/properties/data.py b/components/style/properties/data.py index df1c8d2890a..0ac42b365ee 100644 --- a/components/style/properties/data.py +++ b/components/style/properties/data.py @@ -164,7 +164,7 @@ class Longhand(object): allowed_in_keyframe_block=True, cast_type='u8', logical=False, alias=None, extra_prefixes=None, boxed=False, flags=None, allowed_in_page_rule=False, allow_quirks=False, ignored_when_colors_disabled=False, - vector=False, need_animatable=False, servo_restyle_damage="repaint"): + vector=False, servo_restyle_damage="repaint"): self.name = name if not spec: raise TypeError("Spec should be specified for %s" % name) diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 9d00239f86b..1fc486975fd 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -3249,7 +3249,7 @@ fn static_assert() { pub fn clone_scroll_snap_coordinate(&self) -> longhands::scroll_snap_coordinate::computed_value::T { let vec = self.gecko.mScrollSnapCoordinate.iter().map(|f| f.into()).collect(); - longhands::scroll_snap_coordinate::computed_value::T(vec) + longhands::scroll_snap_coordinate::computed_value::List(vec) } ${impl_css_url('_moz_binding', 'mBinding')} @@ -3755,8 +3755,9 @@ fn static_assert() { <% copy_simple_image_array_property(name, shorthand, layer_field_name, field_name) %> pub fn set_${ident}(&mut self, v: I) - where I: IntoIterator, - I::IntoIter: ExactSizeIterator + where + I: IntoIterator, + I::IntoIter: ExactSizeIterator, { use properties::longhands::${ident}::single_value::computed_value::T as Keyword; use gecko_bindings::structs::nsStyleImageLayers_LayerType as LayerType; @@ -3791,7 +3792,7 @@ fn static_assert() { % endfor % endif - longhands::${ident}::computed_value::T ( + longhands::${ident}::computed_value::List( self.gecko.${layer_field_name}.mLayers.iter() .take(self.gecko.${layer_field_name}.${field_name}Count as usize) .map(|ref layer| { @@ -3858,7 +3859,7 @@ fn static_assert() { } } - longhands::${shorthand}_repeat::computed_value::T ( + longhands::${shorthand}_repeat::computed_value::List( self.gecko.${image_layers_field}.mLayers.iter() .take(self.gecko.${image_layers_field}.mRepeatCount as usize) .map(|ref layer| { @@ -3897,7 +3898,7 @@ fn static_assert() { pub fn clone_${shorthand}_position_${orientation}(&self) -> longhands::${shorthand}_position_${orientation}::computed_value::T { - longhands::${shorthand}_position_${orientation}::computed_value::T( + longhands::${shorthand}_position_${orientation}::computed_value::List( self.gecko.${image_layers_field}.mLayers.iter() .take(self.gecko.${image_layers_field}.mPosition${orientation.upper()}Count as usize) .map(|position| position.mPosition.m${orientation.upper()}Position.into()) @@ -3941,11 +3942,11 @@ fn static_assert() { BackgroundSize::Explicit { width: explicit_width, height: explicit_height } => { let mut w_type = nsStyleImageLayers_Size_DimensionType::eAuto; let mut h_type = nsStyleImageLayers_Size_DimensionType::eAuto; - if let Some(w) = explicit_width.to_calc_value() { + if let Some(w) = explicit_width.0.to_calc_value() { width = w; w_type = nsStyleImageLayers_Size_DimensionType::eLengthPercentage; } - if let Some(h) = explicit_height.to_calc_value() { + if let Some(h) = explicit_height.0.to_calc_value() { height = h; h_type = nsStyleImageLayers_Size_DimensionType::eLengthPercentage; } @@ -3973,22 +3974,23 @@ fn static_assert() { } - pub fn clone_${shorthand}_size(&self) -> longhands::background_size::computed_value::T { + pub fn clone_${shorthand}_size(&self) -> longhands::${shorthand}_size::computed_value::T { use gecko_bindings::structs::nsStyleCoord_CalcValue as CalcValue; use gecko_bindings::structs::nsStyleImageLayers_Size_DimensionType as DimensionType; - use values::computed::LengthOrPercentageOrAuto; + use values::generics::NonNegative; + use values::computed::NonNegativeLengthOrPercentageOrAuto; use values::generics::background::BackgroundSize; - fn to_servo(value: CalcValue, ty: u8) -> LengthOrPercentageOrAuto { + fn to_servo(value: CalcValue, ty: u8) -> NonNegativeLengthOrPercentageOrAuto { if ty == DimensionType::eAuto as u8 { - LengthOrPercentageOrAuto::Auto + NonNegativeLengthOrPercentageOrAuto::auto() } else { debug_assert_eq!(ty, DimensionType::eLengthPercentage as u8); - value.into() + NonNegative(value.into()) } } - longhands::background_size::computed_value::T( + longhands::${shorthand}_size::computed_value::List( self.gecko.${image_layers_field}.mLayers.iter().map(|ref layer| { if DimensionType::eCover as u8 == layer.mSize.mWidthType { debug_assert_eq!(layer.mSize.mHeightType, DimensionType::eCover as u8); @@ -4059,7 +4061,7 @@ fn static_assert() { pub fn clone_${shorthand}_image(&self) -> longhands::${shorthand}_image::computed_value::T { use values::None_; - longhands::${shorthand}_image::computed_value::T( + longhands::${shorthand}_image::computed_value::List( self.gecko.${image_layers_field}.mLayers.iter() .take(self.gecko.${image_layers_field}.mImageCount as usize) .map(|ref layer| { @@ -4315,7 +4317,7 @@ fn static_assert() { pub fn clone_box_shadow(&self) -> longhands::box_shadow::computed_value::T { let buf = self.gecko.mBoxShadow.iter().map(|v| v.to_box_shadow()).collect(); - longhands::box_shadow::computed_value::T(buf) + longhands::box_shadow::computed_value::List(buf) } pub fn set_clip(&mut self, v: longhands::clip::computed_value::T) { @@ -4560,7 +4562,7 @@ fn static_assert() { _ => {}, } } - longhands::filter::computed_value::T(filters) + longhands::filter::computed_value::List(filters) } @@ -4681,7 +4683,7 @@ fn static_assert() { pub fn clone_text_shadow(&self) -> longhands::text_shadow::computed_value::T { let buf = self.gecko.mTextShadow.iter().map(|v| v.to_simple_shadow()).collect(); - longhands::text_shadow::computed_value::T(buf) + longhands::text_shadow::computed_value::List(buf) } pub fn set_line_height(&mut self, v: longhands::line_height::computed_value::T) { diff --git a/components/style/properties/helpers.mako.rs b/components/style/properties/helpers.mako.rs index ff6670c3274..03b588ad654 100644 --- a/components/style/properties/helpers.mako.rs +++ b/components/style/properties/helpers.mako.rs @@ -8,7 +8,8 @@ %> <%def name="predefined_type(name, type, initial_value, parse_method='parse', - needs_context=True, vector=False, computed_type=None, initial_specified_value=None, + needs_context=True, vector=False, + computed_type=None, initial_specified_value=None, allow_quirks=False, allow_empty=False, **kwargs)"> <%def name="predefined_type_inner(name, type, initial_value, parse_method)"> #[allow(unused_imports)] @@ -77,10 +78,12 @@ We assume that the default/initial value is an empty vector for these. `initial_value` need not be defined for these. -<%def name="vector_longhand(name, animation_value_type=None, allow_empty=False, separator='Comma', - need_animatable=False, **kwargs)"> +<%def name="vector_longhand(name, animation_value_type=None, + vector_animation_type=None, allow_empty=False, + separator='Comma', + **kwargs)"> <%call expr="longhand(name, animation_value_type=animation_value_type, vector=True, - need_animatable=need_animatable, **kwargs)"> + **kwargs)"> #[allow(unused_imports)] use smallvec::SmallVec; @@ -115,36 +118,68 @@ % endif use values::computed::ComputedVecIter; - /// The computed value, effectively a list of single values. + /// The generic type defining the value for this property. % if separator == "Comma": #[css(comma)] % endif - #[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)] - % if need_animatable or animation_value_type == "ComputedValue": - #[derive(Animate, ComputeSquaredDistance)] - % endif - pub struct T( + #[derive(Clone, Debug, MallocSizeOf, PartialEq, ToAnimatedValue, + ToCss)] + pub struct List( % if not allow_empty: #[css(iterable)] % else: #[css(if_empty = "none", iterable)] % endif % if allow_empty and allow_empty != "NotInitial": - pub Vec, + pub Vec, % else: - pub SmallVec<[single_value::T; 1]>, + pub SmallVec<[T; 1]>, % endif ); - % if need_animatable or animation_value_type == "ComputedValue": - use values::animated::{ToAnimatedZero}; - impl ToAnimatedZero for T { - #[inline] - fn to_animated_zero(&self) -> Result { Err(()) } - } + /// The computed value, effectively a list of single values. + % if vector_animation_type: + % if not animation_value_type: + Sorry, this is stupid but needed for now. % endif + use properties::animated_properties::ListAnimation; + use values::animated::{Animate, ToAnimatedValue, ToAnimatedZero, Procedure}; + use values::distance::{SquaredDistance, ComputeSquaredDistance}; + + // FIXME(emilio): For some reason rust thinks that this alias is + // unused, even though it's clearly used below? + #[allow(unused)] + type AnimatedList = as ToAnimatedValue>::AnimatedValue; + + impl ToAnimatedZero for AnimatedList { + fn to_animated_zero(&self) -> Result { Err(()) } + } + + impl Animate for AnimatedList { + fn animate( + &self, + other: &Self, + procedure: Procedure, + ) -> Result { + Ok(List( + self.0.animate_${vector_animation_type}(&other.0, procedure)? + )) + } + } + impl ComputeSquaredDistance for AnimatedList { + fn compute_squared_distance( + &self, + other: &Self, + ) -> Result { + self.0.squared_distance_${vector_animation_type}(&other.0) + } + } + % endif + + pub type T = List; + pub type Iter<'a, 'cx, 'cx_a> = ComputedVecIter<'a, 'cx, 'cx_a, super::single_value::SpecifiedValue>; impl IntoIterator for T { @@ -176,16 +211,18 @@ pub fn get_initial_value() -> computed_value::T { % if allow_empty and allow_empty != "NotInitial": - computed_value::T(vec![]) + computed_value::List(vec![]) % else: let mut v = SmallVec::new(); v.push(single_value::get_initial_value()); - computed_value::T(v) + computed_value::List(v) % endif } - pub fn parse<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>) - -> Result> { + pub fn parse<'i, 't>( + context: &ParserContext, + input: &mut Parser<'i, 't>, + ) -> Result> { use style_traits::Separator; % if allow_empty: @@ -202,8 +239,10 @@ pub use self::single_value::SpecifiedValue as SingleSpecifiedValue; impl SpecifiedValue { - pub fn compute_iter<'a, 'cx, 'cx_a>(&'a self, context: &'cx Context<'cx_a>) - -> computed_value::Iter<'a, 'cx, 'cx_a> { + pub fn compute_iter<'a, 'cx, 'cx_a>( + &'a self, + context: &'cx Context<'cx_a>, + ) -> computed_value::Iter<'a, 'cx, 'cx_a> { computed_value::Iter::new(context, &self.0) } } @@ -213,7 +252,7 @@ #[inline] fn to_computed_value(&self, context: &Context) -> computed_value::T { - computed_value::T(self.compute_iter(context).collect()) + computed_value::List(self.compute_iter(context).collect()) } #[inline] diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index fa6166e17ae..057360c9499 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -29,11 +29,10 @@ use std::mem::{self, ManuallyDrop}; #[cfg(feature = "gecko")] use hash::FnvHashMap; use style_traits::{KeywordsCollectFn, ParseError, SpecifiedValueInfo}; use super::ComputedValues; -use values::{CSSFloat, CustomIdent, Either}; +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::animated::effects::FilterList as AnimatedFilterList; use values::computed::{Angle, CalcLengthOrPercentage}; use values::computed::{ClipRect, Context}; use values::computed::{Length, LengthOrPercentage, LengthOrPercentageOrAuto}; @@ -53,13 +52,10 @@ use values::distance::{ComputeSquaredDistance, SquaredDistance}; use values::generics::font::{FontSettings as GenericFontSettings, FontTag, VariationValue}; use values::computed::font::FontVariationSettings; use values::generics::effects::Filter; -use values::generics::position as generic_position; use values::generics::svg::{SVGLength, SvgLengthOrPercentageOrNumber, SVGPaint}; use values::generics::svg::{SVGPaintKind, SVGStrokeDashArray, SVGOpacity}; use void::{self, Void}; -/// -pub trait RepeatableListAnimatable: Animate {} /// Returns true if this nsCSSPropertyID is one of the animatable properties. #[cfg(feature = "gecko")] @@ -755,18 +751,45 @@ impl ToAnimatedZero for AnimationValue { } } -impl RepeatableListAnimatable for LengthOrPercentage {} -impl RepeatableListAnimatable for Either {} -impl RepeatableListAnimatable for Either {} -impl RepeatableListAnimatable for SvgLengthOrPercentageOrNumber {} +/// A trait to abstract away the different kind of animations over a list that +/// there may be. +pub trait ListAnimation : Sized { + /// + fn animate_repeatable_list(&self, other: &Self, procedure: Procedure) -> Result + where + T: Animate; -macro_rules! repeated_vec_impl { - ($($ty:ty),*) => { - $(impl Animate for $ty - where - T: RepeatableListAnimatable, - { - fn animate(&self, other: &Self, procedure: Procedure) -> Result { + /// + fn squared_distance_repeatable_list(&self, other: &Self) -> Result + where + T: ComputeSquaredDistance; + + /// This is the animation used for some of the types like shadows and + /// filters, where the interpolation happens with the zero value if one of + /// the sides is not present. + fn animate_with_zero(&self, other: &Self, procedure: Procedure) -> Result + where + T: Animate + Clone + ToAnimatedZero; + + /// This is the animation used for some of the types like shadows and + /// filters, where the interpolation happens with the zero value if one of + /// the sides is not present. + fn squared_distance_with_zero(&self, other: &Self) -> Result + where + T: ToAnimatedZero + ComputeSquaredDistance; +} + +macro_rules! animated_list_impl { + (<$t:ident> for $ty:ty) => { + impl<$t> ListAnimation<$t> for $ty { + fn animate_repeatable_list( + &self, + other: &Self, + procedure: Procedure, + ) -> Result + where + T: Animate, + { // If the length of either list is zero, the least common multiple is undefined. if self.is_empty() || other.is_empty() { return Err(()); @@ -777,14 +800,14 @@ macro_rules! repeated_vec_impl { this.animate(other, procedure) }).collect() } - } - impl ComputeSquaredDistance for $ty - where - T: ComputeSquaredDistance + RepeatableListAnimatable, - { - #[inline] - fn compute_squared_distance(&self, other: &Self) -> Result { + fn squared_distance_repeatable_list( + &self, + other: &Self, + ) -> Result + where + T: ComputeSquaredDistance, + { if self.is_empty() || other.is_empty() { return Err(()); } @@ -794,11 +817,59 @@ macro_rules! repeated_vec_impl { this.compute_squared_distance(other) }).sum() } - })* - }; + + fn animate_with_zero( + &self, + other: &Self, + procedure: Procedure, + ) -> Result + where + T: Animate + Clone + ToAnimatedZero + { + if procedure == Procedure::Add { + return Ok( + self.iter().chain(other.iter()).cloned().collect() + ); + } + self.iter().zip_longest(other.iter()).map(|it| { + match it { + EitherOrBoth::Both(this, other) => { + this.animate(other, procedure) + }, + EitherOrBoth::Left(this) => { + this.animate(&this.to_animated_zero()?, procedure) + }, + EitherOrBoth::Right(other) => { + other.to_animated_zero()?.animate(other, procedure) + } + } + }).collect() + } + + fn squared_distance_with_zero( + &self, + other: &Self, + ) -> Result + where + T: ToAnimatedZero + ComputeSquaredDistance + { + self.iter().zip_longest(other.iter()).map(|it| { + match it { + EitherOrBoth::Both(this, other) => { + this.compute_squared_distance(other) + }, + EitherOrBoth::Left(list) | EitherOrBoth::Right(list) => { + list.to_animated_zero()?.compute_squared_distance(list) + }, + } + }).sum() + } + } + } } -repeated_vec_impl!(SmallVec<[T; 1]>, Vec); +animated_list_impl!( for SmallVec<[T; 1]>); +animated_list_impl!( for Vec); /// impl Animate for Visibility { @@ -1027,9 +1098,6 @@ impl<'a> Iterator for FontSettingTagIter<'a> { } } -impl RepeatableListAnimatable for generic_position::Position - where H: RepeatableListAnimatable, V: RepeatableListAnimatable {} - /// impl Animate for ClipRect { #[inline] @@ -2668,27 +2736,16 @@ impl ComputeSquaredDistance for ComputedTransformOperation { impl ComputeSquaredDistance for ComputedTransform { #[inline] fn compute_squared_distance(&self, other: &Self) -> Result { - let list1 = &self.0; - let list2 = &other.0; + let squared_dist = self.0.squared_distance_with_zero(&other.0); - let squared_dist: Result = list1.iter().zip_longest(list2).map(|it| { - match it { - EitherOrBoth::Both(this, other) => { - this.compute_squared_distance(other) - }, - EitherOrBoth::Left(list) | EitherOrBoth::Right(list) => { - list.to_animated_zero()?.compute_squared_distance(list) - }, - } - }).sum(); - - // Roll back to matrix interpolation if there is any Err(()) in the transform lists, such - // as mismatched transform functions. - if let Err(_) = squared_dist { + // Roll back to matrix interpolation if there is any Err(()) in the + // transform lists, such as mismatched transform functions. + if squared_dist.is_err() { let matrix1: Matrix3D = self.to_transform_3d_matrix(None)?.0.into(); let matrix2: Matrix3D = other.to_transform_3d_matrix(None)?.0.into(); return matrix1.compute_squared_distance(&matrix2); } + squared_dist } } @@ -2828,7 +2885,7 @@ where /// impl Animate for SVGStrokeDashArray where - L: Clone + RepeatableListAnimatable, + L: Clone + Animate, { #[inline] fn animate(&self, other: &Self, procedure: Procedure) -> Result { @@ -2838,7 +2895,22 @@ where } match (self, other) { (&SVGStrokeDashArray::Values(ref this), &SVGStrokeDashArray::Values(ref other)) => { - Ok(SVGStrokeDashArray::Values(this.animate(other, procedure)?)) + Ok(SVGStrokeDashArray::Values(this.animate_repeatable_list(other, procedure)?)) + }, + _ => Err(()), + } + } +} + +impl ComputeSquaredDistance for SVGStrokeDashArray +where + L: ComputeSquaredDistance, +{ + #[inline] + fn compute_squared_distance(&self, other: &Self) -> Result { + match (self, other) { + (&SVGStrokeDashArray::Values(ref this), &SVGStrokeDashArray::Values(ref other)) => { + this.squared_distance_repeatable_list(other) }, _ => Err(()), } @@ -2929,50 +3001,6 @@ impl ToAnimatedZero for AnimatedFilter { } } -impl Animate for AnimatedFilterList { - #[inline] - fn animate( - &self, - other: &Self, - procedure: Procedure, - ) -> Result { - if procedure == Procedure::Add { - return Ok(AnimatedFilterList( - self.0.iter().chain(other.0.iter()).cloned().collect(), - )); - } - Ok(AnimatedFilterList(self.0.iter().zip_longest(other.0.iter()).map(|it| { - match it { - EitherOrBoth::Both(this, other) => { - this.animate(other, procedure) - }, - EitherOrBoth::Left(this) => { - this.animate(&this.to_animated_zero()?, procedure) - }, - EitherOrBoth::Right(other) => { - other.to_animated_zero()?.animate(other, procedure) - }, - } - }).collect::, _>>()?)) - } -} - -impl ComputeSquaredDistance for AnimatedFilterList { - #[inline] - fn compute_squared_distance(&self, other: &Self) -> Result { - self.0.iter().zip_longest(other.0.iter()).map(|it| { - match it { - EitherOrBoth::Both(this, other) => { - this.compute_squared_distance(other) - }, - EitherOrBoth::Left(list) | EitherOrBoth::Right(list) => { - list.to_animated_zero()?.compute_squared_distance(list) - }, - } - }).sum() - } -} - /// A comparator to sort PropertyIds such that longhands are sorted before shorthands, /// shorthands with fewer components are sorted before shorthands with more components, /// and otherwise shorthands are sorted by IDL name as defined by [Web Animations][property-order]. diff --git a/components/style/properties/longhand/background.mako.rs b/components/style/properties/longhand/background.mako.rs index a1f1b039434..3366376466a 100644 --- a/components/style/properties/longhand/background.mako.rs +++ b/components/style/properties/longhand/background.mako.rs @@ -36,6 +36,7 @@ ${helpers.predefined_type("background-image", "ImageLayer", spec="https://drafts.csswg.org/css-backgrounds-4/#propdef-background-position-" + axis, animation_value_type="ComputedValue", vector=True, + vector_animation_type="repeatable_list", flags="APPLIES_TO_FIRST_LETTER APPLIES_TO_FIRST_LINE APPLIES_TO_PLACEHOLDER", )} % endfor @@ -76,13 +77,15 @@ ${helpers.single_keyword("background-origin", animation_value_type="discrete", flags="APPLIES_TO_FIRST_LETTER APPLIES_TO_FIRST_LINE APPLIES_TO_PLACEHOLDER")} -${helpers.predefined_type("background-size", "BackgroundSize", +${helpers.predefined_type( + "background-size", + "BackgroundSize", initial_value="computed::BackgroundSize::auto()", initial_specified_value="specified::BackgroundSize::auto()", spec="https://drafts.csswg.org/css-backgrounds/#the-background-size", vector=True, + vector_animation_type="repeatable_list", animation_value_type="BackgroundSizeList", - need_animatable=True, flags="APPLIES_TO_FIRST_LETTER APPLIES_TO_FIRST_LINE APPLIES_TO_PLACEHOLDER", extra_prefixes="webkit")} diff --git a/components/style/properties/longhand/effects.mako.rs b/components/style/properties/longhand/effects.mako.rs index 30493bd270d..674340c391a 100644 --- a/components/style/properties/longhand/effects.mako.rs +++ b/components/style/properties/longhand/effects.mako.rs @@ -24,6 +24,7 @@ ${helpers.predefined_type( None, vector=True, animation_value_type="AnimatedBoxShadowList", + vector_animation_type="with_zero", extra_prefixes="webkit", ignored_when_colors_disabled=True, flags="APPLIES_TO_FIRST_LETTER", @@ -45,6 +46,7 @@ ${helpers.predefined_type( vector=True, separator="Space", animation_value_type="AnimatedFilterList", + vector_animation_type="with_zero", extra_prefixes="webkit", flags="CREATES_STACKING_CONTEXT FIXPOS_CB", spec="https://drafts.fxtf.org/filters/#propdef-filter", diff --git a/components/style/properties/longhand/inherited_text.mako.rs b/components/style/properties/longhand/inherited_text.mako.rs index 975d7eabe26..62819a78cbc 100644 --- a/components/style/properties/longhand/inherited_text.mako.rs +++ b/components/style/properties/longhand/inherited_text.mako.rs @@ -192,6 +192,7 @@ ${helpers.predefined_type( "SimpleShadow", None, vector=True, + vector_animation_type="with_zero", animation_value_type="AnimatedTextShadowList", ignored_when_colors_disabled=True, flags="APPLIES_TO_FIRST_LETTER APPLIES_TO_FIRST_LINE APPLIES_TO_PLACEHOLDER", diff --git a/components/style/properties/longhand/svg.mako.rs b/components/style/properties/longhand/svg.mako.rs index bea30301eb2..3236495fc26 100644 --- a/components/style/properties/longhand/svg.mako.rs +++ b/components/style/properties/longhand/svg.mako.rs @@ -103,6 +103,7 @@ ${helpers.predefined_type( initial_specified_value="specified::PositionComponent::Center", spec="https://drafts.fxtf.org/css-masking/#propdef-mask-position", animation_value_type="ComputedValue", + vector_animation_type="repeatable_list", vector=True, )} % endfor @@ -126,26 +127,18 @@ ${helpers.single_keyword("mask-origin", gecko_enum_prefix="StyleGeometryBox", animation_value_type="discrete", spec="https://drafts.fxtf.org/css-masking/#propdef-mask-origin")} - -<%helpers:longhand name="mask-size" products="gecko" animation_value_type="ComputedValue" extra_prefixes="webkit" - spec="https://drafts.fxtf.org/css-masking/#propdef-mask-size"> - use properties::longhands::background_size; - pub use ::properties::longhands::background_size::SpecifiedValue; - pub use ::properties::longhands::background_size::single_value as single_value; - pub use ::properties::longhands::background_size::computed_value as computed_value; - - #[inline] - pub fn get_initial_value() -> computed_value::T { - background_size::get_initial_value() - } - - pub fn parse<'i, 't>( - context: &ParserContext, - input: &mut Parser<'i, 't>, - ) -> Result> { - background_size::parse(context, input) - } - +${helpers.predefined_type( + "mask-size", + "background::BackgroundSize", + "computed::BackgroundSize::auto()", + initial_specified_value="specified::BackgroundSize::auto()", + products="gecko", + extra_prefixes="webkit", + spec="https://drafts.fxtf.org/css-masking/#propdef-mask-size", + animation_value_type="MaskSizeList", + vector=True, + vector_animation_type="repeatable_list", +)} ${helpers.single_keyword("mask-composite", "add subtract intersect exclude", diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index b39d93aff01..f28c04e795b 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -2216,12 +2216,13 @@ pub mod style_structs { #[allow(non_snake_case)] #[inline] pub fn set_${longhand.ident}(&mut self, v: I) - where I: IntoIterator, - I::IntoIter: ExactSizeIterator + where + I: IntoIterator, + I::IntoIter: ExactSizeIterator { self.${longhand.ident} = longhands::${longhand.ident}::computed_value - ::T(v.into_iter().collect()); + ::List(v.into_iter().collect()); } % elif longhand.ident == "display": /// Set `display`. @@ -2404,7 +2405,7 @@ pub mod style_structs { pub fn clone_${longhand.ident}( &self, ) -> longhands::${longhand.ident}::computed_value::T { - longhands::${longhand.ident}::computed_value::T( + longhands::${longhand.ident}::computed_value::List( self.${longhand.ident}_iter().collect() ) } diff --git a/components/style/properties/shorthand/mask.mako.rs b/components/style/properties/shorthand/mask.mako.rs index d38eedb5854..aa73c77e46b 100644 --- a/components/style/properties/shorthand/mask.mako.rs +++ b/components/style/properties/shorthand/mask.mako.rs @@ -188,8 +188,10 @@ use values::specified::position::Position; use parser::Parse; - pub fn parse_value<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>) - -> Result> { + pub fn parse_value<'i, 't>( + context: &ParserContext, + input: &mut Parser<'i, 't>, + ) -> Result> { // Vec grows from 0 to 4 by default on first push(). So allocate with // capacity 1, so in the common case of only one item we don't way // overallocate. Note that we always push at least one item if parsing @@ -205,7 +207,8 @@ any = true; Ok(()) })?; - if any == false { + + if !any { return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError)); } diff --git a/components/style/values/animated/effects.rs b/components/style/values/animated/effects.rs index e0f4cf7b7c0..d0463365fc2 100644 --- a/components/style/values/animated/effects.rs +++ b/components/style/values/animated/effects.rs @@ -4,13 +4,8 @@ //! Animated types for CSS values related to effects. -use properties::longhands::box_shadow::computed_value::T as ComputedBoxShadowList; -use properties::longhands::filter::computed_value::T as ComputedFilterList; -use properties::longhands::text_shadow::computed_value::T as ComputedTextShadowList; -use std::cmp; #[cfg(not(feature = "gecko"))] use values::Impossible; -use values::animated::{Animate, Procedure, ToAnimatedValue, ToAnimatedZero}; use values::animated::color::RGBA; use values::computed::{Angle, Number}; use values::computed::length::Length; @@ -21,27 +16,9 @@ use values::generics::effects::BoxShadow as GenericBoxShadow; use values::generics::effects::Filter as GenericFilter; use values::generics::effects::SimpleShadow as GenericSimpleShadow; -/// An animated value for the `box-shadow` property. -pub type BoxShadowList = ShadowList; - -/// An animated value for the `text-shadow` property. -pub type TextShadowList = ShadowList; - -/// An animated value for shadow lists. -/// -/// -#[cfg_attr(feature = "servo", derive(MallocSizeOf))] -#[derive(Clone, Debug, PartialEq)] -pub struct ShadowList(Vec); - /// An animated value for a single `box-shadow`. pub type BoxShadow = GenericBoxShadow, Length, Length, Length>; -/// An animated value for the `filter` property. -#[cfg_attr(feature = "servo", derive(MallocSizeOf))] -#[derive(Clone, Debug, PartialEq)] -pub struct FilterList(pub Vec); - /// An animated value for a single `filter`. #[cfg(feature = "gecko")] pub type Filter = GenericFilter; @@ -53,86 +30,6 @@ pub type Filter = GenericFilter; /// An animated value for the `drop-shadow()` filter. pub type SimpleShadow = GenericSimpleShadow, Length, Length>; -impl ToAnimatedValue for ComputedBoxShadowList { - type AnimatedValue = BoxShadowList; - - #[inline] - fn to_animated_value(self) -> Self::AnimatedValue { - ShadowList(self.0.to_animated_value()) - } - - #[inline] - fn from_animated_value(animated: Self::AnimatedValue) -> Self { - ComputedBoxShadowList(ToAnimatedValue::from_animated_value(animated.0)) - } -} - -impl Animate for ShadowList -where - S: Animate + Clone + ToAnimatedZero, -{ - #[inline] - fn animate(&self, other: &Self, procedure: Procedure) -> Result { - if procedure == Procedure::Add { - return Ok(ShadowList(self.0.iter().chain(&other.0).cloned().collect())); - } - // FIXME(nox): Use itertools here, to avoid the need for `unreachable!`. - let max_len = cmp::max(self.0.len(), other.0.len()); - let mut shadows = Vec::with_capacity(max_len); - for i in 0..max_len { - shadows.push(match (self.0.get(i), other.0.get(i)) { - (Some(shadow), Some(other)) => shadow.animate(other, procedure)?, - (Some(shadow), None) => shadow.animate(&shadow.to_animated_zero()?, procedure)?, - (None, Some(shadow)) => shadow.to_animated_zero()?.animate(shadow, procedure)?, - (None, None) => unreachable!(), - }); - } - Ok(ShadowList(shadows)) - } -} - -impl ComputeSquaredDistance for ShadowList -where - S: ComputeSquaredDistance + ToAnimatedZero, -{ - #[inline] - fn compute_squared_distance(&self, other: &Self) -> Result { - use itertools::{EitherOrBoth, Itertools}; - - self.0 - .iter() - .zip_longest(other.0.iter()) - .map(|it| match it { - EitherOrBoth::Both(from, to) => from.compute_squared_distance(to), - EitherOrBoth::Left(list) | EitherOrBoth::Right(list) => { - list.compute_squared_distance(&list.to_animated_zero()?) - }, - }) - .sum() - } -} - -impl ToAnimatedZero for ShadowList { - #[inline] - fn to_animated_zero(&self) -> Result { - Ok(ShadowList(vec![])) - } -} - -impl ToAnimatedValue for ComputedTextShadowList { - type AnimatedValue = TextShadowList; - - #[inline] - fn to_animated_value(self) -> Self::AnimatedValue { - ShadowList(self.0.to_animated_value()) - } - - #[inline] - fn from_animated_value(animated: Self::AnimatedValue) -> Self { - ComputedTextShadowList(ToAnimatedValue::from_animated_value(animated.0)) - } -} - impl ComputeSquaredDistance for BoxShadow { #[inline] fn compute_squared_distance(&self, other: &Self) -> Result { @@ -143,24 +40,3 @@ impl ComputeSquaredDistance for BoxShadow { self.spread.compute_squared_distance(&other.spread)?) } } - -impl ToAnimatedValue for ComputedFilterList { - type AnimatedValue = FilterList; - - #[inline] - fn to_animated_value(self) -> Self::AnimatedValue { - FilterList(self.0.to_animated_value()) - } - - #[inline] - fn from_animated_value(animated: Self::AnimatedValue) -> Self { - ComputedFilterList(ToAnimatedValue::from_animated_value(animated.0)) - } -} - -impl ToAnimatedZero for FilterList { - #[inline] - fn to_animated_zero(&self) -> Result { - Ok(FilterList(vec![])) - } -} diff --git a/components/style/values/animated/mod.rs b/components/style/values/animated/mod.rs index 533d917be6f..2bb80df0ec7 100644 --- a/components/style/values/animated/mod.rs +++ b/components/style/values/animated/mod.rs @@ -11,6 +11,7 @@ use app_units::Au; use euclid::{Point2D, Size2D}; use smallvec::SmallVec; +use values::computed::length::CalcLengthOrPercentage; use values::computed::Angle as ComputedAngle; use values::computed::BorderCornerRadius as ComputedBorderCornerRadius; use values::computed::MaxLength as ComputedMaxLength; @@ -257,6 +258,7 @@ macro_rules! trivial_to_animated_value { } trivial_to_animated_value!(Au); +trivial_to_animated_value!(CalcLengthOrPercentage); trivial_to_animated_value!(ComputedAngle); trivial_to_animated_value!(ComputedUrl); trivial_to_animated_value!(bool); diff --git a/components/style/values/computed/background.rs b/components/style/values/computed/background.rs index beca0a7ca7e..e94bece983e 100644 --- a/components/style/values/computed/background.rs +++ b/components/style/values/computed/background.rs @@ -4,85 +4,27 @@ //! Computed types for CSS values related to backgrounds. -use properties::animated_properties::RepeatableListAnimatable; -use properties::longhands::background_size::computed_value::T as BackgroundSizeList; use std::fmt::{self, Write}; use style_traits::{CssWriter, ToCss}; -use values::animated::{ToAnimatedValue, ToAnimatedZero}; use values::computed::{Context, ToComputedValue}; -use values::computed::length::LengthOrPercentageOrAuto; +use values::computed::length::NonNegativeLengthOrPercentageOrAuto; use values::generics::background::BackgroundSize as GenericBackgroundSize; use values::specified::background::BackgroundRepeat as SpecifiedBackgroundRepeat; use values::specified::background::BackgroundRepeatKeyword; /// A computed value for the `background-size` property. -pub type BackgroundSize = GenericBackgroundSize; +pub type BackgroundSize = GenericBackgroundSize; impl BackgroundSize { /// Returns `auto auto`. pub fn auto() -> Self { GenericBackgroundSize::Explicit { - width: LengthOrPercentageOrAuto::Auto, - height: LengthOrPercentageOrAuto::Auto, + width: NonNegativeLengthOrPercentageOrAuto::auto(), + height: NonNegativeLengthOrPercentageOrAuto::auto(), } } } -impl RepeatableListAnimatable for BackgroundSize {} - -impl ToAnimatedZero for BackgroundSize { - #[inline] - fn to_animated_zero(&self) -> Result { - Err(()) - } -} - -impl ToAnimatedValue for BackgroundSize { - type AnimatedValue = Self; - - #[inline] - fn to_animated_value(self) -> Self { - self - } - - #[inline] - fn from_animated_value(animated: Self::AnimatedValue) -> Self { - use values::computed::{Length, Percentage}; - let clamp_animated_value = |value: LengthOrPercentageOrAuto| -> LengthOrPercentageOrAuto { - match value { - LengthOrPercentageOrAuto::Length(len) => { - LengthOrPercentageOrAuto::Length(Length::new(len.px().max(0.))) - }, - LengthOrPercentageOrAuto::Percentage(percent) => { - LengthOrPercentageOrAuto::Percentage(Percentage(percent.0.max(0.))) - }, - _ => value, - } - }; - match animated { - GenericBackgroundSize::Explicit { width, height } => GenericBackgroundSize::Explicit { - width: clamp_animated_value(width), - height: clamp_animated_value(height), - }, - _ => animated, - } - } -} - -impl ToAnimatedValue for BackgroundSizeList { - type AnimatedValue = Self; - - #[inline] - fn to_animated_value(self) -> Self { - self - } - - #[inline] - fn from_animated_value(animated: Self::AnimatedValue) -> Self { - BackgroundSizeList(ToAnimatedValue::from_animated_value(animated.0)) - } -} - /// The computed value of the `background-repeat` property: /// /// https://drafts.csswg.org/css-backgrounds/#the-background-repeat diff --git a/components/style/values/computed/length.rs b/components/style/values/computed/length.rs index beb33553d7f..2518b8c0a3f 100644 --- a/components/style/values/computed/length.rs +++ b/components/style/values/computed/length.rs @@ -324,8 +324,8 @@ impl ToComputedValue for specified::CalcLengthOrPercentage { #[allow(missing_docs)] #[animate(fallback = "Self::animate_fallback")] #[css(derive_debug)] -#[derive(Animate, Clone, ComputeSquaredDistance, Copy, MallocSizeOf, PartialEq, ToAnimatedZero, - ToCss)] +#[derive(Animate, Clone, ComputeSquaredDistance, Copy, MallocSizeOf, PartialEq, + ToAnimatedValue, ToAnimatedZero, ToCss)] #[distance(fallback = "Self::compute_squared_distance_fallback")] pub enum LengthOrPercentage { Length(Length), diff --git a/components/style/values/computed/percentage.rs b/components/style/values/computed/percentage.rs index 653532aea6c..718d74335b7 100644 --- a/components/style/values/computed/percentage.rs +++ b/components/style/values/computed/percentage.rs @@ -14,7 +14,7 @@ use values::generics::NonNegative; #[cfg_attr(feature = "servo", derive(Deserialize, Serialize))] #[derive(Animate, Clone, ComputeSquaredDistance, Copy, Debug, Default, MallocSizeOf, PartialEq, PartialOrd, SpecifiedValueInfo, - ToAnimatedZero, ToComputedValue)] + ToAnimatedValue, ToAnimatedZero, ToComputedValue)] pub struct Percentage(pub CSSFloat); impl Percentage { diff --git a/components/style/values/generics/background.rs b/components/style/values/generics/background.rs index ad42fcfb555..b25b00514d0 100644 --- a/components/style/values/generics/background.rs +++ b/components/style/values/generics/background.rs @@ -6,7 +6,8 @@ /// A generic value for the `background-size` property. #[derive(Animate, Clone, ComputeSquaredDistance, Copy, Debug, MallocSizeOf, - PartialEq, SpecifiedValueInfo, ToComputedValue, ToCss)] + PartialEq, SpecifiedValueInfo, ToAnimatedValue, ToAnimatedZero, + ToComputedValue, ToCss)] pub enum BackgroundSize { /// ` ` Explicit { diff --git a/components/style/values/generics/svg.rs b/components/style/values/generics/svg.rs index 9246175068c..0fbaacf2283 100644 --- a/components/style/values/generics/svg.rs +++ b/components/style/values/generics/svg.rs @@ -202,14 +202,13 @@ pub enum SVGLength { } /// Generic value for stroke-dasharray. -#[derive(Clone, ComputeSquaredDistance, Debug, MallocSizeOf, PartialEq, +#[derive(Clone, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToAnimatedValue, ToComputedValue, ToCss)] pub enum SVGStrokeDashArray { /// `[ | | ]#` #[css(comma)] Values( #[css(if_empty = "none", iterable)] - #[distance(field_bound)] Vec, ), /// `context-value` diff --git a/components/style/values/specified/background.rs b/components/style/values/specified/background.rs index b9feb8b8e27..d7c4ec629ca 100644 --- a/components/style/values/specified/background.rs +++ b/components/style/values/specified/background.rs @@ -9,30 +9,25 @@ use parser::{Parse, ParserContext}; use selectors::parser::SelectorParseErrorKind; use style_traits::ParseError; use values::generics::background::BackgroundSize as GenericBackgroundSize; -use values::specified::length::LengthOrPercentageOrAuto; +use values::specified::length::NonNegativeLengthOrPercentageOrAuto; /// A specified value for the `background-size` property. -pub type BackgroundSize = GenericBackgroundSize; +pub type BackgroundSize = GenericBackgroundSize; impl Parse for BackgroundSize { fn parse<'i, 't>( context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { - if let Ok(width) = input.try(|i| LengthOrPercentageOrAuto::parse_non_negative(context, i)) { + if let Ok(width) = input.try(|i| NonNegativeLengthOrPercentageOrAuto::parse(context, i)) { let height = input - .try(|i| LengthOrPercentageOrAuto::parse_non_negative(context, i)) - .unwrap_or(LengthOrPercentageOrAuto::Auto); + .try(|i| NonNegativeLengthOrPercentageOrAuto::parse(context, i)) + .unwrap_or(NonNegativeLengthOrPercentageOrAuto::auto()); return Ok(GenericBackgroundSize::Explicit { width, height }); } - let location = input.current_source_location(); - let ident = input.expect_ident()?; - (match_ignore_ascii_case! { &ident, - "cover" => Ok(GenericBackgroundSize::Cover), - "contain" => Ok(GenericBackgroundSize::Contain), - _ => Err(()), - }).map_err(|()| { - location.new_custom_error(SelectorParseErrorKind::UnexpectedIdent(ident.clone())) + Ok(try_match_ident_ignore_ascii_case! { input, + "cover" => GenericBackgroundSize::Cover, + "contain" => GenericBackgroundSize::Contain, }) } } @@ -41,8 +36,8 @@ impl BackgroundSize { /// Returns `auto auto`. pub fn auto() -> Self { GenericBackgroundSize::Explicit { - width: LengthOrPercentageOrAuto::Auto, - height: LengthOrPercentageOrAuto::Auto, + width: NonNegativeLengthOrPercentageOrAuto::auto(), + height: NonNegativeLengthOrPercentageOrAuto::auto(), } } } From f82bf81f8e7807085eb24001fd80b121b3542ce7 Mon Sep 17 00:00:00 2001 From: Olli Pettay Date: Tue, 22 May 2018 00:11:11 +0300 Subject: [PATCH 04/24] style: Make mBindingParent strong. Bug: 1463116 Reviewed-by: bz --- components/style/gecko/wrapper.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index c00a41049c2..7a565ec469e 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -665,7 +665,7 @@ impl<'le> GeckoElement<'le> { fn non_xul_xbl_binding_parent_raw_content(&self) -> *mut nsIContent { debug_assert!(!self.is_xul_element()); self.extended_slots() - .map_or(ptr::null_mut(), |slots| slots._base.mBindingParent) + .map_or(ptr::null_mut(), |slots| slots._base.mBindingParent.raw::()) } #[inline] From 5fc6b47d74f7971e378a9b2f299283d981d76602 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 22 May 2018 10:24:03 +0200 Subject: [PATCH 05/24] followup: Add a comment regarding why a type is generic. Bug: 1462829 Rubber-stamped-by: xidorn MozReview-Commit-ID: 5DE8W2n1NP --- components/style/properties/helpers.mako.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/components/style/properties/helpers.mako.rs b/components/style/properties/helpers.mako.rs index 03b588ad654..209c06df095 100644 --- a/components/style/properties/helpers.mako.rs +++ b/components/style/properties/helpers.mako.rs @@ -119,6 +119,10 @@ use values::computed::ComputedVecIter; /// The generic type defining the value for this property. + /// + /// Making this type generic allows the compiler to figure out the + /// animated value for us, instead of having to implement it + /// manually for every type we care about. % if separator == "Comma": #[css(comma)] % endif From 6306cc7e2d75f29fec92993df41de925de85baca Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Wed, 23 May 2018 13:58:38 +1000 Subject: [PATCH 06/24] style: Ensure modified_reset is set when {mutate,take}_reset_struct is called. Bug: 1463603 Reviewed-by: heycam MozReview-Commit-ID: 8mqVb7kl0ok --- components/style/properties/properties.mako.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index f28c04e795b..ba38d9532d0 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -3199,6 +3199,7 @@ impl<'a> StyleBuilder<'a> { % endif % endif % endfor + <% del property %> /// Inherits style from the parent element, accounting for the default /// computed values that need to be provided as well. @@ -3256,7 +3257,7 @@ impl<'a> StyleBuilder<'a> { /// Gets a mutable view of the current `${style_struct.name}` style. pub fn mutate_${style_struct.name_lower}(&mut self) -> &mut style_structs::${style_struct.name} { - % if not property.style_struct.inherited: + % if not style_struct.inherited: self.modified_reset = true; % endif self.${style_struct.ident}.mutate() @@ -3264,7 +3265,7 @@ impl<'a> StyleBuilder<'a> { /// Gets a mutable view of the current `${style_struct.name}` style. pub fn take_${style_struct.name_lower}(&mut self) -> UniqueArc { - % if not property.style_struct.inherited: + % if not style_struct.inherited: self.modified_reset = true; % endif self.${style_struct.ident}.take() @@ -3288,6 +3289,7 @@ impl<'a> StyleBuilder<'a> { StyleStructRef::Borrowed(self.reset_style.${style_struct.name_lower}_arc()); } % endfor + <% del style_struct %> /// Returns whether this computed style represents a floated object. pub fn floated(&self) -> bool { From 8f367be650705b2febb31bcf6131aab9ddec0850 Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Thu, 24 May 2018 14:12:55 +1000 Subject: [PATCH 07/24] style: Don't generate catch-all branch if a image layer keyword value is exhaustive. Bug: 1463978 Reviewed-by: emilio MozReview-Commit-ID: H8T9Low6kqY --- components/style/properties/gecko.mako.rs | 2 + .../properties/longhand/background.mako.rs | 36 +++++++++------- .../style/properties/longhand/svg.mako.rs | 43 +++++++++++-------- 3 files changed, 48 insertions(+), 33 deletions(-) diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 1fc486975fd..4d14405318e 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -3805,7 +3805,9 @@ fn static_assert() { % endif => Keyword::${to_camel_case(value)}, % endfor + % if keyword.gecko_inexhaustive: _ => panic!("Found unexpected value in style struct for ${ident} property"), + % endif } }).collect() ) diff --git a/components/style/properties/longhand/background.mako.rs b/components/style/properties/longhand/background.mako.rs index 3366376466a..9bb4e03e523 100644 --- a/components/style/properties/longhand/background.mako.rs +++ b/components/style/properties/longhand/background.mako.rs @@ -60,22 +60,28 @@ ${helpers.single_keyword("background-attachment", animation_value_type="discrete", flags="APPLIES_TO_FIRST_LETTER APPLIES_TO_FIRST_LINE APPLIES_TO_PLACEHOLDER")} -${helpers.single_keyword("background-clip", - "border-box padding-box content-box", - extra_gecko_values="text", - vector=True, extra_prefixes="webkit", - gecko_enum_prefix="StyleGeometryBox", - spec="https://drafts.csswg.org/css-backgrounds/#the-background-clip", - animation_value_type="discrete", - flags="APPLIES_TO_FIRST_LETTER APPLIES_TO_FIRST_LINE APPLIES_TO_PLACEHOLDER")} +${helpers.single_keyword( + "background-clip", + "border-box padding-box content-box", + extra_gecko_values="text", + vector=True, extra_prefixes="webkit", + gecko_enum_prefix="StyleGeometryBox", + gecko_inexhaustive=True, + spec="https://drafts.csswg.org/css-backgrounds/#the-background-clip", + animation_value_type="discrete", + flags="APPLIES_TO_FIRST_LETTER APPLIES_TO_FIRST_LINE APPLIES_TO_PLACEHOLDER", +)} -${helpers.single_keyword("background-origin", - "padding-box border-box content-box", - vector=True, extra_prefixes="webkit", - gecko_enum_prefix="StyleGeometryBox", - spec="https://drafts.csswg.org/css-backgrounds/#the-background-origin", - animation_value_type="discrete", - flags="APPLIES_TO_FIRST_LETTER APPLIES_TO_FIRST_LINE APPLIES_TO_PLACEHOLDER")} +${helpers.single_keyword( + "background-origin", + "padding-box border-box content-box", + vector=True, extra_prefixes="webkit", + gecko_enum_prefix="StyleGeometryBox", + gecko_inexhaustive=True, + spec="https://drafts.csswg.org/css-backgrounds/#the-background-origin", + animation_value_type="discrete", + flags="APPLIES_TO_FIRST_LETTER APPLIES_TO_FIRST_LINE APPLIES_TO_PLACEHOLDER" +)} ${helpers.predefined_type( "background-size", diff --git a/components/style/properties/longhand/svg.mako.rs b/components/style/properties/longhand/svg.mako.rs index 3236495fc26..2ec7bcc59a1 100644 --- a/components/style/properties/longhand/svg.mako.rs +++ b/components/style/properties/longhand/svg.mako.rs @@ -108,25 +108,32 @@ ${helpers.predefined_type( )} % endfor -${helpers.single_keyword("mask-clip", - "border-box content-box padding-box", - extra_gecko_values="fill-box stroke-box view-box no-clip", - vector=True, - products="gecko", - extra_prefixes="webkit", - gecko_enum_prefix="StyleGeometryBox", - animation_value_type="discrete", - spec="https://drafts.fxtf.org/css-masking/#propdef-mask-clip")} +${helpers.single_keyword( + "mask-clip", + "border-box content-box padding-box", + extra_gecko_values="fill-box stroke-box view-box no-clip", + vector=True, + products="gecko", + extra_prefixes="webkit", + gecko_enum_prefix="StyleGeometryBox", + gecko_inexhaustive=True, + animation_value_type="discrete", + spec="https://drafts.fxtf.org/css-masking/#propdef-mask-clip", +)} + +${helpers.single_keyword( + "mask-origin", + "border-box content-box padding-box", + extra_gecko_values="fill-box stroke-box view-box", + vector=True, + products="gecko", + extra_prefixes="webkit", + gecko_enum_prefix="StyleGeometryBox", + gecko_inexhaustive=True, + animation_value_type="discrete", + spec="https://drafts.fxtf.org/css-masking/#propdef-mask-origin", +)} -${helpers.single_keyword("mask-origin", - "border-box content-box padding-box", - extra_gecko_values="fill-box stroke-box view-box", - vector=True, - products="gecko", - extra_prefixes="webkit", - gecko_enum_prefix="StyleGeometryBox", - animation_value_type="discrete", - spec="https://drafts.fxtf.org/css-masking/#propdef-mask-origin")} ${helpers.predefined_type( "mask-size", "background::BackgroundSize", From a360dbf98ddcd386a03b420bf7e3f435b7f6b5b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 25 May 2018 00:51:43 +0200 Subject: [PATCH 08/24] style: Remove dead and unsafe code. MozReview-Commit-ID: 9seuLNCJAqz --- .../style/gecko_bindings/sugar/ns_com_ptr.rs | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/components/style/gecko_bindings/sugar/ns_com_ptr.rs b/components/style/gecko_bindings/sugar/ns_com_ptr.rs index f50eda02c3d..488dc862608 100644 --- a/components/style/gecko_bindings/sugar/ns_com_ptr.rs +++ b/components/style/gecko_bindings/sugar/ns_com_ptr.rs @@ -13,13 +13,6 @@ impl nsCOMPtr { pub fn raw(&self) -> *mut T { self.mRawPtr } - - /// Set this pointer from an addrefed raw pointer. - /// It leaks the old pointer. - #[inline] - pub unsafe fn set_raw_from_addrefed(&mut self, ptr: *mut T) { - self.mRawPtr = ptr; - } } #[cfg(not(feature = "gecko_debug"))] @@ -29,11 +22,4 @@ impl nsCOMPtr { pub fn raw(&self) -> *mut T { self._base.mRawPtr as *mut _ } - - /// Set this pointer from an addrefed raw pointer. - /// It leaks the old pointer. - #[inline] - pub unsafe fn set_raw_from_addrefed(&mut self, ptr: *mut T) { - self._base.mRawPtr = ptr as *mut _; - } } From 8ad595666fc75919c3b9d286bfae2ae3ea188ff5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 25 May 2018 14:12:44 +0200 Subject: [PATCH 09/24] style: Use a static atom instead of a dynamic one. MozReview-Commit-ID: CUTwfsCrovQ --- components/style/values/computed/font.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/style/values/computed/font.rs b/components/style/values/computed/font.rs index 77a9b09d773..8db4bd7e917 100644 --- a/components/style/values/computed/font.rs +++ b/components/style/values/computed/font.rs @@ -474,7 +474,7 @@ impl SingleFontFamily { FontFamilyType::eFamily_cursive => SingleFontFamily::Generic(atom!("cursive")), FontFamilyType::eFamily_fantasy => SingleFontFamily::Generic(atom!("fantasy")), FontFamilyType::eFamily_moz_fixed => { - SingleFontFamily::Generic(Atom::from("-moz-fixed")) + SingleFontFamily::Generic(atom!("-moz-fixed")) }, FontFamilyType::eFamily_named => { let name = Atom::from(&*family.mName); From 2c1ec899437af18596ce70c42697ec3557e4ef6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 26 May 2018 16:22:52 +0200 Subject: [PATCH 10/24] style: Cleanup single transform functions that can't really error. Also call them "resolve" since it's the general term for computing something more specific than what you have. Though I don't feel strongly about that, feel free to push back. Bug: 1464595 Reviewed-by: hiro MozReview-Commit-ID: KtqjzlppZLp --- .../helpers/animated_properties.mako.rs | 74 ++++++++++--------- 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 057360c9499..d7475992152 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -2376,16 +2376,15 @@ impl Matrix3D { /// impl ComputedRotate { - fn fill_unspecified(rotate: &ComputedRotate) -> Result<(Number, Number, Number, Angle), ()> { + fn resolve(rotate: &ComputedRotate) -> (Number, Number, Number, Angle) { // According to the spec: // https://drafts.csswg.org/css-transforms-2/#individual-transforms // // If the axis is unspecified, it defaults to "0 0 1" match *rotate { - Rotate::None => - Ok((0., 0., 1., Angle::zero())), - Rotate::Rotate3D(rx, ry, rz, angle) => Ok((rx, ry, rz, angle)), - Rotate::Rotate(angle) => Ok((0., 0., 1., angle)), + Rotate::None => (0., 0., 1., Angle::zero()), + Rotate::Rotate3D(rx, ry, rz, angle) => (rx, ry, rz, angle), + Rotate::Rotate(angle) => (0., 0., 1., angle), } } } @@ -2397,11 +2396,13 @@ impl Animate for ComputedRotate { other: &Self, procedure: Procedure, ) -> Result { - let from = ComputedRotate::fill_unspecified(self)?; - let to = ComputedRotate::fill_unspecified(other)?; + let from = ComputedRotate::resolve(self); + let to = ComputedRotate::resolve(other); - let (fx, fy, fz, fa) = transform::get_normalized_vector_and_angle(from.0, from.1, from.2, from.3); - let (tx, ty, tz, ta) = transform::get_normalized_vector_and_angle(to.0, to.1, to.2, to.3); + let (fx, fy, fz, fa) = + transform::get_normalized_vector_and_angle(from.0, from.1, from.2, from.3); + let (tx, ty, tz, ta) = + transform::get_normalized_vector_and_angle(to.0, to.1, to.2, to.3); if (fx, fy, fz) == (tx, ty, tz) { return Ok(Rotate::Rotate3D(fx, fy, fz, fa.animate(&ta, procedure)?)); } @@ -2412,11 +2413,12 @@ impl Animate for ComputedRotate { let tq = Quaternion::from_direction_and_angle(&tv, ta.radians64()); let rq = Quaternion::animate(&fq, &tq, procedure)?; - let (x, y, z, angle) = - transform::get_normalized_vector_and_angle(rq.0 as f32, - rq.1 as f32, - rq.2 as f32, - rq.3.acos() as f32 *2.0); + let (x, y, z, angle) = transform::get_normalized_vector_and_angle( + rq.0 as f32, + rq.1 as f32, + rq.2 as f32, + rq.3.acos() as f32 * 2.0, + ); Ok(Rotate::Rotate3D(x, y, z, Angle::from_radians(angle))) } @@ -2424,21 +2426,24 @@ impl Animate for ComputedRotate { /// impl ComputedTranslate { - fn fill_unspecified(translate: &ComputedTranslate) - -> Result<(LengthOrPercentage, LengthOrPercentage, Length), ()> { + fn resolve( + translate: &ComputedTranslate, + ) -> (LengthOrPercentage, LengthOrPercentage, Length) { // According to the spec: // https://drafts.csswg.org/css-transforms-2/#individual-transforms // // Unspecified translations default to 0px match *translate { Translate::None => { - Ok((LengthOrPercentage::Length(Length::zero()), + ( LengthOrPercentage::Length(Length::zero()), - Length::zero())) + LengthOrPercentage::Length(Length::zero()), + Length::zero(), + ) }, - Translate::Translate3D(tx, ty, tz) => Ok((tx, ty, tz)), - Translate::Translate(tx, ty) => Ok((tx, ty, Length::zero())), - Translate::TranslateX(tx) => Ok((tx, LengthOrPercentage::Length(Length::zero()), Length::zero())), + Translate::Translate3D(tx, ty, tz) => (tx, ty, tz), + Translate::Translate(tx, ty) => (tx, ty, Length::zero()), + Translate::TranslateX(tx) => (tx, LengthOrPercentage::Length(Length::zero()), Length::zero()), } } } @@ -2450,8 +2455,8 @@ impl Animate for ComputedTranslate { other: &Self, procedure: Procedure, ) -> Result { - let from = ComputedTranslate::fill_unspecified(self)?; - let to = ComputedTranslate::fill_unspecified(other)?; + let from = ComputedTranslate::resolve(self); + let to = ComputedTranslate::resolve(other); Ok(Translate::Translate3D(from.0.animate(&to.0, procedure)?, from.1.animate(&to.1, procedure)?, @@ -2461,17 +2466,16 @@ impl Animate for ComputedTranslate { /// impl ComputedScale { - fn fill_unspecified(scale: &ComputedScale) - -> Result<(Number, Number, Number), ()> { + fn resolve(scale: &ComputedScale) -> (Number, Number, Number) { // According to the spec: // https://drafts.csswg.org/css-transforms-2/#individual-transforms // // Unspecified scales default to 1 match *scale { - Scale::None => Ok((1.0, 1.0, 1.0)), - Scale::Scale3D(sx, sy, sz) => Ok((sx, sy, sz)), - Scale::Scale(sx, sy) => Ok((sx, sy, 1.)), - Scale::ScaleX(sx) => Ok((sx, 1., 1.)), + Scale::None => (1.0, 1.0, 1.0), + Scale::Scale3D(sx, sy, sz) => (sx, sy, sz), + Scale::Scale(sx, sy) => (sx, sy, 1.), + Scale::ScaleX(sx) => (sx, 1., 1.), } } } @@ -2483,17 +2487,19 @@ impl Animate for ComputedScale { other: &Self, procedure: Procedure, ) -> Result { - let from = ComputedScale::fill_unspecified(self)?; - let to = ComputedScale::fill_unspecified(other)?; + let from = ComputedScale::resolve(self); + let to = ComputedScale::resolve(other); if procedure == Procedure::Add { // scale(x1,y1,z1)*scale(x2,y2,z2) = scale(x1*x2, y1*y2, z1*z2) return Ok(Scale::Scale3D(from.0 * to.0, from.1 * to.1, from.2 * to.2)); } - Ok(Scale::Scale3D(animate_multiplicative_factor(from.0, to.0, procedure)?, - animate_multiplicative_factor(from.1, to.1, procedure)?, - animate_multiplicative_factor(from.2, to.2, procedure)?)) + Ok(Scale::Scale3D( + animate_multiplicative_factor(from.0, to.0, procedure)?, + animate_multiplicative_factor(from.1, to.1, procedure)?, + animate_multiplicative_factor(from.2, to.2, procedure)?, + )) } } From 6b19318cc881fcdb4f47290b34381d10c7e2d264 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 24 May 2018 15:03:17 +0200 Subject: [PATCH 11/24] style: Remove a bunch of unused style bits. Bug: 1464060 MozReview-Commit-ID: FLACqfSHfSO --- components/style/properties/computed_value_flags.rs | 5 ----- components/style/properties/properties.mako.rs | 7 ------- components/style/style_adjuster.rs | 6 ------ 3 files changed, 18 deletions(-) diff --git a/components/style/properties/computed_value_flags.rs b/components/style/properties/computed_value_flags.rs index bddb451861e..4ddc9a1f42a 100644 --- a/components/style/properties/computed_value_flags.rs +++ b/components/style/properties/computed_value_flags.rs @@ -41,10 +41,6 @@ bitflags! { /// A flag used to mark styles which are a pseudo-element or under one. const IS_IN_PSEUDO_ELEMENT_SUBTREE = 1 << 4; - /// A flag used to mark styles which are in a display: none subtree, or - /// under one. - const IS_IN_DISPLAY_NONE_SUBTREE = 1 << 5; - /// Whether this style inherits the `display` property. /// /// This is important because it may affect our optimizations to avoid @@ -77,7 +73,6 @@ impl ComputedValueFlags { ComputedValueFlags::IS_STYLE_IF_VISITED | ComputedValueFlags::IS_RELEVANT_LINK_VISITED | ComputedValueFlags::CAN_BE_FRAGMENTED | - ComputedValueFlags::IS_IN_DISPLAY_NONE_SUBTREE | ComputedValueFlags::IS_IN_PSEUDO_ELEMENT_SUBTREE | ComputedValueFlags::HAS_TEXT_DECORATION_LINES } diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index ba38d9532d0..81e9a09cbe8 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -2537,13 +2537,6 @@ impl ComputedValues { self.visited_style.as_ref().and_then(|s| s.rules.as_ref()) } - /// Returns whether we're in a display: none subtree. - pub fn is_in_display_none_subtree(&self) -> bool { - use properties::computed_value_flags::ComputedValueFlags; - - self.flags.contains(ComputedValueFlags::IS_IN_DISPLAY_NONE_SUBTREE) - } - /// Gets a reference to the custom properties map (if one exists). pub fn custom_properties(&self) -> Option<<&Arc<::custom_properties::CustomPropertiesMap>> { self.custom_properties.as_ref() diff --git a/components/style/style_adjuster.rs b/components/style/style_adjuster.rs index 4fcbffc5345..2bb400f63ec 100644 --- a/components/style/style_adjuster.rs +++ b/components/style/style_adjuster.rs @@ -204,12 +204,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> { .insert(ComputedValueFlags::HAS_TEXT_DECORATION_LINES); } - if display == Display::None { - self.style - .flags - .insert(ComputedValueFlags::IS_IN_DISPLAY_NONE_SUBTREE); - } - if self.style.is_pseudo_element() { self.style .flags From 01f805af9f8421ddae7d67520799747cad686cbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 24 May 2018 17:49:24 +0200 Subject: [PATCH 12/24] style: Make the general setup for computed style bits nicer. This patch: * Makes StyleStructID an enum class, and moves it to the mozilla namespaces. * Introduces StyleStructConstants with some constants scattered through the codebase. * Makes the computed style bits an enum class, and splits mPseudoType and mBits into their own members, since we were using a uint64_t when we have only a couple flags and CSSPseudoElementType is a byte. We statically assert that the number of style structs is less or equal to 32. * Makes mPseudoTag, mPseudoType and mBits const, since we don't want them to be mutated from C++, and we still need a few more refactorings (mostly getting rid of FinishStyle) to avoid mutating ComputedStyle instead. Bug: 1464060 Reviewed-by: xidorn MozReview-Commit-ID: 7qsTtASGcYB --- components/style/properties/gecko.mako.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 4d14405318e..baa1ed015ee 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -41,7 +41,6 @@ use gecko_bindings::structs; use gecko_bindings::structs::nsCSSPropertyID; use gecko_bindings::structs::mozilla::CSSPseudoElementType; use gecko_bindings::structs::mozilla::CSSPseudoElementType_InheritingAnonBox; -use gecko_bindings::structs::root::NS_STYLE_CONTEXT_TYPE_SHIFT; use gecko_bindings::sugar::ns_style_coord::{CoordDataValue, CoordData, CoordDataMut}; use gecko_bindings::sugar::refptr::RefPtr; use gecko::values::convert_nscolor_to_rgba; @@ -137,12 +136,12 @@ impl ComputedValues { PseudoElement::from_atom(&atom) } + #[inline] fn get_pseudo_type(&self) -> CSSPseudoElementType { - let bits = (self.0).mBits; - let our_type = bits >> NS_STYLE_CONTEXT_TYPE_SHIFT; - unsafe { transmute(our_type as u8) } + self.0.mPseudoType } + #[inline] pub fn is_anon_box(&self) -> bool { let our_type = self.get_pseudo_type(); return our_type == CSSPseudoElementType_InheritingAnonBox || @@ -2985,7 +2984,6 @@ fn static_assert() { I::IntoIter: ExactSizeIterator + Clone { use properties::longhands::animation_${ident}::single_value::computed_value::T as Keyword; - use gecko_bindings::structs; let v = v.into_iter(); From 2e500d70efc15d69448df06ba39ae16f94cff894 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 25 May 2018 20:11:37 +0200 Subject: [PATCH 13/24] style: Match rotate ops. Per bug 1322189 we really should. I've copied the setup we have already for translate / scale, but we should really clean this up a bit more I'd think. In any case, probably skew should be matched as well... Bug: 1464615 Reviewed-by: hiro MozReview-Commit-ID: Jky5k8HVfuH --- .../helpers/animated_properties.mako.rs | 13 +++++++++++- components/style/values/computed/transform.rs | 21 +++++++++++++++++++ components/style/values/generics/transform.rs | 13 ++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index d7475992152..4a88e7a52cf 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -1289,7 +1289,11 @@ impl Animate for ComputedTransformOperation { ) => { Ok(TransformOperation::Scale( animate_multiplicative_factor(*fx, *tx, procedure)?, - Some(animate_multiplicative_factor(fy.unwrap_or(*fx), ty.unwrap_or(*tx), procedure)?), + Some(animate_multiplicative_factor( + fy.unwrap_or(*fx), + ty.unwrap_or(*tx), + procedure + )?), )) }, ( @@ -1371,6 +1375,9 @@ impl Animate for ComputedTransformOperation { _ if self.is_scale() && other.is_scale() => { self.to_scale_3d().animate(&other.to_scale_3d(), procedure) } + _ if self.is_rotate() && other.is_rotate() => { + self.to_rotate_3d().animate(&other.to_rotate_3d(), procedure) + } _ => Err(()), } } @@ -1403,6 +1410,7 @@ fn is_matched_operation(first: &ComputedTransformOperation, second: &ComputedTra // we animate scale and translate operations against each other (a, b) if a.is_translate() && b.is_translate() => true, (a, b) if a.is_scale() && b.is_scale() => true, + (a, b) if a.is_rotate() && b.is_rotate() => true, // InterpolateMatrix and AccumulateMatrix are for mismatched transform. _ => false } @@ -2734,6 +2742,9 @@ impl ComputeSquaredDistance for ComputedTransformOperation { _ if self.is_scale() && other.is_scale() => { self.to_scale_3d().compute_squared_distance(&other.to_scale_3d()) } + _ if self.is_rotate() && other.is_rotate() => { + self.to_rotate_3d().compute_squared_distance(&other.to_rotate_3d()) + } _ => Err(()), } } diff --git a/components/style/values/computed/transform.rs b/components/style/values/computed/transform.rs index fd7a7cf5c2d..bbf710c4295 100644 --- a/components/style/values/computed/transform.rs +++ b/components/style/values/computed/transform.rs @@ -151,6 +151,27 @@ impl TransformOperation { _ => unreachable!(), } } + + /// Convert to a Rotate3D. + /// + /// Must be called on a Rotate function. + pub fn to_rotate_3d(&self) -> Self { + match *self { + generic::TransformOperation::Rotate3D(..) => self.clone(), + generic::TransformOperation::RotateZ(ref angle) | + generic::TransformOperation::Rotate(ref angle) => { + generic::TransformOperation::Rotate3D(0., 0., 1., angle.clone()) + } + generic::TransformOperation::RotateX(ref angle) => { + generic::TransformOperation::Rotate3D(1., 0., 0., angle.clone()) + } + generic::TransformOperation::RotateY(ref angle) => { + generic::TransformOperation::Rotate3D(0., 1., 0., angle.clone()) + } + _ => unreachable!(), + } + } + /// Convert to a Scale3D. /// /// Must be called on a Scale function diff --git a/components/style/values/generics/transform.rs b/components/style/values/generics/transform.rs index ecd40f5a55c..0b980bdf3e1 100644 --- a/components/style/values/generics/transform.rs +++ b/components/style/values/generics/transform.rs @@ -276,6 +276,19 @@ pub struct Transform(#[css(if_empty = "none", iterable)] pub Vec); impl TransformOperation { + /// Check if it is any rotate function. + pub fn is_rotate(&self) -> bool { + use self::TransformOperation::*; + matches!( + *self, + Rotate(..) | + Rotate3D(..) | + RotateX(..) | + RotateY(..) | + RotateZ(..) + ) + } + /// Check if it is any translate function pub fn is_translate(&self) -> bool { use self::TransformOperation::*; From 92b856b70a45c4cbc32e84e279da8b4b78e68890 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 26 May 2018 16:23:36 +0200 Subject: [PATCH 14/24] style: Reuse computed rotate animation. I have the feeling this fixes a bug, but... Bug: 1464615 Reviewed-by: hiro MozReview-Commit-ID: Aj478qCbMV9 --- .../helpers/animated_properties.mako.rs | 45 ++----------------- 1 file changed, 4 insertions(+), 41 deletions(-) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 4a88e7a52cf..3bdd1264a0b 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -1300,18 +1300,10 @@ impl Animate for ComputedTransformOperation { &TransformOperation::Rotate3D(fx, fy, fz, fa), &TransformOperation::Rotate3D(tx, ty, tz, ta), ) => { - let (fx, fy, fz, fa) = transform::get_normalized_vector_and_angle(fx, fy, fz, fa); - let (tx, ty, tz, ta) = transform::get_normalized_vector_and_angle(tx, ty, tz, ta); - if (fx, fy, fz) == (tx, ty, tz) { - let ia = fa.animate(&ta, procedure)?; - Ok(TransformOperation::Rotate3D(fx, fy, fz, ia)) - } else { - let matrix_f = rotate_to_matrix(fx, fy, fz, fa); - let matrix_t = rotate_to_matrix(tx, ty, tz, ta); - Ok(TransformOperation::Matrix3D( - matrix_f.animate(&matrix_t, procedure)?, - )) - } + let animated = Rotate::Rotate3D(fx, fy, fz, fa) + .animate(&Rotate::Rotate3D(tx, ty, tz, ta), procedure)?; + let (fx, fy, fz, fa) = ComputedRotate::resolve(&animated); + Ok(TransformOperation::Rotate3D(fx, fy, fz, fa)) }, ( &TransformOperation::RotateX(fa), @@ -1416,35 +1408,6 @@ fn is_matched_operation(first: &ComputedTransformOperation, second: &ComputedTra } } -/// -fn rotate_to_matrix(x: f32, y: f32, z: f32, a: Angle) -> Matrix3D { - let half_rad = a.radians() / 2.0; - let sc = (half_rad).sin() * (half_rad).cos(); - let sq = (half_rad).sin().powi(2); - - Matrix3D { - m11: 1.0 - 2.0 * (y * y + z * z) * sq, - m12: 2.0 * (x * y * sq + z * sc), - m13: 2.0 * (x * z * sq - y * sc), - m14: 0.0, - - m21: 2.0 * (x * y * sq - z * sc), - m22: 1.0 - 2.0 * (x * x + z * z) * sq, - m23: 2.0 * (y * z * sq + x * sc), - m24: 0.0, - - m31: 2.0 * (x * z * sq + y * sc), - m32: 2.0 * (y * z * sq - x * sc), - m33: 1.0 - 2.0 * (x * x + y * y) * sq, - m34: 0.0, - - m41: 0.0, - m42: 0.0, - m43: 0.0, - m44: 1.0 - } -} - /// A 2d matrix for interpolation. #[derive(Clone, ComputeSquaredDistance, Copy, Debug)] #[cfg_attr(feature = "servo", derive(MallocSizeOf))] From 99d4e7b65c0ec47bd99015c40939c9db0969d3de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 26 May 2018 23:35:52 +0200 Subject: [PATCH 15/24] style: Trivial cleanup. Bug: 1459403 Reviewed-by: hiro MozReview-Commit-ID: K7zVYGiYAn6 --- .../style/properties/helpers/animated_properties.mako.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 3bdd1264a0b..42c531bac83 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -1503,10 +1503,10 @@ impl Animate for MatrixDecomposed2D { let matrix = self.matrix.animate(&other.matrix, procedure)?; Ok(MatrixDecomposed2D { - translate: translate, - scale: scale, - angle: angle, - matrix: matrix, + translate, + scale, + angle, + matrix, }) } } From 6ddb04a483c150248da12bef909d99b912531f5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 26 May 2018 23:40:45 +0200 Subject: [PATCH 16/24] style: Cleanup and add references to Quaternion::animate. No functional change, but I did this while searching for the bug. Bug: 1459403 Reviewed-by: hiro MozReview-Commit-ID: KsJxFoYaOq1 --- .../helpers/animated_properties.mako.rs | 57 +++++++++++-------- 1 file changed, 34 insertions(+), 23 deletions(-) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 42c531bac83..8001ecf3fe1 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -1820,12 +1820,16 @@ impl Animate for Quaternion { use std::f64; let (this_weight, other_weight) = procedure.weights(); - debug_assert!((this_weight + other_weight - 1.0f64).abs() <= f64::EPSILON || - other_weight == 1.0f64 || other_weight == 0.0f64, - "animate should only be used for interpolating or accumulating transforms"); + debug_assert!( + (this_weight + other_weight - 1.0f64).abs() <= f64::EPSILON || + other_weight == 1.0f64 || other_weight == 0.0f64, + "animate should only be used for interpolating or accumulating transforms" + ); - // We take a specialized code path for accumulation (where other_weight is 1) - if other_weight == 1.0 { + // We take a specialized code path for accumulation (where other_weight + // is 1). + if let Procedure::Accumulate { .. } = procedure { + debug_assert_eq!(other_weight, 1.0); if this_weight == 0.0 { return Ok(*other); } @@ -1856,32 +1860,39 @@ impl Animate for Quaternion { )); } - let mut product = self.0 * other.0 + - self.1 * other.1 + - self.2 * other.2 + - self.3 * other.3; + // Straight from gfxQuaternion::Slerp. + // + // Dot product, clamped between -1 and 1. + let dot = + (self.0 * other.0 + + self.1 * other.1 + + self.2 * other.2 + + self.3 * other.3) + .min(1.0).max(-1.0); - // Clamp product to -1.0 <= product <= 1.0 - product = product.min(1.0); - product = product.max(-1.0); - - if product == 1.0 { + if dot == 1.0 { return Ok(*self); } - let theta = product.acos(); - let w = (other_weight * theta).sin() * 1.0 / (1.0 - product * product).sqrt(); + let theta = dot.acos(); + let rsintheta = 1.0 / (1.0 - dot * dot).sqrt(); - let mut a = *self; - let mut b = *other; - let mut result = Quaternion(0., 0., 0., 0.,); + let right_weight = (other_weight * theta).sin() * rsintheta; + let left_weight = (other_weight * theta).cos() - dot * right_weight; + + let mut left = *self; + let mut right = *other; % for i in range(4): - a.${i} *= (other_weight * theta).cos() - product * w; - b.${i} *= w; - result.${i} = a.${i} + b.${i}; + left.${i} *= left_weight; + right.${i} *= right_weight; % endfor - Ok(result) + Ok(Quaternion( + left.0 + right.0, + left.1 + right.1, + left.2 + right.2, + left.3 + right.3, + )) } } From 875c4ff621e5ae4042f7cadbb55771aa3348742c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 26 May 2018 23:42:07 +0200 Subject: [PATCH 17/24] style: Cleanup and add references to decompose_3d_matrix. Same, no functional change, but I basically rewrote this two times so... Bug: 1459403 Reviewed-by: hiro MozReview-Commit-ID: FXOnJDPyPu5 --- .../helpers/animated_properties.mako.rs | 92 ++++++++++--------- 1 file changed, 49 insertions(+), 43 deletions(-) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 8001ecf3fe1..b20889f0db6 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -1908,7 +1908,8 @@ impl ComputeSquaredDistance for Quaternion { } /// Decompose a 3D matrix. -/// +/// https://drafts.csswg.org/css-transforms-2/#decomposing-a-3d-matrix +/// http://www.realtimerendering.com/resources/GraphicsGems/gemsii/unmatrix.c fn decompose_3d_matrix(mut matrix: Matrix3D) -> Result { // Normalize the matrix. if matrix.m44 == 0.0 { @@ -1916,6 +1917,8 @@ fn decompose_3d_matrix(mut matrix: Matrix3D) -> Result { } let scaling_factor = matrix.m44; + + // Normalize the matrix. % for i in range(1, 5): % for j in range(1, 5): matrix.m${i}${j} /= scaling_factor; @@ -1926,9 +1929,9 @@ fn decompose_3d_matrix(mut matrix: Matrix3D) -> Result { // an easy way to test for singularity of the upper 3x3 component. let mut perspective_matrix = matrix; - % for i in range(1, 4): - perspective_matrix.m${i}4 = 0.0; - % endfor + perspective_matrix.m14 = 0.0; + perspective_matrix.m24 = 0.0; + perspective_matrix.m34 = 0.0; perspective_matrix.m44 = 1.0; if perspective_matrix.determinant() == 0.0 { @@ -1944,37 +1947,18 @@ fn decompose_3d_matrix(mut matrix: Matrix3D) -> Result { matrix.m44 ]; - perspective_matrix = perspective_matrix.inverse().unwrap(); - - // Transpose perspective_matrix - perspective_matrix = Matrix3D { - % for i in range(1, 5): - % for j in range(1, 5): - m${i}${j}: perspective_matrix.m${j}${i}, - % endfor - % endfor - }; - - // Multiply right_hand_side with perspective_matrix - let mut tmp: [f32; 4] = [0.0; 4]; - % for i in range(1, 5): - tmp[${i - 1}] = (right_hand_side[0] * perspective_matrix.m1${i}) + - (right_hand_side[1] * perspective_matrix.m2${i}) + - (right_hand_side[2] * perspective_matrix.m3${i}) + - (right_hand_side[3] * perspective_matrix.m4${i}); - % endfor - - Perspective(tmp[0], tmp[1], tmp[2], tmp[3]) + perspective_matrix = perspective_matrix.inverse().unwrap().transpose(); + let perspective = perspective_matrix.pre_mul_point4(&right_hand_side); + // NOTE(emilio): Even though the reference algorithm clears the + // fourth column here (matrix.m14..matrix.m44), they're not used below + // so it's not really needed. + Perspective(perspective[0], perspective[1], perspective[2], perspective[3]) } else { Perspective(0.0, 0.0, 0.0, 1.0) }; - // Next take care of translation - let translate = Translate3D ( - matrix.m41, - matrix.m42, - matrix.m43 - ); + // Next take care of translation (easy). + let translate = Translate3D(matrix.m41, matrix.m42, matrix.m43); // Now get scale and shear. 'row' is a 3 element array of 3 component vectors let mut row: [[f32; 3]; 3] = [[0.0; 3]; 3]; @@ -2015,8 +1999,7 @@ fn decompose_3d_matrix(mut matrix: Matrix3D) -> Result { // At this point, the matrix (in rows) is orthonormal. // Check for a coordinate system flip. If the determinant // is -1, then negate the matrix and the scaling factors. - let pdum3 = cross(row[1], row[2]); - if dot(row[0], pdum3) < 0.0 { + if dot(row[0], cross(row[1], row[2])) < 0.0 { % for i in range(3): scale.${i} *= -1.0; row[${i}][0] *= -1.0; @@ -2025,8 +2008,8 @@ fn decompose_3d_matrix(mut matrix: Matrix3D) -> Result { % endfor } - // Now, get the rotations out - let mut quaternion = Quaternion ( + // Now, get the rotations out. + let mut quaternion = Quaternion( 0.5 * ((1.0 + row[0][0] - row[1][1] - row[2][2]).max(0.0) as f64).sqrt(), 0.5 * ((1.0 - row[0][0] + row[1][1] - row[2][2]).max(0.0) as f64).sqrt(), 0.5 * ((1.0 - row[0][0] - row[1][1] + row[2][2]).max(0.0) as f64).sqrt(), @@ -2044,11 +2027,11 @@ fn decompose_3d_matrix(mut matrix: Matrix3D) -> Result { } Ok(MatrixDecomposed3D { - translate: translate, - scale: scale, - skew: skew, - perspective: perspective, - quaternion: quaternion + translate, + scale, + skew, + perspective, + quaternion, }) } @@ -2277,11 +2260,22 @@ impl Matrix3D { self.m11 * self.m22 * self.m33 * self.m44 } - fn inverse(&self) -> Option { + /// Transpose a matrix. + fn transpose(&self) -> Self { + Self { + % for i in range(1, 5): + % for j in range(1, 5): + m${i}${j}: self.m${j}${i}, + % endfor + % endfor + } + } + + fn inverse(&self) -> Result { let mut det = self.determinant(); if det == 0.0 { - return None; + return Err(()); } det = 1.0 / det; @@ -2352,7 +2346,19 @@ impl Matrix3D { self.m12*self.m21*self.m33 + self.m11*self.m22*self.m33), }; - Some(x) + Ok(x) + } + + /// Multiplies `pin * self`. + fn pre_mul_point4(&self, pin: &[f32; 4]) -> [f32; 4] { + [ + % for i in range(1, 5): + pin[0] * self.m1${i} + + pin[1] * self.m2${i} + + pin[2] * self.m3${i} + + pin[3] * self.m4${i}, + % endfor + ] } } From 4c51624bcfa6921e46b6ccf728b4306ffa0afc5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 26 May 2018 23:44:50 +0200 Subject: [PATCH 18/24] style: Add a FIXME comment which I think reveals a bug but I haven't confirmed it. Bug: 1459403 Reviewed-by: hiro MozReview-Commit-ID: DgCfqA5TNzP --- components/style/properties/helpers/animated_properties.mako.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index b20889f0db6..3178a14add6 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -2478,6 +2478,8 @@ impl Animate for ComputedScale { let from = ComputedScale::resolve(self); let to = ComputedScale::resolve(other); + // FIXME(emilio, bug 1464791): why does this do something different than + // Scale3D / TransformOperation::Scale3D? if procedure == Procedure::Add { // scale(x1,y1,z1)*scale(x2,y2,z2) = scale(x1*x2, y1*y2, z1*z2) return Ok(Scale::Scale3D(from.0 * to.0, from.1 * to.1, from.2 * to.2)); From 66b8bd2829b1aec70c41a082a99a4d18ff0c45dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 26 May 2018 23:46:14 +0200 Subject: [PATCH 19/24] style: Cleanup multiply(). We assign all the members, the result matrix doesn't really need to be any clone of a. Bug: 1459403 Reviewed-by: hiro MozReview-Commit-ID: 3NkhvyfqQL --- .../helpers/animated_properties.mako.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 3178a14add6..1071671968a 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -2213,16 +2213,17 @@ impl From for Matrix3D { // Multiplication of two 4x4 matrices. fn multiply(a: Matrix3D, b: Matrix3D) -> Matrix3D { - let mut a_clone = a; + Matrix3D { % for i in range(1, 5): - % for j in range(1, 5): - a_clone.m${i}${j} = (a.m${i}1 * b.m1${j}) + - (a.m${i}2 * b.m2${j}) + - (a.m${i}3 * b.m3${j}) + - (a.m${i}4 * b.m4${j}); - % endfor + % for j in range(1, 5): + m${i}${j}: + a.m${i}1 * b.m1${j} + + a.m${i}2 * b.m2${j} + + a.m${i}3 * b.m3${j} + + a.m${i}4 * b.m4${j}, % endfor - a_clone + % endfor + } } impl Matrix3D { From b199ca864ac44c6fc131bb4fb43a3d8ce4626962 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 26 May 2018 23:48:55 +0200 Subject: [PATCH 20/24] style: Move skew and rotation application to their own scope for clarity. The skew resetting of temp I think fixes a bug in presence of skew in every direction, but again haven't double-checked. Bug: 1459403 Reviewed-by: hiro MozReview-Commit-ID: Bn93CoaG8Bu --- .../helpers/animated_properties.mako.rs | 66 ++++++++++--------- 1 file changed, 35 insertions(+), 31 deletions(-) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 1071671968a..673ba86f8b3 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -2161,43 +2161,47 @@ impl From for Matrix3D { % endfor // Apply rotation - let x = decomposed.quaternion.0; - let y = decomposed.quaternion.1; - let z = decomposed.quaternion.2; - let w = decomposed.quaternion.3; + { + let x = decomposed.quaternion.0; + let y = decomposed.quaternion.1; + let z = decomposed.quaternion.2; + let w = decomposed.quaternion.3; - // Construct a composite rotation matrix from the quaternion values - // rotationMatrix is a identity 4x4 matrix initially - let mut rotation_matrix = Matrix3D::identity(); - rotation_matrix.m11 = 1.0 - 2.0 * (y * y + z * z) as f32; - rotation_matrix.m12 = 2.0 * (x * y + z * w) as f32; - rotation_matrix.m13 = 2.0 * (x * z - y * w) as f32; - rotation_matrix.m21 = 2.0 * (x * y - z * w) as f32; - rotation_matrix.m22 = 1.0 - 2.0 * (x * x + z * z) as f32; - rotation_matrix.m23 = 2.0 * (y * z + x * w) as f32; - rotation_matrix.m31 = 2.0 * (x * z + y * w) as f32; - rotation_matrix.m32 = 2.0 * (y * z - x * w) as f32; - rotation_matrix.m33 = 1.0 - 2.0 * (x * x + y * y) as f32; + // Construct a composite rotation matrix from the quaternion values + // rotationMatrix is a identity 4x4 matrix initially + let mut rotation_matrix = Matrix3D::identity(); + rotation_matrix.m11 = 1.0 - 2.0 * (y * y + z * z) as f32; + rotation_matrix.m12 = 2.0 * (x * y + z * w) as f32; + rotation_matrix.m13 = 2.0 * (x * z - y * w) as f32; + rotation_matrix.m21 = 2.0 * (x * y - z * w) as f32; + rotation_matrix.m22 = 1.0 - 2.0 * (x * x + z * z) as f32; + rotation_matrix.m23 = 2.0 * (y * z + x * w) as f32; + rotation_matrix.m31 = 2.0 * (x * z + y * w) as f32; + rotation_matrix.m32 = 2.0 * (y * z - x * w) as f32; + rotation_matrix.m33 = 1.0 - 2.0 * (x * x + y * y) as f32; - matrix = multiply(rotation_matrix, matrix); + matrix = multiply(rotation_matrix, matrix); + } // Apply skew - let mut temp = Matrix3D::identity(); - if decomposed.skew.2 != 0.0 { - temp.m32 = decomposed.skew.2; - matrix = multiply(temp, matrix); - } + { + let mut temp = Matrix3D::identity(); + if decomposed.skew.2 != 0.0 { + temp.m32 = decomposed.skew.2; + matrix = multiply(temp, matrix); + temp.m32 = 0.0; + } - if decomposed.skew.1 != 0.0 { - temp.m32 = 0.0; - temp.m31 = decomposed.skew.1; - matrix = multiply(temp, matrix); - } + if decomposed.skew.1 != 0.0 { + temp.m31 = decomposed.skew.1; + matrix = multiply(temp, matrix); + temp.m31 = 0.0; + } - if decomposed.skew.0 != 0.0 { - temp.m31 = 0.0; - temp.m21 = decomposed.skew.0; - matrix = multiply(temp, matrix); + if decomposed.skew.0 != 0.0 { + temp.m21 = decomposed.skew.0; + matrix = multiply(temp, matrix); + } } // Apply scale From 82e97b8ec5d5323dd6ff001065f54edc9fb94bb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 26 May 2018 23:50:11 +0200 Subject: [PATCH 21/24] style: The fix: scale and translate appropriately. Loops are hard. This hasn't been the funniest Saturday evening. Bug: 1459403 Reviewed-by: hiro MozReview-Commit-ID: I8rfuIRIsrY --- .../style/properties/helpers/animated_properties.mako.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index 673ba86f8b3..1c282e46b51 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -2154,7 +2154,7 @@ impl From for Matrix3D { % endfor // Apply translation - % for i in range(1, 4): + % for i in range(1, 5): % for j in range(1, 4): matrix.m4${i} += decomposed.translate.${j - 1} * matrix.m${j}${i}; % endfor @@ -2206,9 +2206,9 @@ impl From for Matrix3D { // Apply scale % for i in range(1, 4): - % for j in range(1, 4): - matrix.m${i}${j} *= decomposed.scale.${i - 1}; - % endfor + % for j in range(1, 5): + matrix.m${i}${j} *= decomposed.scale.${i - 1}; + % endfor % endfor matrix From 38cbada278ae655a006cfdd7aa5ab3ad63a38e10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 25 May 2018 16:56:41 +0200 Subject: [PATCH 22/24] style: Optimize QuerySelector in shadow trees. Pretty much the same setup we have for document. We have the awkwardness of having to check containing shadow manually for ShadowRoot because it's not available in TNode (and making it available added a bit more complexity that wasn't worth it IMO). Bug: 1464428 Reviewed-by: xidorn MozReview-Commit-ID: CqOh0sLHf6o --- components/style/dom.rs | 24 ++++++++++-- components/style/dom_apis.rs | 62 ++++++++++++++++++++----------- components/style/gecko/wrapper.rs | 59 ++++++++++++++++++++--------- 3 files changed, 103 insertions(+), 42 deletions(-) diff --git a/components/style/dom.rs b/components/style/dom.rs index bee4d88329e..0e5da1542d7 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -135,14 +135,17 @@ pub trait TDocument: Sized + Copy + Clone { fn quirks_mode(&self) -> QuirksMode; /// Get a list of elements with a given ID in this document, sorted by - /// document position. + /// tree position. /// /// Can return an error to signal that this list is not available, or also /// return an empty slice. - fn elements_with_id( + fn elements_with_id<'a>( &self, _id: &Atom, - ) -> Result<&[::ConcreteElement], ()> { + ) -> Result<&'a [::ConcreteElement], ()> + where + Self: 'a, + { Err(()) } } @@ -342,6 +345,21 @@ pub trait TShadowRoot: Sized + Copy + Clone + PartialEq { fn style_data<'a>(&self) -> &'a CascadeData where Self: 'a; + + /// Get a list of elements with a given ID in this shadow root, sorted by + /// tree position. + /// + /// Can return an error to signal that this list is not available, or also + /// return an empty slice. + fn elements_with_id<'a>( + &self, + _id: &Atom, + ) -> Result<&'a [::ConcreteElement], ()> + where + Self: 'a, + { + Err(()) + } } /// The element trait, the main abstraction the style crate acts over. diff --git a/components/style/dom_apis.rs b/components/style/dom_apis.rs index 2da4b476c93..393fb6e119e 100644 --- a/components/style/dom_apis.rs +++ b/components/style/dom_apis.rs @@ -221,14 +221,31 @@ where } } -/// Returns whether a given element is descendant of a given `root` node. +/// Returns whether a given element connected to `root` is descendant of `root`. /// /// NOTE(emilio): if root == element, this returns false. -fn element_is_descendant_of(element: E, root: E::ConcreteNode) -> bool +fn connected_element_is_descendant_of(element: E, root: E::ConcreteNode) -> bool where E: TElement, { - if element.as_node().is_in_document() && root == root.owner_doc().as_node() { + // Optimize for when the root is a document or a shadow root and the element + // is connected to that root. + if root.as_document().is_some() { + debug_assert!(element.as_node().is_in_document(), "Not connected?"); + debug_assert_eq!( + root, + root.owner_doc().as_node(), + "Where did this element come from?", + ); + return true; + } + + if root.as_shadow_root().is_some() { + debug_assert_eq!( + element.containing_shadow().unwrap().as_node(), + root, + "Not connected?" + ); return true; } @@ -244,28 +261,33 @@ where } /// Fast path for iterating over every element with a given id in the document -/// that `root` is connected to. -fn fast_connected_elements_with_id<'a, D>( - doc: &'a D, - root: D::ConcreteNode, +/// or shadow root that `root` is connected to. +fn fast_connected_elements_with_id<'a, N>( + root: N, id: &Atom, quirks_mode: QuirksMode, -) -> Result<&'a [::ConcreteElement], ()> +) -> Result<&'a [N::ConcreteElement], ()> where - D: TDocument, + N: TNode + 'a, { - debug_assert_eq!(root.owner_doc().as_node(), doc.as_node()); - let case_sensitivity = quirks_mode.classes_and_ids_case_sensitivity(); if case_sensitivity != CaseSensitivity::CaseSensitive { return Err(()); } - if !root.is_in_document() { - return Err(()); + if root.is_in_document() { + return root.owner_doc().elements_with_id(id); } - doc.elements_with_id(id) + if let Some(shadow) = root.as_shadow_root() { + return shadow.elements_with_id(id); + } + + if let Some(shadow) = root.as_element().and_then(|e| e.containing_shadow()) { + return shadow.elements_with_id(id); + } + + Err(()) } /// Collects elements with a given id under `root`, that pass `filter`. @@ -280,8 +302,7 @@ fn collect_elements_with_id( Q: SelectorQuery, F: FnMut(E) -> bool, { - let doc = root.owner_doc(); - let elements = match fast_connected_elements_with_id(&doc, root, id, quirks_mode) { + let elements = match fast_connected_elements_with_id(root, id, quirks_mode) { Ok(elements) => elements, Err(()) => { let case_sensitivity = quirks_mode.classes_and_ids_case_sensitivity(); @@ -297,7 +318,7 @@ fn collect_elements_with_id( for element in elements { // If the element is not an actual descendant of the root, even though // it's connected, we don't really care about it. - if !element_is_descendant_of(*element, root) { + if !connected_element_is_descendant_of(*element, root) { continue; } @@ -405,9 +426,8 @@ where return Ok(()); } - let doc = root.owner_doc(); - let elements = fast_connected_elements_with_id(&doc, root, id, quirks_mode)?; - + let elements = + fast_connected_elements_with_id(root, id, quirks_mode)?; if elements.is_empty() { return Ok(()); } @@ -432,7 +452,7 @@ where // // Give up on trying to optimize based on this id and // keep walking our selector. - if !element_is_descendant_of(*element, root) { + if !connected_element_is_descendant_of(*element, root) { continue 'component_loop; } diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 7a565ec469e..4b8148bb701 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -87,6 +87,29 @@ use std::ptr; use string_cache::{Atom, Namespace, WeakAtom, WeakNamespace}; use stylist::CascadeData; + +#[inline] +fn elements_with_id<'a, 'le>( + array: *const structs::nsTArray<*mut RawGeckoElement>, +) -> &'a [GeckoElement<'le>] { + unsafe { + if array.is_null() { + return &[]; + } + + let elements: &[*mut RawGeckoElement] = &**array; + + // NOTE(emilio): We rely on the in-memory representation of + // GeckoElement<'ld> and *mut RawGeckoElement being the same. + #[allow(dead_code)] + unsafe fn static_assert() { + mem::transmute::<*mut RawGeckoElement, GeckoElement<'static>>(0xbadc0de as *mut _); + } + + mem::transmute(elements) + } +} + /// A simple wrapper over `nsIDocument`. #[derive(Clone, Copy)] pub struct GeckoDocument<'ld>(pub &'ld structs::nsIDocument); @@ -109,24 +132,14 @@ impl<'ld> TDocument for GeckoDocument<'ld> { self.0.mCompatMode.into() } - fn elements_with_id(&self, id: &Atom) -> Result<&[GeckoElement<'ld>], ()> { - unsafe { - let array = bindings::Gecko_GetElementsWithId(self.0, id.as_ptr()); - if array.is_null() { - return Ok(&[]); - } - - let elements: &[*mut RawGeckoElement] = &**array; - - // NOTE(emilio): We rely on the in-memory representation of - // GeckoElement<'ld> and *mut RawGeckoElement being the same. - #[allow(dead_code)] - unsafe fn static_assert() { - mem::transmute::<*mut RawGeckoElement, GeckoElement<'static>>(0xbadc0de as *mut _); - } - - Ok(mem::transmute(elements)) - } + #[inline] + fn elements_with_id<'a>(&self, id: &Atom) -> Result<&'a [GeckoElement<'ld>], ()> + where + Self: 'a, + { + Ok(elements_with_id(unsafe { + bindings::Gecko_Document_GetElementsWithId(self.0, id.as_ptr()) + })) } } @@ -176,6 +189,16 @@ impl<'lr> TShadowRoot for GeckoShadowRoot<'lr> { &author_styles.data } + + #[inline] + fn elements_with_id<'a>(&self, id: &Atom) -> Result<&'a [GeckoElement<'lr>], ()> + where + Self: 'a, + { + Ok(elements_with_id(unsafe { + bindings::Gecko_ShadowRoot_GetElementsWithId(self.0, id.as_ptr()) + })) + } } /// A simple wrapper over a non-null Gecko node (`nsINode`) pointer. From f4cff206490ebf6a3b55365685470420908bec39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 28 May 2018 15:58:18 +0200 Subject: [PATCH 23/24] style: Appease tidy. --- components/style/values/animated/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/style/values/animated/mod.rs b/components/style/values/animated/mod.rs index 2bb80df0ec7..53d3719b021 100644 --- a/components/style/values/animated/mod.rs +++ b/components/style/values/animated/mod.rs @@ -11,11 +11,11 @@ use app_units::Au; use euclid::{Point2D, Size2D}; use smallvec::SmallVec; -use values::computed::length::CalcLengthOrPercentage; use values::computed::Angle as ComputedAngle; use values::computed::BorderCornerRadius as ComputedBorderCornerRadius; use values::computed::MaxLength as ComputedMaxLength; use values::computed::MozLength as ComputedMozLength; +use values::computed::length::CalcLengthOrPercentage; use values::computed::url::ComputedUrl; pub mod color; From bbb59614fa528da3bd484cc2c834f6be2e8e9fd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 28 May 2018 16:00:00 +0200 Subject: [PATCH 24/24] Fix Servo build. --- components/layout/construct.rs | 5 +++-- components/layout/display_list/background.rs | 21 ++++++++++---------- components/layout/display_list/builder.rs | 4 ++-- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/components/layout/construct.rs b/components/layout/construct.rs index 0adde8d7255..efa3ccd176a 100644 --- a/components/layout/construct.rs +++ b/components/layout/construct.rs @@ -1533,8 +1533,9 @@ impl<'a, ConcreteThreadSafeLayoutNode> PostorderNodeMutTraversal(arr: &[T], index: usize) -> &T { /// For a given area and an image compute how big the /// image should be displayed on the background. fn compute_background_image_size( - bg_size: BackgroundSize, + bg_size: ComputedBackgroundSize, bounds_size: Size2D, intrinsic_size: Option>, ) -> Size2D { @@ -99,9 +100,9 @@ fn compute_background_image_size( None => match bg_size { BackgroundSize::Cover | BackgroundSize::Contain => bounds_size, BackgroundSize::Explicit { width, height } => Size2D::new( - MaybeAuto::from_style(width, bounds_size.width) + MaybeAuto::from_style(width.0, bounds_size.width) .specified_or_default(bounds_size.width), - MaybeAuto::from_style(height, bounds_size.height) + MaybeAuto::from_style(height.0, bounds_size.height) .specified_or_default(bounds_size.height), ), }, @@ -123,29 +124,29 @@ fn compute_background_image_size( ( BackgroundSize::Explicit { width, - height: LengthOrPercentageOrAuto::Auto, + height: NonNegative(LengthOrPercentageOrAuto::Auto), }, _, ) => { - let width = MaybeAuto::from_style(width, bounds_size.width) + let width = MaybeAuto::from_style(width.0, bounds_size.width) .specified_or_default(own_size.width); Size2D::new(width, width.scale_by(image_aspect_ratio.recip())) }, ( BackgroundSize::Explicit { - width: LengthOrPercentageOrAuto::Auto, + width: NonNegative(LengthOrPercentageOrAuto::Auto), height, }, _, ) => { - let height = MaybeAuto::from_style(height, bounds_size.height) + let height = MaybeAuto::from_style(height.0, bounds_size.height) .specified_or_default(own_size.height); Size2D::new(height.scale_by(image_aspect_ratio), height) }, (BackgroundSize::Explicit { width, height }, _) => Size2D::new( - MaybeAuto::from_style(width, bounds_size.width) + MaybeAuto::from_style(width.0, bounds_size.width) .specified_or_default(own_size.width), - MaybeAuto::from_style(height, bounds_size.height) + MaybeAuto::from_style(height.0, bounds_size.height) .specified_or_default(own_size.height), ), } diff --git a/components/layout/display_list/builder.rs b/components/layout/display_list/builder.rs index b4f8d68282b..a3a40a98cc0 100644 --- a/components/layout/display_list/builder.rs +++ b/components/layout/display_list/builder.rs @@ -916,9 +916,9 @@ impl FragmentDisplayListBuilding for Fragment { get_cyclic(&style.get_background().background_size.0, i).clone(); let size = match background_size { BackgroundSize::Explicit { width, height } => Size2D::new( - MaybeAuto::from_style(width, bounding_box_size.width) + MaybeAuto::from_style(width.0, bounding_box_size.width) .specified_or_default(bounding_box_size.width), - MaybeAuto::from_style(height, bounding_box_size.height) + MaybeAuto::from_style(height.0, bounding_box_size.height) .specified_or_default(bounding_box_size.height), ), _ => bounding_box_size,