Bug 179941 – [Web Animations] Adopt KeyframeList in KeyframeEffect

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

[Antoine Quint]

KeyframeEffect currently models its keyframes by creating a new Keyframe struct and storing them in a Vector<Keyframe>. As it turns out, there already is a way to model keyframes in WebCore, it's KeyframeList. We should adopt this class, which will also make it possible to run hardware-composited animations using RenderBoxModelObject::startAnimation() in the future since this function expects a KeyframeList.

[Antoine Quint]

Created attachment 327450 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/35666924>

[Darin Adler]

Comment on attachment 327450 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffect.cpp:54
> +    , m_keyframes("")

empytString() is more efficient than writing ""

> Source/WebCore/animation/KeyframeEffect.cpp:78
> +    KeyframeList newKeyframes("keyframe-effect-" + createCanonicalUUIDString());
> +    newKeyframes.clear();

Seems annoying to have to construct this with two values, and then clear them out. Is there a more elegant construction we can come up with?

> Source/WebCore/animation/KeyframeEffect.cpp:127
> -    m_keyframes = WTFMove(newKeyframes);
> +    m_keyframes.swap(newKeyframes);

This seems unnecessary. Seems like KeyFrameList could and should easily support move assignment since AtomicString, Vector, and HashSet all support that.

> Source/WebCore/rendering/style/KeyframeList.h:81
> +    void swap(KeyframeList& other)
> +    {
> +        std::swap(m_animationName, other.m_animationName);
> +        std::swap(m_keyframes, other.m_keyframes);
> +        std::swap(m_properties, other.m_properties);
> +    }

Should just add the move assignment operator instead:

    KeyFrameList& operator=(KeyFrameList&&) = default;

But also, why doesn’t that get generated automatically for us?

[Antoine Quint]

(In reply to Darin Adler from comment #3)
> Comment on attachment 327450 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327450&action=review
> 
> > Source/WebCore/animation/KeyframeEffect.cpp:54
> > +    , m_keyframes("")
> 
> empytString() is more efficient than writing ""

Will fix.

> > Source/WebCore/animation/KeyframeEffect.cpp:78
> > +    KeyframeList newKeyframes("keyframe-effect-" + createCanonicalUUIDString());
> > +    newKeyframes.clear();
> 
> Seems annoying to have to construct this with two values, and then clear
> them out. Is there a more elegant construction we can come up with?

Agreed. I think we might be able to remove the addition of the two values in the KeyframeList constructor actually. Running WKTR through the animations and transitions directories reveals no failures, so I'll upload a new patch with that removal to see what the bots say, but I doubt any of our code relies those initial values.

> > Source/WebCore/animation/KeyframeEffect.cpp:127
> > -    m_keyframes = WTFMove(newKeyframes);
> > +    m_keyframes.swap(newKeyframes);
> 
> This seems unnecessary. Seems like KeyFrameList could and should easily
> support move assignment since AtomicString, Vector, and HashSet all support
> that.

Will add the move assignment operator you've suggested and revert to the previous move assignment.

> > Source/WebCore/rendering/style/KeyframeList.h:81
> > +    void swap(KeyframeList& other)
> > +    {
> > +        std::swap(m_animationName, other.m_animationName);
> > +        std::swap(m_keyframes, other.m_keyframes);
> > +        std::swap(m_properties, other.m_properties);
> > +    }
> 
> Should just add the move assignment operator instead:
> 
>     KeyFrameList& operator=(KeyFrameList&&) = default;

Easy. Thanks for the tip.

> But also, why doesn’t that get generated automatically for us?

That goes well beyond my current understand of how WebCore works.

[Antoine Quint]

Created attachment 327458 [details]
Patch

[Antoine Quint]

Committed r225099: <https://trac.webkit.org/changeset/225099>