Bug 235504 – Refactor KeyframeEffect::getKeyframes()

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

[Antoine Quint]

During the review of bug 232086, Darin made some comments about getKeyframes() and made some suggestions some improvements. We should look into that.

[Antoine Quint]

We could probably solve this by using an intermediate struct that is the output of getKeyframes() and have some other function translate this to the required JS object. That struct would probably keep a map of CSSPropertyID to values which we'd serialize in the bindings code.

[Antoine Quint]

Plan here is to:

1. mark getKeyframes() as [Custom] in the idl
2. change the BaseComputedKeyframe struct to a ComputedKeyframe struct, add a map with CSSPropertyID as a key and String as values to store all the keyframe CSS properties
3. make getKeyframes() return a Vector<ComputedKeyframe>
4. add code in JSKeyframeEffectCustom.cpp to convert that returned Vector<ComputedKeyframe> to a Vector<JSC::Strong<JSC::JSObject>>, the code for which mostly exists in the current implementation of KeyframeEffect::getKeyframes().

[Antoine Quint]

Created attachment 449914 [details]
Patch

[Chris Dumez]

Comment on attachment 449914 [details]
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.

[Darin Adler]

Comment on attachment 449914 [details]
Patch

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

>> 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());
> ```

Even better, use Vector:.map to create the new vector.

[Antoine Quint]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 449914 [details]
> 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>

Is this a new style we're adopting throughout WebKit? Does it have any benefit? I've seen this used before, but wasn't sure whether it had any practical benefit.

> > 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();

This is awesome, didn't know it existed. Will adopt this.

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

Will fix.

> > 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);

Yes, will do this.

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

Indeed, will fix.

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

Will fix.

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

This is a KeyframeList, and it cannot (yet?) be iterated.

> > 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() ?

Yes, will fix.

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

Will fix.

> > 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());
> ```

Darin's suggestion to use Vector::map(), which I didn't know we had, will be even better.

> > 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)

I didn't know this could be done either, this is so much better.

> > Source/WebCore/bindings/js/JSKeyframeEffectCustom.cpp:53
> > +        keyframeObjects.append({ lexicalGlobalObject.vm(), keyframeObject });
> 
> Could use uncheckedAppend() if you reserve capacity as suggested above.

Thanks so much for all of this, this will greatly improve this patch and my coding going forward.

[Darin Adler]

Comment on attachment 449914 [details]
Patch

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

>>> 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>
> 
> Is this a new style we're adopting throughout WebKit? Does it have any benefit? I've seen this used before, but wasn't sure whether it had any practical benefit.

The benefit is not having to qualify from the class’s scope. Vector<ComputedKeyframe> instead of Vector<KeyframeEffect::ComputedKeyframe>. In some cases it can get even more extreme.

[Antoine Quint]

> > > Source/WebCore/animation/KeyframeEffect.cpp:651
> > > +    for (size_t i = 0; i < computedKeyframeList.size(); ++i) {
> > 
> > Why for use:
> > for (auto& keyframe : computedKeyframeList)
> 
> This is a KeyframeList, and it cannot (yet?) be iterated.

But KeyframeList expose the keyframes via keyframes() so we can write this as:

for (auto& keyframe : computedKeyframeList.keyframes())

[Antoine Quint]

Committed r288560 (?): <https://commits.webkit.org/r288560>

[Radar WebKit Bug Importer]

<rdar://problem/88028485>

[Antoine Quint]

I'll make similar changes to setKeyframes() in bug 235593.

[Darin Adler]

Comment on attachment 449914 [details]
Patch

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

>>> 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();
> 
> This is awesome, didn't know it existed. Will adopt this.

Maybe because Chris added it less than two weeks ago?

[Chris Dumez]

(In reply to Darin Adler from comment #12)
> Comment on attachment 449914 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449914&action=review
> 
> >>> 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();
> > 
> > This is awesome, didn't know it existed. Will adopt this.
> 
> Maybe because Chris added it less than two weeks ago?

Hehe, yes, it is very new, which is why I am publicizing it :P