Summary: | CSS animation sorting may crash due to AnimationList copy upon CSS Animation removal | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||
Component: | Animations | Assignee: | Antoine Quint <graouts> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, dino, gnavamarino, graouts, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Antoine Quint
2021-12-03 09:24:04 PST
Created attachment 445868 [details]
Patch
Comment on attachment 445868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445868&action=review > Source/WebCore/ChangeLog:52 > + When the ChangeLog is much bigger than the change. > Source/WebCore/platform/animation/AnimationList.h:39 > + Ref<AnimationList> copy(CopyAnimations copyAnimations = CopyAnimations::Yes) const { return adoptRef(*new AnimationList(*this, copyAnimations)); } It seems weird that you could have copy(CopyAnimations::No) - until you realise you're talking about the members of the list. So maybe it should be called CopyAnimationMembers? Or flip it and call it Clone? I don't know. It's just a bit confusing. (In reply to Dean Jackson from comment #3) > Comment on attachment 445868 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=445868&action=review > > > Source/WebCore/platform/animation/AnimationList.h:39 > > + Ref<AnimationList> copy(CopyAnimations copyAnimations = CopyAnimations::Yes) const { return adoptRef(*new AnimationList(*this, copyAnimations)); } > > It seems weird that you could have copy(CopyAnimations::No) - until you > realise you're talking about the members of the list. So maybe it should be > called CopyAnimationMembers? Or flip it and call it Clone? I don't know. > It's just a bit confusing. I'll go with enum class `CopyBehavior { Clone, Reference }`. Comment on attachment 445868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445868&action=review >>> Source/WebCore/platform/animation/AnimationList.h:39 >>> + Ref<AnimationList> copy(CopyAnimations copyAnimations = CopyAnimations::Yes) const { return adoptRef(*new AnimationList(*this, copyAnimations)); } >> >> It seems weird that you could have copy(CopyAnimations::No) - until you realise you're talking about the members of the list. So maybe it should be called CopyAnimationMembers? Or flip it and call it Clone? I don't know. It's just a bit confusing. > > I'll go with enum class `CopyBehavior { Clone, Reference }`. I think it might be better to just have two separate named functions, maybe copy and shallowCopy? We can still use just a single constructor with an argument, but that would be private. (In reply to Darin Adler from comment #5) > Comment on attachment 445868 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=445868&action=review > > >>> Source/WebCore/platform/animation/AnimationList.h:39 > >>> + Ref<AnimationList> copy(CopyAnimations copyAnimations = CopyAnimations::Yes) const { return adoptRef(*new AnimationList(*this, copyAnimations)); } > >> > >> It seems weird that you could have copy(CopyAnimations::No) - until you realise you're talking about the members of the list. So maybe it should be called CopyAnimationMembers? Or flip it and call it Clone? I don't know. It's just a bit confusing. > > > > I'll go with enum class `CopyBehavior { Clone, Reference }`. > > I think it might be better to just have two separate named functions, maybe > copy and shallowCopy? We can still use just a single constructor with an > argument, but that would be private. Event better. Created attachment 445969 [details]
Patch for landing
Committed r286532 (244864@main): <https://commits.webkit.org/244864@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 445969 [details]. *** Bug 233672 has been marked as a duplicate of this bug. *** |