[webkit-reviews] review granted: [Bug 179941] [Web Animations] Adopt KeyframeList in KeyframeEffect : [Attachment 327450] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 179941: [Web Animations] Adopt KeyframeList in KeyframeEffect
https://bugs.webkit.org/show_bug.cgi?id=179941

Attachment 327450: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 327450
  --> https://bugs.webkit.org/attachment.cgi?id=327450
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?