598099 - add tests for nsStyleAnimation::ComputeDistance

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In bug 520234 comment 22 bzbarsky pointed out the lack of tests for ComputeDistance (which I knew about, and was perhaps planning to deal with as part of bug 526784).  However, it should really have its own bug.

I have patches; they need a little more work.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Marking this as a blocker, because I have tests, and they turned up a bug that I think needs to be fixed.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: fix AnimValuesStyleRule::MapRuleInfoInto

I need this because patch 3 uses nsStyleAnimation::ComputeValue, which in turn uses ResolveStyleByAddingRules.

But I believe it also fixes a SMIL+transitions interaction bug.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: fix distance metric for transforms

This should at least be capable of reversing AddWeighted correctly.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: tests for ComputeDistance

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 484519 [details] [diff] [review]
patch 1: fix AnimValuesStyleRule::MapRuleInfoInto

r=me

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 484520 [details] [diff] [review]
patch 2: fix distance metric for transforms

This looks fine, for the most part.  The one worry is the skew distance calculation.  It can end up with diff == Infinity, I think, or worse yet diff being a NaN (e.g. when the float value is pi/2 for both v1 and v2, modulo rounding error).  Why did you decide to go with tan() here?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 484522 [details] [diff] [review]
patch 3: tests for ComputeDistance

>+  if (property != eCSSProperty_UNKNOWN && nsCSSProps::IsShorthand(property)) {
>+    nsCSSProperty subprop0 = *nsCSSProps::SubpropertyEntryFor(property);
>+    if (nsCSSProps::PropHasFlags(subprop0, CSS_PROPERTY_REPORT_OTHER_NAME) &&
>+        nsCSSProps::OtherNameFor(subprop0) == property) {
>+      property = subprop0;
>+    } else {
>+      property = eCSSProperty_UNKNOWN;
>+    }
>+  }

I don't understand this chunk of code.... it's mapping shorthands to some other property names sometimes?  Why?

The rest looks fine; make sure to land the interface change for b7?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> This looks fine, for the most part.  The one worry is the skew distance
> calculation.  It can end up with diff == Infinity, I think, or worse yet diff
> being a NaN (e.g. when the float value is pi/2 for both v1 and v2, modulo
> rounding error).  Why did you decide to go with tan() here?

I went with tan() because we also use tan() for interpolation of skew (since we animate it as though it was shear() rather than skew()).  I guess this is an issue for both distance and interpolation, though.

(In reply to comment #7)
> I don't understand this chunk of code.... it's mapping shorthands to some other
> property names sometimes?  Why?

It's mapping border-left to border-left-value, etc.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 484522 [details] [diff] [review]
patch 3: tests for ComputeDistance

> It's mapping border-left to border-left-value, etc.

Please comment that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 484520 [details] [diff] [review]
patch 2: fix distance metric for transforms

> I went with tan() because we also use tan() for interpolation of skew 

OK.  Let's do this for now, then, but file a followup on addressing this issue?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #10)
> Comment on attachment 484520 [details] [diff] [review]
> patch 2: fix distance metric for transforms
> 
> > I went with tan() because we also use tan() for interpolation of skew 
> 
> OK.  Let's do this for now, then, but file a followup on addressing this issue?

filed bug 606722

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/af70dc124ddd
http://hg.mozilla.org/mozilla-central/rev/567e9ffd65cb
http://hg.mozilla.org/mozilla-central/rev/de2d90ff2ac7

I also filed bug 606726 on syncing nsIDOMWindowUtils on the 2.0b7 branch, which is easiest to do *after* compartments lands on that branch.