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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 1

[Daniel Holbert [:dholbert]]

(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"

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: 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.)

Comment 5

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

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.

Comment 6

[Daniel Holbert [:dholbert]]

No worries -- thanks for the review!
  patch: http://hg.mozilla.org/mozilla-central/rev/6108fdf296de
  tests: http://hg.mozilla.org/mozilla-central/rev/5a68b843f4d9