Bug 179707

<rdar://problem/34932456>

Created attachment 326954 [details] Patch

Comment on attachment 326954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326954&action=review > Source/WebCore/animation/KeyframeEffect.cpp:55 > +ExceptionOr<void> KeyframeEffect::setKeyframes(ExecState& state, Strong<JSObject>&& keyframes) It seems really wrong for something down here in animation/ to be doing stuff with ExecState. Why isn't that limited to the bindings layer?

(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 326954 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=326954&action=review > > > Source/WebCore/animation/KeyframeEffect.cpp:55 > > +ExceptionOr<void> KeyframeEffect::setKeyframes(ExecState& state, Strong<JSObject>&& keyframes) > > It seems really wrong for something down here in animation/ to be doing > stuff with ExecState. Why isn't that limited to the bindings layer? Alas, we cannot currently express this in terms of IDL since the object can contain mostly arbitrary properties as dictionary keys. So we need to perform some ad-hoc parsing.

Created attachment 326955 [details] Patch

Created attachment 326956 [details] Patch

Created attachment 326958 [details] Patch

I think doing this processing in KeyframeEffect.cpp is fine for now. I think it is really unfortunate that the spec, https://w3c.github.io/web-animations/#process-a-keyframes-argument, resorts to custom bindings here, and would much prefer that this was expressible in WebIDL, but, alas it is not.

Comment on attachment 326958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326958&action=review > Source/WebCore/animation/KeyframeEffect.cpp:55 > +ExceptionOr<void> KeyframeEffect::setKeyframes(ExecState& state, Strong<JSObject>&& keyframes) I would extract this into a "processKeyframes" function and annotate it with the spec link, https://w3c.github.io/web-animations/#process-a-keyframes-argument (assuming that is the right link).

(In reply to Sam Weinig from comment #9) > Comment on attachment 326958 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=326958&action=review > > > Source/WebCore/animation/KeyframeEffect.cpp:55 > > +ExceptionOr<void> KeyframeEffect::setKeyframes(ExecState& state, Strong<JSObject>&& keyframes) > > I would extract this into a "processKeyframes" function and annotate it with > the spec link, > https://w3c.github.io/web-animations/#process-a-keyframes-argument (assuming > that is the right link). Can do, and it is the right link.

Created attachment 326988 [details] Patch

Created attachment 326993 [details] Patch

Comment on attachment 326993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326993&action=review > Source/WebCore/animation/KeyframeEffect.cpp:114 > + Vector<CSSPropertyID> properties; > + for (unsigned k = 0; k < numberOfCSSProperties; ++k) { > + properties.append(styleProperties->propertyAt(k).id()); > + styleResolver.applyPropertyToStyle(styleProperties->propertyAt(k).id(), styleProperties->propertyAt(k).value(), WTFMove(renderStyle)); > + renderStyle = styleResolver.state().takeStyle(); > + } We should make StyleResolver interface saner for this sort of thing at some point but this is fine for now.

Comment on attachment 326993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326993&action=review > Source/WebCore/animation/AnimationTimeline.h:73 > + HashMap<RefPtr<Element>, Vector<RefPtr<WebAnimation>>> elementToAnimationsMap() const { return m_elementToAnimationsMap; } Should be returning &. > Source/WebCore/animation/KeyframeEffect.cpp:46 > + auto result = adoptRef(*new KeyframeEffect(target)); > + > + auto setKeyframesResult = result->setKeyframes(state, WTFMove(keyframes)); > + if (setKeyframesResult.hasException()) > + return setKeyframesResult.releaseException(); > + > + return WTFMove(result); I'd rename result to keyframeEffect, but whatevs. > Source/WebCore/style/StyleTreeResolver.cpp:283 > + std::unique_ptr<RenderStyle> animatedStyle = RenderStyle::clonePtr(*newStyle); Could use auto here. > LayoutTests/http/wpt/web-animations/timing-model/animations/set-the-animation-start-time-expected.txt:3 > -FAIL Setting the start time of an animation without an active timeline assert_equals: Start time remains null after setting current time expected (object) null but got (number) -999.896 > -FAIL Setting an unresolved start time an animation without an active timeline does not clear the current time assert_equals: Start time remains null after setting current time expected (object) null but got (number) -999.896 > +FAIL Setting the start time of an animation without an active timeline assert_equals: Start time remains null after setting current time expected (object) null but got (number) -999.7033 > +FAIL Setting an unresolved start time an animation without an active timeline does not clear the current time assert_equals: Start time remains null after setting current time expected (object) null but got (number) -999.7033 Why has this changed?

Comment on attachment 326993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326993&action=review >> Source/WebCore/animation/AnimationTimeline.h:73 >> + HashMap<RefPtr<Element>, Vector<RefPtr<WebAnimation>>> elementToAnimationsMap() const { return m_elementToAnimationsMap; } > > Should be returning &. A const & > Source/WebCore/animation/KeyframeEffect.cpp:40 > + auto result = adoptRef(*new KeyframeEffect(target)); We never use bare new. Should be KeyframeEffect::create(). >> Source/WebCore/animation/KeyframeEffect.cpp:46 >> + return WTFMove(result); > > I'd rename result to keyframeEffect, but whatevs. +1

Committed r224897: <https://trac.webkit.org/changeset/224897>

(In reply to Simon Fraser (smfr) from comment #15) > Comment on attachment 326993 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=326993&action=review > > >> Source/WebCore/animation/AnimationTimeline.h:73 > >> + HashMap<RefPtr<Element>, Vector<RefPtr<WebAnimation>>> elementToAnimationsMap() const { return m_elementToAnimationsMap; } > > > > Should be returning &. > > A const & This was fixed in the commit. > > Source/WebCore/animation/KeyframeEffect.cpp:40 > > + auto result = adoptRef(*new KeyframeEffect(target)); > > We never use bare new. Should be KeyframeEffect::create(). This is the body of ::create(). > >> Source/WebCore/animation/KeyframeEffect.cpp:46 > >> + return WTFMove(result); > > > > I'd rename result to keyframeEffect, but whatevs. This was not addressed, alas. I'll make a note to fix this in a future patch.

Comment on attachment 326993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326993&action=review >>>> Source/WebCore/animation/KeyframeEffect.cpp:46 >>>> + return WTFMove(result); >>> >>> I'd rename result to keyframeEffect, but whatevs. >> >> +1 > > This was not addressed, alas. I'll make a note to fix this in a future patch. Is this WTFMove() necessary? > Source/WebCore/animation/KeyframeEffect.cpp:74 > + Vector<Keyframe> newKeyframes { }; No need for the braces. > Source/WebCore/animation/KeyframeEffect.cpp:82 > + const auto* array = jsCast<const JSArray*>(keyframes.get()); No need to repeat the const on the left hand side. > Source/WebCore/animation/KeyframeEffect.cpp:87 > + for (unsigned i = 0; i < length; ++i) { What is the data type of length. Is it unsigned? Otherwise, we should change to use the same data type as length. > Source/WebCore/animation/KeyframeEffect.cpp:97 > + for (unsigned j = 0; j < numberOfProperties; ++j) { Although this code is unlikely to cause problems in practice, the data type of the index, j, differs from the data type of numberOfProperties. We should use the same data type for both, size_t. > Source/WebCore/animation/KeyframeEffect.cpp:110 > + for (unsigned k = 0; k < numberOfCSSProperties; ++k) { We are adding every CSS property in this loop. So, we should reserve initial capacity in the Vector and use uncheckedAppend() to avoid reallocating the underlying storage of the Vector when appending items to it. > Source/WebCore/animation/KeyframeEffect.cpp:129 > + // FIXME: Assume animations only apply in the range [0, duration[ duration[? > Source/WebCore/dom/Element.cpp:3709 > + return Vector<RefPtr<WebAnimation>> { }; This should be “return { };”

(In reply to Daniel Bates from comment #18) > Comment on attachment 326993 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=326993&action=review > > >>>> Source/WebCore/animation/KeyframeEffect.cpp:46 > >>>> + return WTFMove(result); > >>> > >>> I'd rename result to keyframeEffect, but whatevs. > >> > >> +1 > > > > This was not addressed, alas. I'll make a note to fix this in a future patch. > > Is this WTFMove() necessary? Yes, since the return type is ExceptionOr<Ref<KeyframeEffect>>. > > Source/WebCore/animation/KeyframeEffect.cpp:74 > > + Vector<Keyframe> newKeyframes { }; > > No need for the braces. Will fix in a cleanup patch. > > Source/WebCore/animation/KeyframeEffect.cpp:82 > > + const auto* array = jsCast<const JSArray*>(keyframes.get()); > > No need to repeat the const on the left hand side. Will fix in a cleanup patch. > > Source/WebCore/animation/KeyframeEffect.cpp:87 > > + for (unsigned i = 0; i < length; ++i) { > > What is the data type of length. Is it unsigned? Otherwise, we should change > to use the same data type as length. It is unsigned. > > Source/WebCore/animation/KeyframeEffect.cpp:97 > > + for (unsigned j = 0; j < numberOfProperties; ++j) { > > Although this code is unlikely to cause problems in practice, the data type > of the index, j, differs from the data type of numberOfProperties. We should > use the same data type for both, size_t. Will fix in a cleanup patch. > > Source/WebCore/animation/KeyframeEffect.cpp:110 > > + for (unsigned k = 0; k < numberOfCSSProperties; ++k) { > > We are adding every CSS property in this loop. So, we should reserve initial > capacity in the Vector and use uncheckedAppend() to avoid reallocating the > underlying storage of the Vector when appending items to it. Great! Will fix in a cleanup patch. > > Source/WebCore/animation/KeyframeEffect.cpp:129 > > + // FIXME: Assume animations only apply in the range [0, duration[ > > duration[? This means the value of duration is excluded in the range, see https://en.wikipedia.org/wiki/Interval_(mathematics). > > Source/WebCore/dom/Element.cpp:3709 > > + return Vector<RefPtr<WebAnimation>> { }; > > This should be “return { };” Will fix in a cleanup patch.

Clean-up is performed in https://bugs.webkit.org/show_bug.cgi?id=179777.