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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
271 bytes,
image/svg+xml
|
Details | |
4.62 KB,
patch
|
Details | Diff | Splinter Review | |
1.26 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•14 years ago
|
Attachment #405355 -
Attachment description: testcase 1 → testcase 1 (should go blank for a second, without spamming assertions)
Assignee | ||
Comment 1•14 years ago
|
||
(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"
Assignee | ||
Comment 2•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Attachment #405367 -
Flags: review?(dbaron)
Assignee | ||
Comment 3•14 years ago
|
||
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 | ||
Updated•14 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #407191 -
Attachment description: updated fix → rebased fix
Attachment #407191 -
Flags: review?(dbaron)
Comment 5•14 years ago
|
||
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+
Assignee | ||
Comment 6•14 years ago
|
||
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
Updated•14 years ago
|
Blocks: css-transitions
You need to log in
before you can comment on or make changes to this bug.
Description
•