CSSKeyframesRule::findRule() and deleteRule() should delete the last matching rule, not the first

• Component: Animations
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

css-wg resolved that findRule and deleteRule should match the last rule, not the first.

[Simon Fraser (smfr)]

Also need to make sure we normalize whitespace, and allow "100.0%" to match "100%" etc.

[Sylvain Galineau]

Created attachment 244565 [details]
fix.patch

This patch:
- Changes the stored representation of StyleKeyframe key values from a String to a Vector<double> (per FIXME comment)
- Makes sure findRule/deleteRule apply to the last specified rule with a matching keyframe selector

I would also note the following: I made StyleKeyframe::parseKeyString() a public static since StyleRuleKeyframes::findKeyframeIndex() also needs to parse the findRule/deleteRule String argument into a Vector<double>. It feels like the wrong place for this parsing helper though. On the Blink side, they have moved this to CSSParser. Thoughts on doing this here as well?

[Dean Jackson]

Comment on attachment 244565 [details]
fix.patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244565&action=review

> Source/WebCore/css/CSSKeyframeRule.cpp:81
> +String StyleKeyframe::keyText() const

I agree that this could be moved to CSSParser.

> Source/WebCore/css/CSSKeyframeRule.cpp:99
> +}
> +    
> +
> +    

Nit: few extra blank lines here

> Source/WebCore/css/CSSKeyframesRule.cpp:81
> +    for (size_t i = m_keyframes.size(); i--; ) {

Nit: space after the i--;

[Dean Jackson]

All good. If you have time to make the changes and upload a patch before you disappear, I'll mark it cq+.

[Darin Adler]

Comment on attachment 244565 [details]
fix.patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244565&action=review

> Source/WebCore/css/CSSKeyframeRule.cpp:64
> +        if (equalIgnoringCase(cur, "from"))

It would be better if this used an equalIgnoringASCIICase function. The standard equalIgnoringCase function is overkill since it implements the entire Unicode case folding algorithm.

Is this bug fix covered by new tests? I didn’t see any new testing for this, nor was there any mention of this fix in the bug title.

> Source/WebCore/css/CSSKeyframeRule.cpp:66
> +        else if (equalIgnoringCase(cur, "to"))

Same comments as above.

> Source/WebCore/css/CSSKeyframeRule.cpp:83
> +    ASSERT(!m_keys.isEmpty());

Do we really need this assertion? Would it be bad to have a function that returns an empty string if m_keys is empty?

> Source/WebCore/css/CSSKeyframeRule.h:48
> +    void setKeyText(const String& s) { parseKeyString(s, m_keys); }

Better to use the word “text” rather than “s” for this argument name.

> Source/WebCore/css/CSSKeyframeRule.h:57
> +    static void parseKeyString(const String&, Vector<double>& keys);

This should be changed to just use a return value, not an out argument. We used to think we needed out arguments for things like this before we better understood the return value optimization and move semantics. Now it’s just peculiar that this is an out argument.

> Source/WebCore/css/CSSKeyframeRule.h:63
> +    Vector<double> m_keys;

Seems like we should consider some inline capacity to save memory here. Maybe not use a Vector at all, since it has additional overhead to support resizing.

> Source/WebCore/css/CSSKeyframesRule.cpp:79
> +    if (!keys.size())
> +        return -1;

No need for this extra code. The function would work fine without it.

>> Source/WebCore/css/CSSKeyframesRule.cpp:81
>> +    for (size_t i = m_keyframes.size(); i--; ) {
> 
> Nit: space after the i--;

I think that space is good formatting; helps point out there is no third clause in the for loop.

> Source/WebCore/css/StyleResolver.cpp:902
> +        const Vector<double>& keys = keyframe->keys();
>          for (size_t keyIndex = 0; keyIndex < keys.size(); ++keyIndex) {
>              keyframeValue.setKey(keys[keyIndex]);

This should be a modern for loop:

    for (auto& key : keyframe->keys()) {
        keyframeValue.setKey(key);

Way nicer.

[Sylvain Galineau]

Created attachment 244708 [details]
fix.patch

Patch updated per feedback. Some comments:

- I moved the StyleKeyframe::parseKeyString() static to CSSParser::parseKeyframeSelector; per Darin's suggestion I made it return a Vector<double> instead of modifying an out argument. I'd note this causes a copy in StyleKeyframe::setKeyText() that we did not have before. StyleRuleKeyframes::findKeyframeIndex is able to use rvalue ref & move semantics though.
- Regarding adding an inline capacity for this Vector<double> I must admit I do not know what a good size is. Personal experience suggests most keyframe selectors rarely have more than a handful of values but tool/dynamically-generated rules may go past this and I have no data to judge how often this happens in the wild. I have left this unspecified in this patch.
- Last, regarding equalIgnoringASCIICase; I assume we are talking about the SelectorChecker helpers and pulled that into CSSParser.

[Dean Jackson]

Committed r179197: <http://trac.webkit.org/changeset/179197>

[Dean Jackson]

There were a few things that Darin mentioned that had not been updated in the latest patch - I fixed them manually.

[Csaba Osztrogonác]

(In reply to comment #7)
> Committed r179197: <http://trac.webkit.org/changeset/179197>

It broke the Windows build:
https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/66961