Bug 237073 – [css-cascade] Support 'revert' in @keyframes

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Oriol Brufau]

Created attachment 452930 [details]
testcase

'initial', 'unset' and 'inherit' work well in @keyframes.
But 'revert' doesn't work, it just behaves as 'unset'.
It works in Blink: https://chromium-review.googlesource.com/c/chromium/src/+/2132249

[Oriol Brufau]

Created attachment 452931 [details]
Patch

[Oriol Brufau]

Created attachment 452933 [details]
Patch

[Oriol Brufau]

Created attachment 452968 [details]
Patch

[Oriol Brufau]

Created attachment 452969 [details]
Patch

[Darin Adler]

Comment on attachment 452969 [details]
Patch

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

Seems fine as is, a few thoughts

> Source/WebCore/style/StyleResolver.cpp:283
> +    unsigned propertyCount = keyframe.properties().propertyCount();
> +    bool hasRevert = false;
> +    for (unsigned i = 0; i < propertyCount; ++i) {

I think in future we should add begin/end to StyleProperties so loops like this can be a modern style for loop instead of an indexed one like this. Not in this patch, but important for the future, I think.

> Source/WebCore/style/StyleResolver.cpp:284
> +        const auto& propertyReference = keyframe.properties().propertyAt(i);

This should just be "auto". The wordier "const auto&" has no benefit here as far as I can tell.

> Source/WebCore/style/StyleResolver.cpp:292
> +        if (const CSSValue* value = propertyReference.value())
> +            hasRevert = hasRevert || value->isRevertValue();

I would write it this way:

    if (auto* value = propertyReference.value(); value && value->isRevertValue())
        hasRevert = true;

There are many different styles, but once we are doing an if, I think it’s best to include both checks in the if, it’s useful to use auto in a case like this to make sure we don’t accidentally upcast? If we had a helper function, could also consider:

    hasRevert = hasRevert || isRevertValue(propertyReference.value());

Might be nice to save the work of getting the value and type-checking it entirely once we found a revert, but also that doesn’t seem to be an important optimization.

> Source/WebCore/style/StyleResolver.cpp:309
> +    ElementRuleCollector collector(element, m_ruleSets, context.selectorMatchingState);
> +    collector.setPseudoElementRequest({ elementStyle.styleType() });
> +    if (hasRevert) {
> +        // In the animation origin, 'revert' rolls back the cascaded value to the user level.
> +        // Therefore, we need to collect UA and user rules.
> +        collector.setMedium(&m_mediaQueryEvaluator);
> +        collector.matchUARules();
> +        collector.matchUserRules();
> +    }
> +    collector.addAuthorKeyframeRules(keyframe);

This looks kind of expensive. Would like to know whether Antti or some other expert thinks this is OK. Not necessarily before landing? Also, if there is a lot of state in the ElementRuleCollector might be nice to get the match result and then let that go out of scope before allocating the Builder, by using a lambda, braces, or a helper function.

[Oriol Brufau]

Comment on attachment 452969 [details]
Patch

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

>> Source/WebCore/style/StyleResolver.cpp:309
>> +    collector.addAuthorKeyframeRules(keyframe);
> 
> This looks kind of expensive. Would like to know whether Antti or some other expert thinks this is OK. Not necessarily before landing? Also, if there is a lot of state in the ElementRuleCollector might be nice to get the match result and then let that go out of scope before allocating the Builder, by using a lambda, braces, or a helper function.

But ElementRuleCollector::matchResult() returns a reference to a member, if the ElementRuleCollector instance goes out of scope, wouldn't using the match result be undefined behavior? Or if I copy it, then it can also be slow if the vectors of matched properties are big.

[Darin Adler]

Comment on attachment 452969 [details]
Patch

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

>>> Source/WebCore/style/StyleResolver.cpp:309
>>> +    collector.addAuthorKeyframeRules(keyframe);
>> 
>> This looks kind of expensive. Would like to know whether Antti or some other expert thinks this is OK. Not necessarily before landing? Also, if there is a lot of state in the ElementRuleCollector might be nice to get the match result and then let that go out of scope before allocating the Builder, by using a lambda, braces, or a helper function.
> 
> But ElementRuleCollector::matchResult() returns a reference to a member, if the ElementRuleCollector instance goes out of scope, wouldn't using the match result be undefined behavior? Or if I copy it, then it can also be slow if the vectors of matched properties are big.

Good point, we would have to make a copy. For now, I guess we can leave this as-is.

We can later add takeMatchResult which moves it instead of copying. For now, I we can leave this as-is, but I am not sure the current approach is ideal for memory use.

[Oriol Brufau]

Comment on attachment 452969 [details]
Patch

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

>> Source/WebCore/style/StyleResolver.cpp:284
>> +        const auto& propertyReference = keyframe.properties().propertyAt(i);
> 
> This should just be "auto". The wordier "const auto&" has no benefit here as far as I can tell.

Not an expert, doesn't "auto" alone make an unnecessary copy? https://stackoverflow.com/questions/7138588/c11-auto-what-if-it-gets-a-constant-reference

[Darin Adler]

Comment on attachment 452969 [details]
Patch

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

>>> Source/WebCore/style/StyleResolver.cpp:284
>>> +        const auto& propertyReference = keyframe.properties().propertyAt(i);
>> 
>> This should just be "auto". The wordier "const auto&" has no benefit here as far as I can tell.
> 
> Not an expert, doesn't "auto" alone make an unnecessary copy? https://stackoverflow.com/questions/7138588/c11-auto-what-if-it-gets-a-constant-reference

No.

The return value of propertyAt is a PropertyReference object. That object will always be copied (or moved). Using the "const auto&" syntax has no effect on that one way or the other.

If the return value was a reference, that would be different, and then I would suggest "auto&", not "const auto&".

[Oriol Brufau]

Created attachment 452997 [details]
Patch

[Oriol Brufau]

Comment on attachment 452969 [details]
Patch

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

>>>> Source/WebCore/style/StyleResolver.cpp:284
>>>> +        const auto& propertyReference = keyframe.properties().propertyAt(i);
>>> 
>>> This should just be "auto". The wordier "const auto&" has no benefit here as far as I can tell.
>> 
>> Not an expert, doesn't "auto" alone make an unnecessary copy? https://stackoverflow.com/questions/7138588/c11-auto-what-if-it-gets-a-constant-reference
> 
> No.
> 
> The return value of propertyAt is a PropertyReference object. That object will always be copied (or moved). Using the "const auto&" syntax has no effect on that one way or the other.
> 
> If the return value was a reference, that would be different, and then I would suggest "auto&", not "const auto&".

Ah, sure, somehow "PropertyReference" made my mind think it was a a reference XD.
Done.

>> Source/WebCore/style/StyleResolver.cpp:292
>> +            hasRevert = hasRevert || value->isRevertValue();
> 
> I would write it this way:
> 
>     if (auto* value = propertyReference.value(); value && value->isRevertValue())
>         hasRevert = true;
> 
> There are many different styles, but once we are doing an if, I think it’s best to include both checks in the if, it’s useful to use auto in a case like this to make sure we don’t accidentally upcast? If we had a helper function, could also consider:
> 
>     hasRevert = hasRevert || isRevertValue(propertyReference.value());
> 
> Might be nice to save the work of getting the value and type-checking it entirely once we found a revert, but also that doesn’t seem to be an important optimization.

Done.

>>>> Source/WebCore/style/StyleResolver.cpp:309
>>>> +    collector.addAuthorKeyframeRules(keyframe);
>>> 
>>> This looks kind of expensive. Would like to know whether Antti or some other expert thinks this is OK. Not necessarily before landing? Also, if there is a lot of state in the ElementRuleCollector might be nice to get the match result and then let that go out of scope before allocating the Builder, by using a lambda, braces, or a helper function.
>> 
>> But ElementRuleCollector::matchResult() returns a reference to a member, if the ElementRuleCollector instance goes out of scope, wouldn't using the match result be undefined behavior? Or if I copy it, then it can also be slow if the vectors of matched properties are big.
> 
> Good point, we would have to make a copy. For now, I guess we can leave this as-is.
> 
> We can later add takeMatchResult which moves it instead of copying. For now, I we can leave this as-is, but I am not sure the current approach is ideal for memory use.

OK, I guess I will wait a bit in case Antti says something, otherwise land anyways.

[EWS]

Committed r290457 (247758@main): <https://commits.webkit.org/247758@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452997 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89436026>