We use StyleAnimationValue::ComputeDistance() for a Web Animation API, spacing [1], so we can do something like Interpolate in animated_properties.mako.rs: implement a ComputedDistance trait for AnimationValue, and add a FFI, so we can use it from Gecko. However, I think we might not need to implement ComputeDistance for Shape, Filter, and Transform because we don't have the official spec for them now. * CSS Shape: https://github.com/w3c/csswg-drafts/issues/662 * CSS Filter: https://github.com/w3c/fxtf-drafts/issues/91 * CSS Transform: Bug 1318591 Besides, nsDOMWindowUtils::ComputeAniamtionDistance() also uses it [3]. [1] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/animation/KeyframeUtils.cpp#1778,1793 [2] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/layout/style/StyleAnimationValue.cpp#1808,1814,1819 [3] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/base/nsDOMWindowUtils.cpp#2725

Making this P3 for now, even though we have discussed possibly dropping paced animation. Even if we do that, we'll still need this for SMIL animation I think.

This blocks a P2 bug (i.e. SMIL animation), so promote to P2.

Work on this now.

Comment on attachment 8859634 [details] Bug 1332633 - Part 1: Implement ComputeDistance trait. https://reviewboard.mozilla.org/r/123418/#review134396 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1979 (Diff revision 1) > } > } > } > + > + > +% if product == "gecko": I'm not sure whether for gecko only or for both servo and gecko. If you think this is ok for servo, I will remove this.

Comment on attachment 8859634 [details] Bug 1332633 - Part 1: Implement ComputeDistance trait. https://reviewboard.mozilla.org/r/123418/#review134396 > I'm not sure whether for gecko only or for both servo and gecko. If you think this is ok for servo, I will remove this. Yeah, seems harmless for servo, so let's compile it for both, if only because people working on servo won't accidentally break it and then realizing they need to fix this when CI runs.

Comment on attachment 8859634 [details] Bug 1332633 - Part 1: Implement ComputeDistance trait. https://reviewboard.mozilla.org/r/123418/#review134480 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:2017 (Diff revision 1) > +/// In order to compute the Euclidean distance of a list, we need to compute squared distance > +/// for each element, so the vector can sum it and then get its squared root as the distance. > +pub trait ComputeSquaredDistance: Sized { > + /// Compute squared distance between a value and another for a given property. > + /// This is used for list or if there are many components in a property value. > + fn compute_squared_distance(&self, other: &Self) -> Result<f64, ()>; One thought, without digging on the patch yet since I have to run now. Why not doing: ``` pub trait ComputeDistance: Sized { fn compute_distance(&self, other: &Self) -> Result<f64, ()>; fn compute_squared_distance(&self, other: &self) -> Result<f64, ()> { self.compute_distance(other).map(|d| d * d) } } ``` Or are we doing something smarter than that? (as I said, still haven't dug on the patch)

Comment on attachment 8859634 [details] Bug 1332633 - Part 1: Implement ComputeDistance trait. https://reviewboard.mozilla.org/r/123418/#review134548 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1984 (Diff revision 1) > + % for prop in data.longhands: > + % if prop.animation_type == "normal": > + (&AnimationValue::${prop.camel_case}(ref from), As we talked on IRC, would you mind checking animatable instead of animation_type?

Comment on attachment 8859634 [details] Bug 1332633 - Part 1: Implement ComputeDistance trait. https://reviewboard.mozilla.org/r/123418/#review134548 > As we talked on IRC, would you mind checking animatable instead of animation_type? Sure. This is my earlier version, so I haven't revise this part.

Comment on attachment 8859634 [details] Bug 1332633 - Part 1: Implement ComputeDistance trait. https://reviewboard.mozilla.org/r/123418/#review134480 > One thought, without digging on the patch yet since I have to run now. Why not doing: > > ``` > pub trait ComputeDistance: Sized { > fn compute_distance(&self, other: &Self) -> Result<f64, ()>; > > fn compute_squared_distance(&self, other: &self) -> Result<f64, ()> { > self.compute_distance(other).map(|d| d * d) > } > } > ``` > > Or are we doing something smarter than that? (as I said, still haven't dug on the patch) For compute_squared_distance, there are a case I want to avoid: For example, transform-origin type: (horizonal + vertical + depth) The distance is: ```sqrt(horizonal_diff^2 + vertical_diff^2 + depth_diff^2)``` The squared distance is: ```horizonal_diff^2 + vertical_diff^2 + depth_diff^2``` I don't want to get square-root and then sqaure it for squared distance, so we should implement compute_squared_distance by different ways for some types. However, put them together is fine to me, I will combine these two traits.

Comment on attachment 8859634 [details] Bug 1332633 - Part 1: Implement ComputeDistance trait. https://reviewboard.mozilla.org/r/123418/#review134396 > Yeah, seems harmless for servo, so let's compile it for both, if only because people working on servo won't accidentally break it and then realizing they need to fix this when CI runs. OK. I will remove this

Comment on attachment 8859634 [details] Bug 1332633 - Part 1: Implement ComputeDistance trait. https://reviewboard.mozilla.org/r/123418/#review134824 Looks great! I reviewed high-level top-to-bottom, and then looked into the details while I went up, so the comments are probably in the wrong order, but r=me with those addressed. ::: servo/components/style/properties/helpers.mako.rs:831 (Diff revision 2) > + match (self, other) { > + (&T(Some(ref this)), &T(Some(ref other))) => { > + this.compute_distance(other) > + }, > + (&T(Some(ref this)), &T(None)) => { > + this.compute_distance(&${value_for_none}) And here. ::: servo/components/style/properties/helpers.mako.rs:848 (Diff revision 2) > + fn compute_squared_distance(&self, other: &Self) -> Result<f64, ()> { > + match (self, other) { > + (&T(Some(ref this)), &T(Some(ref other))) => { > + this.compute_squared_distance(other) > + }, > + (&T(Some(ref this)), &T(None)) => { You could also merge branches here. ::: servo/components/style/properties/helpers/animated_properties.mako.rs:2483 (Diff revision 2) > + for i in 0..max_len { > + diff_squared += match (self.0.get(i), other.0.get(i)) { > + (Some(shadow), Some(other)) => { > + try!(shadow.compute_squared_distance(other)) > + }, > + (Some(shadow), None) => { ditto ::: servo/components/style/properties/helpers/animated_properties.mako.rs:2541 (Diff revision 2) > + for i in 0..max_len { > + diff_squared += match (self.0.get(i), other.0.get(i)) { > + (Some(shadow), Some(other)) => { > + try!(shadow.compute_squared_distance(other)) > + }, > + (Some(shadow), None) => { Since the squared distance should always be positive, I believe you could merge this and the following branch: ``` (Some(shadow), None) | (None, Some(shadow)) => { zero.inset = shadow.inset; try!(zero.compute_squared_distance(shadow)) } ```

Comment on attachment 8859635 [details] Bug 1332633 - Add Servo_AnimationValues_ComputeDistance. https://reviewboard.mozilla.org/r/123420/#review134826 ::: servo/ports/geckolib/glue.rs:275 (Diff revision 2) > +pub extern "C" fn Servo_AnimationValues_ComputeDistance(from: RawServoAnimationValueBorrowed, > + to: RawServoAnimationValueBorrowed) > + -> f64 { > + let from_value = AnimationValue::as_arc(&from); > + let to_value = AnimationValue::as_arc(&to); > + match from_value.compute_distance(to_value) { nit: you can use `from_value.compute_distance(to_value).unwrap_or(0.0)`.

Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f902d870c642 Add Servo_AnimationValues_ComputeDistance. r=emilio