Bug 233819 – CSS animation sorting may crash due to AnimationList copy upon CSS Animation removal

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

CSS animation sorting may crash due to AnimationList copy upon CSS Animation removal

[Antoine Quint]

Created attachment 445868 [details]
Patch

[Antoine Quint]

<rdar://problem/85596065>

[Dean Jackson]

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.

[Antoine Quint]

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

[Darin Adler]

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.

[Antoine Quint]

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

[Antoine Quint]

Created attachment 445969 [details]
Patch for landing

[EWS]

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

[Antoine Quint]

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