servo: Merge #17776 - More grid serialization and parsing fixes (from canaltinova:disabled-mochitests); r=emilio

These are the bugs I discovered while I was re-enabling the disabled grid mochitests. Now all grid mochitests are passing! Additionally I converted `Vec<CustomIdent>`'s into `Box<[CustomIdent]>`. We are storing so many line names in vectors. These should help a bit.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

Source-Repo: https://github.com/servo/servo
Source-Revision: 20a3b0236d20e621591e751646758e833526084b

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : f13ef9b3cff0cff4ff2c71f1ca7ece1ccc8483b2

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

@@ -1371,10 +1371,11 @@ fn static_assert() {
         let ident = v.ident.as_ref().map_or(&[] as &[_], |ident| ident.0.as_slice());
         self.gecko.${value.gecko}.mLineName.assign(ident);
         self.gecko.${value.gecko}.mHasSpan = v.is_span;
-        self.gecko.${value.gecko}.mInteger = v.line_num.map(|i| {
+        if let Some(integer) = v.line_num {
             // clamping the integer between a range
-            cmp::max(nsStyleGridLine_kMinLine, cmp::min(i.value(), nsStyleGridLine_kMaxLine))
+            self.gecko.${value.gecko}.mInteger = cmp::max(nsStyleGridLine_kMinLine,
-        }).unwrap_or(0);
+                cmp::min(integer.value(), nsStyleGridLine_kMaxLine));
+        }
     }
 
     pub fn copy_${value.name}_from(&mut self, other: &Self) {
@@ -1520,7 +1521,7 @@ fn static_assert() {
                         &mut ${self_grid}.mRepeatAutoLineNameListAfter, 0);
                 }
             },
