[webkit-reviews] review granted: [Bug 235504] Refactor KeyframeEffect::getKeyframes() : [Attachment 449914] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 235504: Refactor KeyframeEffect::getKeyframes()
https://bugs.webkit.org/show_bug.cgi?id=235504

Attachment 449914: Patch

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




--- Comment #4 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 449914
  --> https://bugs.webkit.org/attachment.cgi?id=449914
Patch

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

r=me with suggestions

> Source/WebCore/animation/KeyframeEffect.cpp:585
> +Vector<KeyframeEffect::ComputedKeyframe>
KeyframeEffect::getKeyframes(Document& document)

This can also be written as so (if you prefer):
auto KeyframeEffect::getKeyframes(Document& document) ->
Vector<ComputedKeyframe>

> Source/WebCore/animation/KeyframeEffect.cpp:589
> +    if (is<DeclarativeAnimation>(animation()))

To avoid calling animation() twice, you could write:
if (auto* declarativeAnimation =
dynamicDowncast<DeclarativeAnimation>(animation()))
    declarativeAnimation->flushPendingStyleChanges();

> Source/WebCore/animation/KeyframeEffect.cpp:598
> +	       computedKeyframes.append(computedKeyframe);

computedKeyframes.append(WTFMove(computedKeyframe));

> Source/WebCore/animation/KeyframeEffect.cpp:607
> +    auto computedStyleExtractor = ComputedStyleExtractor(target, false,
m_pseudoId);

I know this is pre-existing code but I personally think this would look nicer
(more concise):
ComputedStyleExtractor computedStyleExtractor(target, false, m_pseudoId);

> Source/WebCore/animation/KeyframeEffect.cpp:647
> +    HashSet<CSSPropertyID> zeroKeyframeProperties =
computedKeyframeList.properties();

could use auto

> Source/WebCore/animation/KeyframeEffect.cpp:648
> +    HashSet<CSSPropertyID> oneKeyframeProperties =
computedKeyframeList.properties();

ditto.

> Source/WebCore/animation/KeyframeEffect.cpp:651
> +    for (size_t i = 0; i < computedKeyframeList.size(); ++i) {

Why for use:
for (auto& keyframe : computedKeyframeList)

> Source/WebCore/animation/KeyframeEffect.cpp:662
> +    for (size_t i = 0; i < computedKeyframeList.size(); ++i) {

ditto.

> Source/WebCore/animation/KeyframeEffect.cpp:679
> +	       String styleString = "";

= emptyString() ?

> Source/WebCore/animation/KeyframeEffect.cpp:718
> +	   computedKeyframes.append(computedKeyframe);

computedKeyframes.append(WTFMove(computedKeyframe));

> Source/WebCore/bindings/js/JSKeyframeEffectCustom.cpp:46
> +    for (auto& computedKeyframe :
wrapped().getKeyframes(downcast<Document>(*context))) {

May be nice to store the result of wrapped().getKeyframes() in a local so we
can reserve capacity for keyframeObjects.
E.g.
```
Vector<Strong<JSObject>> keyframeObjects;
auto keyFrames = wrapped().getKeyframes(downcast<Document>(*context));
keyframeObjects.reserveInitialCapacity(keyFrames.size());
```

> Source/WebCore/bindings/js/JSKeyframeEffectCustom.cpp:48
> +	   for (auto it = computedKeyframe.styleStrings.begin(), end =
computedKeyframe.styleStrings.end(); it != end; ++it) {

Could use:
for (auto& [propertyID, propertyValue] : computedKeyframe.styleStrings)

> Source/WebCore/bindings/js/JSKeyframeEffectCustom.cpp:53
> +	   keyframeObjects.append({ lexicalGlobalObject.vm(), keyframeObject
});

Could use uncheckedAppend() if you reserve capacity as suggested above.