Bug 66118 – HANDLE_ANIMATION_INHERIT_AND_INITIAL macro fails to actually iterate.

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Luke Macpherson]

Noticed this during refactoring:

CSSStyleSelector.cpp:183    list->animation(0)->set##Prop(Animation::initialAnimation##Prop()); \
CSSStyleSelector.cpp:184    for (size_t i = 1; i < list->size(); ++i) \
CSSStyleSelector.cpp:185        list->animation(0)->clear##Prop(); \

As you can see, i is not used in the body of the loop, and instead the 0th element is set repeatedly, which also negates the effect of line 183.
Looks like this code has been in since 36634, and I don't know what user-visible bugs this causes, if any.

[Luke Macpherson]

This bug also appears in the HANDLE_TRANSITION_INHERIT_AND_INITIAL macro.

[Darin Adler]

Bad news. Somebody should figure out what the symptom is and make a test so we can fix it!

[Dean Jackson]

I'll take a look.

[Luke Macpherson]

FYI: https://bugs.webkit.org/show_bug.cgi?id=66126 would fix this en passant, but doesn't add tests etc. It may be better to fix/test/close this bug before that goes in, but that's up to reviewers I suppose.

[Dean Jackson]

I agree. We should fix this separately.

[Radar WebKit Bug Importer]

<rdar://problem/9973753>

[Dean Jackson]

The reason we've never noticed this is that it would mean setting an animation property value to 'initial'. I doubt many people have ever done this. Not only that, since the properties are not inherited you always have the initial value. I'm trying to work out how to write test that hits this code (in which case, why do we even have it?)

[Dean Jackson]

I don't want to hold up this much longer, especially since the linked bug would fix it, but I'm wondering if the fact we never saw this cause breakage is that it is hidden by another bug.

This code would only be triggered in the case where an animation property is given the value 'initial'. Animation properties don't inherit, so it isn't a big deal. But I notice this macro does something weird as it is creating the list of animations:

    for (CSSValueListIterator i = value; i.hasMore(); i.advance()) { \
        if (childIndex <= list->size()) \
            list->append(Animation::create()); \
        mapAnimation##Prop(list->animation(childIndex), i.value()); \
        ++childIndex; \

childIndex is 0 when this loop begins. Notice that we append to the list, but then set the value of the first element. We never touch the element we appended.

Unless anyone objects, I'm going to close this as WONTFIX and we'll investigate in https://bugs.webkit.org/show_bug.cgi?id=66126 if the above code is buggy, or is doing unnecessary work.

[Brent Fulgham]

Fixed in http://trac.webkit.org/changeset/93848