Bug 1614198 - Some preparation to start using cbindgen for shape-outside and clip-path. r=boris

Tweak the ShapeSourceRepresentation so that it doesn't store Option<>s.

Some renames so that GeometryBox doesn't conflict with the Gecko type, and some
other usual bits / re-exports to deal with cbindgen and generics.

Also, drive-by derive parsing of GeometryBox as it's trivial.

Doing this unfortunately is not possible without removing nsStyleImage first, so
let's do that before.

This makes us serialize in the shortest form for shape-outside, but that's what
we should do anyway.

(aside: the shapes code is a bit too generic, maybe we should unify
ClippingShape and FloatAreaShape...)

Differential Revision: https://phabricator.services.mozilla.com/D62163

--HG--
extra : moz-landing-system : lando

servo/components/style/gecko/conversions.rs

@@ -99,7 +99,7 @@ pub mod basic_shape {
     };
     use crate::values::computed::basic_shape::{BasicShape, ClippingShape, FloatAreaShape};
     use crate::values::computed::motion::OffsetPath;
-    use crate::values::generics::basic_shape::{GeometryBox, Path, ShapeBox, ShapeSource};
+    use crate::values::generics::basic_shape::{ShapeGeometryBox, Path, ShapeBox, ShapeSource};
     use crate::values::specified::SVGPathData;
 
     impl StyleShapeSource {
@@ -109,7 +109,7 @@ pub mod basic_shape {
             &self,
         ) -> Option<ShapeSource<BasicShape, ReferenceBox, ImageOrUrl>>
         where
-            ReferenceBox: From<StyleGeometryBox>,
+            ReferenceBox: From<StyleGeometryBox> + Default + PartialEq,
         {
             match self.mType {
                 StyleShapeSourceType::None => Some(ShapeSource::None),
@@ -117,11 +117,7 @@ pub mod basic_shape {
                 StyleShapeSourceType::Shape => {
                     let other_shape = unsafe { &*self.__bindgen_anon_1.mBasicShape.as_ref().mPtr };
                     let shape = Box::new(other_shape.clone());
-                    let reference_box = if self.mReferenceBox == StyleGeometryBox::NoBox {
-                        None
-                    } else {
-                        Some(self.mReferenceBox.into())
-                    };
+                    let reference_box = self.mReferenceBox.into();
                     Some(ShapeSource::Shape(shape, reference_box))
                 },
                 StyleShapeSourceType::Image => None,
@@ -207,33 +203,32 @@ pub mod basic_shape {
         }
     }
 
-    impl From<GeometryBox> for StyleGeometryBox {
-        fn from(reference: GeometryBox) -> Self {
+    impl From<ShapeGeometryBox> for StyleGeometryBox {
+        fn from(reference: ShapeGeometryBox) -> Self {
             use crate::gecko_bindings::structs::StyleGeometryBox::*;
             match reference {
-                GeometryBox::ShapeBox(shape_box) => From::from(shape_box),
-                GeometryBox::FillBox => FillBox,
-                GeometryBox::StrokeBox => StrokeBox,
-                GeometryBox::ViewBox => ViewBox,
+                ShapeGeometryBox::ShapeBox(shape_box) => From::from(shape_box),
+                ShapeGeometryBox::FillBox => FillBox,
+                ShapeGeometryBox::StrokeBox => StrokeBox,
+                ShapeGeometryBox::ViewBox => ViewBox,
+                ShapeGeometryBox::ElementDependent => NoBox,
             }
         }
     }
 
-    // Will panic on NoBox
-    // Ideally these would be implemented on Option<T>,
-    // but coherence doesn't like that and TryFrom isn't stable
-    impl From<StyleGeometryBox> for GeometryBox {
+    impl From<StyleGeometryBox> for ShapeGeometryBox {
         fn from(reference: StyleGeometryBox) -> Self {
             use crate::gecko_bindings::structs::StyleGeometryBox::*;
             match reference {
-                ContentBox => GeometryBox::ShapeBox(ShapeBox::ContentBox),
-                PaddingBox => GeometryBox::ShapeBox(ShapeBox::PaddingBox),
-                BorderBox => GeometryBox::ShapeBox(ShapeBox::BorderBox),
-                MarginBox => GeometryBox::ShapeBox(ShapeBox::MarginBox),
-                FillBox => GeometryBox::FillBox,
-                StrokeBox => GeometryBox::StrokeBox,
-                ViewBox => GeometryBox::ViewBox,
-                _ => panic!("Unexpected StyleGeometryBox while converting to GeometryBox"),
+                ContentBox => ShapeGeometryBox::ShapeBox(ShapeBox::ContentBox),
+                PaddingBox => ShapeGeometryBox::ShapeBox(ShapeBox::PaddingBox),
+                BorderBox => ShapeGeometryBox::ShapeBox(ShapeBox::BorderBox),
+                MarginBox => ShapeGeometryBox::ShapeBox(ShapeBox::MarginBox),
+                FillBox => ShapeGeometryBox::FillBox,
+                StrokeBox => ShapeGeometryBox::StrokeBox,
+                ViewBox => ShapeGeometryBox::ViewBox,
+                NoBox => ShapeGeometryBox::ElementDependent,
+                NoClip | Text | MozAlmostPadding => unreachable!(),
             }
         }
     }

servo/components/style/properties/gecko.mako.rs

@@ -2247,7 +2247,6 @@ fn set_style_svg_path(
     pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) {
         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};
 
@@ -2275,13 +2274,12 @@ fn set_style_svg_path(
                 ${ident}.mType = StyleShapeSourceType::Box;
             }
             ShapeSource::Path(p) => set_style_svg_path(${ident}, p.path, p.fill),
-            ShapeSource::Shape(servo_shape, maybe_box) => {
+            ShapeSource::Shape(servo_shape, reference_box) => {
                 unsafe {
                     ${ident}.__bindgen_anon_1.mBasicShape.as_mut().mPtr =
                         Box::into_raw(servo_shape);
                 }
-                ${ident}.mReferenceBox =
-                    maybe_box.map(Into::into).unwrap_or(StyleGeometryBox::NoBox);
+                ${ident}.mReferenceBox = reference_box.into();
                 ${ident}.mType = StyleShapeSourceType::Shape;
             }
         }

servo/components/style/values/computed/basic_shape.rs

@@ -15,10 +15,10 @@ use crate::values::generics::basic_shape as generic;
 pub use crate::values::generics::basic_shape::FillRule;
 
 /// A computed clipping shape.
-pub type ClippingShape = generic::ClippingShape<BasicShape, ComputedUrl>;
+pub type ClippingShape = generic::GenericClippingShape<BasicShape, ComputedUrl>;
 
 /// A computed float area shape.
-pub type FloatAreaShape = generic::FloatAreaShape<BasicShape, Image>;
+pub type FloatAreaShape = generic::GenericFloatAreaShape<BasicShape, Image>;
 
 /// A computed basic shape.
 pub type BasicShape = generic::GenericBasicShape<

servo/components/style/values/generics/basic_shape.rs

@@ -16,7 +16,7 @@ use std::fmt::{self, Write};
 use style_traits::{CssWriter, ToCss};
 
 /// A clipping shape, for `clip-path`.
-pub type ClippingShape<BasicShape, Url> = ShapeSource<BasicShape, GeometryBox, Url>;
+pub type GenericClippingShape<BasicShape, Url> = GenericShapeSource<BasicShape, ShapeGeometryBox, Url>;
 
 /// <https://drafts.fxtf.org/css-masking-1/#typedef-geometry-box>
 #[allow(missing_docs)]
@@ -27,6 +27,7 @@ pub type ClippingShape<BasicShape, Url> = ShapeSource<BasicShape, GeometryBox, U
     Debug,
     MallocSizeOf,
     PartialEq,
+    Parse,
     SpecifiedValueInfo,
     ToAnimatedValue,
     ToComputedValue,
@@ -34,15 +35,30 @@ pub type ClippingShape<BasicShape, Url> = ShapeSource<BasicShape, GeometryBox, U
     ToResolvedValue,
     ToShmem,
 )]
-pub enum GeometryBox {
+#[repr(u8)]
+pub enum ShapeGeometryBox {
+    /// Depending on which kind of element this style value applied on, the
+    /// default value of the reference-box can be different.  For an HTML
+    /// element, the default value of reference-box is border-box; for an SVG
+    /// element, the default value is fill-box.  Since we can not determine the
+    /// default value at parsing time, we keep this value to make a decision on
+    /// it.
+    #[css(skip)]
+    ElementDependent,
     FillBox,
     StrokeBox,
     ViewBox,
     ShapeBox(ShapeBox),
 }
 
+impl Default for ShapeGeometryBox {
+    fn default() -> Self {
+        Self::ElementDependent
+    }
+}
+
 /// A float area shape, for `shape-outside`.
-pub type FloatAreaShape<BasicShape, Image> = ShapeSource<BasicShape, ShapeBox, Image>;
+pub type GenericFloatAreaShape<BasicShape, Image> = GenericShapeSource<BasicShape, ShapeBox, Image>;
 
 /// https://drafts.csswg.org/css-shapes-1/#typedef-shape-box
 #[allow(missing_docs)]
@@ -63,6 +79,7 @@ pub type FloatAreaShape<BasicShape, Image> = ShapeSource<BasicShape, ShapeBox, I
     ToResolvedValue,
     ToShmem,
 )]
+#[repr(u8)]
 pub enum ShapeBox {
     MarginBox,
     BorderBox,
@@ -70,9 +87,15 @@ pub enum ShapeBox {
     ContentBox,
 }
 
+impl Default for ShapeBox {
+    fn default() -> Self {
+        ShapeBox::MarginBox
+    }
+}
+
 /// A shape source, for some reference box.
 #[allow(missing_docs)]
-#[animation(no_bound(ImageOrUrl))]
+#[animation(no_bound(I))]
 #[derive(
     Animate,
     Clone,
@@ -86,10 +109,14 @@ pub enum ShapeBox {
     ToResolvedValue,
     ToShmem,
 )]
-pub enum ShapeSource<BasicShape, ReferenceBox, ImageOrUrl> {
+#[repr(u8)]
+pub enum GenericShapeSource<BasicShape, ReferenceBox, I>
+where
+    ReferenceBox: Default + PartialEq,
+{
     #[animation(error)]
-    ImageOrUrl(ImageOrUrl),
-    Shape(Box<BasicShape>, Option<ReferenceBox>),
+    ImageOrUrl(I),
+    Shape(Box<BasicShape>, #[css(skip_if = "is_default")] ReferenceBox),
     #[animation(error)]
     Box(ReferenceBox),
     #[css(function)]
@@ -98,6 +125,8 @@ pub enum ShapeSource<BasicShape, ReferenceBox, ImageOrUrl> {
     None,
 }
 
+pub use self::GenericShapeSource as ShapeSource;
+
 #[allow(missing_docs)]
 #[derive(
     Animate,
@@ -252,7 +281,7 @@ pub use self::GenericShapeRadius as ShapeRadius;
 #[repr(C)]
 pub struct GenericPolygon<LengthPercentage> {
     /// The filling rule for a polygon.
-    #[css(skip_if = "fill_is_default")]
+    #[css(skip_if = "is_default")]
     pub fill: FillRule,
     /// A collection of (x, y) coordinates to draw the polygon.
     #[css(iterable)]
@@ -321,9 +350,10 @@ pub enum FillRule {
     ToResolvedValue,
     ToShmem,
 )]
+#[repr(C)]
 pub struct Path {
     /// The filling rule for the svg path.
-    #[css(skip_if = "fill_is_default")]
+    #[css(skip_if = "is_default")]
     #[animation(constant)]
     pub fill: FillRule,
     /// The svg path data.
@@ -335,7 +365,7 @@ pub struct Path {
 impl<B, T, U> ComputeSquaredDistance for ShapeSource<B, T, U>
 where
     B: ComputeSquaredDistance,
-    T: PartialEq,
+    T: Default + PartialEq,
 {
     fn compute_squared_distance(&self, other: &Self) -> Result<SquaredDistance, ()> {
         match (self, other) {
@@ -353,7 +383,10 @@ where
     }
 }
 
-impl<B, T, U> ToAnimatedZero for ShapeSource<B, T, U> {
+impl<B, T, U> ToAnimatedZero for ShapeSource<B, T, U>
+where
+    T: Default + PartialEq,
+{
     fn to_animated_zero(&self) -> Result<Self, ()> {
         Err(())
     }
@@ -488,6 +521,6 @@ impl Default for FillRule {
 }
 
 #[inline]
-fn fill_is_default(fill: &FillRule) -> bool {
-    *fill == FillRule::default()
+fn is_default<T: Default + PartialEq>(fill: &T) -> bool {
+    *fill == Default::default()
 }

servo/components/style/values/specified/basic_shape.rs

@@ -9,8 +9,7 @@
 
 use crate::parser::{Parse, ParserContext};
 use crate::values::generics::basic_shape as generic;
-use crate::values::generics::basic_shape::{GeometryBox, Path, PolygonCoord};
-use crate::values::generics::basic_shape::{ShapeBox, ShapeSource};
+use crate::values::generics::basic_shape::{Path, PolygonCoord, ShapeSource};
 use crate::values::generics::rect::Rect;
 use crate::values::specified::border::BorderRadius;
 use crate::values::specified::image::Image;
@@ -26,13 +25,13 @@ use style_traits::{ParseError, StyleParseErrorKind};
 pub use crate::values::generics::basic_shape::FillRule;
 
 /// A specified clipping shape.
-pub type ClippingShape = generic::ClippingShape<BasicShape, SpecifiedUrl>;
+pub type ClippingShape = generic::GenericClippingShape<BasicShape, SpecifiedUrl>;
 
 /// A specified float area shape.
-pub type FloatAreaShape = generic::FloatAreaShape<BasicShape, Image>;
+pub type FloatAreaShape = generic::GenericFloatAreaShape<BasicShape, Image>;
 
 /// A specified basic shape.
-pub type BasicShape = generic::BasicShape<
+pub type BasicShape = generic::GenericBasicShape<
     HorizontalPosition,
     VerticalPosition,
     LengthPercentage,
@@ -112,7 +111,7 @@ impl Parse for FloatAreaShape {
 
 impl<ReferenceBox, ImageOrUrl> ShapeSource<BasicShape, ReferenceBox, ImageOrUrl>
 where
-    ReferenceBox: Parse,
+    ReferenceBox: Parse + Default + PartialEq,
 {
     /// The internal parser for ShapeSource.
     fn parse_common<'i, 't>(
@@ -142,7 +141,7 @@ where
         }
 
         if let Some(shp) = shape {
-            return Ok(ShapeSource::Shape(Box::new(shp), ref_box));
+            return Ok(ShapeSource::Shape(Box::new(shp), ref_box.unwrap_or_default()));
         }
 
         ref_box
@@ -151,23 +150,6 @@ where
     }
 }
 
-impl Parse for GeometryBox {
-    fn parse<'i, 't>(
-        _context: &ParserContext,
-        input: &mut Parser<'i, 't>,
-    ) -> Result<Self, ParseError<'i>> {
-        if let Ok(shape_box) = input.try(ShapeBox::parse) {
-            return Ok(GeometryBox::ShapeBox(shape_box));
-        }
-
-        try_match_ident_ignore_ascii_case! { input,
-            "fill-box" => Ok(GeometryBox::FillBox),
-            "stroke-box" => Ok(GeometryBox::StrokeBox),
-            "view-box" => Ok(GeometryBox::ViewBox),
-        }
-    }
-}
-
 impl Parse for BasicShape {
     fn parse<'i, 't>(
         context: &ParserContext,

testing/web-platform/tests/css/css-shapes/parsing/shape-outside-valid.html

@@ -56,7 +56,7 @@ test_valid_value("shape-outside", "circle(7% at 8% 9%) border-box");
 // <shape-box> <basic-shape>
 test_valid_value("shape-outside", "padding-box inset(10em)", "inset(10em) padding-box");
 test_valid_value("shape-outside", "border-box circle(7% at 8% 9%)", "circle(7% at 8% 9%) border-box");
-test_valid_value("shape-outside", "margin-box ellipse(at 1em 2em)", "ellipse(at 1em 2em) margin-box");
+test_valid_value("shape-outside", "margin-box ellipse(at 1em 2em)", ["ellipse(at 1em 2em) margin-box", "ellipse(at 1em 2em)"]);
 test_valid_value("shape-outside", "content-box polygon(1% 2%)", "polygon(1% 2%) content-box");
 
 // <image>

testing/web-platform/tests/css/css-shapes/shape-outside/values/shape-outside-shape-box-pair-000.html

@@ -34,8 +34,8 @@
             },
             {
               "actual": "polygon(nonzero, 1px 1px) margin-box",
-              "expected_inline": "polygon(1px 1px) margin-box",
-              "expected_computed": "polygon(1px 1px) margin-box"
+              "expected_inline": "polygon(1px 1px)",
+              "expected_computed": "polygon(1px 1px)"
             },
             {
               "actual": " content-box polygon(nonzero, 1px 1px)",
@@ -54,8 +54,8 @@
             },
             {
               "actual": "margin-box polygon(nonzero, 1px 1px)",
-              "expected_inline": "polygon(1px 1px) margin-box",
-              "expected_computed": "polygon(1px 1px) margin-box"
+              "expected_inline": "polygon(1px 1px)",
+              "expected_computed": "polygon(1px 1px)"
             }
         ];
         generate_tests( ParsingUtils.testInlineStyle,