Bug 139732 - CSSKeyframesRule::findRule() and deleteRule() should delete the last matching rule, not the first
Summary: CSSKeyframesRule::findRule() and deleteRule() should delete the last matching...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sylvain Galineau
Depends on:
Reported: 2014-12-17 09:30 PST by Simon Fraser (smfr)
Modified: 2015-01-27 12:15 PST (History)
4 users (show)

See Also:

fix.patch (18.07 KB, patch)
2015-01-13 17:27 PST, Sylvain Galineau
dino: review+
Details | Formatted Diff | Diff
fix.patch (21.28 KB, patch)
2015-01-15 13:06 PST, Sylvain Galineau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2014-12-17 09:30:49 PST
css-wg resolved that findRule and deleteRule should match the last rule, not the first.
Comment 1 Simon Fraser (smfr) 2014-12-17 09:35:48 PST
Also need to make sure we normalize whitespace, and allow "100.0%" to match "100%" etc.
Comment 2 Sylvain Galineau 2015-01-13 17:27:31 PST
Created attachment 244565 [details]

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?
Comment 3 Dean Jackson 2015-01-14 10:40:36 PST
Comment on attachment 244565 [details]

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--;
Comment 4 Dean Jackson 2015-01-14 10:41:11 PST
All good. If you have time to make the changes and upload a patch before you disappear, I'll mark it cq+.
Comment 5 Darin Adler 2015-01-14 16:06:44 PST
Comment on attachment 244565 [details]

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()) {

Way nicer.
Comment 6 Sylvain Galineau 2015-01-15 13:06:44 PST
Created attachment 244708 [details]

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.
Comment 7 Dean Jackson 2015-01-27 11:39:35 PST
Committed r179197: <http://trac.webkit.org/changeset/179197>
Comment 8 Dean Jackson 2015-01-27 11:40:26 PST
There were a few things that Darin mentioned that had not been updated in the latest patch - I fixed them manually.
Comment 9 Csaba Osztrogonác 2015-01-27 12:15:37 PST
(In reply to comment #7)
> Committed r179197: <http://trac.webkit.org/changeset/179197>

It broke the Windows build: