Closed Bug 521335 Opened 14 years ago Closed 14 years ago

nsStyleAnimation::Interpolate/Add/ComputeDistance need to not spam warnings for eStyleUnit_None

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

So after bug 520325, nsStyleAnimation::Interpolate/Add/ComputeDistance will quietly fail (as expected) when given e.g. "none" and a color value.  They silently fail because GetCommonUnit returns eStyleUnit_Null (because the units don't match), which drops us into the silent-failure case.

However, if Interpolate/Add/ComputeDistance receive two "none" values, then their units *do* match, and GetCommonUnit will return eStyleUnit_None -- *not* eStyleUnit_Null -- and we'll drop into the "default" case and spam a NS_NOTREACHED like this:
{
###!!! ASSERTION: Can't interpolate using the given common unit: 'Not Reached', file /mozilla/layout/style/nsStyleAnimation.cpp, line 323
}
Our is still correct, but we shouldn't be spamming assertions-failures.

Patch coming up.
Attachment #405355 - Attachment description: testcase 1 → testcase 1 (should go blank for a second, without spamming assertions)
(In reply to comment #0)
> Our is still correct, but we shouldn't be spamming assertions-failures.

Sorry -- I meant "Our _behavior_ is still correct"
Attached patch fix (obsolete) — Splinter Review
This patch just makes eStyleUnit_None share the same case as eStyleUnit_Null.  (We'll need to do the same for a newly-created "eStyleUnit_Enum" unit when we add support for enumerated values in bug 520486)

Alternately, we could get rid of the eStyleUnit_Null case, let it fall through to "default", and remove the NOTREACHED from the default block.  But I think I prefer the patch as-is... It feels safer to have the expected cases explicitly enumerated, with the "default" case that spams an error message to warn us if we forget to add a case for a new unit-type later on.
Attachment #405367 - Flags: review?(dbaron)
Attached patch mochitest patchSplinter Review
This patch adds some mochitests that pass "none"/"none" into Interpolate, Add, and ComputeDistance.  These tests spam NOTREACHED assertions like the one in Comment 0 without the fix, but they're nice & quiet with the fix.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attached patch rebased fixSplinter Review
Here's the fix again, rebased to apply on top of bug 522852 and bug 523193.  (It's just a 2-line fix.)
Attachment #405367 - Attachment is obsolete: true
Attachment #405367 - Flags: review?(dbaron)
Attachment #407191 - Attachment description: updated fix → rebased fix
Attachment #407191 - Flags: review?(dbaron)
Comment on attachment 407191 [details] [diff] [review]
rebased fix

r=dbaron

Sorry, should have reviewed this sooner so you didn't have to rebase it.
Attachment #407191 - Flags: review?(dbaron) → review+
No worries -- thanks for the review!
  patch: http://hg.mozilla.org/mozilla-central/rev/6108fdf296de
  tests: http://hg.mozilla.org/mozilla-central/rev/5a68b843f4d9
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.