From 559235edadc415b750c75ee4fd2d3d7afadf90bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 9 May 2019 11:24:57 +0000 Subject: [PATCH] style: Use the owned slice type for basic shape polygon coordinates. This enables destructors for tagged unions in cbindgen, implemented in: * https://github.com/eqrion/cbindgen/pull/333 Which allow us to properly generate a destructor for the cbindgen-generated StyleBasicShape (which now contains an OwnedSlice). For now, we still use the glue code to go from Box to UniquePtr. But that will change in the future when we generate even more stuff and remove all the glue. I could add support for copy-constructor generation to cbindgen for tagged enums, but I'm not sure if it'll end up being needed, and copy-constructing unions in C++ is always very tricky. Differential Revision: https://phabricator.services.mozilla.com/D29769 --- components/style/gecko/conversions.rs | 78 +---------------- components/style/gecko/values.rs | 33 +------- components/style/owned_slice.rs | 3 + components/style/properties/gecko.mako.rs | 83 +++---------------- .../style/properties/longhands/svg.mako.rs | 2 +- components/style/values/animated/mod.rs | 31 ++++--- .../style/values/computed/basic_shape.rs | 4 +- .../style/values/generics/basic_shape.rs | 46 ++++++---- .../style/values/specified/basic_shape.rs | 17 ++-- 9 files changed, 79 insertions(+), 218 deletions(-) diff --git a/components/style/gecko/conversions.rs b/components/style/gecko/conversions.rs index 4cd5722bdb0..748396630fb 100644 --- a/components/style/gecko/conversions.rs +++ b/components/style/gecko/conversions.rs @@ -532,28 +532,17 @@ impl nsStyleImage { pub mod basic_shape { //! Conversions from and to CSS shape representations. - - use crate::gecko::values::GeckoStyleCoordConvertible; - use crate::gecko_bindings::structs::nsStyleCoord; - use crate::gecko_bindings::structs::{StyleBasicShape, StyleBasicShapeType}; use crate::gecko_bindings::structs::{ StyleGeometryBox, StyleShapeSource, StyleShapeSourceType, }; use crate::gecko_bindings::sugar::refptr::RefPtr; use crate::values::computed::basic_shape::{ - BasicShape, ClippingShape, FloatAreaShape, ShapeRadius, + BasicShape, ClippingShape, FloatAreaShape, }; - use crate::values::computed::length::LengthPercentage; use crate::values::computed::motion::OffsetPath; use crate::values::computed::url::ComputedUrl; - use crate::values::generics::basic_shape::{ - BasicShape as GenericBasicShape, InsetRect, Polygon, - }; - use crate::values::generics::basic_shape::{Circle, Ellipse, Path, PolygonCoord}; - use crate::values::generics::basic_shape::{GeometryBox, ShapeBox, ShapeSource}; - use crate::values::generics::rect::Rect; + use crate::values::generics::basic_shape::{Path, GeometryBox, ShapeBox, ShapeSource}; use crate::values::specified::SVGPathData; - use std::borrow::Borrow; impl StyleShapeSource { /// Convert StyleShapeSource to ShapeSource except URL and Image @@ -569,7 +558,7 @@ pub mod basic_shape { StyleShapeSourceType::Box => Some(ShapeSource::Box(self.mReferenceBox.into())), StyleShapeSourceType::Shape => { let other_shape = unsafe { &*self.__bindgen_anon_1.mBasicShape.as_ref().mPtr }; - let shape = other_shape.into(); + let shape = Box::new(other_shape.clone()); let reference_box = if self.mReferenceBox == StyleGeometryBox::NoBox { None } else { @@ -653,67 +642,6 @@ pub mod basic_shape { } } - impl<'a> From<&'a StyleBasicShape> for BasicShape { - fn from(other: &'a StyleBasicShape) -> Self { - match other.mType { - StyleBasicShapeType::Inset => { - let t = LengthPercentage::from_gecko_style_coord(&other.mCoordinates[0]); - let r = LengthPercentage::from_gecko_style_coord(&other.mCoordinates[1]); - let b = LengthPercentage::from_gecko_style_coord(&other.mCoordinates[2]); - let l = LengthPercentage::from_gecko_style_coord(&other.mCoordinates[3]); - let round = other.mRadius; - let rect = Rect::new( - t.expect("inset() offset should be a length, percentage, or calc value"), - r.expect("inset() offset should be a length, percentage, or calc value"), - b.expect("inset() offset should be a length, percentage, or calc value"), - l.expect("inset() offset should be a length, percentage, or calc value"), - ); - GenericBasicShape::Inset(InsetRect { rect, round }) - }, - StyleBasicShapeType::Circle => GenericBasicShape::Circle(Circle { - radius: (&other.mCoordinates[0]).into(), - position: other.mPosition, - }), - StyleBasicShapeType::Ellipse => GenericBasicShape::Ellipse(Ellipse { - semiaxis_x: (&other.mCoordinates[0]).into(), - semiaxis_y: (&other.mCoordinates[1]).into(), - position: other.mPosition, - }), - StyleBasicShapeType::Polygon => { - let mut coords = Vec::with_capacity(other.mCoordinates.len() / 2); - for i in 0..(other.mCoordinates.len() / 2) { - let x = 2 * i; - let y = x + 1; - coords.push(PolygonCoord( - LengthPercentage::from_gecko_style_coord(&other.mCoordinates[x]) - .expect( - "polygon() coordinate should be a length, percentage, \ - or calc value", - ), - LengthPercentage::from_gecko_style_coord(&other.mCoordinates[y]) - .expect( - "polygon() coordinate should be a length, percentage, \ - or calc value", - ), - )) - } - GenericBasicShape::Polygon(Polygon { - fill: other.mFillRule, - coordinates: coords, - }) - }, - } - } - } - - impl<'a> From<&'a nsStyleCoord> for ShapeRadius { - fn from(other: &'a nsStyleCoord) -> Self { - let other = other.borrow(); - ShapeRadius::from_gecko_style_coord(other) - .expect(" should be a length, percentage, calc, or keyword value") - } - } - impl From for StyleGeometryBox { fn from(reference: ShapeBox) -> Self { use crate::gecko_bindings::structs::StyleGeometryBox::*; diff --git a/components/style/gecko/values.rs b/components/style/gecko/values.rs index 8b05e1520b4..ba2074d8fdc 100644 --- a/components/style/gecko/values.rs +++ b/components/style/gecko/values.rs @@ -8,12 +8,10 @@ use crate::counter_style::{Symbol, Symbols}; use crate::gecko_bindings::structs::{nsStyleCoord, CounterStylePtr}; -use crate::gecko_bindings::structs::{StyleGridTrackBreadth, StyleShapeRadius}; +use crate::gecko_bindings::structs::StyleGridTrackBreadth; use crate::gecko_bindings::sugar::ns_style_coord::{CoordData, CoordDataMut, CoordDataValue}; -use crate::values::computed::basic_shape::ShapeRadius as ComputedShapeRadius; use crate::values::computed::{Angle, Length, LengthPercentage}; use crate::values::computed::{Number, NumberOrPercentage, Percentage}; -use crate::values::generics::basic_shape::ShapeRadius; use crate::values::generics::gecko::ScrollSnapPoint; use crate::values::generics::grid::{TrackBreadth, TrackKeyword}; use crate::values::generics::length::LengthPercentageOrAuto; @@ -192,35 +190,6 @@ impl GeckoStyleCoordConvertible for TrackBreadth< } } -impl GeckoStyleCoordConvertible for ComputedShapeRadius { - fn to_gecko_style_coord(&self, coord: &mut T) { - match *self { - ShapeRadius::ClosestSide => coord.set_value(CoordDataValue::Enumerated( - StyleShapeRadius::ClosestSide as u32, - )), - ShapeRadius::FarthestSide => coord.set_value(CoordDataValue::Enumerated( - StyleShapeRadius::FarthestSide as u32, - )), - ShapeRadius::Length(lp) => lp.to_gecko_style_coord(coord), - } - } - - fn from_gecko_style_coord(coord: &T) -> Option { - match coord.as_value() { - CoordDataValue::Enumerated(v) => { - if v == StyleShapeRadius::ClosestSide as u32 { - Some(ShapeRadius::ClosestSide) - } else if v == StyleShapeRadius::FarthestSide as u32 { - Some(ShapeRadius::FarthestSide) - } else { - None - } - }, - _ => GeckoStyleCoordConvertible::from_gecko_style_coord(coord).map(ShapeRadius::Length), - } - } -} - impl GeckoStyleCoordConvertible for Option { fn to_gecko_style_coord(&self, coord: &mut U) { if let Some(ref me) = *self { diff --git a/components/style/owned_slice.rs b/components/style/owned_slice.rs index a92a6269e70..e1a82c0176f 100644 --- a/components/style/owned_slice.rs +++ b/components/style/owned_slice.rs @@ -20,6 +20,9 @@ use to_shmem::{SharedMemoryBuilder, ToShmem}; /// /// But handling fat pointers with cbindgen both in structs and argument /// positions more generally is a bit tricky. +/// +/// cbindgen:derive-eq=false +/// cbindgen:derive-neq=false #[repr(C)] pub struct OwnedSlice { ptr: NonNull, diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 537dbb661f4..1f7603c923d 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -4046,16 +4046,15 @@ fn set_style_svg_path( <%def name="impl_shape_source(ident, gecko_ffi_name)"> pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) { - use crate::gecko_bindings::bindings::{Gecko_NewBasicShape, Gecko_DestroyShapeSource}; - use crate::gecko_bindings::structs::{StyleBasicShape, StyleBasicShapeType, StyleShapeSourceType}; - use crate::gecko_bindings::structs::{StyleGeometryBox, StyleShapeSource}; - use crate::gecko::values::GeckoStyleCoordConvertible; - use crate::values::generics::basic_shape::{BasicShape, ShapeSource}; + use crate::values::generics::basic_shape::ShapeSource; + use crate::gecko_bindings::structs::StyleShapeSourceType; + use crate::gecko_bindings::structs::StyleGeometryBox; let ref mut ${ident} = self.gecko.${gecko_ffi_name}; - // clean up existing struct - unsafe { Gecko_DestroyShapeSource(${ident}) }; + // clean up existing struct. + unsafe { bindings::Gecko_DestroyShapeSource(${ident}) }; + ${ident}.mType = StyleShapeSourceType::None; match v { @@ -4084,72 +4083,12 @@ fn set_style_svg_path( } ShapeSource::Path(p) => set_style_svg_path(${ident}, &p.path, p.fill), ShapeSource::Shape(servo_shape, maybe_box) => { - fn init_shape(${ident}: &mut StyleShapeSource, basic_shape_type: StyleBasicShapeType) - -> &mut StyleBasicShape { - unsafe { - // Create StyleBasicShape in StyleShapeSource. mReferenceBox and mType - // will be set manually later. - Gecko_NewBasicShape(${ident}, basic_shape_type); - &mut *${ident}.__bindgen_anon_1.mBasicShape.as_mut().mPtr - } + unsafe { + ${ident}.__bindgen_anon_1.mBasicShape.as_mut().mPtr = + Box::into_raw(servo_shape); } - match servo_shape { - BasicShape::Inset(inset) => { - let shape = init_shape(${ident}, StyleBasicShapeType::Inset); - unsafe { shape.mCoordinates.set_len(4) }; - - // set_len() can't call constructors, so the coordinates - // can contain any value. set_value() attempts to free - // allocated coordinates, so we don't want to feed it - // garbage values which it may misinterpret. - // Instead, we use leaky_set_value to blindly overwrite - // the garbage data without - // attempting to clean up. - shape.mCoordinates[0].leaky_set_null(); - inset.rect.0.to_gecko_style_coord(&mut shape.mCoordinates[0]); - shape.mCoordinates[1].leaky_set_null(); - inset.rect.1.to_gecko_style_coord(&mut shape.mCoordinates[1]); - shape.mCoordinates[2].leaky_set_null(); - inset.rect.2.to_gecko_style_coord(&mut shape.mCoordinates[2]); - shape.mCoordinates[3].leaky_set_null(); - inset.rect.3.to_gecko_style_coord(&mut shape.mCoordinates[3]); - shape.mRadius = inset.round; - } - BasicShape::Circle(circ) => { - let shape = init_shape(${ident}, StyleBasicShapeType::Circle); - unsafe { shape.mCoordinates.set_len(1) }; - shape.mCoordinates[0].leaky_set_null(); - circ.radius.to_gecko_style_coord(&mut shape.mCoordinates[0]); - - shape.mPosition = circ.position.into(); - } - BasicShape::Ellipse(el) => { - let shape = init_shape(${ident}, StyleBasicShapeType::Ellipse); - unsafe { shape.mCoordinates.set_len(2) }; - shape.mCoordinates[0].leaky_set_null(); - el.semiaxis_x.to_gecko_style_coord(&mut shape.mCoordinates[0]); - shape.mCoordinates[1].leaky_set_null(); - el.semiaxis_y.to_gecko_style_coord(&mut shape.mCoordinates[1]); - - shape.mPosition = el.position.into(); - } - BasicShape::Polygon(poly) => { - let shape = init_shape(${ident}, StyleBasicShapeType::Polygon); - unsafe { - shape.mCoordinates.set_len(poly.coordinates.len() as u32 * 2); - } - for (i, coord) in poly.coordinates.iter().enumerate() { - shape.mCoordinates[2 * i].leaky_set_null(); - shape.mCoordinates[2 * i + 1].leaky_set_null(); - coord.0.to_gecko_style_coord(&mut shape.mCoordinates[2 * i]); - coord.1.to_gecko_style_coord(&mut shape.mCoordinates[2 * i + 1]); - } - shape.mFillRule = poly.fill; - } - } - - ${ident}.mReferenceBox = maybe_box.map(Into::into) - .unwrap_or(StyleGeometryBox::NoBox); + ${ident}.mReferenceBox = + maybe_box.map(Into::into).unwrap_or(StyleGeometryBox::NoBox); ${ident}.mType = StyleShapeSourceType::Shape; } } diff --git a/components/style/properties/longhands/svg.mako.rs b/components/style/properties/longhands/svg.mako.rs index 1a84c1d669f..19699aa1c96 100644 --- a/components/style/properties/longhands/svg.mako.rs +++ b/components/style/properties/longhands/svg.mako.rs @@ -87,8 +87,8 @@ ${helpers.predefined_type( "basic_shape::ClippingShape", "generics::basic_shape::ShapeSource::None", products="gecko", - boxed=True, animation_value_type="basic_shape::ClippingShape", + boxed=True, flags="CREATES_STACKING_CONTEXT", spec="https://drafts.fxtf.org/css-masking/#propdef-clip-path", )} diff --git a/components/style/values/animated/mod.rs b/components/style/values/animated/mod.rs index a1e5ca4618c..6f8de1decca 100644 --- a/components/style/values/animated/mod.rs +++ b/components/style/values/animated/mod.rs @@ -16,7 +16,6 @@ use crate::values::computed::Image; use crate::values::specified::SVGPathData; use crate::values::CSSFloat; use app_units::Au; -use euclid::Point2D; use smallvec::SmallVec; use std::cmp; @@ -241,16 +240,10 @@ impl Animate for Au { } } -impl Animate for Point2D -where - T: Animate, -{ +impl Animate for Box { #[inline] fn animate(&self, other: &Self, procedure: Procedure) -> Result { - Ok(Point2D::new( - self.x.animate(&other.x, procedure)?, - self.y.animate(&other.y, procedure)?, - )) + Ok(Box::new((**self).animate(&other, procedure)?)) } } @@ -288,6 +281,24 @@ where } } +impl ToAnimatedValue for Box +where + T: ToAnimatedValue, +{ + type AnimatedValue = Box<::AnimatedValue>; + + #[inline] + fn to_animated_value(self) -> Self::AnimatedValue { + Box::new((*self).to_animated_value()) + } + + #[inline] + fn from_animated_value(animated: Self::AnimatedValue) -> Self { + Box::new(T::from_animated_value(*animated)) + } +} + + impl ToAnimatedValue for Box<[T]> where T: ToAnimatedValue, @@ -328,7 +339,7 @@ where #[inline] fn from_animated_value(animated: Self::AnimatedValue) -> Self { - Box::from_animated_value(animated.into_box()).into() + Self::from(Box::from_animated_value(animated.into_box())) } } diff --git a/components/style/values/computed/basic_shape.rs b/components/style/values/computed/basic_shape.rs index 28067fb0c81..9a245aa77f1 100644 --- a/components/style/values/computed/basic_shape.rs +++ b/components/style/values/computed/basic_shape.rs @@ -23,7 +23,7 @@ pub type ClippingShape = generic::ClippingShape; pub type FloatAreaShape = generic::FloatAreaShape; /// A computed basic shape. -pub type BasicShape = generic::BasicShape< +pub type BasicShape = generic::GenericBasicShape< LengthPercentage, LengthPercentage, LengthPercentage, @@ -41,7 +41,7 @@ pub type Ellipse = generic::Ellipse; /// The computed value of `ShapeRadius` -pub type ShapeRadius = generic::ShapeRadius; +pub type ShapeRadius = generic::GenericShapeRadius; impl ToCss for Circle { fn to_css(&self, dest: &mut CssWriter) -> fmt::Result diff --git a/components/style/values/generics/basic_shape.rs b/components/style/values/generics/basic_shape.rs index 5999e914676..8c63d064817 100644 --- a/components/style/values/generics/basic_shape.rs +++ b/components/style/values/generics/basic_shape.rs @@ -7,8 +7,8 @@ use crate::values::animated::{Animate, Procedure, ToAnimatedZero}; use crate::values::distance::{ComputeSquaredDistance, SquaredDistance}; -use crate::values::generics::border::BorderRadius; -use crate::values::generics::position::Position; +use crate::values::generics::border::GenericBorderRadius; +use crate::values::generics::position::GenericPosition; use crate::values::generics::rect::Rect; use crate::values::specified::SVGPathData; use crate::Zero; @@ -89,7 +89,7 @@ pub enum ShapeBox { pub enum ShapeSource { #[animation(error)] ImageOrUrl(ImageOrUrl), - Shape(BasicShape, Option), + Shape(Box, Option), #[animation(error)] Box(ReferenceBox), #[css(function)] @@ -113,7 +113,8 @@ pub enum ShapeSource { ToResolvedValue, ToShmem, )] -pub enum BasicShape { +#[repr(C, u8)] +pub enum GenericBasicShape { Inset( #[css(field_bound)] #[shmem(field_bound)] @@ -129,9 +130,11 @@ pub enum BasicShape { #[shmem(field_bound)] Ellipse, ), - Polygon(Polygon), + Polygon(GenericPolygon), } +pub use self::GenericBasicShape as BasicShape; + /// #[allow(missing_docs)] #[css(function = "inset")] @@ -148,10 +151,11 @@ pub enum BasicShape { ToResolvedValue, ToShmem, )] +#[repr(C)] pub struct InsetRect { pub rect: Rect, #[shmem(field_bound)] - pub round: BorderRadius, + pub round: GenericBorderRadius, } /// @@ -171,9 +175,10 @@ pub struct InsetRect { ToResolvedValue, ToShmem, )] +#[repr(C)] pub struct Circle { - pub position: Position, - pub radius: ShapeRadius, + pub position: GenericPosition, + pub radius: GenericShapeRadius, } /// @@ -193,10 +198,11 @@ pub struct Circle { ToResolvedValue, ToShmem, )] +#[repr(C)] pub struct Ellipse { - pub position: Position, - pub semiaxis_x: ShapeRadius, - pub semiaxis_y: ShapeRadius, + pub position: GenericPosition, + pub semiaxis_x: GenericShapeRadius, + pub semiaxis_y: GenericShapeRadius, } /// @@ -216,7 +222,8 @@ pub struct Ellipse { ToResolvedValue, ToShmem, )] -pub enum ShapeRadius { +#[repr(C, u8)] +pub enum GenericShapeRadius { Length(NonNegativeLengthPercentage), #[animation(error)] ClosestSide, @@ -224,10 +231,12 @@ pub enum ShapeRadius { FarthestSide, } +pub use self::GenericShapeRadius as ShapeRadius; + /// A generic type for representing the `polygon()` function /// /// -#[css(comma, function)] +#[css(comma, function = "polygon")] #[derive( Clone, Debug, @@ -240,15 +249,18 @@ pub enum ShapeRadius { ToResolvedValue, ToShmem, )] -pub struct Polygon { +#[repr(C)] +pub struct GenericPolygon { /// The filling rule for a polygon. #[css(skip_if = "fill_is_default")] pub fill: FillRule, /// A collection of (x, y) coordinates to draw the polygon. #[css(iterable)] - pub coordinates: Vec>, + pub coordinates: crate::OwnedSlice>, } +pub use self::GenericPolygon as Polygon; + /// Coordinates for Polygon. #[derive( Clone, @@ -262,6 +274,7 @@ pub struct Polygon { ToResolvedValue, ToShmem, )] +#[repr(C)] pub struct PolygonCoord(pub LengthPercentage, pub LengthPercentage); // https://drafts.csswg.org/css-shapes/#typedef-fill-rule @@ -393,7 +406,8 @@ where this.1.animate(&other.1, procedure)?, )) }) - .collect::, _>>()?; + .collect::, _>>()? + .into(); Ok(Polygon { fill: self.fill, coordinates, diff --git a/components/style/values/specified/basic_shape.rs b/components/style/values/specified/basic_shape.rs index 014ce439d3c..70eab983510 100644 --- a/components/style/values/specified/basic_shape.rs +++ b/components/style/values/specified/basic_shape.rs @@ -55,7 +55,7 @@ pub type Ellipse = pub type ShapeRadius = generic::ShapeRadius; /// The specified value of `Polygon` -pub type Polygon = generic::Polygon; +pub type Polygon = generic::GenericPolygon; #[cfg(feature = "gecko")] fn is_clip_path_path_enabled(context: &ParserContext) -> bool { @@ -138,11 +138,11 @@ where } if let Some(shp) = shape { - return Ok(ShapeSource::Shape(shp, ref_box)); + return Ok(ShapeSource::Shape(Box::new(shp), ref_box)); } ref_box - .map(|v| ShapeSource::Box(v)) + .map(ShapeSource::Box) .ok_or(input.new_custom_error(StyleParseErrorKind::UnspecifiedError)) } } @@ -152,7 +152,7 @@ impl Parse for GeometryBox { _context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { - if let Ok(shape_box) = input.try(|i| ShapeBox::parse(i)) { + if let Ok(shape_box) = input.try(ShapeBox::parse) { return Ok(GeometryBox::ShapeBox(shape_box)); } @@ -352,17 +352,14 @@ impl Polygon { }) .unwrap_or_default(); - let buf = input.parse_comma_separated(|i| { + let coordinates = input.parse_comma_separated(|i| { Ok(PolygonCoord( LengthPercentage::parse(context, i)?, LengthPercentage::parse(context, i)?, )) - })?; + })?.into(); - Ok(Polygon { - fill: fill, - coordinates: buf, - }) + Ok(Polygon { fill, coordinates }) } }