Clean up KeyframeEffect

[Antoine Quint]

Clean up KeyframeEffect

[Antoine Quint]

Created attachment 327069 [details]
Patch

[Daniel Bates]

Comment on attachment 327069 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffect.cpp:111
> +        Vector<CSSPropertyID> properties(numberOfCSSProperties);
>          for (unsigned k = 0; k < numberOfCSSProperties; ++k) {
> -            properties.append(styleProperties->propertyAt(k).id());
> +            properties.uncheckedAppend(styleProperties->propertyAt(k).id());

This is not correct. This will allocate numberOfCSSProperties CSSPropertyID default constructed objects; => we are both allocating capacity and changing the size of the Vector. Then we are using unchecked append to append new elements outside the bounds of the Vector. If we allocate up-front then we should be modifying the existing elements in the Vector. That is, we should not using uncheckedAppend. Alternatively, we should use Vector::reserveInitialCapacity() and Vector::uncheckedAppend() to allocate the underlying buffer without object construction and then safely construct new objects in the first free position in the buffer (increasing the size of the Vector).

[Antoine Quint]

Created attachment 327072 [details]
Patch for landing

[Daniel Bates]

Comment on attachment 327072 [details]
Patch for landing

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

> Source/WebCore/animation/KeyframeEffect.cpp:111
> +            properties[k] = WTFMove(styleProperties->propertyAt(k).id());

This is bad programming practice. We should not be moving this value as we are using it below. In practice, this code will work because CSSPropertyID is a POD type and is always copied.

[Antoine Quint]

Created attachment 327074 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 327074 [details]
Patch for landing

Clearing flags on attachment: 327074

Committed r224934: <https://trac.webkit.org/changeset/224934>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35595728>