Closed Bug 598099 Opened 9 years ago Closed 9 years ago

add tests for nsStyleAnimation::ComputeDistance

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(3 files)

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.
Marking this as a blocker, because I have tests, and they turned up a bug that I think needs to be fixed.
blocking2.0: --- → betaN+
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.
Attachment #484519 - Flags: review?(bzbarsky)
This should at least be capable of reversing AddWeighted correctly.
Attachment #484520 - Flags: review?(bzbarsky)
Whiteboard: [needs review]
Comment on attachment 484519 [details] [diff] [review]
patch 1: fix AnimValuesStyleRule::MapRuleInfoInto

r=me
Attachment #484519 - Flags: review?(bzbarsky) → review+
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 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?
> 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 on attachment 484522 [details] [diff] [review]
patch 3: tests for ComputeDistance

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

Please comment that.
Attachment #484522 - Flags: review?(bzbarsky) → review+
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?
Attachment #484520 - Flags: review?(bzbarsky) → review+
(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
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Priority: -- → P3
Resolution: --- → FIXED
Whiteboard: [needs review]
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.