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

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Created attachment 405355 [details]
testcase 1 (should go blank for a second, without spamming assertions)

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

9 years ago
Attachment #405355 - Attachment description: testcase 1 → testcase 1 (should go blank for a second, without spamming assertions)
(Assignee)

Comment 1

9 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

9 years ago
Created attachment 405367 [details] [diff] [review]
fix

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

9 years ago
Attachment #405367 - Flags: review?(dbaron)
(Assignee)

Comment 3

9 years ago
Created attachment 405368 [details] [diff] [review]
mochitest patch

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

9 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Comment 4

9 years ago
Created attachment 407191 [details] [diff] [review]
rebased fix

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

9 years ago
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+
(Assignee)

Comment 6

9 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
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.