Created attachment 448426 [details] Patch

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

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.

Created attachment 448484 [details] Patch for landing

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

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

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.

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

Created attachment 448504 [details] Patch for landing

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.

(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?

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

<rdar://problem/87212387>

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

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!

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

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

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