During the review of bug 232086, Darin made some comments about getKeyframes() and made some suggestions some improvements. We should look into that.
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.
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().
Created attachment 449914 [details] Patch
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.
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.
(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.
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.
> > > 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())
Committed r288560 (?): <https://commits.webkit.org/r288560>
<rdar://problem/88028485>
I'll make similar changes to setKeyframes() in bug 235593.
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?
(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