Need to make following properties animatable discretely. * grid-template-areas * grid-template-columns * grid-template-rows

Comment on attachment 8891945 [details] Bug 1379922 - Part 2: Support calc for fit-content(). https://reviewboard.mozilla.org/r/162976/#review168264 ::: commit-message-380ae:1 (Diff revision 1) > +Bug 1379922 - Part 2: fit-content() should support calc. r?hiro Support calc for fit-content(). dholbert taught me that "do not use 'should' in the summary of the commit message". Also I did remember I told it to you at the last all hands. :)

Comment on attachment 8891947 [details] Bug 1379922 - Part 4: remove test fail annotations from meta in wpt. https://reviewboard.mozilla.org/r/162980/#review168268

Comment on attachment 8891944 [details] Bug 1379922 - Part 1: make grid-template-areas animatable. https://reviewboard.mozilla.org/r/162974/#review168260 ::: commit-message-3a747:3 (Diff revision 1) > +Bug 1379922 - Part 1: make grid-template-areas animatable. r?hiro > + > +grid-template-areas property should animate discretely. I don't think this message is necessary.

Comment on attachment 8891946 [details] Bug 1379922 - Part 3: make grid-template-columns/rows animatable. https://reviewboard.mozilla.org/r/162978/#review168584 I asked waffles on IRC to review this. Thank you waffles!

Comment on attachment 8891946 [details] Bug 1379922 - Part 3: make grid-template-columns/rows animatable. https://reviewboard.mozilla.org/r/162978/#review168604 ::: servo/components/style/properties/gecko.mako.rs:1601 (Diff revision 1) > + } > + > + #[inline] > + fn to_boxed_customident_slice(gecko_names: &nsTArray<nsStringRepr>) -> Box<[CustomIdent]> { > + let idents: Vec<CustomIdent> = > + gecko_names.iter().map(|gecko_name| { Nit: This line isn't really long. It could just be inlined along with the previous line. ::: servo/components/style/properties/gecko.mako.rs:1618 (Diff revision 1) > + } > + > + if ${self_grid}.mIsSubgrid() { > + let mut names_vec = to_line_names_vec(&${self_grid}.mLineNameLists); > + let fill_idx = > + if ${self_grid}.mIsAutoFill() { Nit: Please write it like, ```rust let fill_idx = if ${self_grid}.mIsAutoFill() { // } else { // }; ``` ::: servo/components/style/properties/gecko.mako.rs:1620 (Diff revision 1) > + if ${self_grid}.mIsSubgrid() { > + let mut names_vec = to_line_names_vec(&${self_grid}.mLineNameLists); > + let fill_idx = > + if ${self_grid}.mIsAutoFill() { > + names_vec.insert( > + ${self_grid}.mRepeatAutoIndex as usize, Nit: Might be worth to have the index and the value as a variable above. ::: servo/components/style/properties/gecko.mako.rs:1640 (Diff revision 1) > + let min_max_iter = ${self_grid}.mMinTrackSizingFunctions.iter() > + .zip(${self_grid}.mMaxTrackSizingFunctions.iter()); > + for (i, (gecko_min, gecko_max)) in min_max_iter.enumerate() { > + let track_size = TrackSize::from_gecko_style_coords(gecko_min, gecko_max); > + > + if i == ${self_grid}.mRepeatAutoIndex as usize { Nit: We seem to be using the repeat index a lot, so it's probably better to have it in a variable in the top than casting every time in the loop.

Comment on attachment 8891944 [details] Bug 1379922 - Part 1: make grid-template-areas animatable. https://reviewboard.mozilla.org/r/162974/#review168594 ::: servo/components/style/properties/gecko.mako.rs:1622 (Diff revision 1) > + pub fn clone_grid_template_areas(&self) -> longhands::grid_template_areas::computed_value::T { > + use properties::longhands::grid_template_areas::{NamedArea, TemplateAreas}; > + use std::ops::Range; > + > + if self.gecko.mGridTemplateAreas.mRawPtr.is_null() { > + return Either::Second(::values::None_); Nit: Import the value above please. ::: servo/components/style/properties/gecko.mako.rs:1631 (Diff revision 1) > + let areas = unsafe { > + let vec: Vec<NamedArea> = > + (*gecko_grid_template_areas).mNamedAreas.iter().map(|gecko_name_area| { > + let name = gecko_name_area.mName.to_string().into_boxed_str(); > + let rows = > + Range{ start: gecko_name_area.mRowStart, end: gecko_name_area.mRowEnd }; Nit: Please have a space between struct names and the open curly braces (here and everywhere). We usually go for something like, ```rust let rows = Range { start: ..., end: ... }; ```

It looks good to me. Mostly just style nits. r=me after fixing that :)

(In reply to Ravi Shankar [:waffles] from comment #11) > It looks good to me. Mostly just style nits. r=me after fixing that :) Thank you very much, waffles! I'll fix those soon!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b7d62e70d4e555f57221c0d0b9e81f2f0aa6163

Comment on attachment 8891946 [details] Bug 1379922 - Part 3: make grid-template-columns/rows animatable. https://reviewboard.mozilla.org/r/162978/#review168644 r+ on behalf of waffles.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #18) > Comment on attachment 8891946 [details] > Bug 1379922 - Part 3: make grid-template-columns/rows animatable. > > https://reviewboard.mozilla.org/r/162978/#review168644 > > r+ on behalf of waffles. Thanks, Hiro!

Pushed by dakatsuka@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/406fa7a9a33d Part 4: remove test fail annotations from meta in wpt. r=hiro