Bug 179708

<rdar://problem/34953942>

Created attachment 330032 [details] Patch

Created attachment 330043 [details] Patch

Comment on attachment 330032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330032&action=review It stresses me out that so much code is landing without many tests. > Source/WebCore/animation/KeyframeEffect.cpp:101 > +static inline ExceptionOr<void> processIterableKeyframes(ExecState& state, Strong<JSObject>&& keyframesInput, JSValue method, Vector<KeyframeEffect::ProcessedKeyframe>& processedKeyframes) It might be nicer to have this function return an ExceptionOr<Vector<KeyframeEffect::ProcessedKeyframe>>, to avoid having an out parameter. > Source/WebCore/animation/KeyframeEffect.cpp:132 > + auto ownPropertyName = ownPropertyNames[j]; > + auto ownPropertyNameAsString = ownPropertyName.string(); > + auto ownPropertyRawValue = keyframe->get(&state, ownPropertyName); > + if (ownPropertyName == "easing") > + easing = convert<IDLDOMString>(state, ownPropertyRawValue); > + else if (ownPropertyNameAsString == "offset") > + offset = convert<IDLNullable<IDLDouble>>(state, ownPropertyRawValue); > + else if (ownPropertyName == "composite") What is ownPropertyNameAsString for? You already compare ownPropertyName == "easing" and "composite". Why is "offset" different?

Comment on attachment 330043 [details] Patch copying review from previous patch

I'm sure Darin or someone will find lots of nits.

(In reply to Dean Jackson from comment #4) > Comment on attachment 330032 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330032&action=review > > It stresses me out that so much code is landing without many tests. There are many tests, they're simply all using Element.animate(), which is landing next. > > Source/WebCore/animation/KeyframeEffect.cpp:101 > > +static inline ExceptionOr<void> processIterableKeyframes(ExecState& state, Strong<JSObject>&& keyframesInput, JSValue method, Vector<KeyframeEffect::ProcessedKeyframe>& processedKeyframes) > > It might be nicer to have this function return an > ExceptionOr<Vector<KeyframeEffect::ProcessedKeyframe>>, to avoid having an > out parameter. Yes, I can do that. It was easier to follow the spec text by passing around the out parameter though. > > Source/WebCore/animation/KeyframeEffect.cpp:132 > > + auto ownPropertyName = ownPropertyNames[j]; > > + auto ownPropertyNameAsString = ownPropertyName.string(); > > + auto ownPropertyRawValue = keyframe->get(&state, ownPropertyName); > > + if (ownPropertyName == "easing") > > + easing = convert<IDLDOMString>(state, ownPropertyRawValue); > > + else if (ownPropertyNameAsString == "offset") > > + offset = convert<IDLNullable<IDLDouble>>(state, ownPropertyRawValue); > > + else if (ownPropertyName == "composite") > > What is ownPropertyNameAsString for? You already compare ownPropertyName == > "easing" and "composite". Why is "offset" different? I started out by only implementing offset, and then added easing and composite. But I see that JSC::Identifier, which is the type of ownPropertyName, supports string comparisons, so I'll remove ownPropertyName.

Committed r226234: <https://trac.webkit.org/changeset/226234>