From 1fba297bbc1816b4b5498c56bef3add4cf886666 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Tue, 7 May 2019 03:16:21 +0000
Subject: [PATCH 01/14] style: ThinArc should use NonNull.

If only for parallelism with Arc<>.

Differential Revision: https://phabricator.services.mozilla.com/D30131
---
 components/servo_arc/lib.rs | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/components/servo_arc/lib.rs b/components/servo_arc/lib.rs
index f4a929e23f8..7d89238603f 100644
--- a/components/servo_arc/lib.rs
+++ b/components/servo_arc/lib.rs
@@ -21,7 +21,7 @@
 //!
 //! [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1360883
 
-// The semantics of `Arc` are alread documented in the Rust docs, so we don't
+// The semantics of `Arc` are already documented in the Rust docs, so we don't
 // duplicate those here.
 #![allow(missing_docs)]
 
@@ -767,7 +767,7 @@ type HeaderSliceWithLength<H, T> = HeaderSlice<HeaderWithLength<H>, T>;
 /// via `HeaderSliceWithLength`.
 #[repr(C)]
 pub struct ThinArc<H, T> {
-    ptr: *mut ArcInner<HeaderSliceWithLength<H, [T; 1]>>,
+    ptr: ptr::NonNull<ArcInner<HeaderSliceWithLength<H, [T; 1]>>>,
 }
 
 unsafe impl<H: Sync + Send, T: Sync + Send> Send for ThinArc<H, T> {}
@@ -796,7 +796,7 @@ impl<H, T> ThinArc<H, T> {
         // Synthesize transient Arc, which never touches the refcount of the ArcInner.
         let transient = unsafe {
             NoDrop::new(Arc {
-                p: ptr::NonNull::new_unchecked(thin_to_thick(self.ptr)),
+                p: ptr::NonNull::new_unchecked(thin_to_thick(self.ptr.as_ptr())),
             })
         };
 
@@ -851,7 +851,7 @@ impl<H, T> ThinArc<H, T> {
         if is_static {
             ptr::null()
         } else {
-            self.ptr as *const ArcInner<T> as *const c_void
+            self.ptr.as_ptr() as *const ArcInner<T> as *const c_void
         }
     }
 }
@@ -861,7 +861,7 @@ impl<H, T> Deref for ThinArc<H, T> {
 
     #[inline]
     fn deref(&self) -> &Self::Target {
-        unsafe { &(*thin_to_thick(self.ptr)).data }
+        unsafe { &(*thin_to_thick(self.ptr.as_ptr())).data }
     }
 }
 
@@ -893,7 +893,9 @@ impl<H, T> Arc<HeaderSliceWithLength<H, [T]>> {
         mem::forget(a);
         let thin_ptr = fat_ptr as *mut [usize] as *mut usize;
         ThinArc {
-            ptr: thin_ptr as *mut ArcInner<HeaderSliceWithLength<H, [T; 1]>>,
+            ptr: unsafe {
+                ptr::NonNull::new_unchecked(thin_ptr as *mut ArcInner<HeaderSliceWithLength<H, [T; 1]>>)
+            },
         }
     }
 
@@ -901,7 +903,7 @@ impl<H, T> Arc<HeaderSliceWithLength<H, [T]>> {
     /// is not modified.
     #[inline]
     pub fn from_thin(a: ThinArc<H, T>) -> Self {
-        let ptr = thin_to_thick(a.ptr);
+        let ptr = thin_to_thick(a.ptr.as_ptr());
         mem::forget(a);
         unsafe {
             Arc {

From 29cecf65b833aaf7b44f6f0932981ba36efe6372 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Wed, 8 May 2019 08:01:01 +0000
Subject: [PATCH 02/14] style: Use PhantomData<T> in servo_arc.

See https://github.com/rust-lang/rust/pull/60594#issuecomment-489966933

Differential Revision: https://phabricator.services.mozilla.com/D30169
---
 components/servo_arc/lib.rs | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/components/servo_arc/lib.rs b/components/servo_arc/lib.rs
index 7d89238603f..780f7485eb9 100644
--- a/components/servo_arc/lib.rs
+++ b/components/servo_arc/lib.rs
@@ -86,10 +86,14 @@ const STATIC_REFCOUNT: usize = usize::MAX;
 /// See the documentation for [`Arc`] in the standard library. Unlike the
 /// standard library `Arc`, this `Arc` does not support weak reference counting.
 ///
+/// See the discussion in https://github.com/rust-lang/rust/pull/60594 for the
+/// usage of PhantomData.
+///
 /// [`Arc`]: https://doc.rust-lang.org/stable/std/sync/struct.Arc.html
 #[repr(C)]
 pub struct Arc<T: ?Sized> {
     p: ptr::NonNull<ArcInner<T>>,
+    phantom: PhantomData<T>,
 }
 
 /// An `Arc` that is known to be uniquely owned
@@ -169,6 +173,7 @@ impl<T> Arc<T> {
         unsafe {
             Arc {
                 p: ptr::NonNull::new_unchecked(Box::into_raw(x)),
+                phantom: PhantomData,
             }
         }
     }
@@ -198,6 +203,7 @@ impl<T> Arc<T> {
         let ptr = (ptr as *const u8).offset(-offset_of!(ArcInner<T>, data));
         Arc {
             p: ptr::NonNull::new_unchecked(ptr as *mut ArcInner<T>),
+            phantom: PhantomData,
         }
     }
 
@@ -224,6 +230,7 @@ impl<T> Arc<T> {
 
         Arc {
             p: ptr::NonNull::new_unchecked(ptr),
+            phantom: PhantomData,
         }
     }
 
@@ -334,6 +341,7 @@ impl<T: ?Sized> Clone for Arc<T> {
         unsafe {
             Arc {
                 p: ptr::NonNull::new_unchecked(self.ptr()),
+                phantom: PhantomData,
             }
         }
     }
@@ -687,6 +695,7 @@ impl<H, T> Arc<HeaderSlice<H, [T]>> {
         unsafe {
             Arc {
                 p: ptr::NonNull::new_unchecked(ptr),
+                phantom: PhantomData,
             }
         }
     }
@@ -768,6 +777,7 @@ type HeaderSliceWithLength<H, T> = HeaderSlice<HeaderWithLength<H>, T>;
 #[repr(C)]
 pub struct ThinArc<H, T> {
     ptr: ptr::NonNull<ArcInner<HeaderSliceWithLength<H, [T; 1]>>>,
+    phantom: PhantomData<(H, T)>,
 }
 
 unsafe impl<H: Sync + Send, T: Sync + Send> Send for ThinArc<H, T> {}
@@ -797,6 +807,7 @@ impl<H, T> ThinArc<H, T> {
         let transient = unsafe {
             NoDrop::new(Arc {
                 p: ptr::NonNull::new_unchecked(thin_to_thick(self.ptr.as_ptr())),
+                phantom: PhantomData,
             })
         };
 
@@ -875,7 +886,7 @@ impl<H, T> Clone for ThinArc<H, T> {
 impl<H, T> Drop for ThinArc<H, T> {
     #[inline]
     fn drop(&mut self) {
-        let _ = Arc::from_thin(ThinArc { ptr: self.ptr });
+        let _ = Arc::from_thin(ThinArc { ptr: self.ptr, phantom: PhantomData, });
     }
 }
 
@@ -896,6 +907,7 @@ impl<H, T> Arc<HeaderSliceWithLength<H, [T]>> {
             ptr: unsafe {
                 ptr::NonNull::new_unchecked(thin_ptr as *mut ArcInner<HeaderSliceWithLength<H, [T; 1]>>)
             },
+            phantom: PhantomData,
         }
     }
 
@@ -908,6 +920,7 @@ impl<H, T> Arc<HeaderSliceWithLength<H, [T]>> {
         unsafe {
             Arc {
                 p: ptr::NonNull::new_unchecked(ptr),
+                phantom: PhantomData,
             }
         }
     }

From 02210264e7f0dae8221826c60df2706d5e8f9194 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Wed, 8 May 2019 12:44:36 +0000
Subject: [PATCH 03/14] style: Centralize a bit invalid value error reporting.

Also, buffer the errors, since we're going to want to look at the whole
declaration block to skip reporting them.

This shouldn't change behavior, just moves some work to the caller, and defers a
bit the work so that it happens only when error reporting is enabled.

Differential Revision: https://phabricator.services.mozilla.com/D30200
---
 components/style/parser.rs                    |  6 ++
 .../style/properties/declaration_block.rs     | 90 +++++++++++++++----
 .../style/properties/properties.mako.rs       | 34 ++-----
 3 files changed, 87 insertions(+), 43 deletions(-)

diff --git a/components/style/parser.rs b/components/style/parser.rs
index f3190f7f72b..f9322a7bcb3 100644
--- a/components/style/parser.rs
+++ b/components/style/parser.rs
@@ -136,6 +136,12 @@ impl<'a> ParserContext<'a> {
             .expect("Rule type expected, but none was found.")
     }
 
+    /// Returns whether CSS error reporting is enabled.
+    #[inline]
+    pub fn error_reporting_enabled(&self) -> bool {
+        self.error_reporter.is_some()
+    }
+
     /// Record a CSS parse error with this context’s error reporting.
     pub fn log_css_error(&self, location: SourceLocation, error: ContextualParseError) {
         let error_reporter = match self.error_reporter {
diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs
index 42fc833e4b9..29e6ce1fd24 100644
--- a/components/style/properties/declaration_block.rs
+++ b/components/style/properties/declaration_block.rs
@@ -1240,19 +1240,28 @@ pub fn parse_one_declaration_into(
         None,
     );
 
+    let property_id_for_error_reporting = if context.error_reporting_enabled() {
+        Some(id.clone())
+    } else {
+        None
+    };
+
     let mut input = ParserInput::new(input);
     let mut parser = Parser::new(&mut input);
     let start_position = parser.position();
     parser.parse_entirely(|parser| {
         PropertyDeclaration::parse_into(declarations, id, &context, parser)
     }).map_err(|err| {
-        let location = err.location;
-        let error = ContextualParseError::UnsupportedPropertyDeclaration(
-            parser.slice_from(start_position),
-            err,
-            None
-        );
-        context.log_css_error(location, error);
+        if context.error_reporting_enabled() {
+            report_one_css_error(
+                &context,
+                None,
+                None,
+                err,
+                parser.slice_from(start_position),
+                property_id_for_error_reporting,
+            )
+        }
     })
 }
 
@@ -1260,6 +1269,8 @@ pub fn parse_one_declaration_into(
 struct PropertyDeclarationParser<'a, 'b: 'a> {
     context: &'a ParserContext<'b>,
     declarations: &'a mut SourcePropertyDeclaration,
+    /// The last parsed property id if non-custom, and if any.
+    last_parsed_property_id: Option<PropertyId>,
 }
 
 
@@ -1296,6 +1307,9 @@ impl<'a, 'b, 'i> DeclarationParser<'i> for PropertyDeclarationParser<'a, 'b> {
                 }));
             }
         };
+        if self.context.error_reporting_enabled() {
+            self.last_parsed_property_id = Some(id.clone());
+        }
         input.parse_until_before(Delimiter::Bang, |input| {
             PropertyDeclaration::parse_into(self.declarations, id, self.context, input)
         })?;
@@ -1309,6 +1323,48 @@ impl<'a, 'b, 'i> DeclarationParser<'i> for PropertyDeclarationParser<'a, 'b> {
     }
 }
 
+type SmallParseErrorVec<'i> = SmallVec<[(ParseError<'i>, &'i str, Option<PropertyId>); 2]>;
+
+#[cold]
+fn report_one_css_error<'i>(
+    context: &ParserContext,
+    _block: Option<&PropertyDeclarationBlock>,
+    selectors: Option<&SelectorList<SelectorImpl>>,
+    mut error: ParseError<'i>,
+    slice: &str,
+    property: Option<PropertyId>,
+) {
+    debug_assert!(context.error_reporting_enabled());
+
+    // If the unrecognized property looks like a vendor-specific property,
+    // silently ignore it instead of polluting the error output.
+    if let ParseErrorKind::Custom(StyleParseErrorKind::UnknownVendorProperty) = error.kind {
+        return;
+    }
+
+    if let Some(ref property) = property {
+        error = match *property {
+            PropertyId::Custom(ref c) => StyleParseErrorKind::new_invalid(format!("--{}", c), error),
+            _ => StyleParseErrorKind::new_invalid(property.non_custom_id().unwrap().name(), error),
+        };
+    }
+
+    let location = error.location;
+    let error = ContextualParseError::UnsupportedPropertyDeclaration(slice, error, selectors);
+    context.log_css_error(location, error);
+}
+
+#[cold]
+fn report_css_errors(
+    context: &ParserContext,
+    block: &PropertyDeclarationBlock,
+    selectors: Option<&SelectorList<SelectorImpl>>,
+    errors: &mut SmallParseErrorVec,
+) {
+    for (mut error, slice, property) in errors.drain() {
+        report_one_css_error(context, Some(block), selectors, error, slice, property)
+    }
+}
 
 /// Parse a list of property declarations and return a property declaration
 /// block.
