Bug 234895 – [Web Animations] inserting a rule within a @keyframes rule should update animations

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

[Web Animations] inserting a rule within a @keyframes rule should update animations

[Antoine Quint]

Created attachment 448426 [details]
Patch

[Darin Adler]

Comment on attachment 448426 [details]
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();

[Antti Koivisto]

Comment on attachment 448426 [details]
Patch

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

>> 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.

The shared function exist already. This should be done in CSSStyleSheet::didMutateRules. There should probably be a new RuleMutationType for it.

[Antoine Quint]

Created attachment 448484 [details]
Patch for landing

[Antoine Quint]

(In reply to Darin Adler from comment #2)
> Comment on attachment 448426 [details]
> 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?

I started a thread about this on Slack: https://webkit.slack.com/archives/C01ARTA5TDM/p1641467768004500.

> > 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.

Changed to `void keyframesRuleDidChange(const String& name);`

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

Fixed.

[Antoine Quint]

(In reply to Antti Koivisto from comment #3)
> Comment on attachment 448426 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448426&action=review
> 
> >> 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.
> 
> The shared function exist already. This should be done in
> CSSStyleSheet::didMutateRules. There should probably be a new
> RuleMutationType for it.

This is addressed in the patch for landing.

[Antti Koivisto]

Comment on attachment 448484 [details]
Patch for landing

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

> Source/WebCore/css/CSSStyleSheet.h:105
> -        RuleMutationScope(CSSStyleSheet*, RuleMutationType = OtherMutation, StyleRuleKeyframes* insertedKeyframesRule = nullptr);
> +        RuleMutationScope(CSSStyleSheet*, RuleMutationType = RuleMutationType::OtherMutation, StyleRuleKeyframes* insertedKeyframesRule = nullptr);
>          RuleMutationScope(CSSRule*);

I think it would be better to add RuleMutationType argument to the second constructor version and use it for CSSKeyframesRule mutations (or just test for CSSKeyframesRule in the constructor to set the appropriate mutation type). The idea here is that the first constructor is used when stylesheet is itself mutated while the seconds is used when some rule mutates.

[Antoine Quint]

(In reply to Antti Koivisto from comment #7)
> Comment on attachment 448484 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448484&action=review
> 
> > Source/WebCore/css/CSSStyleSheet.h:105
> > -        RuleMutationScope(CSSStyleSheet*, RuleMutationType = OtherMutation, StyleRuleKeyframes* insertedKeyframesRule = nullptr);
> > +        RuleMutationScope(CSSStyleSheet*, RuleMutationType = RuleMutationType::OtherMutation, StyleRuleKeyframes* insertedKeyframesRule = nullptr);
> >          RuleMutationScope(CSSRule*);
> 
> I think it would be better to add RuleMutationType argument to the second
> constructor version and use it for CSSKeyframesRule mutations (or just test
> for CSSKeyframesRule in the constructor to set the appropriate mutation
> type). The idea here is that the first constructor is used when stylesheet
> is itself mutated while the seconds is used when some rule mutates.

OK, I'll infer the mutation type from the CSSRule type and add a `String m_modifiedKeyframesRuleName` member to `RuleMutationScope` to track the modified @keyframes rule name.

[Antoine Quint]

Created attachment 448504 [details]
Patch for landing

[Darin Adler]

Two other thoughts for refinement (tiny nits), neither urgent:

I think that "hasPendingKeyframesUpdate" would be better than "isPendingKeyframesUpdate".

One place here we have "is<CSSAnimation>(animation)" but it should be "is<CSSAnimation>(*animation)" since fact that the set uses pointers is an implementation detail and the pointers can never be null.

[Antoine Quint]

(In reply to Darin Adler from comment #10)
> Two other thoughts for refinement (tiny nits), neither urgent:
> 
> I think that "hasPendingKeyframesUpdate" would be better than
> "isPendingKeyframesUpdate".

I went back and forth between the two while writing the patch, but this tips me over to the other side. I've made that change to the commit.

> One place here we have "is<CSSAnimation>(animation)" but it should be
> "is<CSSAnimation>(*animation)" since fact that the set uses pointers is an
> implementation detail and the pointers can never be null.

Are you saying that the HashSet iterator skips over null values?

[Antoine Quint]

Committed r287707 (245793@trunk): <https://commits.webkit.org/245793@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/87212387>

[Darin Adler]

(In reply to Antoine Quint from comment #11)
> (In reply to Darin Adler from comment #10)
> > I think that "hasPendingKeyframesUpdate" would be better than
> > "isPendingKeyframesUpdate".
> 
> I went back and forth between the two while writing the patch, but this tips
> me over to the other side. I've made that change to the commit.

Great!

> Are you saying that the HashSet iterator skips over null values?

Sort of. You can’t put a nullptr into a HashSet of pointers, no values in the set can be nullptr. And yes the iterator does skip the null values, technically they are empty buckets.

[Antti Koivisto]

Comment on attachment 448504 [details]
Patch for landing

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

> Source/WebCore/css/CSSStyleSheet.cpp:181
> +    if (mutationType == RuleInsertion && !contentsWereClonedForMutation && !scope->activeStyleSheetsContains(this)) {
> +        if (insertedKeyframesRule) {
> +            if (auto* resolver = scope->resolverIfExists())
> +                resolver->addKeyframeStyle(*insertedKeyframesRule);
> +            return;
> +        }
> +        scope->didChangeActiveStyleSheetCandidates();
> +        return;
> +    }

We don't need two copies if this section of code!

[Antoine Quint]

(In reply to Antti Koivisto from comment #15)
> Comment on attachment 448504 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448504&action=review
> 
> > Source/WebCore/css/CSSStyleSheet.cpp:181
> > +    if (mutationType == RuleInsertion && !contentsWereClonedForMutation && !scope->activeStyleSheetsContains(this)) {
> > +        if (insertedKeyframesRule) {
> > +            if (auto* resolver = scope->resolverIfExists())
> > +                resolver->addKeyframeStyle(*insertedKeyframesRule);
> > +            return;
> > +        }
> > +        scope->didChangeActiveStyleSheetCandidates();
> > +        return;
> > +    }
> 
> We don't need two copies if this section of code!

Oh no! Fixed in r287741.

[Antoine Quint]

(In reply to Darin Adler from comment #14)
> (In reply to Antoine Quint from comment #11)
> > Are you saying that the HashSet iterator skips over null values?
> 
> Sort of. You can’t put a nullptr into a HashSet of pointers, no values in
> the set can be nullptr. And yes the iterator does skip the null values,
> technically they are empty buckets.

Filed a patch to remove the null checks in bug 234948.

[Antoine Quint]

*** Bug 210989 has been marked as a duplicate of this bug. ***