Bug 1559814 - Use a more similar representation in Rust and C++ for grid lines. r=heycam
☠☠ backed out by c95bc282ae96 ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 26 Jun 2019 20:32:49 +0000
changeset 543052 362d9435ca4e6d90ee16fe2c6a9d620a03c03e90
parent 543051 fd8b8f6dad49964aa9f5ba8d94bf8d50b1528723
child 543053 62f9d89fb827af89e810b728a54c2bb2024085e4
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1559814
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1559814 - Use a more similar representation in Rust and C++ for grid lines. r=heycam Option<> is not FFI-safe, so if we want to use the same representation everywhere we need to get rid of it. This also makes it take the same amount of memory as the C++ representation, and it's not very complex, I'd think. Differential Revision: https://phabricator.services.mozilla.com/D35195
servo/components/style/properties/gecko.mako.rs
servo/components/style/properties/shorthands/position.mako.rs
servo/components/style/values/animated/grid.rs
servo/components/style/values/generics/grid.rs
servo/components/style/values/specified/mod.rs
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -1133,64 +1133,45 @@ fn static_assert() {
     ${impl_simple_copy('order', 'mOrder')}
 
     % for value in GRID_LINES:
     pub fn set_${value.name}(&mut self, v: longhands::${value.name}::computed_value::T) {
         use crate::gecko_bindings::structs::{nsStyleGridLine_kMinLine, nsStyleGridLine_kMaxLine};
 
         let line = &mut self.gecko.${value.gecko};
         line.mLineName.set_move(unsafe {
-            RefPtr::from_addrefed(match v.ident {
-                Some(i) => i.0,
-                None => atom!(""),
-            }.into_addrefed())
+            RefPtr::from_addrefed(v.ident.into_addrefed())
         });
         line.mHasSpan = v.is_span;
-        if let Some(integer) = v.line_num {
-            // clamping the integer between a range
-            line.mInteger = cmp::max(
-                nsStyleGridLine_kMinLine,
-                cmp::min(integer, nsStyleGridLine_kMaxLine),
-            );
-        }
+        // clamping the integer between a range
+        line.mInteger = cmp::max(
+            nsStyleGridLine_kMinLine,
+            cmp::min(v.line_num, nsStyleGridLine_kMaxLine),
+        );
     }
 
     pub fn copy_${value.name}_from(&mut self, other: &Self) {
         self.gecko.${value.gecko}.mHasSpan = other.gecko.${value.gecko}.mHasSpan;
         self.gecko.${value.gecko}.mInteger = other.gecko.${value.gecko}.mInteger;
         unsafe {
             self.gecko.${value.gecko}.mLineName.set(&other.gecko.${value.gecko}.mLineName);
         }
     }
 
     pub fn reset_${value.name}(&mut self, other: &Self) {
         self.copy_${value.name}_from(other)
     }
 
     pub fn clone_${value.name}(&self) -> longhands::${value.name}::computed_value::T {
-        use crate::gecko_bindings::structs::{nsStyleGridLine_kMinLine, nsStyleGridLine_kMaxLine};
-
         longhands::${value.name}::computed_value::T {
             is_span: self.gecko.${value.gecko}.mHasSpan,
-            ident: {
-                let name = unsafe { Atom::from_raw(self.gecko.${value.gecko}.mLineName.mRawPtr) };
-                if name == atom!("") {
-                    None
-                } else {
-                    Some(CustomIdent(name))
-                }
+            ident: unsafe {
+                Atom::from_raw(self.gecko.${value.gecko}.mLineName.mRawPtr)
             },
-            line_num:
-                if self.gecko.${value.gecko}.mInteger == 0 {
-                    None
-                } else {
-                    debug_assert!(nsStyleGridLine_kMinLine <= self.gecko.${value.gecko}.mInteger);
-                    debug_assert!(self.gecko.${value.gecko}.mInteger <= nsStyleGridLine_kMaxLine);
-                    Some(self.gecko.${value.gecko}.mInteger)
-                },
+            line_num: self.gecko.${value.gecko}.mInteger,
         }
     }
     % endfor
 
     % for kind in ["rows", "columns"]:
     pub fn set_grid_auto_${kind}(&mut self, v: longhands::grid_auto_${kind}::computed_value::T) {
         let gecko = &mut *self.gecko;
         v.to_gecko_style_coords(&mut gecko.mGridAuto${kind.title()}Min,
--- a/servo/components/style/properties/shorthands/position.mako.rs
+++ b/servo/components/style/properties/shorthands/position.mako.rs
@@ -139,31 +139,32 @@
 </%helpers:shorthand>
 
 % for kind in ["row", "column"]:
 <%helpers:shorthand name="grid-${kind}" sub_properties="grid-${kind}-start grid-${kind}-end"
                     spec="https://drafts.csswg.org/css-grid/#propdef-grid-${kind}"
                     products="gecko">
     use crate::values::specified::GridLine;
     use crate::parser::Parse;
+    use crate::Zero;
 
     // NOTE: Since both the shorthands have the same code, we should (re-)use code from one to implement
     // the other. This might not be a big deal for now, but we should consider looking into this in the future
     // to limit the amount of code generated.
     pub fn parse_value<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Longhands, ParseError<'i>> {
         let start = input.try(|i| GridLine::parse(context, i))?;
         let end = if input.try(|i| i.expect_delim('/')).is_ok() {
             GridLine::parse(context, input)?
         } else {
             let mut line = GridLine::auto();
-            if start.line_num.is_none() && !start.is_span {
-                line.ident = start.ident.clone();       // ident from start value should be taken
+            if start.line_num.is_zero() && !start.is_span {
+                line.ident = start.ident.clone(); // ident from start value should be taken
             }
 
             line
         };
 
         Ok(expanded! {
             grid_${kind}_start: start,
             grid_${kind}_end: end,
@@ -181,25 +182,26 @@
 % endfor
 
 <%helpers:shorthand name="grid-area"
                     sub_properties="grid-row-start grid-row-end grid-column-start grid-column-end"
                     spec="https://drafts.csswg.org/css-grid/#propdef-grid-area"
                     products="gecko">
     use crate::values::specified::GridLine;
     use crate::parser::Parse;
+    use crate::Zero;
 
     // The code is the same as `grid-{row,column}` except that this can have four values at most.
     pub fn parse_value<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Longhands, ParseError<'i>> {
         fn line_with_ident_from(other: &GridLine) -> GridLine {
             let mut this = GridLine::auto();
-            if other.line_num.is_none() && !other.is_span {
+            if other.line_num.is_zero() && !other.is_span {
                 this.ident = other.ident.clone();
             }
 
             this
         }
 
         let row_start = input.try(|i| GridLine::parse(context, i))?;
         let (column_start, row_end, column_end) = if input.try(|i| i.expect_delim('/')).is_ok() {
--- a/servo/components/style/values/animated/grid.rs
+++ b/servo/components/style/values/animated/grid.rs
@@ -57,20 +57,17 @@ impl Animate for TrackSize {
                 &generics::TrackSize::FitContent(ref to),
             ) => animate_with_discrete_fallback(from, to, procedure)
                 .map(generics::TrackSize::FitContent),
             (_, _) => discrete(self, other, procedure),
         }
     }
 }
 
-impl Animate for generics::TrackRepeat<LengthPercentage, Integer>
-where
-    generics::RepeatCount<Integer>: PartialEq,
-{
+impl Animate for generics::TrackRepeat<LengthPercentage, Integer> {
     fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
         // If the keyword, auto-fit/fill, is the same it can result in different
         // number of tracks. For both auto-fit/fill, the number of columns isn't
         // known until you do layout since it depends on the container size, item
         // placement and other factors, so we cannot do the correct interpolation
         // by computed values. Therefore, return Err(()) if it's keywords. If it
         // is Number, we support animation only if the count is the same and the
         // length of track_sizes is the same.
--- a/servo/components/style/values/generics/grid.rs
+++ b/servo/components/style/values/generics/grid.rs
@@ -1,15 +1,16 @@
 /* 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/. */
 
 //! Generic types for the handling of
 //! [grids](https://drafts.csswg.org/css-grid/).
 
+use crate::{Atom, Zero};
 use crate::parser::{Parse, ParserContext};
 use crate::values::computed::{Context, ToComputedValue};
 use crate::values::specified;
 use crate::values::specified::grid::parse_line_names;
 use crate::values::{CSSFloat, CustomIdent};
 use cssparser::Parser;
 use std::fmt::{self, Write};
 use std::{mem, usize};
@@ -24,71 +25,77 @@ use style_traits::{CssWriter, ParseError
     Default,
     MallocSizeOf,
     PartialEq,
     SpecifiedValueInfo,
     ToComputedValue,
     ToResolvedValue,
     ToShmem,
 )]
-pub struct GridLine<Integer> {
+#[repr(C)]
+pub struct GenericGridLine<Integer> {
     /// Flag to check whether it's a `span` keyword.
     pub is_span: bool,
-    /// A custom identifier for named lines.
+    /// A custom identifier for named lines, or the empty atom otherwise.
     ///
     /// <https://drafts.csswg.org/css-grid/#grid-placement-slot>
-    pub ident: Option<CustomIdent>,
+    pub ident: Atom,
     /// Denotes the nth grid line from grid item's placement.
-    pub line_num: Option<Integer>,
+    pub line_num: Integer,
 }
 
-impl<Integer> GridLine<Integer> {
+pub use self::GenericGridLine as GridLine;
+
+impl<Integer> GridLine<Integer>
+where
+    Integer: Zero,
+{
     /// The `auto` value.
     pub fn auto() -> Self {
         Self {
             is_span: false,
-            line_num: None,
-            ident: None,
+            line_num: Zero::zero(),
+            ident: atom!(""),
         }
     }
 
     /// Check whether this `<grid-line>` represents an `auto` value.
     pub fn is_auto(&self) -> bool {
-        self.ident.is_none() && self.line_num.is_none() && !self.is_span
+        self.ident == atom!("") && self.line_num.is_zero() && !self.is_span
     }
 }
 
 impl<Integer> ToCss for GridLine<Integer>
 where
-    Integer: ToCss,
+    Integer: ToCss + Zero,
 {
     fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
     where
         W: Write,
     {
         if self.is_auto() {
             return dest.write_str("auto");
         }
 
         if self.is_span {
             dest.write_str("span")?;
         }
 
-        if let Some(ref i) = self.line_num {
+        if !self.line_num.is_zero() {
             if self.is_span {
                 dest.write_str(" ")?;
             }
-            i.to_css(dest)?;
+            self.line_num.to_css(dest)?;
         }
 
-        if let Some(ref s) = self.ident {
-            if self.is_span || self.line_num.is_some() {
+        if self.ident != atom!("") {
+            if self.is_span || !self.line_num.is_zero() {
                 dest.write_str(" ")?;
             }
-            s.to_css(dest)?;
+            CustomIdent(self.ident.clone()).to_css(dest)?;
         }
 
         Ok(())
     }
 }
 
 impl Parse for GridLine<specified::Integer> {
     fn parse<'i, 't>(
@@ -109,51 +116,51 @@ impl Parse for GridLine<specified::Integ
         for _ in 0..3 {
             // Maximum possible entities for <grid-line>
             let location = input.current_source_location();
             if input.try(|i| i.expect_ident_matching("span")).is_ok() {
                 if grid_line.is_span {
                     return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError));
                 }
 
-                if grid_line.line_num.is_some() || grid_line.ident.is_some() {
+                if !grid_line.line_num.is_zero() || grid_line.ident != atom!("") {
                     val_before_span = true;
                 }
 
                 grid_line.is_span = true;
             } else if let Ok(i) = input.try(|i| specified::Integer::parse(context, i)) {
                 // FIXME(emilio): Probably shouldn't reject if it's calc()...
-                if i.value() == 0 || val_before_span || grid_line.line_num.is_some() {
+                if i.value() == 0 || val_before_span || grid_line.line_num.value() != 0 {
                     return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError));
                 }
 
-                grid_line.line_num = Some(i);
+                grid_line.line_num = i;
             } else if let Ok(name) = input.try(|i| i.expect_ident_cloned()) {
-                if val_before_span || grid_line.ident.is_some() {
+                if val_before_span || grid_line.ident != atom!("") {
                     return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError));
                 }
                 // NOTE(emilio): `span` is consumed above, so we only need to
                 // reject `auto`.
-                grid_line.ident = Some(CustomIdent::from_ident(location, &name, &["auto"])?);
+                grid_line.ident = CustomIdent::from_ident(location, &name, &["auto"])?.0;
             } else {
                 break;
             }
         }
 
         if grid_line.is_auto() {
             return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
         }
 
         if grid_line.is_span {
-            if let Some(i) = grid_line.line_num {
-                if i.value() <= 0 {
+            if !grid_line.line_num.is_zero() {
+                if grid_line.line_num.value() <= 0 {
                     // disallow negative integers for grid spans
                     return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
                 }
-            } else if grid_line.ident.is_none() {
+            } else if grid_line.ident == atom!("") {
                 // integer could be omitted
                 return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
             }
         }
 
         Ok(grid_line)
     }
 }
--- a/servo/components/style/values/specified/mod.rs
+++ b/servo/components/style/values/specified/mod.rs
@@ -12,19 +12,19 @@ use super::generics::grid::{GridLine as 
 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, None_};
 use crate::context::QuirksMode;
 use crate::parser::{Parse, ParserContext};
 use crate::values::serialize_atom_identifier;
 use crate::values::specified::calc::CalcNode;
-use crate::{Atom, Namespace, Prefix};
+use crate::{Atom, Namespace, Prefix, Zero};
 use cssparser::{Parser, Token};
-use num_traits::{One, Zero};
+use num_traits::One;
 use std::f32;
 use std::fmt::{self, Write};
 use std::ops::Add;
 use style_traits::values::specified::AllowedNumericType;
 use style_traits::{CssWriter, ParseError, SpecifiedValueInfo, StyleParseErrorKind, ToCss};
 
 #[cfg(feature = "gecko")]
 pub use self::align::{AlignContent, AlignItems, AlignSelf, ContentDistribution};
@@ -444,16 +444,28 @@ impl ToComputedValue for Opacity {
 ///
 /// <https://drafts.csswg.org/css-values/#integers>
 #[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, PartialOrd, ToShmem)]
 pub struct Integer {
     value: CSSInteger,
     was_calc: bool,
 }
 
+impl Zero for Integer {
+    #[inline]
+    fn zero() -> Self {
+        Self::new(0)
+    }
+
+    #[inline]
+    fn is_zero(&self) -> bool {
+        self.value() == 0
+    }
+}
+
 impl One for Integer {
     #[inline]
     fn one() -> Self {
         Self::new(1)
     }
 }
 
 // This is not great, because it loses calc-ness, but it's necessary for One.