+++ This bug was initially created as a clone of Bug #1336668 +++ In bug 1336668 I am going to just introduce a mechanism for discrete type animation and implement a property as discrete type animation. In this bug we will implement rest of properties of discrete animations.

Hi Hiro, I implemented discrete animation for only position related properties. I'd like to get your review at first since I'd like to know whether the basic thinking is not wrong. Thanks!

Comment on attachment 8865273 [details] Bug 1353966 - Part 1: Implement discrete type animation for position related properties. https://reviewboard.mozilla.org/r/136906/#review139872 Thought I haven't look into clone_grid_auto_xx() closely, this looks basically good to me. ::: servo/components/style/values/specified/align.rs:118 (Diff revision 1) > /// > /// The 16-bit field stores the primary value in its lower 8 bits, and the optional fallback value > /// in its upper 8 bits. This matches the representation of these properties in Gecko. > #[derive(Copy, Clone, Debug, Eq, PartialEq)] > #[cfg_attr(feature = "servo", derive(HeapSizeOf, Deserialize, Serialize))] > -pub struct AlignJustifyContent(u16); > +pub struct AlignJustifyContent(pub u16); Do we really need to make this pub?

Comment on attachment 8865273 [details] Bug 1353966 - Part 1: Implement discrete type animation for position related properties. https://reviewboard.mozilla.org/r/136906/#review139886 ::: servo/components/style/gecko_bindings/sugar/ns_style_coord.rs:35 (Diff revision 1) > + unsafe { > + match self.unit() { > + eStyleUnit_Null | > + eStyleUnit_Normal | > + eStyleUnit_Auto | > + eStyleUnit_None => { true } It seems to me that this PartialEq is not correct, especially here. Instead using PartialEq, we should use from_gecko_style_coord() and then use match. Given that grid-auto-{columns,rows} is not popular for animations, I think it's OK with performance loss.

Comment on attachment 8865273 [details] Bug 1353966 - Part 1: Implement discrete type animation for position related properties. https://reviewboard.mozilla.org/r/136906/#review139872 Thank you very much! > Do we really need to make this pub? I got "cannot construct with a private field" error if there is no "pub". But, I found "new" fn, I'll use this instead. Thanks!

Comment on attachment 8865273 [details] Bug 1353966 - Part 1: Implement discrete type animation for position related properties. https://reviewboard.mozilla.org/r/136906/#review145898

Thank you for your review, Hiro! I'll check and fix your points. Also, I'm so sorry, please let me append two more patches before test patch. * box related properties as patch 7 * position related properties as patch 8 * test patch as patch 9 Thanks.

Oh, sorry, not this bug.. (In reply to Daisuke Akatsuka (:daisuke) from comment #8) > Thank you for your review, Hiro! > I'll check and fix your points. > > Also, I'm so sorry, please let me append two more patches before test patch. > * box related properties as patch 7 > * position related properties as patch 8 > * test patch as patch 9 > > Thanks.

Significant behavioral change from Gecko, blocks turning on important regression test(s), P1.

Finally! Great works daisuke!

Too late for 56. Mass won't fix for 56.

Too late for 56. Mass won't fix for 56.