[webkit-reviews] review granted: [Bug 234895] [Web Animations] inserting a rule within a @keyframes rule should update animations : [Attachment 448426] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 234895: [Web Animations] inserting a rule within a @keyframes rule should
update animations
https://bugs.webkit.org/show_bug.cgi?id=234895

Attachment 448426: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 448426
  --> https://bugs.webkit.org/attachment.cgi?id=448426
Patch

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

> Source/WebCore/animation/CSSAnimation.cpp:249
> +    auto owningElement = this->owningElement();

Should this be RefPtr instead of auto?

> Source/WebCore/css/CSSKeyframesRule.cpp:168
> +    if (CSSStyleSheet* parent = parentStyleSheet()) {
> +	   if (Document* ownerDocument = parent->ownerDocument())
> +	       ownerDocument->cssKeyframesRuleDidChange(name());
> +    }

Would be nice to not have this be pasted twice; a shared function perhaps.
Separately, I suggest using auto more here.

> Source/WebCore/dom/Document.h:1541
> +    void cssKeyframesRuleDidChange(String);

I am not sure that the "css" here is needed; is there any other kind of
keyframes rule other than a CSS one?

It’s not obvious that the string argument here is the name of the rule, so
there should be an argument name "name" here.

I suggest either StringView (preferred) or const String& as the type so we
don’t do unnecessary reference count churn.

> Source/WebCore/dom/Element.cpp:4067
> +    if (auto* animationData = animationRareData(pseudoId))
> +	   return animationData->isPendingKeyframesUpdate();
> +    return false;

How I would write this:

    auto data = animationRareData(pseudoId);
    return data && data->isPendingKeyframesUpdate();