Closed Bug 1113419 Opened 5 years ago Closed 5 years ago

eUnit_Filter is inconsistently listed before/after eUnit_Shadow in enum definition / switch statements

Categories

(Core :: CSS Parsing and Computation, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

eUnit_Filter is listed between eUnit_Dasharray and eUnit_Shadow in StyleAnimationValue.h:
> 222     eUnit_Dasharray, // nsCSSValueList* (never null)
> 223     eUnit_Filter, // nsCSSValueList* (may be null)
> 224     eUnit_Shadow, // nsCSSValueList* (may be null)
> 225     eUnit_Transform, // nsCSSValueList* (never null)
https://mxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.h?rev=156fa5c40afc#222

But in ComputeDistance, we've got it listed after _Shadow:
> 761     case eUnit_Shadow: {
[...]
> 830     }
> 831     case eUnit_Filter:
> 832       // FIXME: Support paced animations for filter function interpolation.
> 833     case eUnit_Transform: {
> 834       return false;
> 835     }
https://mxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp?rev=983af7fa41f5#831

We should shift it earlier, to keep the order consistent.
Actually, AddWeighted has the same issue -- _Filter comes after _Shadow there as well.

It's a smaller change to just reorder the enum list (to make the ComputeDistance/AddWeighted ordering correct), so let's just do that.
Summary: "case eUnit_Filter" is at wrong position in StyleAnimationValue::ComputeDistance switch statement → eUnit_Filter is inconsistently listed before/after eUnit_Shadow in enum definition / switch statements
Attached patch fix v1Splinter Review
This makes the ordering consistently: _Dasharray, _Shadow, _Filter (matching the existing ordering in ComputeDistance & AddWeighted.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8538853 - Flags: review?(krit)
Comment on attachment 8538853 [details] [diff] [review]
fix v1

Review of attachment 8538853 [details] [diff] [review]:
-----------------------------------------------------------------

The changes look good to me.
Attachment #8538853 - Flags: review?(krit) → review+
https://hg.mozilla.org/mozilla-central/rev/056b301fd982
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.