@@ -1320,10 +1376,12 @@ pub fn parse_property_declaration_list(
     let mut declarations = SourcePropertyDeclaration::new();
     let mut block = PropertyDeclarationBlock::new();
     let parser = PropertyDeclarationParser {
-        context: context,
+        context,
+        last_parsed_property_id: None,
         declarations: &mut declarations,
     };
     let mut iter = DeclarationListParser::new(input, parser);
+    let mut errors = SmallParseErrorVec::new();
     while let Some(declaration) = iter.next() {
         match declaration {
             Ok(importance) => {
@@ -1335,17 +1393,17 @@ pub fn parse_property_declaration_list(
             Err((error, slice)) => {
                 iter.parser.declarations.clear();
 
-                // If the unrecognized property looks like a vendor-specific property,
-                // silently ignore it instead of polluting the error output.
-                if let ParseErrorKind::Custom(StyleParseErrorKind::UnknownVendorProperty) = error.kind {
-                    continue;
+                if context.error_reporting_enabled() {
+                    let property = iter.parser.last_parsed_property_id.take();
+                    errors.push((error, slice, property));
                 }
-
-                let location = error.location;
-                let error = ContextualParseError::UnsupportedPropertyDeclaration(slice, error, selectors);
-                context.log_css_error(location, error);
             }
         }
     }
+
+    if !errors.is_empty() {
+        report_css_errors(context, &block, selectors, &mut errors)
+    }
+
     block
 }
diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs
index e7f9c72413e..a9fa35ba444 100644
--- a/components/style/properties/properties.mako.rs
+++ b/components/style/properties/properties.mako.rs
@@ -2209,13 +2209,9 @@ impl PropertyDeclaration {
                 // This probably affects some test results.
                 let value = match input.try(CSSWideKeyword::parse) {
                     Ok(keyword) => CustomDeclarationValue::CSSWideKeyword(keyword),
-                    Err(()) => match crate::custom_properties::SpecifiedValue::parse(input) {
-                        Ok(value) => CustomDeclarationValue::Value(value),
-                        Err(e) => return Err(StyleParseErrorKind::new_invalid(
-                            format!("--{}", property_name),
-                            e,
-                        )),
-                    }
+                    Err(()) => CustomDeclarationValue::Value(
+                        crate::custom_properties::SpecifiedValue::parse(input)?
+                    ),
                 };
                 declarations.push(PropertyDeclaration::Custom(CustomDeclaration {
                     name: property_name,
@@ -2236,19 +2232,11 @@ impl PropertyDeclaration {
                     .or_else(|err| {
                         while let Ok(_) = input.next() {}  // Look for var() after the error.
                         if !input.seen_var_or_env_functions() {
-                            return Err(StyleParseErrorKind::new_invalid(
-                                non_custom_id.unwrap().name(),
-                                err,
-                            ));
+                            return Err(err);
                         }
                         input.reset(&start);
                         let (first_token_type, css) =
-                            crate::custom_properties::parse_non_custom_with_var(input).map_err(|e| {
-                                StyleParseErrorKind::new_invalid(
-                                    non_custom_id.unwrap().name(),
-                                    e,
-                                )
-                            })?;
+                            crate::custom_properties::parse_non_custom_with_var(input)?;
                         Ok(PropertyDeclaration::WithVariables(VariableDeclaration {
                             id,
                             value: Arc::new(UnparsedValue {
@@ -2287,20 +2275,12 @@ impl PropertyDeclaration {
                     id.parse_into(declarations, context, input).or_else(|err| {
                         while let Ok(_) = input.next() {}  // Look for var() after the error.
                         if !input.seen_var_or_env_functions() {
-                            return Err(StyleParseErrorKind::new_invalid(
-                                non_custom_id.unwrap().name(),
-                                err,
-                            ));
+                            return Err(err);
                         }
 
                         input.reset(&start);
                         let (first_token_type, css) =
-                            crate::custom_properties::parse_non_custom_with_var(input).map_err(|e| {
-                                StyleParseErrorKind::new_invalid(
-                                    non_custom_id.unwrap().name(),
-                                    e,
-                                )
-                            })?;
+                            crate::custom_properties::parse_non_custom_with_var(input)?;
                         let unparsed = Arc::new(UnparsedValue {
                             css: css.into_owned(),
                             first_token_type: first_token_type,

From dd6252e34f208eede34125facd067c4601870de4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Wed, 8 May 2019 12:44:46 +0000
Subject: [PATCH 04/14] style: Don't report errors for properties for which
 we've parsed another value in the same declaration block.

I thought a bit about how to test it and it's not particularly great.
test_css_parse_error_smoketest.html is great to assert that something _gets_
reported, but not that it doesn't :)

Differential Revision: https://phabricator.services.mozilla.com/D30201
---
 .../style/properties/declaration_block.rs     | 25 +++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs
index 29e6ce1fd24..5ce80955a48 100644
--- a/components/style/properties/declaration_block.rs
+++ b/components/style/properties/declaration_block.rs
@@ -1269,7 +1269,7 @@ pub fn parse_one_declaration_into(
 struct PropertyDeclarationParser<'a, 'b: 'a> {
     context: &'a ParserContext<'b>,
     declarations: &'a mut SourcePropertyDeclaration,
-    /// The last parsed property id if non-custom, and if any.
+    /// The last parsed property id if any.
     last_parsed_property_id: Option<PropertyId>,
 }
 
@@ -1300,6 +1300,7 @@ impl<'a, 'b, 'i> DeclarationParser<'i> for PropertyDeclarationParser<'a, 'b> {
         let id = match PropertyId::parse(&name, self.context) {
             Ok(id) => id,
             Err(..) => {
+                self.last_parsed_property_id = None;
                 return Err(input.new_custom_error(if is_non_mozilla_vendor_identifier(&name) {
                     StyleParseErrorKind::UnknownVendorProperty
                 } else {
@@ -1328,7 +1329,7 @@ type SmallParseErrorVec<'i> = SmallVec<[(ParseError<'i>, &'i str, Option<Propert
 #[cold]
 fn report_one_css_error<'i>(
     context: &ParserContext,
-    _block: Option<&PropertyDeclarationBlock>,
+    block: Option<&PropertyDeclarationBlock>,
     selectors: Option<&SelectorList<SelectorImpl>>,
     mut error: ParseError<'i>,
     slice: &str,
@@ -1336,6 +1337,21 @@ fn report_one_css_error<'i>(
 ) {
     debug_assert!(context.error_reporting_enabled());
 
+    fn all_properties_in_block(block: &PropertyDeclarationBlock, property: &PropertyId) -> bool {
+        match *property {
+            PropertyId::LonghandAlias(id, _) |
+            PropertyId::Longhand(id) => block.contains(id),
+            PropertyId::ShorthandAlias(id, _) |
+            PropertyId::Shorthand(id) => {
+                id.longhands().all(|longhand| block.contains(longhand))
+            },
+            // NOTE(emilio): We could do this, but it seems of limited utility,
+            // and it's linear on the size of the declaration block, so let's
+            // not.
+            PropertyId::Custom(..) => false,
+        }
+    }
+
     // If the unrecognized property looks like a vendor-specific property,
     // silently ignore it instead of polluting the error output.
     if let ParseErrorKind::Custom(StyleParseErrorKind::UnknownVendorProperty) = error.kind {
@@ -1343,6 +1359,11 @@ fn report_one_css_error<'i>(
     }
 
     if let Some(ref property) = property {
+        if let Some(block) = block {
+            if all_properties_in_block(block, property) {
+                return;
+            }
+        }
         error = match *property {
             PropertyId::Custom(ref c) => StyleParseErrorKind::new_invalid(format!("--{}", c), error),
             _ => StyleParseErrorKind::new_invalid(property.non_custom_id().unwrap().name(), error),

From 99b97737fea30228eea35214029d4e454b5d6e41 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Wed, 8 May 2019 17:54:25 +0000
Subject: [PATCH 05/14] style: Don't panic when creating empty ThinArcs.

The change from [T; 1] to [T; 0] shouldn't change behavior since they have the
right alignment and we never poke at that particular array, but feels more
correct to avoid creating types that point to uninitialized data or outside of
their allocation, now that we allow empty slices.

Differential Revision: https://phabricator.services.mozilla.com/D30352
---
 components/servo_arc/lib.rs | 48 ++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/components/servo_arc/lib.rs b/components/servo_arc/lib.rs
index 780f7485eb9..3cc2b0e7fcc 100644
--- a/components/servo_arc/lib.rs
+++ b/components/servo_arc/lib.rs
@@ -168,7 +168,7 @@ impl<T> Arc<T> {
     pub fn new(data: T) -> Self {
         let x = Box::new(ArcInner {
             count: atomic::AtomicUsize::new(1),
-            data: data,
+            data,
         });
         unsafe {
             Arc {
@@ -610,7 +610,7 @@ impl<H, T> Arc<HeaderSlice<H, [T]>> {
         let size = {
             // First, determine the alignment of a hypothetical pointer to a
             // HeaderSlice.
-            let fake_slice_ptr_align: usize = mem::align_of::<ArcInner<HeaderSlice<H, [T; 1]>>>();
+            let fake_slice_ptr_align: usize = mem::align_of::<ArcInner<HeaderSlice<H, [T; 0]>>>();
 
             // Next, synthesize a totally garbage (but properly aligned) pointer
             // to a sequence of T.
@@ -667,23 +667,24 @@ impl<H, T> Arc<HeaderSlice<H, [T]>> {
             };
             ptr::write(&mut ((*ptr).count), count);
             ptr::write(&mut ((*ptr).data.header), header);
-            let mut current: *mut T = &mut (*ptr).data.slice[0];
-            for _ in 0..num_items {
-                ptr::write(
-                    current,
-                    items
-                        .next()
-                        .expect("ExactSizeIterator over-reported length"),
-                );
-                current = current.offset(1);
+            if num_items != 0 {
+                let mut current: *mut T = &mut (*ptr).data.slice[0];
+                for _ in 0..num_items {
+                    ptr::write(
+                        current,
+                        items
+                            .next()
+                            .expect("ExactSizeIterator over-reported length"),
+                    );
+                    current = current.offset(1);
+                }
+                // We should have consumed the buffer exactly.
+                debug_assert_eq!(current as *mut u8, buffer.offset(size as isize));
             }
             assert!(
                 items.next().is_none(),
                 "ExactSizeIterator under-reported length"
             );
-
-            // We should have consumed the buffer exactly.
-            debug_assert_eq!(current as *mut u8, buffer.offset(size as isize));
         }
 
         // Return the fat Arc.
@@ -772,11 +773,13 @@ type HeaderSliceWithLength<H, T> = HeaderSlice<HeaderWithLength<H>, T>;
 /// have a thin pointer instead, perhaps for FFI compatibility
 /// or space efficiency.
 ///
+/// Note that we use `[T; 0]` in order to have the right alignment for `T`.
+///
 /// `ThinArc` solves this by storing the length in the allocation itself,
 /// via `HeaderSliceWithLength`.
 #[repr(C)]
 pub struct ThinArc<H, T> {
-    ptr: ptr::NonNull<ArcInner<HeaderSliceWithLength<H, [T; 1]>>>,
+    ptr: ptr::NonNull<ArcInner<HeaderSliceWithLength<H, [T; 0]>>>,
     phantom: PhantomData<(H, T)>,
 }
 
@@ -787,7 +790,7 @@ unsafe impl<H: Sync + Send, T: Sync + Send> Sync for ThinArc<H, T> {}
 //
 // See the comment around the analogous operation in from_header_and_iter.
 fn thin_to_thick<H, T>(
-    thin: *mut ArcInner<HeaderSliceWithLength<H, [T; 1]>>,
+    thin: *mut ArcInner<HeaderSliceWithLength<H, [T; 0]>>,
 ) -> *mut ArcInner<HeaderSliceWithLength<H, [T]>> {
     let len = unsafe { (*thin).data.header.length };
     let fake_slice: *mut [T] = unsafe { slice::from_raw_parts_mut(thin as *mut T, len) };
@@ -905,7 +908,7 @@ impl<H, T> Arc<HeaderSliceWithLength<H, [T]>> {
         let thin_ptr = fat_ptr as *mut [usize] as *mut usize;
         ThinArc {
             ptr: unsafe {
-                ptr::NonNull::new_unchecked(thin_ptr as *mut ArcInner<HeaderSliceWithLength<H, [T; 1]>>)
+                ptr::NonNull::new_unchecked(thin_ptr as *mut ArcInner<HeaderSliceWithLength<H, [T; 0]>>)
             },
             phantom: PhantomData,
         }
@@ -1320,6 +1323,17 @@ mod tests {
         }
     }
 
+    #[test]
+    fn empty_thin() {
+        let header = HeaderWithLength::new(100u32, 0);
+        let x = Arc::from_header_and_iter(header, std::iter::empty::<i32>());
+        let y = Arc::into_thin(x.clone());
+        assert_eq!(y.header.header, 100);
+        assert!(y.slice.is_empty());
+        assert_eq!(x.header.header, 100);
+        assert!(x.slice.is_empty());
+    }
+
     #[test]
     fn slices_and_thin() {
         let mut canary = atomic::AtomicUsize::new(0);

From 81e706469d7138d693775e7d4c381b2e2c28f93e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Wed, 8 May 2019 18:03:37 +0000
Subject: [PATCH 06/14] style: Fix an assertion that doesn't account for
 alignment padding.

I'm hitting this when trying to add bindings for box shadows, which are 32-bit
aligned.

Differential Revision: https://phabricator.services.mozilla.com/D30353
---
 components/servo_arc/lib.rs | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/components/servo_arc/lib.rs b/components/servo_arc/lib.rs
index 3cc2b0e7fcc..40a8ff3d193 100644
--- a/components/servo_arc/lib.rs
+++ b/components/servo_arc/lib.rs
@@ -602,7 +602,7 @@ impl<H, T> Arc<HeaderSlice<H, [T]>> {
         F: FnOnce(Layout) -> *mut u8,
         I: Iterator<Item = T> + ExactSizeIterator,
     {
-        use std::mem::size_of;
+        use std::mem::{align_of, size_of};
         assert_ne!(size_of::<T>(), 0, "Need to think about ZST");
 
         // Compute the required size for the allocation.
@@ -678,8 +678,9 @@ impl<H, T> Arc<HeaderSlice<H, [T]>> {
                     );
                     current = current.offset(1);
                 }
-                // We should have consumed the buffer exactly.
-                debug_assert_eq!(current as *mut u8, buffer.offset(size as isize));
+                // We should have consumed the buffer exactly, maybe accounting
+                // for some padding from the alignment.
+                debug_assert!((buffer.offset(size as isize) as usize - current as *mut u8 as usize) < align_of::<Self>());
             }
             assert!(
                 items.next().is_none(),
@@ -1334,6 +1335,23 @@ mod tests {
         assert!(x.slice.is_empty());
     }
 
+    #[test]
+    fn thin_assert_padding() {
+        #[derive(Clone, Default)]
+        #[repr(C)]
+        struct Padded {
+            i: u16,
+        }
+
+        // The header will have more alignment than `Padded`
+        let header = HeaderWithLength::new(0i32, 2);
+        let items = vec![Padded { i: 0xdead }, Padded { i: 0xbeef }];
+        let a = ThinArc::from_header_and_iter(header, items.into_iter());
+        assert_eq!(a.slice.len(), 2);
+        assert_eq!(a.slice[0].i, 0xdead);
+        assert_eq!(a.slice[1].i, 0xbeef);
+    }
+
     #[test]
     fn slices_and_thin() {
         let mut canary = atomic::AtomicUsize::new(0);

From ca756a855040edd61d3a4ff174f36b3243404f17 Mon Sep 17 00:00:00 2001
From: Cameron McCormack <cam@mcc.id.au>
Date: Thu, 9 May 2019 02:32:30 +0000
Subject: [PATCH 07/14] style: Implement -webkit-line-clamp.

Differential Revision: https://phabricator.services.mozilla.com/D20115
---
 components/style/properties/data.py           |  1 +
 components/style/properties/gecko.mako.rs     | 23 ++++++++++++++++++-
 .../style/properties/longhands/box.mako.rs    | 12 ++++++++++
 components/style/values/computed/mod.rs       |  3 +++
 components/style/values/specified/mod.rs      |  5 +++-
 5 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/components/style/properties/data.py b/components/style/properties/data.py
index 7a96402c4a1..ae307077690 100644
--- a/components/style/properties/data.py
+++ b/components/style/properties/data.py
@@ -337,6 +337,7 @@ class Longhand(object):
                 "OverflowWrap",
                 "OverscrollBehavior",
                 "Percentage",
+                "PositiveIntegerOrNone",
                 "Resize",
                 "SVGOpacity",
                 "SVGPaintOrder",
diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs
index b73bb073fa6..537dbb661f4 100644
--- a/components/style/properties/gecko.mako.rs
+++ b/components/style/properties/gecko.mako.rs
@@ -2515,7 +2515,7 @@ fn static_assert() {
                           rotate scroll-snap-points-x scroll-snap-points-y
                           scroll-snap-coordinate -moz-binding will-change
                           offset-path shape-outside
-                          translate scale""" %>
+                          translate scale -webkit-line-clamp""" %>
 <%self:impl_trait style_struct_name="Box" skip_longhands="${skip_box_longhands}">
     #[inline]
     pub fn generate_combined_transform(&mut self) {
@@ -2924,6 +2924,27 @@ fn static_assert() {
         self.copy_offset_path_from(other);
     }
 
+    #[allow(non_snake_case)]
+    pub fn set__webkit_line_clamp(&mut self, v: longhands::_webkit_line_clamp::computed_value::T) {
+        self.gecko.mLineClamp = match v {
+            Either::First(n) => n.0 as u32,
+            Either::Second(None_) => 0,
+        };
+    }
+
+    ${impl_simple_copy('_webkit_line_clamp', 'mLineClamp')}
+
+    #[allow(non_snake_case)]
+    pub fn clone__webkit_line_clamp(&self) -> longhands::_webkit_line_clamp::computed_value::T {
+        match self.gecko.mLineClamp {
+            0 => Either::Second(None_),
+            n => {
+                debug_assert!(n <= std::i32::MAX as u32);
+                Either::First((n as i32).into())
+            }
+        }
+    }
+
 </%self:impl_trait>
 
 <%def name="simple_image_array_property(name, shorthand, field_name)">
diff --git a/components/style/properties/longhands/box.mako.rs b/components/style/properties/longhands/box.mako.rs
index a7a2cb9e348..64239eeb83a 100644
--- a/components/style/properties/longhands/box.mako.rs
+++ b/components/style/properties/longhands/box.mako.rs
@@ -670,3 +670,15 @@ ${helpers.predefined_type(
     animation_value_type="discrete",
     spec="https://compat.spec.whatwg.org/#touch-action",
 )}
+
+// Note that we only implement -webkit-line-clamp as a single, longhand
+// property for now, but the spec defines line-clamp as a shorthand for separate
+// max-lines, block-ellipsis, and continue properties.
+${helpers.predefined_type(
+    "-webkit-line-clamp",
+    "PositiveIntegerOrNone",
+    "Either::Second(None_)",
+    gecko_pref="layout.css.webkit-line-clamp.enabled",
+    animation_value_type="Integer",
+    spec="https://drafts.csswg.org/css-overflow-3/#line-clamp",
+)}
diff --git a/components/style/values/computed/mod.rs b/components/style/values/computed/mod.rs
index 3eb0c16836c..f7f14c79d9c 100644
--- a/components/style/values/computed/mod.rs
+++ b/components/style/values/computed/mod.rs
@@ -640,6 +640,9 @@ impl From<CSSInteger> for PositiveInteger {
     }
 }
 
+/// A computed positive `<integer>` value or `none`.
+pub type PositiveIntegerOrNone = Either<PositiveInteger, None_>;
+
 /// rect(...)
 pub type ClipRect = generics::ClipRect<LengthOrAuto>;
 
diff --git a/components/style/values/specified/mod.rs b/components/style/values/specified/mod.rs
index 7dbb93f6319..83fc71444bb 100644
--- a/components/style/values/specified/mod.rs
+++ b/components/style/values/specified/mod.rs
@@ -12,7 +12,7 @@ use super::generics::grid::{GridLine as GenericGridLine, TrackBreadth as Generic
 use super::generics::grid::{TrackList as GenericTrackList, TrackSize as GenericTrackSize};
 use super::generics::transform::IsParallelTo;
 use super::generics::{self, GreaterThanOrEqualToOne, NonNegative};
-use super::{Auto, CSSFloat, CSSInteger, Either};
+use super::{Auto, CSSFloat, CSSInteger, Either, None_};
 use crate::context::QuirksMode;
 use crate::parser::{Parse, ParserContext};
 use crate::values::serialize_atom_identifier;
@@ -593,6 +593,9 @@ impl Parse for PositiveInteger {
     }
 }
 
+/// A specified positive `<integer>` value or `none`.
+pub type PositiveIntegerOrNone = Either<PositiveInteger, None_>;
+
 /// The specified value of a grid `<track-breadth>`
 pub type TrackBreadth = GenericTrackBreadth<LengthPercentage>;
 

From 4b761848a05b95a9b254c4e59e82fffa8e7b32bd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 9 May 2019 05:45:10 +0200
Subject: [PATCH 08/14] style: Remove unnecessary mut usage.

---
 components/style/properties/declaration_block.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs
index 5ce80955a48..1ce083c944f 100644
--- a/components/style/properties/declaration_block.rs
+++ b/components/style/properties/declaration_block.rs
@@ -1382,7 +1382,7 @@ fn report_css_errors(
     selectors: Option<&SelectorList<SelectorImpl>>,
     errors: &mut SmallParseErrorVec,
 ) {
-    for (mut error, slice, property) in errors.drain() {
+    for (error, slice, property) in errors.drain() {
         report_one_css_error(context, Some(block), selectors, error, slice, property)
     }
 }

From 330bccd6595007b62a6b0f8629b717a0c6ac30bf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 9 May 2019 10:49:22 +0000
Subject: [PATCH 09/14] style: Add an owned slice type which cbindgen can
 understand.

Passing these by value won't be ok of course, but that's fine.

I plan to combine this with https://github.com/eqrion/cbindgen/pull/333 to
actually be able to share representation for ~all the things, this is just the
first bit.

Box<T>, Atom and Arc<T> will be much easier since cbindgen can understand them
without issues.

It's boxed slices the only ones I should need something like this. I could avoid
it if I rely on Rust's internal representation, which we can per [1], but then I
need to teach cbindgen all about slices, which is generally hard, I think.

[1]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/reference/src/layout/pointers.md

Differential Revision: https://phabricator.services.mozilla.com/D29768
---
 components/style/lib.rs                 |   3 +
 components/style/owned_slice.rs         | 163 ++++++++++++++++++++++++
 components/style/values/animated/mod.rs |  44 +++++++
 components/style/values/computed/mod.rs |  24 ++++
 components/style/values/resolved/mod.rs |  17 +++
 components/to_shmem/lib.rs              |   2 +-
 6 files changed, 252 insertions(+), 1 deletion(-)
 create mode 100644 components/style/owned_slice.rs

diff --git a/components/style/lib.rs b/components/style/lib.rs
index bd33dbeafe4..65f2eda52ff 100644
--- a/components/style/lib.rs
+++ b/components/style/lib.rs
@@ -143,6 +143,7 @@ pub mod logical_geometry;
 pub mod matching;
 #[macro_use]
 pub mod media_queries;
+pub mod owned_slice;
 pub mod parallel;
 pub mod parser;
 pub mod rule_cache;
@@ -188,6 +189,8 @@ pub use html5ever::Prefix;
 #[cfg(feature = "servo")]
 pub use servo_atoms::Atom;
 
+pub use owned_slice::OwnedSlice;
+
 /// The CSS properties supported by the style system.
 /// Generated from the properties.mako.rs template by build.rs
 #[macro_use]
diff --git a/components/style/owned_slice.rs b/components/style/owned_slice.rs
new file mode 100644
index 00000000000..a92a6269e70
--- /dev/null
+++ b/components/style/owned_slice.rs
@@ -0,0 +1,163 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+//! A replacement for `Box<[T]>` that cbindgen can understand.
+
+use std::marker::PhantomData;
+use std::{fmt, mem, slice};
+use std::ptr::NonNull;
+use std::ops::{Deref, DerefMut};
+use malloc_size_of::{MallocSizeOf, MallocShallowSizeOf, MallocSizeOfOps};
+use to_shmem::{SharedMemoryBuilder, ToShmem};
+
+/// A struct that basically replaces a `Box<[T]>`, but which cbindgen can
+/// understand.
+///
+/// We could rely on the struct layout of `Box<[T]>` per:
+///
+///   https://github.com/rust-lang/unsafe-code-guidelines/blob/master/reference/src/layout/pointers.md
+///
+/// But handling fat pointers with cbindgen both in structs and argument
+/// positions more generally is a bit tricky.
+#[repr(C)]
+pub struct OwnedSlice<T: Sized> {
+    ptr: NonNull<T>,
+    len: usize,
+    _phantom: PhantomData<T>,
+}
+
+impl<T: Sized> Default for OwnedSlice<T> {
+    #[inline]
+    fn default() -> Self {
+        Self {
+            len: 0,
+            ptr: NonNull::dangling(),
+            _phantom: PhantomData,
+        }
+    }
+}
+
+impl<T: Sized> Drop for OwnedSlice<T> {
+    #[inline]
+    fn drop(&mut self) {
+        if self.len != 0 {
+            let _ = mem::replace(self, Self::default()).into_vec();
+        }
+    }
+}
+
+unsafe impl<T: Sized + Send> Send for OwnedSlice<T> {}
+unsafe impl<T: Sized + Sync> Sync for OwnedSlice<T> {}
+
+impl<T: Clone> Clone for OwnedSlice<T> {
+    #[inline]
+    fn clone(&self) -> Self {
+        Self::from_slice(&**self)
+    }
+}
+
+impl<T: fmt::Debug> fmt::Debug for OwnedSlice<T> {
+    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+        self.deref().fmt(formatter)
+    }
+}
+
+impl<T: PartialEq> PartialEq for OwnedSlice<T> {
+    fn eq(&self, other: &Self) -> bool {
+        self.deref().eq(other.deref())
+    }
+}
+
+impl<T: Eq> Eq for OwnedSlice<T> {}
+
+impl<T: Sized> OwnedSlice<T> {
+    /// Convert the OwnedSlice into a boxed slice.
+    #[inline]
+    pub fn into_box(self) -> Box<[T]> {
+        self.into_vec().into_boxed_slice()
+    }
+
+    /// Convert the OwnedSlice into a Vec.
+    #[inline]
+    pub fn into_vec(self) -> Vec<T> {
+        let ret = unsafe {
+            Vec::from_raw_parts(self.ptr.as_ptr(), self.len, self.len)
+        };
+        mem::forget(self);
+        ret
+    }
+
+    /// Iterate over all the elements in the slice taking ownership of them.
+    #[inline]
+    pub fn into_iter(self) -> impl Iterator<Item = T> {
+        self.into_vec().into_iter()
+    }
+
+    /// Convert the regular slice into an owned slice.
+    #[inline]
+    pub fn from_slice(s: &[T]) -> Self
+    where
+        T: Clone,
+    {
+        Self::from(s.to_vec())
+    }
+}
+
+impl<T> Deref for OwnedSlice<T> {
+    type Target = [T];
+
+    #[inline(always)]
+    fn deref(&self) -> &Self::Target {
+        unsafe { slice::from_raw_parts(self.ptr.as_ptr(), self.len) }
+    }
+}
+
+impl<T> DerefMut for OwnedSlice<T> {
+    #[inline(always)]
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        unsafe { slice::from_raw_parts_mut(self.ptr.as_ptr(), self.len) }
+    }
+}
+
+impl<T> From<Box<[T]>> for OwnedSlice<T> {
+    #[inline]
+    fn from(mut b: Box<[T]>) -> Self {
+        let len = b.len();
+        let ptr = unsafe { NonNull::new_unchecked(b.as_mut_ptr()) };
+        mem::forget(b);
+        Self {
+            len,
+            ptr,
+            _phantom: PhantomData,
+        }
+    }
+}
+
+impl<T> From<Vec<T>> for OwnedSlice<T> {
+    #[inline]
+    fn from(b: Vec<T>) -> Self {
+        Self::from(b.into_boxed_slice())
+    }
+}
+
+impl<T: Sized> MallocShallowSizeOf for OwnedSlice<T> {
+    fn shallow_size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
+        unsafe { ops.malloc_size_of(self.ptr.as_ptr()) }
+    }
+}
+
+impl<T: MallocSizeOf + Sized> MallocSizeOf for OwnedSlice<T> {
+    fn size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
+        self.shallow_size_of(ops) + (**self).size_of(ops)
+    }
+}
+
+impl<T: ToShmem + Sized> ToShmem for OwnedSlice<T> {
+    fn to_shmem(&self, builder: &mut SharedMemoryBuilder) -> mem::ManuallyDrop<Self> {
+        unsafe {
+            let dest = to_shmem::to_shmem_slice(self.iter(), builder);
+            mem::ManuallyDrop::new(Self::from(Box::from_raw(dest)))
+        }
+    }
+}
diff --git a/components/style/values/animated/mod.rs b/components/style/values/animated/mod.rs
index 3b83c71e6e2..a1e5ca4618c 100644
--- a/components/style/values/animated/mod.rs
+++ b/components/style/values/animated/mod.rs
@@ -288,6 +288,50 @@ where
     }
 }
 
+impl<T> ToAnimatedValue for Box<[T]>
+where
+    T: ToAnimatedValue,
+{
+    type AnimatedValue = Box<[<T as ToAnimatedValue>::AnimatedValue]>;
+
+    #[inline]
+    fn to_animated_value(self) -> Self::AnimatedValue {
+        self
+            .into_vec()
+            .into_iter()
+            .map(T::to_animated_value)
+            .collect::<Vec<_>>()
+            .into_boxed_slice()
+    }
+
+    #[inline]
+    fn from_animated_value(animated: Self::AnimatedValue) -> Self {
+        animated
+            .into_vec()
+            .into_iter()
+            .map(T::from_animated_value)
+            .collect::<Vec<_>>()
+            .into_boxed_slice()
+    }
+}
+
+impl<T> ToAnimatedValue for crate::OwnedSlice<T>
+where
+    T: ToAnimatedValue,
+{
+    type AnimatedValue = crate::OwnedSlice<<T as ToAnimatedValue>::AnimatedValue>;
+
+    #[inline]
+    fn to_animated_value(self) -> Self::AnimatedValue {
+        self.into_box().to_animated_value().into()
+    }
+
+    #[inline]
+    fn from_animated_value(animated: Self::AnimatedValue) -> Self {
+        Box::from_animated_value(animated.into_box()).into()
+    }
+}
+
 impl<T> ToAnimatedValue for SmallVec<[T; 1]>
 where
     T: ToAnimatedValue,
diff --git a/components/style/values/computed/mod.rs b/components/style/values/computed/mod.rs
index f7f14c79d9c..5bf0333dbfe 100644
--- a/components/style/values/computed/mod.rs
+++ b/components/style/values/computed/mod.rs
@@ -444,6 +444,30 @@ where
     }
 }
 
+impl<T> ToComputedValue for crate::OwnedSlice<T>
+where
+    T: ToComputedValue,
+{
+    type ComputedValue = crate::OwnedSlice<<T as ToComputedValue>::ComputedValue>;
+
+    #[inline]
+    fn to_computed_value(&self, context: &Context) -> Self::ComputedValue {
+        self.iter()
+            .map(|item| item.to_computed_value(context))
+            .collect::<Vec<_>>()
+            .into()
+    }
+
+    #[inline]
+    fn from_computed_value(computed: &Self::ComputedValue) -> Self {
+        computed
+            .iter()
+            .map(T::from_computed_value)
+            .collect::<Vec<_>>()
+            .into()
+    }
+}
+
 trivial_to_computed_value!(());
 trivial_to_computed_value!(bool);
 trivial_to_computed_value!(f32);
diff --git a/components/style/values/resolved/mod.rs b/components/style/values/resolved/mod.rs
index 4003060148c..379ea83ec6b 100644
--- a/components/style/values/resolved/mod.rs
+++ b/components/style/values/resolved/mod.rs
@@ -193,3 +193,20 @@ where
         Vec::from_resolved_value(Vec::from(resolved)).into_boxed_slice()
     }
 }
+
+impl<T> ToResolvedValue for crate::OwnedSlice<T>
+where
+    T: ToResolvedValue,
+{
+    type ResolvedValue = crate::OwnedSlice<<T as ToResolvedValue>::ResolvedValue>;
+
+    #[inline]
+    fn to_resolved_value(self, context: &Context) -> Self::ResolvedValue {
+        self.into_box().to_resolved_value(context).into()
+    }
+
+    #[inline]
+    fn from_resolved_value(resolved: Self::ResolvedValue) -> Self {
+        Self::from(Box::from_resolved_value(resolved.into_box()))
+    }
+}
diff --git a/components/to_shmem/lib.rs b/components/to_shmem/lib.rs
index 96dfbb3a5e9..07c5fc9ccaa 100644
--- a/components/to_shmem/lib.rs
+++ b/components/to_shmem/lib.rs
@@ -311,7 +311,7 @@ where
 
 /// Writes all the items in `src` into a slice in the shared memory buffer and
 /// returns a pointer to the slice.
-unsafe fn to_shmem_slice<'a, T, I>(src: I, builder: &mut SharedMemoryBuilder) -> *mut [T]
+pub unsafe fn to_shmem_slice<'a, T, I>(src: I, builder: &mut SharedMemoryBuilder) -> *mut [T]
 where
     T: 'a + ToShmem,
     I: ExactSizeIterator<Item = &'a T>,

From 559235edadc415b750c75ee4fd2d3d7afadf90bb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 9 May 2019 11:24:57 +0000
Subject: [PATCH 10/14] 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<BasicShape> to
UniquePtr<BasicShape>. 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("<shape-radius> should be a length, percentage, calc, or keyword value")
-        }
-    }
-
     impl From<ShapeBox> 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<L: GeckoStyleCoordConvertible> GeckoStyleCoordConvertible for TrackBreadth<
     }
 }
 
-impl GeckoStyleCoordConvertible for ComputedShapeRadius {
-    fn to_gecko_style_coord<T: CoordDataMut>(&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<T: CoordData>(coord: &T) -> Option<Self> {
-        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<T: GeckoStyleCoordConvertible> GeckoStyleCoordConvertible for Option<T> {
     fn to_gecko_style_coord<U: CoordDataMut>(&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<T: Sized> {
     ptr: NonNull<T>,
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<T> Animate for Point2D<T>
-where
-    T: Animate,
-{
+impl<T: Animate> Animate for Box<T> {
     #[inline]
     fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
-        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<T> ToAnimatedValue for Box<T>
+where
+    T: ToAnimatedValue,
+{
+    type AnimatedValue = Box<<T as ToAnimatedValue>::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<T> 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<BasicShape, ComputedUrl>;
 pub type FloatAreaShape = generic::FloatAreaShape<BasicShape, Image>;
 
 /// 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<LengthPercentage, LengthPercentage, NonNegativeLengthPercentage>;
 
 /// The computed value of `ShapeRadius`
-pub type ShapeRadius = generic::ShapeRadius<NonNegativeLengthPercentage>;
+pub type ShapeRadius = generic::GenericShapeRadius<NonNegativeLengthPercentage>;
 
 impl ToCss for Circle {
     fn to_css<W>(&self, dest: &mut CssWriter<W>) -> 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<BasicShape, ReferenceBox, ImageOrUrl> {
     #[animation(error)]
     ImageOrUrl(ImageOrUrl),
-    Shape(BasicShape, Option<ReferenceBox>),
+    Shape(Box<BasicShape>, Option<ReferenceBox>),
     #[animation(error)]
     Box(ReferenceBox),
     #[css(function)]
@@ -113,7 +113,8 @@ pub enum ShapeSource<BasicShape, ReferenceBox, ImageOrUrl> {
     ToResolvedValue,
     ToShmem,
 )]
-pub enum BasicShape<H, V, LengthPercentage, NonNegativeLengthPercentage> {
+#[repr(C, u8)]
+pub enum GenericBasicShape<H, V, LengthPercentage, NonNegativeLengthPercentage> {
     Inset(
         #[css(field_bound)]
         #[shmem(field_bound)]
@@ -129,9 +130,11 @@ pub enum BasicShape<H, V, LengthPercentage, NonNegativeLengthPercentage> {
         #[shmem(field_bound)]
         Ellipse<H, V, NonNegativeLengthPercentage>,
     ),
-    Polygon(Polygon<LengthPercentage>),
+    Polygon(GenericPolygon<LengthPercentage>),
 }
 
+pub use self::GenericBasicShape as BasicShape;
+
 /// <https://drafts.csswg.org/css-shapes/#funcdef-inset>
 #[allow(missing_docs)]
 #[css(function = "inset")]
@@ -148,10 +151,11 @@ pub enum BasicShape<H, V, LengthPercentage, NonNegativeLengthPercentage> {
     ToResolvedValue,
     ToShmem,
 )]
+#[repr(C)]
 pub struct InsetRect<LengthPercentage, NonNegativeLengthPercentage> {
     pub rect: Rect<LengthPercentage>,
     #[shmem(field_bound)]
-    pub round: BorderRadius<NonNegativeLengthPercentage>,
+    pub round: GenericBorderRadius<NonNegativeLengthPercentage>,
 }
 
 /// <https://drafts.csswg.org/css-shapes/#funcdef-circle>
@@ -171,9 +175,10 @@ pub struct InsetRect<LengthPercentage, NonNegativeLengthPercentage> {
     ToResolvedValue,
     ToShmem,
 )]
+#[repr(C)]
 pub struct Circle<H, V, NonNegativeLengthPercentage> {
-    pub position: Position<H, V>,
-    pub radius: ShapeRadius<NonNegativeLengthPercentage>,
+    pub position: GenericPosition<H, V>,
+    pub radius: GenericShapeRadius<NonNegativeLengthPercentage>,
 }
 
 /// <https://drafts.csswg.org/css-shapes/#funcdef-ellipse>
@@ -193,10 +198,11 @@ pub struct Circle<H, V, NonNegativeLengthPercentage> {
     ToResolvedValue,
     ToShmem,
 )]
+#[repr(C)]
 pub struct Ellipse<H, V, NonNegativeLengthPercentage> {
-    pub position: Position<H, V>,
-    pub semiaxis_x: ShapeRadius<NonNegativeLengthPercentage>,
-    pub semiaxis_y: ShapeRadius<NonNegativeLengthPercentage>,
+    pub position: GenericPosition<H, V>,
+    pub semiaxis_x: GenericShapeRadius<NonNegativeLengthPercentage>,
+    pub semiaxis_y: GenericShapeRadius<NonNegativeLengthPercentage>,
 }
 
 /// <https://drafts.csswg.org/css-shapes/#typedef-shape-radius>
@@ -216,7 +222,8 @@ pub struct Ellipse<H, V, NonNegativeLengthPercentage> {
     ToResolvedValue,
     ToShmem,
 )]
-pub enum ShapeRadius<NonNegativeLengthPercentage> {
+#[repr(C, u8)]
+pub enum GenericShapeRadius<NonNegativeLengthPercentage> {
     Length(NonNegativeLengthPercentage),
     #[animation(error)]
     ClosestSide,
@@ -224,10 +231,12 @@ pub enum ShapeRadius<NonNegativeLengthPercentage> {
     FarthestSide,
 }
 
+pub use self::GenericShapeRadius as ShapeRadius;
+
 /// A generic type for representing the `polygon()` function
 ///
 /// <https://drafts.csswg.org/css-shapes/#funcdef-polygon>
-#[css(comma, function)]
+#[css(comma, function = "polygon")]
 #[derive(
     Clone,
     Debug,
@@ -240,15 +249,18 @@ pub enum ShapeRadius<NonNegativeLengthPercentage> {
     ToResolvedValue,
     ToShmem,
 )]
-pub struct Polygon<LengthPercentage> {
+#[repr(C)]
+pub struct GenericPolygon<LengthPercentage> {
     /// 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<PolygonCoord<LengthPercentage>>,
+    pub coordinates: crate::OwnedSlice<PolygonCoord<LengthPercentage>>,
 }
 
+pub use self::GenericPolygon as Polygon;
+
 /// Coordinates for Polygon.
 #[derive(
     Clone,
@@ -262,6 +274,7 @@ pub struct Polygon<LengthPercentage> {
     ToResolvedValue,
     ToShmem,
 )]
+#[repr(C)]
 pub struct PolygonCoord<LengthPercentage>(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::<Result<Vec<_>, _>>()?;
+            .collect::<Result<Vec<_>, _>>()?
+            .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<NonNegativeLengthPercentage>;
 
 /// The specified value of `Polygon`
-pub type Polygon = generic::Polygon<LengthPercentage>;
+pub type Polygon = generic::GenericPolygon<LengthPercentage>;
 
 #[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<Self, ParseError<'i>> {
-        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 })
     }
 }
 

From 2ed2151b3dfe9e98ad997a64b4cf334b967e5db3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 9 May 2019 10:49:50 +0000
Subject: [PATCH 11/14] style: Move OwnedSlice to style_traits.

Differential Revision: https://phabricator.services.mozilla.com/D30126
---
 components/style/lib.rs                           | 3 +--
 components/style_traits/lib.rs                    | 1 +
 components/{style => style_traits}/owned_slice.rs | 2 ++
 3 files changed, 4 insertions(+), 2 deletions(-)
 rename components/{style => style_traits}/owned_slice.rs (99%)

diff --git a/components/style/lib.rs b/components/style/lib.rs
index 65f2eda52ff..10d45408065 100644
--- a/components/style/lib.rs
+++ b/components/style/lib.rs
@@ -143,7 +143,6 @@ pub mod logical_geometry;
 pub mod matching;
 #[macro_use]
 pub mod media_queries;
-pub mod owned_slice;
 pub mod parallel;
 pub mod parser;
 pub mod rule_cache;
@@ -189,7 +188,7 @@ pub use html5ever::Prefix;
 #[cfg(feature = "servo")]
 pub use servo_atoms::Atom;
 
-pub use owned_slice::OwnedSlice;
+pub use style_traits::owned_slice::OwnedSlice;
 
 /// The CSS properties supported by the style system.
 /// Generated from the properties.mako.rs template by build.rs
diff --git a/components/style_traits/lib.rs b/components/style_traits/lib.rs
index bce0a507f75..14ce2c9801e 100644
--- a/components/style_traits/lib.rs
+++ b/components/style_traits/lib.rs
@@ -89,6 +89,7 @@ pub mod specified_value_info;
 pub mod values;
 #[macro_use]
 pub mod viewport;
+pub mod owned_slice;
 
 pub use crate::specified_value_info::{CssType, KeywordsCollectFn, SpecifiedValueInfo};
 pub use crate::values::{
diff --git a/components/style/owned_slice.rs b/components/style_traits/owned_slice.rs
similarity index 99%
rename from components/style/owned_slice.rs
rename to components/style_traits/owned_slice.rs
index e1a82c0176f..6e54aaeef5f 100644
--- a/components/style/owned_slice.rs
+++ b/components/style_traits/owned_slice.rs
@@ -2,6 +2,8 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
+#![allow(unsafe_code)]
+
 //! A replacement for `Box<[T]>` that cbindgen can understand.
 
 use std::marker::PhantomData;

From 0d5c4481b8b5f96325260ee0ffc2b5f206c97548 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 9 May 2019 10:53:50 +0000
Subject: [PATCH 12/14] style: Introduce ArcSlice, a small wrapper over ThinArc
 but without an explicit header.

We could make the header PhantomData or something, but then we wouldn't be able
to bind to C++, since C++ doesn't have ZSTs. So add a canary instead to add a
runtime check of stuff being sane.

Differential Revision: https://phabricator.services.mozilla.com/D30133
---
 components/servo_arc/lib.rs                   | 17 ++++-
 components/style/lib.rs                       |  1 +
 components/style/values/animated/mod.rs       | 14 ++++
 components/style_traits/arc_slice.rs          | 66 +++++++++++++++++++
 components/style_traits/lib.rs                |  1 +
 .../style_traits/specified_value_info.rs      |  2 +
 6 files changed, 98 insertions(+), 3 deletions(-)
 create mode 100644 components/style_traits/arc_slice.rs

diff --git a/components/servo_arc/lib.rs b/components/servo_arc/lib.rs
index 40a8ff3d193..09b81920164 100644
--- a/components/servo_arc/lib.rs
+++ b/components/servo_arc/lib.rs
@@ -784,6 +784,13 @@ pub struct ThinArc<H, T> {
     phantom: PhantomData<(H, T)>,
 }
 
+
+impl<H: fmt::Debug, T: fmt::Debug> fmt::Debug for ThinArc<H, T> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        fmt::Debug::fmt(self.deref(), f)
+    }
+}
+
 unsafe impl<H: Sync + Send, T: Sync + Send> Send for ThinArc<H, T> {}
 unsafe impl<H: Sync + Send, T: Sync + Send> Sync for ThinArc<H, T> {}
 
@@ -856,8 +863,12 @@ impl<H, T> ThinArc<H, T> {
     }
 
     /// Returns the address on the heap of the ThinArc itself -- not the T
-    /// within it -- for memory reporting.
-    ///
+    /// within it -- for memory reporting, and bindings.
+    #[inline]
+    pub fn ptr(&self) -> *const c_void {
+        self.ptr.as_ptr() as *const ArcInner<T> as *const c_void
+    }
+
     /// If this is a static ThinArc, this returns null.
     #[inline]
     pub fn heap_ptr(&self) -> *const c_void {
@@ -866,7 +877,7 @@ impl<H, T> ThinArc<H, T> {
         if is_static {
             ptr::null()
         } else {
-            self.ptr.as_ptr() as *const ArcInner<T> as *const c_void
+            self.ptr()
         }
     }
 }
diff --git a/components/style/lib.rs b/components/style/lib.rs
index 10d45408065..4d523d65c4f 100644
--- a/components/style/lib.rs
+++ b/components/style/lib.rs
@@ -188,6 +188,7 @@ pub use html5ever::Prefix;
 #[cfg(feature = "servo")]
 pub use servo_atoms::Atom;
 
+pub use style_traits::arc_slice::ArcSlice;
 pub use style_traits::owned_slice::OwnedSlice;
 
 /// The CSS properties supported by the style system.
diff --git a/components/style/values/animated/mod.rs b/components/style/values/animated/mod.rs
index 6f8de1decca..29e74859cd6 100644
--- a/components/style/values/animated/mod.rs
+++ b/components/style/values/animated/mod.rs
@@ -462,3 +462,17 @@ where
         Ok(v.into_boxed_slice())
     }
 }
+
+impl<T> ToAnimatedZero for crate::ArcSlice<T>
+where
+    T: ToAnimatedZero,
+{
+    #[inline]
+    fn to_animated_zero(&self) -> Result<Self, ()> {
+        let v = self
+            .iter()
+            .map(|v| v.to_animated_zero())
+            .collect::<Result<Vec<_>, _>>()?;
+        Ok(crate::ArcSlice::from_iter(v.into_iter()))
+    }
+}
diff --git a/components/style_traits/arc_slice.rs b/components/style_traits/arc_slice.rs
new file mode 100644
index 00000000000..f7097e3d086
--- /dev/null
+++ b/components/style_traits/arc_slice.rs
@@ -0,0 +1,66 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+//! A thin atomically-reference-counted slice.
+
+use servo_arc::ThinArc;
+use std::mem;
+use std::ops::Deref;
+use std::ptr::NonNull;
+
+/// A canary that we stash in ArcSlices.
+///
+/// Given we cannot use a zero-sized-type for the header, since well, C++
+/// doesn't have zsts, and we want to use cbindgen for this type, we may as well
+/// assert some sanity at runtime.
+const ARC_SLICE_CANARY: u32 = 0xf3f3f3f3;
+
+/// A wrapper type for a refcounted slice using ThinArc.
+///
+/// cbindgen:derive-eq=false
+/// cbindgen:derive-neq=false
+#[repr(C)]
+#[derive(Debug, Clone, PartialEq, Eq, ToShmem)]
+pub struct ArcSlice<T>(#[shmem(field_bound)] ThinArc<u32, T>);
+
+impl<T> Deref for ArcSlice<T> {
+    type Target = [T];
+
+    #[inline]
+    fn deref(&self) -> &Self::Target {
+        debug_assert_eq!(self.0.header.header, ARC_SLICE_CANARY);
+        &self.0.slice
+    }
+}
+
+/// The inner pointer of an ArcSlice<T>, to be sent via FFI.
+/// The type of the pointer is a bit of a lie, we just want to preserve the type
+/// but these pointers cannot be constructed outside of this crate, so we're
+/// good.
+#[repr(C)]
+pub struct ForgottenArcSlicePtr<T>(NonNull<T>);
+
+impl<T> ArcSlice<T> {
+    /// Creates an Arc for a slice using the given iterator to generate the
+    /// slice.
+    #[inline]
+    pub fn from_iter<I>(items: I) -> Self
+    where
+        I: Iterator<Item = T> + ExactSizeIterator,
+    {
+        ArcSlice(ThinArc::from_header_and_iter(ARC_SLICE_CANARY, items))
+    }
+
+    /// Creates a value that can be passed via FFI, and forgets this value
+    /// altogether.
+    #[inline]
+    #[allow(unsafe_code)]
+    pub fn forget(self) -> ForgottenArcSlicePtr<T> {
+        let ret = unsafe {
+            ForgottenArcSlicePtr(NonNull::new_unchecked(self.0.ptr() as *const _ as *mut _))
+        };
+        mem::forget(self);
+        ret
+    }
+}
diff --git a/components/style_traits/lib.rs b/components/style_traits/lib.rs
index 14ce2c9801e..d4a907fa956 100644
--- a/components/style_traits/lib.rs
+++ b/components/style_traits/lib.rs
@@ -84,6 +84,7 @@ pub enum CSSPixel {}
 //   / hidpi_ratio => DeviceIndependentPixel
 //     / desktop_zoom => CSSPixel
 
+pub mod arc_slice;
 pub mod specified_value_info;
 #[macro_use]
 pub mod values;
diff --git a/components/style_traits/specified_value_info.rs b/components/style_traits/specified_value_info.rs
index c978727b47a..63a7b71d990 100644
--- a/components/style_traits/specified_value_info.rs
+++ b/components/style_traits/specified_value_info.rs
@@ -4,6 +4,7 @@
 
 //! Value information for devtools.
 
+use crate::arc_slice::ArcSlice;
 use servo_arc::Arc;
 use std::ops::Range;
 use std::sync::Arc as StdArc;
@@ -116,6 +117,7 @@ impl_generic_specified_value_info!(Option<T>);
 impl_generic_specified_value_info!(Vec<T>);
 impl_generic_specified_value_info!(Arc<T>);
 impl_generic_specified_value_info!(StdArc<T>);
+impl_generic_specified_value_info!(ArcSlice<T>);
 impl_generic_specified_value_info!(Range<Idx>);
 
 impl<T1, T2> SpecifiedValueInfo for (T1, T2)

From f429c28f23bb21812deb042bf50e4bf16b9e1f1b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 9 May 2019 12:43:19 +0000
Subject: [PATCH 13/14] style: Add bindings for ArcSlice and ThinArc, and use
 them to reduce copies of SVG path data.

As I said over bug 1549593, the eventual goal is to use ArcSlice in all
inherited properties. But this seemed like a good first candidate that doesn't
require me to move around a lot more code, since we were already using cbindgen
for the path commands.

Differential Revision: https://phabricator.services.mozilla.com/D30134
---
 components/servo_arc/lib.rs                   |  8 ++++
 components/style/gecko/conversions.rs         |  4 +-
 components/style/properties/gecko.mako.rs     | 28 +++++--------
 .../style/properties/longhands/svg.mako.rs    |  1 -
 components/style/values/specified/svg_path.rs | 39 ++++++++++---------
 5 files changed, 39 insertions(+), 41 deletions(-)

diff --git a/components/servo_arc/lib.rs b/components/servo_arc/lib.rs
index 09b81920164..b7c179cd13f 100644
--- a/components/servo_arc/lib.rs
+++ b/components/servo_arc/lib.rs
@@ -308,6 +308,9 @@ impl<T: ?Sized> Arc<T> {
 impl<T: ?Sized> Clone for Arc<T> {
     #[inline]
     fn clone(&self) -> Self {
+        // NOTE(emilio): If you change anything here, make sure that the
+        // implementation in layout/style/ServoStyleConstsInlines.h matches!
+        //
         // Using a relaxed ordering to check for STATIC_REFCOUNT is safe, since
         // `count` never changes between STATIC_REFCOUNT and other values.
         if self.inner().count.load(Relaxed) != STATIC_REFCOUNT {
@@ -416,6 +419,9 @@ impl<T: ?Sized> Arc<T> {
 impl<T: ?Sized> Drop for Arc<T> {
     #[inline]
     fn drop(&mut self) {
+        // NOTE(emilio): If you change anything here, make sure that the
+        // implementation in layout/style/ServoStyleConstsInlines.h matches!
+        //
         // Using a relaxed ordering to check for STATIC_REFCOUNT is safe, since
         // `count` never changes between STATIC_REFCOUNT and other values.
         if self.inner().count.load(Relaxed) == STATIC_REFCOUNT {
@@ -571,6 +577,7 @@ impl<T: Serialize> Serialize for Arc<T> {
 /// Structure to allow Arc-managing some fixed-sized data and a variably-sized
 /// slice in a single allocation.
 #[derive(Debug, Eq, PartialEq, PartialOrd)]
+#[repr(C)]
 pub struct HeaderSlice<H, T: ?Sized> {
     /// The fixed-sized data.
     pub header: H,
@@ -743,6 +750,7 @@ impl<H, T> Arc<HeaderSlice<H, [T]>> {
 /// Header data with an inline length. Consumers that use HeaderWithLength as the
 /// Header type in HeaderSlice can take advantage of ThinArc.
 #[derive(Debug, Eq, PartialEq, PartialOrd)]
+#[repr(C)]
 pub struct HeaderWithLength<H> {
     /// The fixed-sized data.
     pub header: H,
diff --git a/components/style/gecko/conversions.rs b/components/style/gecko/conversions.rs
index 748396630fb..607c7388c3d 100644
--- a/components/style/gecko/conversions.rs
+++ b/components/style/gecko/conversions.rs
@@ -577,12 +577,10 @@ pub mod basic_shape {
 
         /// Generate a SVGPathData from StyleShapeSource if possible.
         fn to_svg_path(&self) -> Option<SVGPathData> {
-            use crate::values::specified::svg_path::PathCommand;
             match self.mType {
                 StyleShapeSourceType::Path => {
                     let gecko_path = unsafe { &*self.__bindgen_anon_1.mSVGPath.as_ref().mPtr };
-                    let result: Vec<PathCommand> = gecko_path.mPath.iter().cloned().collect();
-                    Some(SVGPathData::new(result.into_boxed_slice()))
+                    Some(SVGPathData(gecko_path.mPath.clone()))
                 },
                 _ => None,
             }
diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs
index 1f7603c923d..703cbed2c0b 100644
--- a/components/style/properties/gecko.mako.rs
+++ b/components/style/properties/gecko.mako.rs
@@ -2901,7 +2901,7 @@ fn static_assert() {
         match v {
             OffsetPath::None => motion.mOffsetPath.mType = StyleShapeSourceType::None,
             OffsetPath::Path(p) => {
-                set_style_svg_path(&mut motion.mOffsetPath, &p, FillRule::Nonzero)
+                set_style_svg_path(&mut motion.mOffsetPath, p, FillRule::Nonzero)
             },
         }
         unsafe { Gecko_SetStyleMotion(&mut self.gecko.mMotion, motion) };
@@ -4023,25 +4023,17 @@ fn static_assert() {
 // Set SVGPathData to StyleShapeSource.
 fn set_style_svg_path(
     shape_source: &mut structs::mozilla::StyleShapeSource,
-    servo_path: &values::specified::svg_path::SVGPathData,
+    servo_path: values::specified::svg_path::SVGPathData,
     fill: values::generics::basic_shape::FillRule,
 ) {
-    use crate::gecko_bindings::bindings::Gecko_NewStyleSVGPath;
-    use crate::gecko_bindings::structs::StyleShapeSourceType;
-
-    // Setup type.
-    shape_source.mType = StyleShapeSourceType::Path;
-
     // Setup path.
-    let gecko_path = unsafe {
-        Gecko_NewStyleSVGPath(shape_source);
-        &mut shape_source.__bindgen_anon_1.mSVGPath.as_mut().mPtr.as_mut().unwrap()
-    };
-
-    gecko_path.mPath.assign_from_iter_pod(servo_path.commands().iter().cloned());
-
-    // Setup fill-rule.
-    gecko_path.mFillRule = fill;
+    unsafe {
+        bindings::Gecko_SetToSVGPath(
+            shape_source,
+            servo_path.0.forget(),
+            fill,
+        );
+    }
 }
 
 <%def name="impl_shape_source(ident, gecko_ffi_name)">
@@ -4081,7 +4073,7 @@ fn set_style_svg_path(
                 ${ident}.mReferenceBox = reference.into();
                 ${ident}.mType = StyleShapeSourceType::Box;
             }
-            ShapeSource::Path(p) => set_style_svg_path(${ident}, &p.path, p.fill),
+            ShapeSource::Path(p) => set_style_svg_path(${ident}, p.path, p.fill),
             ShapeSource::Shape(servo_shape, maybe_box) => {
                 unsafe {
                     ${ident}.__bindgen_anon_1.mBasicShape.as_mut().mPtr =
diff --git a/components/style/properties/longhands/svg.mako.rs b/components/style/properties/longhands/svg.mako.rs
index 19699aa1c96..3e4d207397a 100644
--- a/components/style/properties/longhands/svg.mako.rs
+++ b/components/style/properties/longhands/svg.mako.rs
@@ -88,7 +88,6 @@ ${helpers.predefined_type(
     "generics::basic_shape::ShapeSource::None",
     products="gecko",
     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/specified/svg_path.rs b/components/style/values/specified/svg_path.rs
index 5509312256a..e5276e5cce8 100644
--- a/components/style/values/specified/svg_path.rs
+++ b/components/style/values/specified/svg_path.rs
@@ -29,16 +29,15 @@ use style_traits::{CssWriter, ParseError, StyleParseErrorKind, ToCss};
     ToResolvedValue,
     ToShmem,
 )]
-pub struct SVGPathData(Box<[PathCommand]>);
+#[repr(C)]
+pub struct SVGPathData(
+    // TODO(emilio): Should probably measure this somehow only from the
+    // specified values.
+    #[ignore_malloc_size_of = "Arc"]
+    pub crate::ArcSlice<PathCommand>
+);
 
 impl SVGPathData {
-    /// Return SVGPathData by a slice of PathCommand.
-    #[inline]
-    pub fn new(cmd: Box<[PathCommand]>) -> Self {
-        debug_assert!(!cmd.is_empty());
-        SVGPathData(cmd)
-    }
-
     /// Get the array of PathCommand.
     #[inline]
     pub fn commands(&self) -> &[PathCommand] {
@@ -46,9 +45,9 @@ impl SVGPathData {
         &self.0
     }
 
-    /// Create a normalized copy of this path by converting each relative command to an absolute
-    /// command.
-    fn normalize(&self) -> Self {
+    /// Create a normalized copy of this path by converting each relative
+    /// command to an absolute command.
+    fn normalize(&self) -> Box<[PathCommand]> {
         let mut state = PathTraversalState {
             subpath_start: CoordPair::new(0.0, 0.0),
             pos: CoordPair::new(0.0, 0.0),
@@ -58,7 +57,7 @@ impl SVGPathData {
             .iter()
             .map(|seg| seg.normalize(&mut state))
             .collect::<Vec<_>>();
-        SVGPathData(result.into_boxed_slice())
+        result.into_boxed_slice()
     }
 }
 
@@ -71,7 +70,7 @@ impl ToCss for SVGPathData {
         dest.write_char('"')?;
         {
             let mut writer = SequenceWriter::new(dest, " ");
-            for command in self.0.iter() {
+            for command in self.commands() {
                 writer.item(command)?;
             }
         }
@@ -104,7 +103,7 @@ impl Parse for SVGPathData {
             }
         }
 
-        Ok(SVGPathData::new(path_parser.path.into_boxed_slice()))
+        Ok(SVGPathData(crate::ArcSlice::from_iter(path_parser.path.into_iter())))
     }
 }
 
@@ -114,14 +113,17 @@ impl Animate for SVGPathData {
             return Err(());
         }
 
+        // FIXME(emilio): This allocates three copies of the path, that's not
+        // great! Specially, once we're normalized once, we don't need to
+        // re-normalize again.
         let result = self
             .normalize()
-            .0
             .iter()
-            .zip(other.normalize().0.iter())
+            .zip(other.normalize().iter())
             .map(|(a, b)| a.animate(&b, procedure))
             .collect::<Result<Vec<_>, _>>()?;
-        Ok(SVGPathData::new(result.into_boxed_slice()))
+
+        Ok(SVGPathData(crate::ArcSlice::from_iter(result.into_iter())))
     }
 }
 
@@ -131,9 +133,8 @@ impl ComputeSquaredDistance for SVGPathData {
             return Err(());
         }
         self.normalize()
-            .0
             .iter()
-            .zip(other.normalize().0.iter())
+            .zip(other.normalize().iter())
             .map(|(this, other)| this.compute_squared_distance(&other))
             .sum()
     }

From db2f6aa8ca1f23f78a497cb7cc960ff99f23ac04 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 9 May 2019 19:12:32 +0200
Subject: [PATCH 14/14] style: Rustfmt + build fix.

---
 components/servo_arc/lib.rs                       | 15 +++++++++++----
 components/style/gecko/conversions.rs             |  6 ++----
 components/style/gecko/values.rs                  |  2 +-
 components/style/properties/longhands/box.mako.rs |  1 +
 components/style/values/animated/mod.rs           |  4 +---
 components/style/values/specified/basic_shape.rs  | 14 ++++++++------
 components/style/values/specified/svg_path.rs     |  7 ++++---
 components/style_traits/arc_slice.rs              |  2 +-
 components/style_traits/owned_slice.rs            | 10 ++++------
 9 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/components/servo_arc/lib.rs b/components/servo_arc/lib.rs
index b7c179cd13f..f2f9fa00602 100644
--- a/components/servo_arc/lib.rs
+++ b/components/servo_arc/lib.rs
@@ -687,7 +687,10 @@ impl<H, T> Arc<HeaderSlice<H, [T]>> {
                 }
                 // We should have consumed the buffer exactly, maybe accounting
                 // for some padding from the alignment.
-                debug_assert!((buffer.offset(size as isize) as usize - current as *mut u8 as usize) < align_of::<Self>());
+                debug_assert!(
+                    (buffer.offset(size as isize) as usize - current as *mut u8 as usize) <
+                        align_of::<Self>()
+                );
             }
             assert!(
                 items.next().is_none(),
@@ -792,7 +795,6 @@ pub struct ThinArc<H, T> {
     phantom: PhantomData<(H, T)>,
 }
 
-
 impl<H: fmt::Debug, T: fmt::Debug> fmt::Debug for ThinArc<H, T> {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         fmt::Debug::fmt(self.deref(), f)
@@ -909,7 +911,10 @@ impl<H, T> Clone for ThinArc<H, T> {
 impl<H, T> Drop for ThinArc<H, T> {
     #[inline]
     fn drop(&mut self) {
-        let _ = Arc::from_thin(ThinArc { ptr: self.ptr, phantom: PhantomData, });
+        let _ = Arc::from_thin(ThinArc {
+            ptr: self.ptr,
+            phantom: PhantomData,
+        });
     }
 }
 
@@ -928,7 +933,9 @@ impl<H, T> Arc<HeaderSliceWithLength<H, [T]>> {
         let thin_ptr = fat_ptr as *mut [usize] as *mut usize;
         ThinArc {
             ptr: unsafe {
-                ptr::NonNull::new_unchecked(thin_ptr as *mut ArcInner<HeaderSliceWithLength<H, [T; 0]>>)
+                ptr::NonNull::new_unchecked(
+                    thin_ptr as *mut ArcInner<HeaderSliceWithLength<H, [T; 0]>>,
+                )
             },
             phantom: PhantomData,
         }
diff --git a/components/style/gecko/conversions.rs b/components/style/gecko/conversions.rs
index 607c7388c3d..43082a7c04a 100644
--- a/components/style/gecko/conversions.rs
+++ b/components/style/gecko/conversions.rs
@@ -536,12 +536,10 @@ pub mod basic_shape {
         StyleGeometryBox, StyleShapeSource, StyleShapeSourceType,
     };
     use crate::gecko_bindings::sugar::refptr::RefPtr;
-    use crate::values::computed::basic_shape::{
-        BasicShape, ClippingShape, FloatAreaShape,
-    };
+    use crate::values::computed::basic_shape::{BasicShape, ClippingShape, FloatAreaShape};
     use crate::values::computed::motion::OffsetPath;
     use crate::values::computed::url::ComputedUrl;
-    use crate::values::generics::basic_shape::{Path, GeometryBox, ShapeBox, ShapeSource};
+    use crate::values::generics::basic_shape::{GeometryBox, Path, ShapeBox, ShapeSource};
     use crate::values::specified::SVGPathData;
 
     impl StyleShapeSource {
diff --git a/components/style/gecko/values.rs b/components/style/gecko/values.rs
index ba2074d8fdc..98fe90fe3d0 100644
--- a/components/style/gecko/values.rs
+++ b/components/style/gecko/values.rs
@@ -7,8 +7,8 @@
 //! Different kind of helpers to interact with Gecko values.
 
 use crate::counter_style::{Symbol, Symbols};
-use crate::gecko_bindings::structs::{nsStyleCoord, CounterStylePtr};
 use crate::gecko_bindings::structs::StyleGridTrackBreadth;
+use crate::gecko_bindings::structs::{nsStyleCoord, CounterStylePtr};
 use crate::gecko_bindings::sugar::ns_style_coord::{CoordData, CoordDataMut, CoordDataValue};
 use crate::values::computed::{Angle, Length, LengthPercentage};
 use crate::values::computed::{Number, NumberOrPercentage, Percentage};
diff --git a/components/style/properties/longhands/box.mako.rs b/components/style/properties/longhands/box.mako.rs
index 64239eeb83a..6d754964ecc 100644
--- a/components/style/properties/longhands/box.mako.rs
+++ b/components/style/properties/longhands/box.mako.rs
@@ -680,5 +680,6 @@ ${helpers.predefined_type(
     "Either::Second(None_)",
     gecko_pref="layout.css.webkit-line-clamp.enabled",
     animation_value_type="Integer",
+    products="gecko",
     spec="https://drafts.csswg.org/css-overflow-3/#line-clamp",
 )}
diff --git a/components/style/values/animated/mod.rs b/components/style/values/animated/mod.rs
index 29e74859cd6..267ef0c8e6f 100644
--- a/components/style/values/animated/mod.rs
+++ b/components/style/values/animated/mod.rs
@@ -298,7 +298,6 @@ where
     }
 }
 
-
 impl<T> ToAnimatedValue for Box<[T]>
 where
     T: ToAnimatedValue,
@@ -307,8 +306,7 @@ where
 
     #[inline]
     fn to_animated_value(self) -> Self::AnimatedValue {
-        self
-            .into_vec()
+        self.into_vec()
             .into_iter()
             .map(T::to_animated_value)
             .collect::<Vec<_>>()
diff --git a/components/style/values/specified/basic_shape.rs b/components/style/values/specified/basic_shape.rs
index 70eab983510..4f627ceb07f 100644
--- a/components/style/values/specified/basic_shape.rs
+++ b/components/style/values/specified/basic_shape.rs
@@ -352,12 +352,14 @@ impl Polygon {
             })
             .unwrap_or_default();
 
-        let coordinates = input.parse_comma_separated(|i| {
-            Ok(PolygonCoord(
-                LengthPercentage::parse(context, i)?,
-                LengthPercentage::parse(context, i)?,
-            ))
-        })?.into();
+        let coordinates = input
+            .parse_comma_separated(|i| {
+                Ok(PolygonCoord(
+                    LengthPercentage::parse(context, i)?,
+                    LengthPercentage::parse(context, i)?,
+                ))
+            })?
+            .into();
 
         Ok(Polygon { fill, coordinates })
     }
diff --git a/components/style/values/specified/svg_path.rs b/components/style/values/specified/svg_path.rs
index e5276e5cce8..e5282e0a164 100644
--- a/components/style/values/specified/svg_path.rs
+++ b/components/style/values/specified/svg_path.rs
@@ -33,8 +33,7 @@ use style_traits::{CssWriter, ParseError, StyleParseErrorKind, ToCss};
 pub struct SVGPathData(
     // TODO(emilio): Should probably measure this somehow only from the
     // specified values.
-    #[ignore_malloc_size_of = "Arc"]
-    pub crate::ArcSlice<PathCommand>
+    #[ignore_malloc_size_of = "Arc"] pub crate::ArcSlice<PathCommand>,
 );
 
 impl SVGPathData {
@@ -103,7 +102,9 @@ impl Parse for SVGPathData {
             }
         }
 
-        Ok(SVGPathData(crate::ArcSlice::from_iter(path_parser.path.into_iter())))
+        Ok(SVGPathData(crate::ArcSlice::from_iter(
+            path_parser.path.into_iter(),
+        )))
     }
 }
 
diff --git a/components/style_traits/arc_slice.rs b/components/style_traits/arc_slice.rs
index f7097e3d086..dd1b99d3424 100644
--- a/components/style_traits/arc_slice.rs
+++ b/components/style_traits/arc_slice.rs
@@ -21,7 +21,7 @@ const ARC_SLICE_CANARY: u32 = 0xf3f3f3f3;
 /// cbindgen:derive-eq=false
 /// cbindgen:derive-neq=false
 #[repr(C)]
-#[derive(Debug, Clone, PartialEq, Eq, ToShmem)]
+#[derive(Clone, Debug, Eq, PartialEq, ToShmem)]
 pub struct ArcSlice<T>(#[shmem(field_bound)] ThinArc<u32, T>);
 
 impl<T> Deref for ArcSlice<T> {
diff --git a/components/style_traits/owned_slice.rs b/components/style_traits/owned_slice.rs
index 6e54aaeef5f..f2b0de4a4a0 100644
--- a/components/style_traits/owned_slice.rs
+++ b/components/style_traits/owned_slice.rs
@@ -6,11 +6,11 @@
 
 //! A replacement for `Box<[T]>` that cbindgen can understand.
 
+use malloc_size_of::{MallocShallowSizeOf, MallocSizeOf, MallocSizeOfOps};
 use std::marker::PhantomData;
-use std::{fmt, mem, slice};
-use std::ptr::NonNull;
 use std::ops::{Deref, DerefMut};
-use malloc_size_of::{MallocSizeOf, MallocShallowSizeOf, MallocSizeOfOps};
+use std::ptr::NonNull;
+use std::{fmt, mem, slice};
 use to_shmem::{SharedMemoryBuilder, ToShmem};
 
 /// A struct that basically replaces a `Box<[T]>`, but which cbindgen can
@@ -86,9 +86,7 @@ impl<T: Sized> OwnedSlice<T> {
     /// Convert the OwnedSlice into a Vec.
     #[inline]
     pub fn into_vec(self) -> Vec<T> {
-        let ret = unsafe {
-            Vec::from_raw_parts(self.ptr.as_ptr(), self.len, self.len)
-        };
+        let ret = unsafe { Vec::from_raw_parts(self.ptr.as_ptr(), self.len, self.len) };
         mem::forget(self);
         ret
     }