[webkit-reviews] review granted: [Bug 237073] [css-cascade] Support 'revert' in @keyframes : [Attachment 452969] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Oriol Brufau <obrufau at igalia.com>'s
request for review:
Bug 237073: [css-cascade] Support 'revert' in @keyframes
https://bugs.webkit.org/show_bug.cgi?id=237073

Attachment 452969: Patch

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




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 452969
  --> https://bugs.webkit.org/attachment.cgi?id=452969
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.