-            GridTemplateComponent::Subgrid(mut list) => {
+            GridTemplateComponent::Subgrid(list) => {
                 ${self_grid}.set_mIsSubgrid(true);
                 let names_length = match list.fill_idx {
                     Some(_) => list.names.len() - 1,
@@ -1537,14 +1538,15 @@ fn static_assert() {
                         &mut ${self_grid}.mRepeatAutoLineNameListAfter, 0);
                 }
 
+                let mut names = list.names.into_vec();
                 if let Some(idx) = list.fill_idx {
                     ${self_grid}.set_mIsAutoFill(true);
                     ${self_grid}.mRepeatAutoIndex = idx as i16;
-                    set_line_names(&list.names.swap_remove(idx as usize),
+                    set_line_names(&names.swap_remove(idx as usize),
                                    &mut ${self_grid}.mRepeatAutoLineNameListBefore);
                 }
 
-                for (servo_names, gecko_names) in list.names.iter().zip(${self_grid}.mLineNameLists.iter_mut()) {
+                for (servo_names, gecko_names) in names.iter().zip(${self_grid}.mLineNameLists.iter_mut()) {
                     set_line_names(servo_names, gecko_names);
                 }
             },

servo/components/style/properties/shorthand/position.mako.rs

@@ -252,7 +252,6 @@
                                        -> Result<(GridTemplateComponent,
                                                   GridTemplateComponent,
                                                   Either<TemplateAreas, None_>), ParseError<'i>> {
-
         // Other shorthand sub properties also parse `none` and `subgrid` keywords and this
         // shorthand should know after these keywords there is nothing to parse. Otherwise it
         // gets confused and rejects the sub properties that contains `none` or `subgrid`.
@@ -278,34 +277,35 @@
             }
         % endfor
 
-        let first_line_names = input.try(parse_line_names).unwrap_or(vec![]);
+        let first_line_names = input.try(parse_line_names).unwrap_or(vec![].into_boxed_slice());
         if let Ok(s) = input.try(Parser::expect_string) {
             let mut strings = vec![];
             let mut values = vec![];
             let mut line_names = vec![];
-            let mut names = first_line_names;
+            let mut names = first_line_names.into_vec();
             let mut string = s.into_owned().into_boxed_str();
             loop {
-                line_names.push(names);
+                line_names.push(names.into_boxed_slice());
                 strings.push(string);
                 let size = input.try(|i| TrackSize::parse(context, i)).unwrap_or_default();
                 values.push(size);
-                names = input.try(parse_line_names).unwrap_or(vec![]);
+                names = input.try(parse_line_names).unwrap_or(vec![].into_boxed_slice()).into_vec();
                 if let Ok(v) = input.try(parse_line_names) {
-                    names.extend(v);
+                    names.extend(v.into_vec());
                 }
 
                 string = match input.try(Parser::expect_string) {
                     Ok(s) => s.into_owned().into_boxed_str(),
                     _ => {      // only the named area determines whether we should bail out
-                        line_names.push(names);
+                        line_names.push(names.into_boxed_slice());
                         break
                     },
                 };
             }
 
             if line_names.len() == values.len() {
-                line_names.push(vec![]);        // should be one longer than track sizes
+                // should be one longer than track sizes
+                line_names.push(vec![].into_boxed_slice());
             }
 
             let template_areas = TemplateAreas::from_vec(strings)
@@ -313,7 +313,7 @@
             let template_rows = TrackList {
                 list_type: TrackListType::Normal,
                 values: values,
-                line_names: line_names,
+                line_names: line_names.into_boxed_slice(),
                 auto_repeat: None,
             };
 
@@ -372,11 +372,41 @@
                 template_columns.to_css(dest)
             },
             Either::First(ref areas) => {
+                // The length of template-area and template-rows values should be equal.
+                if areas.strings.len() != template_rows.track_list_len() {
+                    return Ok(());
+                }
+
                 let track_list = match *template_rows {
-                    GenericGridTemplateComponent::TrackList(ref list) => list,
+                    GenericGridTemplateComponent::TrackList(ref list) => {
-                    _ => unreachable!(),        // should exist!
+                        // We should fail if there is a `repeat` function. `grid` and
+                        // `grid-template` shorthands doesn't accept that. Only longhand accepts.
+                        if list.auto_repeat.is_some() {
+                            return Ok(());
+                        }
+                        list
+                    },
+                    // Others template components shouldn't exist with normal shorthand values.
+                    // But if we need to serialize a group of longhand sub-properties for
+                    // the shorthand, we should be able to return empty string instead of crashing.
+                    _ => return Ok(()),
                 };
 
+                // We need to check some values that longhand accepts but shorthands don't.
+                match *template_columns {
+                    // We should fail if there is a `repeat` function. `grid` and
+                    // `grid-template` shorthands doesn't accept that. Only longhand accepts that.
+                    GenericGridTemplateComponent::TrackList(ref list) if list.auto_repeat.is_some() => {
+                        return Ok(());
+                    },
+                    // Also the shorthands don't accept subgrids unlike longhand.
+                    // We should fail without an error here.
+                    GenericGridTemplateComponent::Subgrid(_) => {
+                        return Ok(());
+                    },
+                    _ => {},
+                }
+
                 let mut names_iter = track_list.line_names.iter();
                 for (((i, string), names), size) in areas.strings.iter().enumerate()
                                                                  .zip(&mut names_iter)
@@ -428,8 +458,8 @@
     use properties::longhands::{grid_auto_columns, grid_auto_rows, grid_auto_flow};
     use properties::longhands::grid_auto_flow::computed_value::{AutoFlow, T as SpecifiedAutoFlow};
     use values::{Either, None_};
-    use values::generics::grid::GridTemplateComponent;
+    use values::generics::grid::{GridTemplateComponent, TrackListType};
-    use values::specified::{LengthOrPercentage, TrackSize};
+    use values::specified::{GenericGridTemplateComponent, LengthOrPercentage, TrackSize};
 
     pub fn parse_value<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>)
                                -> Result<Longhands, ParseError<'i>> {
@@ -507,6 +537,14 @@
 
     impl<'a> ToCss for LonghandsToSerialize<'a> {
         fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+            // `grid` shorthand resets these properties. If they are not zero, that means they
+            // are changed by longhands and in that case we should fail serializing `grid`.
+            if *self.grid_row_gap != LengthOrPercentage::zero() ||
+               *self.grid_column_gap != LengthOrPercentage::zero() {
+                return Ok(());
+            }
+
+
             if *self.grid_template_areas != Either::Second(None_) ||
                (*self.grid_template_rows != GridTemplateComponent::None &&
                    *self.grid_template_columns != GridTemplateComponent::None) ||
@@ -517,6 +555,19 @@
             }
 
             if self.grid_auto_flow.autoflow == AutoFlow::Column {
+                // It should fail to serialize if other branch of the if condition's values are set.
+                if *self.grid_auto_rows != TrackSize::default() ||
+                   *self.grid_template_columns != GridTemplateComponent::None {
+                    return Ok(());
+                }
+
+                // It should fail to serialize if template-rows value is not Explicit.
+                if let GenericGridTemplateComponent::TrackList(ref list) = *self.grid_template_rows {
+                    if list.list_type != TrackListType::Explicit {
+                        return Ok(());
+                    }
+                }
+
                 self.grid_template_rows.to_css(dest)?;
                 dest.write_str(" / auto-flow")?;
                 if self.grid_auto_flow.dense {
@@ -528,6 +579,19 @@
                     self.grid_auto_columns.to_css(dest)?;
                 }
             } else {
+                // It should fail to serialize if other branch of the if condition's values are set.
+                if *self.grid_auto_columns != TrackSize::default() ||
+                   *self.grid_template_rows != GridTemplateComponent::None {
+                    return Ok(());
+                }
+
+                // It should fail to serialize if template-column value is not Explicit.
+                if let GenericGridTemplateComponent::TrackList(ref list) = *self.grid_template_columns {
+                    if list.list_type != TrackListType::Explicit {
+                        return Ok(());
+                    }
+                }
+
                 dest.write_str("auto-flow")?;
                 if self.grid_auto_flow.dense {
                     dest.write_str(" dense")?;

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

@@ -119,9 +119,7 @@ impl Parse for GridLine {
                 if i.value() <= 0 {       // disallow negative integers for grid spans
                     return Err(StyleParseError::UnspecifiedError.into())
                 }
-            } else if grid_line.ident.is_some() {       // integer could be omitted
+            } else if grid_line.ident.is_none() {       // integer could be omitted
-                grid_line.line_num = Some(Integer::new(1));
-            } else {
                 return Err(StyleParseError::UnspecifiedError.into())
             }
         }
@@ -205,7 +203,7 @@ impl<L: ToComputedValue> ToComputedValue for TrackBreadth<L> {
 ///
 /// https://drafts.csswg.org/css-grid/#typedef-track-size
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
-#[derive(Clone, Debug, HasViewportPercentage, PartialEq, ToCss)]
+#[derive(Clone, Debug, HasViewportPercentage, PartialEq)]
 pub enum TrackSize<L> {
     /// A flexible `<track-breadth>`
     Breadth(TrackBreadth<L>),
@@ -213,12 +211,10 @@ pub enum TrackSize<L> {
     /// and a flexible `<track-breadth>`
     ///
     /// https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-minmax
-    #[css(comma, function)]
     Minmax(TrackBreadth<L>, TrackBreadth<L>),
     /// A `fit-content` function.
     ///
     /// https://drafts.csswg.org/css-grid/#valdef-grid-template-columns-fit-content
-    #[css(function)]
     FitContent(L),
 }
 
@@ -261,6 +257,34 @@ impl<L: PartialEq> TrackSize<L> {
     }
 }
 
+impl<L: ToCss> ToCss for TrackSize<L> {
+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+        match *self {
+            TrackSize::Breadth(ref breadth) => breadth.to_css(dest),
+            TrackSize::Minmax(ref min, ref max) => {
+                // According to gecko minmax(auto, <flex>) is equivalent to <flex>,
+                // and both are serialized as <flex>.
+                if let TrackBreadth::Keyword(TrackKeyword::Auto) = *min {
+                    if let TrackBreadth::Flex(_) = *max {
+                        return max.to_css(dest);
+                    }
+                }
+
+                dest.write_str("minmax(")?;
+                min.to_css(dest)?;
+                dest.write_str(", ")?;
+                max.to_css(dest)?;
+                dest.write_str(")")
+            },
+            TrackSize::FitContent(ref lop) => {
+                dest.write_str("fit-content(")?;
+                lop.to_css(dest)?;
+                dest.write_str(")")
+            },
+        }
+    }
+}
+
 impl<L: ToComputedValue> ToComputedValue for TrackSize<L> {
     type ComputedValue = TrackSize<L::ComputedValue>;
 
@@ -330,8 +354,13 @@ pub enum RepeatCount {
 
 impl Parse for RepeatCount {
     fn parse<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {
-        if let Ok(i) = input.try(|i| Integer::parse(context, i)) {
+        // Maximum number of repeat is 10000. The greater numbers should be clamped.
+        const MAX_LINE: i32 = 10000;
+        if let Ok(mut i) = input.try(|i| Integer::parse(context, i)) {
             if i.value() > 0 {
+                if i.value() > MAX_LINE {
+                    i = Integer::new(MAX_LINE);
+                }
                 Ok(RepeatCount::Number(i))
             } else {
                 Err(StyleParseError::UnspecifiedError.into())
@@ -362,7 +391,7 @@ pub struct TrackRepeat<L> {
     /// If there's no `<line-names>`, then it's represented by an empty vector.
     /// For N `<track-size>` values, there will be N+1 `<line-names>`, and so this vector's
     /// length is always one value more than that of the `<track-size>`.
-    pub line_names: Vec<Vec<CustomIdent>>,
+    pub line_names: Box<[Box<[CustomIdent]>]>,
     /// `<track-size>` values.
     pub track_sizes: Vec<TrackSize<L>>,
 }
@@ -423,7 +452,8 @@ impl<L: Clone> TrackRepeat<L> {
                 let mut names_iter = self.line_names.iter();
                 for (size, names) in self.track_sizes.iter().zip(&mut names_iter) {
                     prev_names.extend_from_slice(&names);
-                    line_names.push(mem::replace(&mut prev_names, vec![]));
+                    let vec = mem::replace(&mut prev_names, vec![]);
+                    line_names.push(vec.into_boxed_slice());
                     track_sizes.push(size.clone());
                 }
 
@@ -432,11 +462,11 @@ impl<L: Clone> TrackRepeat<L> {
                 }
             }
 
-            line_names.push(prev_names);
+            line_names.push(prev_names.into_boxed_slice());
             TrackRepeat {
                 count: self.count,
                 track_sizes: track_sizes,
-                line_names: line_names,
+                line_names: line_names.into_boxed_slice(),
             }
 
         } else {    // if it's auto-fit/auto-fill, then it's left to the layout.
@@ -517,7 +547,7 @@ pub struct TrackList<T> {
     /// If there's no `<line-names>`, then it's represented by an empty vector.
     /// For N values, there will be N+1 `<line-names>`, and so this vector's
     /// length is always one value more than that of the `<track-size>`.
-    pub line_names: Vec<Vec<CustomIdent>>,
+    pub line_names: Box<[Box<[CustomIdent]>]>,
     /// `<auto-repeat>` value. There can only be one `<auto-repeat>` in a TrackList.
     pub auto_repeat: Option<TrackRepeat<T>>,
 }
@@ -574,7 +604,7 @@ impl<T: ToCss> ToCss for TrackList<T> {
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub struct LineNameList {
     /// The optional `<line-name-list>`
-    pub names: Vec<Vec<CustomIdent>>,
+    pub names: Box<[Box<[CustomIdent]>]>,
     /// Indicates the line name that requires `auto-fill`
     pub fill_idx: Option<u32>,
 }
@@ -626,7 +656,7 @@ impl Parse for LineNameList {
         }
 
         Ok(LineNameList {
-            names: line_names,
+            names: line_names.into_boxed_slice(),
             fill_idx: fill_idx,
         })
     }
@@ -677,3 +707,13 @@ pub enum GridTemplateComponent<L> {
     /// A `subgrid <line-name-list>?`
     Subgrid(LineNameList),
 }
+
+impl<L> GridTemplateComponent<L> {
+    /// Returns length of the <track-list>s <track-size>
+    pub fn track_list_len(&self) -> usize {
+        match *self {
+            GridTemplateComponent::TrackList(ref tracklist) => tracklist.values.len(),
+            _ => 0,
+        }
+    }
+}

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

@@ -81,7 +81,7 @@ impl Parse for TrackSize<LengthOrPercentage> {
 /// Parse the grid line names into a vector of owned strings.
 ///
 /// https://drafts.csswg.org/css-grid/#typedef-line-names
-pub fn parse_line_names<'i, 't>(input: &mut Parser<'i, 't>) -> Result<Vec<CustomIdent>, ParseError<'i>> {
+pub fn parse_line_names<'i, 't>(input: &mut Parser<'i, 't>) -> Result<Box<[CustomIdent]>, ParseError<'i>> {
     input.expect_square_bracket_block()?;
     input.parse_nested_block(|input| {
         let mut values = vec![];
@@ -90,7 +90,7 @@ pub fn parse_line_names<'i, 't>(input: &mut Parser<'i, 't>) -> Result<Vec<Custom
             values.push(ident);
         }
 
-        Ok(values)
+        Ok(values.into_boxed_slice())
     })
 }
 
@@ -128,7 +128,7 @@ impl TrackRepeat<LengthOrPercentage> {
                 let mut current_names;
 
                 loop {
-                    current_names = input.try(parse_line_names).unwrap_or(vec![]);
+                    current_names = input.try(parse_line_names).unwrap_or(vec![].into_boxed_slice());
                     if let Ok(track_size) = input.try(|i| TrackSize::parse(context, i)) {
                         if !track_size.is_fixed() {
                             if is_auto {
@@ -150,7 +150,7 @@ impl TrackRepeat<LengthOrPercentage> {
                             // one `TrackSize`. But in current version of the spec, this is deprecated
                             // but we are adding this for gecko parity. We should remove this when
                             // gecko implements new spec.
-                            names.push(input.try(parse_line_names).unwrap_or(vec![]));
+                            names.push(input.try(parse_line_names).unwrap_or(vec![].into_boxed_slice()));
                             break
                         }
                     } else {
@@ -167,7 +167,7 @@ impl TrackRepeat<LengthOrPercentage> {
                 let repeat = TrackRepeat {
                     count: count,
                     track_sizes: values,
-                    line_names: names,
+                    line_names: names.into_boxed_slice(),
                 };
 
                 Ok((repeat, repeat_type))
@@ -187,6 +187,8 @@ impl Parse for TrackList<LengthOrPercentage> {
     fn parse<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {
         // Merge the line names while parsing values. The resulting values will
         // all be bunch of `<track-size>` and one <auto-repeat>.
+        // FIXME: We need to decide which way is better for repeat function in
+        // https://bugzilla.mozilla.org/show_bug.cgi?id=1382369.
         //
         // For example,
         // `[a b] 100px [c d] repeat(1, 30px [g]) [h]` will be merged as `[a b] 100px [c d] 30px [g h]`
@@ -205,7 +207,7 @@ impl Parse for TrackList<LengthOrPercentage> {
         let mut atleast_one_not_fixed = false;
 
         loop {
-            current_names.append(&mut input.try(parse_line_names).unwrap_or(vec![]));
+            current_names.extend_from_slice(&mut input.try(parse_line_names).unwrap_or(vec![].into_boxed_slice()));
             if let Ok(track_size) = input.try(|i| TrackSize::parse(context, i)) {
                 if !track_size.is_fixed() {
                     atleast_one_not_fixed = true;
@@ -215,7 +217,8 @@ impl Parse for TrackList<LengthOrPercentage> {
                     }
                 }
 
-                names.push(mem::replace(&mut current_names, vec![]));
+                let vec = mem::replace(&mut current_names, vec![]);
+                names.push(vec.into_boxed_slice());
                 values.push(track_size);
             } else if let Ok((repeat, type_)) = input.try(|i| TrackRepeat::parse_with_repeat_type(context, i)) {
                 if list_type == TrackListType::Explicit {
@@ -237,7 +240,8 @@ impl Parse for TrackList<LengthOrPercentage> {
 
                         list_type = TrackListType::Auto(values.len() as u16);
                         auto_repeat = Some(repeat);
-                        names.push(mem::replace(&mut current_names, vec![]));
+                        let vec = mem::replace(&mut current_names, vec![]);
+                        names.push(vec.into_boxed_slice());
                         continue
                     },
                     RepeatType::Fixed => (),
@@ -245,10 +249,11 @@ impl Parse for TrackList<LengthOrPercentage> {
 
                 // If the repeat count is numeric, we axpand and merge the values.
                 let mut repeat = repeat.expand();
-                let mut repeat_names_iter = repeat.line_names.drain(..);
+                let mut repeat_names_iter = repeat.line_names.iter();
                 for (size, repeat_names) in repeat.track_sizes.drain(..).zip(&mut repeat_names_iter) {
                     current_names.extend_from_slice(&repeat_names);
-                    names.push(mem::replace(&mut current_names, vec![]));
+                    let vec = mem::replace(&mut current_names, vec![]);
+                    names.push(vec.into_boxed_slice());
                     values.push(size);
                 }
 
@@ -260,7 +265,7 @@ impl Parse for TrackList<LengthOrPercentage> {
                     return Err(StyleParseError::UnspecifiedError.into())
                 }
 
-                names.push(current_names);
+                names.push(current_names.into_boxed_slice());
                 break
             }
         }
@@ -268,7 +273,7 @@ impl Parse for TrackList<LengthOrPercentage> {
         Ok(TrackList {
             list_type: list_type,
             values: values,
-            line_names: names,
+            line_names: names.into_boxed_slice(),
             auto_repeat: auto_repeat,
         })
     }

servo/tests/unit/style/parsing/position.rs

@@ -237,7 +237,7 @@ fn test_computed_grid_template_rows_colums() {
 
     assert_computed_serialization(grid_template_rows::parse,
         "10px repeat(2, 1fr auto minmax(200px, 1fr))",
-        "10px minmax(auto, 1fr) auto minmax(200px, 1fr) minmax(auto, 1fr) auto minmax(200px, 1fr)");
+        "10px 1fr auto minmax(200px, 1fr) 1fr auto minmax(200px, 1fr)");
 
     assert_computed_serialization(grid_template_rows::parse,
         "subgrid [a] [] repeat(auto-fill, [])", "subgrid [a] [] repeat(auto-fill, [])");