Brian Birtles (:birtles) Reporter
	Description • 7 years ago

In bug 1318591 we implemented ComputeDistance but not for TransformList. We need to implement this for TransformList (or Filter or Shape according to bug 1318591 comment 0). We need to do this because this is used in the DevTools animation inspector where inspecting transform lists is very common.

	Brian Birtles (:birtles) Reporter
	Comment 1 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #0)
> In bug 1318591 we implemented ComputeDistance but not for TransformList. We
> need to implement this for TransformList (or Filter or Shape according to
> bug 1318591 comment 0). We need to do this because this is used in the
> DevTools animation inspector where inspecting transform lists is very common.

Sorry, that should refer to bug 1332633.

	Hiroyuki Ikezoe (:hiro)
	Comment 2 • 7 years ago

(In reply to Brian Birtles (:birtles) from comment #0)
> In bug 1318591 we implemented ComputeDistance but not for TransformList. We
> need to implement this for TransformList (or Filter or Shape according to
> bug 1318591 comment 0). We need to do this because this is used in the
> DevTools animation inspector where inspecting transform lists is very common.

'filter' is not animatable yet (bug 1362897).

	Boris Chiou [:boris] Assignee
	Comment 3 • 7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> (In reply to Brian Birtles (:birtles) from comment #0)
> > In bug 1318591 we implemented ComputeDistance but not for TransformList. We
> > need to implement this for TransformList (or Filter or Shape according to
> > bug 1318591 comment 0). We need to do this because this is used in the
> > DevTools animation inspector where inspecting transform lists is very common.
> 
> 'filter' is not animatable yet (bug 1362897).

OK. Looks like we can implement Shape and TransformList first.

	Brian Birtles (:birtles) Reporter
	Comment 4 • 7 years ago

Bear in mind that the main requirement here is just that the keyframes panel in DevTools produces a sensible result so it's probably not worth spending a lot of time on difficult edge cases (but we should document any shortcuts we took with code comments and bugs as necessary).

	Boris Chiou [:boris] Assignee
	Comment 5 • 7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> (In reply to Brian Birtles (:birtles) from comment #0)
> > In bug 1318591 we implemented ComputeDistance but not for TransformList. We
> > need to implement this for TransformList (or Filter or Shape according to
> > bug 1318591 comment 0). We need to do this because this is used in the
> > DevTools animation inspector where inspecting transform lists is very common.
> 
> 'filter' is not animatable yet (bug 1362897).

There are two properties using basic shape: shape-outside and clip-path, and both are not animatable now. So let's implement compute distance for transform first.

	Boris Chiou [:boris] Assignee
	Comment 6 • 7 years ago

I will follow the current implementation on Gecko:
1. TransformList: Bug 1272549 Comment 25
2. Shape: Bug 1286150, (https://github.com/w3c/csswg-drafts/issues/662)
3. Filter: Bug 1286151, (https://github.com/w3c/fxtf-drafts/issues/91)

	Boris Chiou [:boris] Assignee
	Comment 7 • 7 years ago

(In reply to Boris Chiou [:boris] from comment #6)
> I will follow the current implementation on Gecko:
> 1. TransformList: Bug 1272549 Comment 25
> 2. Shape: Bug 1286150, (https://github.com/w3c/csswg-drafts/issues/662)
> 3. Filter: Bug 1286151, (https://github.com/w3c/fxtf-drafts/issues/91)

Sorry, transform list should use this: bug 1318591 comment 0

	Boris Chiou [:boris] Assignee
	Comment 8 • 7 years ago

(In reply to Boris Chiou [:boris] from comment #7)
> (In reply to Boris Chiou [:boris] from comment #6)
> > I will follow the current implementation on Gecko:
> > 1. TransformList: Bug 1272549 Comment 25
> > 2. Shape: Bug 1286150, (https://github.com/w3c/csswg-drafts/issues/662)
> > 3. Filter: Bug 1286151, (https://github.com/w3c/fxtf-drafts/issues/91)
> 
> Sorry, transform list should use this: bug 1318591 comment 0

And bug 1318591 comment 0 might be much harder on stylo because we need to get layout info from Gecko side, and I guess that might be also a problem for mismatched transform list. Maybe we should wait for https://github.com/servo/servo/issues/13267 first.

	Brian Birtles (:birtles) Reporter
	Comment 9 • 7 years ago

P2 assuming this breaks the DevTools keyframes panel for transform animations. We can downgrade if that's not the case.

	Boris Chiou [:boris] Assignee
	Comment 10 • 7 years ago

Start to think about this bug.

1. Transform: bug 1318591 is much more complicate than the current implementation in Gecko, so in short-term, I'd like to implement this as what we do in Gecko, and skip the cases of mismatched transform lists and interpolatematrix/accumulatematrix (i.e. distance == 0 in these cases)

2. Shape: just follow the implementation in Gecko.

	Brian Birtles (:birtles) Reporter
	Comment 11 • 7 years ago

Yes please! Matching Gecko sounds fine here. I've noticed some funny looking graphs in DevTools when using Stylo and I suspect this might be the cause. Thanks Boris!

	Boris Chiou [:boris] Assignee
	Comment 12 • 7 years ago

Bug 1376495 will add Animatable trait for BasicShape, so it's better to implement TransformList in this bug first, and file another bug which depends on Bug 1376495 for BasicShape.

	Anthony Ramine [:nox]
	Comment 13 • 7 years ago

Is this still needed for Web Animations, or only for some inspector features?

	Brian Birtles (:birtles) Reporter
	Comment 14 • 7 years ago

Right, only inspector features (but pretty important ones).

	Boris Chiou [:boris] Assignee
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8896192 [details]
Bug 1362896 - Part 3: Implement compute_distance for TransformList.

https://reviewboard.mozilla.org/r/167468/#review172980

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2663
(Diff revision 1)
> +                let mut vector1 = DirectionVector::new(fx as f64, fy as f64, fz as f64);
> +                let mut vector2 = DirectionVector::new(tx as f64, ty as f64, tz as f64);
> +                if !vector1.normalize() || !vector2.normalize() {
> +                    continue;
> +                }

Sorry, this is not correct. If a vector cannot be normalized, we should treat it as an idnetity matrix (just like 'none').

	Brian Birtles (:birtles) Reporter
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8896190 [details]
Bug 1362896 - Part 1: Fix the computation of distance for non-normalizable direction vector of rotate3d.

https://reviewboard.mozilla.org/r/167464/#review173200

Nice.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1107
(Diff revision 1)
>          self.compute_squared_distance(other).map(|sq| sq.sqrt())
>      }
>  
>      #[inline]
>      fn compute_squared_distance(&self, other: &Self) -> Result<f64, ()> {
> -        let length_diff = (self.unclamped_length().0 - other.unclamped_length().0) as f64;
> +        let length_diff = self.unclamped_length().to_f64_px() - other.unclamped_length().to_f64_px();

(Nit: Over 100 chars)

	Brian Birtles (:birtles) Reporter
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8896191 [details]
Bug 1362896 - Part 2: Use f64 for Quaternion.

https://reviewboard.mozilla.org/r/167466/#review173202

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2319
(Diff revision 1)
>              let clamped_w = self.quaternion.3.min(1.0).max(-1.0);
>  
>              // Determine the scale factor.
>              let mut theta = clamped_w.acos();
>              let mut scale = if theta == 0.0 { 0.0 } else { 1.0 / theta.sin() };
> -            theta *= self_portion as f32;
> +            theta *= self_portion as f64;

Isn't |self_portion| already f64?

	Anthony Ramine [:nox]
	Comment 23 • 7 years ago

Note that https://github.com/servo/servo/pull/18058 landed and you'll have to rebase. Feel free to address Brian's comments here and then file a rebased PR directly on Servo and I'll take over the review over there.

	Brian Birtles (:birtles) Reporter
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8896192 [details]
Bug 1362896 - Part 3: Implement compute_distance for TransformList.

https://reviewboard.mozilla.org/r/167468/#review173204

r=me with comments addressed (and I'm assuming nox will review the subsequent Servo PR)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2085
(Diff revision 1)
> +#[derive(Clone, Copy, Debug, PartialEq)]
> +#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
> +pub struct DirectionVector(Point3D<f64>);
> +
> +impl Quaternion {
> +    /// Return a quaternion from an unit direction vector and angle (unit: radian).

Nit: a unit direction...

Also, do we need any of the comment from the below here:

  http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/gfx/thebes/gfxQuaternion.h#34-46

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2110
(Diff revision 1)
> +    #[inline]
> +    fn new(x: f64, y: f64, z: f64) -> Self {
> +        DirectionVector(Point3D::new(x, y, z))
> +    }
> +
> +    /// Normalize this vector.

Mention the meaning of the return value?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2627
(Diff revision 1)
> +// FIXME: A potential new solution for computing the distance between two transform lists,
> +// https://bugzilla.mozilla.org/show_bug.cgi?id=1318591#c0

I think I'd drop this comment or just say, "This might not be the most useful definition of distance. It might be better, for example, to trace the distance travelled by a point as its transform is interpolated between the two lists. That, however, proves to be quite complicated so we take a simple approach for now. See https://bugzilla.mozilla.org/show_bug.cgi?id=1318591#c0."

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2645
(Diff revision 1)
> +                // Drop percentage part because we compute distance by computed values, so we
> +                // cannot resolve the percentage.

(This comment could be more clear. Specifically, I guess we're saying that we don't want to require doing layout in order to calculate the result. Also, I think we probably could do something better for the percentage-percentage case. But I guess Gecko doesn't so this is fine for now.)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2663
(Diff revision 1)
> +                let mut vector1 = DirectionVector::new(fx as f64, fy as f64, fz as f64);
> +                let mut vector2 = DirectionVector::new(tx as f64, ty as f64, tz as f64);
> +                if !vector1.normalize() || !vector2.normalize() {
> +                    continue;
> +                }

Yeah, I noticed this code is not in Gecko and I'm curious how it handles this case.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2790
(Diff revision 1)
> +                    // Note: we don't handle mismatch transform lists now.
> +                    Err(())

Should we? Gecko does and it seems pretty likely to occur?

Do we plan to do this in a separate bug?

	Brian Birtles (:birtles) Reporter
	Comment 25 • 7 years ago
mozreview-review

Comment on attachment 8896193 [details]
Bug 1362896 - Part 4: Implement compute_distance for Matrix and Perspective.

https://reviewboard.mozilla.org/r/167470/#review173218

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1827
(Diff revision 1)
> +        // If possible, we should convert the InnerMatrix2D into types with physical meaning.
> +        // Therefore, we compute the squared distance from each matrix item, and this makes the
> +        // result different from that in Gecko if we have skew/shear factor in the ComputedMatrix.

I don't quite follow this comment. Care to clarify the difference?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2404
(Diff revision 1)
> +        // so we use atan() to get the angle. (Actually, we have to divide the skew factor by scale
> +        // values first, but it's fine for now because both Gecko and Servo use the same formula.)

I don't understand the parenthetical (the part in parentheses). Does Gecko have a bug here?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2511
(Diff revision 1)
>          Ok(sum)
>      }
> +
> +    #[inline]
> +    fn compute_squared_distance(&self, other: &Self) -> Result<f64, ()> {
> +        println!("[Boris] decomposed 3d: {:?} and {:?}", self, other);

I think you probably meant to drop this.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2796
(Diff revision 1)
> +                let mut p_matrix = ComputedMatrix::identity();
> +                if p.0 > 0 {
> +                    p_matrix.m34 = -1. / p.to_f32_px();
> +                }
> +                p_matrix.compute_squared_distance(&m)?

I couldn't find the corresponding code for this in Gecko so I couldn't check that, but it seems right to me.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2806
(Diff revision 1)
> +
>              }
>              _ => {
>                  // We can implement this for InterpolateMatrix and AccumulateMatrix after
> -                // we finish the implementation of computing distance of two mismatched transform
> -                // lists.
> +                // finishing the implementation of computing distance of two mismatched
> +                // transform lists. Now, treat this as zero distance.

Nit: s/Now,/For now,/

(As with my comment on the previous patch--I wonder when we plan to do this.)

	Boris Chiou [:boris] Assignee
	Comment 26 • 7 years ago
mozreview-review-reply

Comment on attachment 8896191 [details]
Bug 1362896 - Part 2: Use f64 for Quaternion.

https://reviewboard.mozilla.org/r/167466/#review173202

> Isn't |self_portion| already f64?

Oh yes. I should remove the type coercion.

	Boris Chiou [:boris] Assignee
	Comment 27 • 7 years ago
mozreview-review-reply

Comment on attachment 8896192 [details]
Bug 1362896 - Part 3: Implement compute_distance for TransformList.

https://reviewboard.mozilla.org/r/167468/#review173204

> Nit: a unit direction...
> 
> Also, do we need any of the comment from the below here:
> 
>   http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/gfx/thebes/gfxQuaternion.h#34-46

I will copy the comment from there. Thanks.

> Yeah, I noticed this code is not in Gecko and I'm curious how it handles this case.

In Gecko [1], if we cannot normalize the vector, we will do nothing on it, and then convert the un-normalized vector into a quaternion. This may cause an assertion [2]. Therefoe, Gecko is not correct, either. I will update both in an extra patch to let you review. And then merge it together in the Servo PR..

[1] http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/layout/style/StyleAnimationValue.cpp#1345-1368
[2] http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/gfx/thebes/gfxQuaternion.h#48-49

> Should we? Gecko does and it seems pretty likely to occur?
> 
> Do we plan to do this in a separate bug?

I would like to file another bug for this. Thanks. This needs to convert a transform list into a matrix, and we need the layout info to convert the calc value in translate() into the used value (Servo: [1], Gecko: [2]). That is the only one who uses the layout info for computing the distance. (Maybe we can just drop the percent value in Servo, but it would be better to do that in another bug.)

[1] http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/servo/components/layout/fragment.rs#2909-2912
[2] http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/layout/style/StyleAnimationValue.cpp#1462-1463

	Boris Chiou [:boris] Assignee
	Comment 28 • 7 years ago
mozreview-review-reply

Comment on attachment 8896193 [details]
Bug 1362896 - Part 4: Implement compute_distance for Matrix and Perspective.

https://reviewboard.mozilla.org/r/167470/#review173218

> I don't quite follow this comment. Care to clarify the difference?

1. It seems Gecko doesn't match the spec exactly, and it uses a different way to decompose 2d matrix, and it's output [1] is:
  a) scale factors
  b) translate factors
  c) rotate factor
  d) _skew angle (shearXY)_
2. Servo follows the spec, and the output is:
  a) scale factors
  b) translate factors
  c) rotate angle
  d) _inner matrix (2x2 matrix)_

Therefore, (d) in Servo is a matrix, not an angle. That's the difference. So I compute the distance by the matrix items directly here, instead of the difference of skew angle. (We may have a way to convert the matrix into the skew factors, but that needs more time to understand the physical meaning of the inner matrix.)

  
[1] http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/layout/style/nsStyleTransformMatrix.cpp#1191-1194
[2] http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/servo/components/style/properties/helpers/animated_properties.mako.rs#1800-1808

> I don't understand the parenthetical (the part in parentheses). Does Gecko have a bug here?

Yes. Both Gecko and Servo should divide the skew factor by scale value to get the correct atan() value. Maybe I have to add an extra patch to fix this and Gecko.

> I couldn't find the corresponding code for this in Gecko so I couldn't check that, but it seems right to me.

We use nsStyleTransformMatrix::ApplyPerspectiveToMatrix [1] to apply the perspective length to a matrix.

[1] http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/layout/style/StyleAnimationValue.cpp#1392

	Boris Chiou [:boris] Assignee
	Comment 29 • 7 years ago

(In reply to Anthony Ramine [:nox] from comment #23)
> Note that https://github.com/servo/servo/pull/18058 landed and you'll have
> to rebase. Feel free to address Brian's comments here and then file a
> rebased PR directly on Servo and I'll take over the review over there.

Thanks, Anthony. I will rebase it and r?you in the PR.

	Brian Birtles (:birtles) Reporter
	Comment 30 • 7 years ago
mozreview-review

Comment on attachment 8896194 [details]
Bug 1362896 - Part 2: Add a test for computation of distance of transform.

https://reviewboard.mozilla.org/r/167472/#review173280

That's some pretty thorough testing. Thanks!
I most skimmed the last few tests.

::: dom/animation/test/mozilla/test_distance_of_transform.html:22
(Diff revision 1)
> +function get_distance(div, prop, v1, v2) {
> +  return SpecialPowers.DOMWindowUtils
> +           .computeAnimationDistance(div, prop, v1, v2);
> +}
> +
> +// Please make sure the |v| is an unit vector.

// |v| should be a unit vector (i.e. having length 1)

::: dom/animation/test/mozilla/test_distance_of_transform.html:23
(Diff revision 1)
> +function get_quaternion(v, angle) {
> +  return [ v[0] * Math.sin(angle / 2.0),
> +           v[1] * Math.sin(angle / 2.0),
> +           v[2] * Math.sin(angle / 2.0),
> +           Math.cos(angle / 2.0) ];
> +}
> +
> +function compute_rotate_distance(q1, q2) {

These two functions should probably use camelCase

::: dom/animation/test/mozilla/test_distance_of_transform.html:24
(Diff revision 1)
> +  return [ v[0] * Math.sin(angle / 2.0),
> +           v[1] * Math.sin(angle / 2.0),
> +           v[2] * Math.sin(angle / 2.0),
> +           Math.cos(angle / 2.0) ];

Nit: Alignment here (looks like the last 3 lines need one more space)

::: dom/animation/test/mozilla/test_distance_of_transform.html:31
(Diff revision 1)
> +  var dot = q1.reduce(function (sum, e, i) {
> +    return sum + e * q2[i];
> +  }, 0);

Note, feel free to use ES6 for these tests (especially the Mozilla-specific ones, but, depending on the feature and test suite, potentially for web-platform-tests too). That would let you right this a little more succinctly as:

  const dot = q1.reduce((sum, e, i) => sum + e * q2[i], 0);

::: dom/animation/test/mozilla/test_distance_of_transform.html:38
(Diff revision 1)
> +  }, 0);
> +  return Math.acos(Math.min(Math.max(dot, -1.0), 1.0)) * 2.0;
> +}
> +
> +function createMatrixFromArray(array) {
> +  return (array.length == 16 ? 'matrix3d' : 'matrix') +

Nit: ===

::: dom/animation/test/mozilla/test_distance_of_transform.html:39
(Diff revision 1)
> +  return Math.acos(Math.min(Math.max(dot, -1.0), 1.0)) * 2.0;
> +}
> +
> +function createMatrixFromArray(array) {
> +  return (array.length == 16 ? 'matrix3d' : 'matrix') +
> +         '(' + array.join() + ')';

Likewise this would become `(${array.join()})`

It's up to you to decide what you think is more clear, however.

::: dom/animation/test/mozilla/test_distance_of_transform.html:192
(Diff revision 1)
> +                          'perspective(128px)',
> +                          'perspective(0)');
> +  assert_equals(dist, 1/128, 'distance of perspective');

Is that really how it works? That the distance between perspective(0) -> perspective(128px) is *greater* than the distance from perspective(0) -> perspective(500px)?

	Boris Chiou [:boris] Assignee
	Comment 31 • 7 years ago
mozreview-review-reply

Comment on attachment 8896194 [details]
Bug 1362896 - Part 2: Add a test for computation of distance of transform.

https://reviewboard.mozilla.org/r/167472/#review173280

> Is that really how it works? That the distance between perspective(0) -> perspective(128px) is *greater* than the distance from perspective(0) -> perspective(500px)?

Yes. We convert a perspective function into a matrix, and then do matrix decomposion to get the perspective factor, which is the reciprocal of the perspective length. Your question is what I didn't notice before: 0<->128px should be smaller than 0<->500px. Maybe it's better to compute the distance by using the original perspective length. However, we have to update this and Gecko together. I can add an extra patch to fix them.

	Boris Chiou [:boris] Assignee
	Comment 32 • 7 years ago

Thanks, Brian. I will add 2 or 3 extra patches to fix some minor bugs which also happened in Gecko, (so it would be much easier to know what change we will apply in both Servo and Gecko,) and add the related tests in the test file.

	Boris Chiou [:boris] Assignee
	Comment 33 • 7 years ago
mozreview-review-reply

Comment on attachment 8896194 [details]
Bug 1362896 - Part 2: Add a test for computation of distance of transform.

https://reviewboard.mozilla.org/r/167472/#review173280

> Yes. We convert a perspective function into a matrix, and then do matrix decomposion to get the perspective factor, which is the reciprocal of the perspective length. Your question is what I didn't notice before: 0<->128px should be smaller than 0<->500px. Maybe it's better to compute the distance by using the original perspective length. However, we have to update this and Gecko together. I can add an extra patch to fix them.

> Is that really how it works? That the distance between perspective(0) -> perspective(128px) is *greater* than the distance from perspective(0) -> perspective(500px)?

Sorry, it seems I have to update the description of this test case. According to the spec: The value for depth must be greater than zero, otherwise the function is invalid. And the "invalid" means we use an identity matrix, so perspective(0) == perspective(inf), and the distance between perspective(inf) -> perspective(128px) is *greater* than the distance from perspective(inf) -> perspective(500px). However, this comparison doesn't make sense because we use "inf". Another example:
perspective(2px) -> perspective(128px): 1/2 - 1/128 = 63/128 = 0.4921875
perspective(2px) -> perspective(500px): 1/2 - 1/500 = 249/500 = 0.498, so the second one is greater than the first one.

	Brian Birtles (:birtles) Reporter
	Comment 40 • 7 years ago
mozreview-review

Comment on attachment 8897262 [details]
Bug 1362896 - Part 6: Fix the computation of distance for non-normalizable direction vector of rotate3d.

https://reviewboard.mozilla.org/r/168210/#review173794

::: dom/animation/test/mozilla/test_distance_of_transform.html:172
(Diff revision 1)
>    var target = addDiv(t);
> +  var dist = getDistance(target, 'transform',
> +                         'rotate3d(0, 0, 1, 90deg)',
> +                         'rotate3d(0, 0, 0, 90deg)');
> +  assert_equals(dist, Math.PI / 2, 'distance of rotate3d');
> +}, 'Test distance of rotate3d functions');

This should have a unique test name (so we can tell easily which test failed by looking at the log but also so we know how it is supposed to be different from the previous test)

::: layout/style/StyleAnimationValue.cpp:1355
(Diff revision 1)
> +      if (vector1.Length() > 0) {
> +        vector1.Normalize();
> +      } else {
> +        vector1 = identity;
> +        angle1 = 0.0;
> +      }
> +
> +      if (vector2.Length() > 0) {
> -      vector2.Normalize();
> +        vector2.Normalize();
> +      } else {
> +        vector2 = identity;
> +        angle2 = 0.0;
> +      }

Make a little lambda function for this and pass (vector1, angle1) / (vector2, angle2) to it?

Also, it might be little more clear with ordering like

  Point3D vector1(...)
  double angle1 = ...

  Point3D vector2(...)
  double angle2 = ...

  auto normalizeVector = [](Point3D& vector, double& angle) ...
  normalizeVector(vector1, angle1);
  normalizeVector(vector2, angle2);

	Boris Chiou [:boris] Assignee
	Comment 44 • 7 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 47 • 7 years ago

try with new updates: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd3bcf48ad64054d6784bcbf1875ea47f8318ab0

	Mozilla Autoland
	Comment 48 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b6997bdab2e0 -d 3e233c4c7867: rebasing 414545:b6997bdab2e0 "Bug 1362896 - Part 1: Fix the computation of distance for non-normalizable direction vector of rotate3d. r=birtles"
rebasing 414546:a3c88dbcd9bc "Bug 1362896 - Part 2: Add a test for computation of distance of transform. r=birtles" (tip)
merging dom/animation/test/mochitest.ini
warning: conflicts while merging dom/animation/test/mochitest.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Pulsebot
	Comment 51 • 7 years ago

Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dfa270d2b720
Part 1: Fix the computation of distance for non-normalizable direction vector of rotate3d. r=birtles
https://hg.mozilla.org/integration/autoland/rev/e333c1e6900f
Part 2: Add a test for computation of distance of transform. r=birtles

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 52 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/dfa270d2b720
https://hg.mozilla.org/mozilla-central/rev/e333c1e6900f