Summary: | [Web Animations] Add basic timing and target properties | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||||||||||||||||||
Component: | Animations | Assignee: | Antoine Quint <graouts> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, dino, simon.fraser, webkit-bug-importer | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=122912 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Antoine Quint
2017-10-23 11:04:26 PDT
Created attachment 324567 [details]
Patch
Comment on attachment 324567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324567&action=review > Source/WebCore/animation/AnimationEffectTiming.h:45 > + double m_duration; Can we use Seconds here? > Source/WebCore/animation/WebAnimation.h:55 > + std::optional<double> m_startTime; MonotonicTime Comment on attachment 324567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324567&action=review > Source/WebCore/animation/AnimationEffect.cpp:41 > +AnimationEffect::~AnimationEffect() > +{ > +} Will this eventually have code? If not, put it in the header. > Source/WebCore/animation/WebAnimation.h:55 > + std::optional<double> m_startTime; As smfr probably said, use Seconds or something. > Source/WebCore/animation/WebAnimation.idl:35 > ] interface WebAnimation { > + attribute AnimationEffect? effect; > readonly attribute AnimationTimeline? timeline; > + attribute double? startTime; > }; Is this weird spacing standard? > Source/WebCore/bindings/js/JSAnimationEffectCustom.cpp:42 > +JSValue toJSNewlyCreated(ExecState*, JSDOMGlobalObject* globalObject, Ref<AnimationEffect>&& value) > +{ > + if (value->isKeyframeEffect()) > + return createWrapper<KeyframeEffect>(globalObject, WTFMove(value)); > + return createWrapper<AnimationEffect>(globalObject, WTFMove(value)); > +} Wish this code was automatic. Paging Rabbi Weinig. > LayoutTests/ChangeLog:23 > + * webanimations/animation-effect-expected.txt: Added. > + * webanimations/animation-effect-timing-expected.txt: Added. > + * webanimations/animation-effect-timing.html: Added. > + * webanimations/animation-effect.html: Added. > + * webanimations/animation-interface-effect-property-expected.txt: Added. > + * webanimations/animation-interface-effect-property.html: Added. > + * webanimations/animation-interface-start-time-property-expected.txt: Added. > + * webanimations/animation-interface-start-time-property.html: Added. > + * webanimations/keyframe-effect-expected.txt: Added. > + * webanimations/keyframe-effect-interface-timing-duration-expected.txt: Added. > + * webanimations/keyframe-effect-interface-timing-duration.html: Added. > + * webanimations/keyframe-effect.html: Added. > + All these tests should be written for Web Platform Tests. Put them in http/wpt/ for now, with the goal of submitting them to WPT. Use the WPT API for testing. Comment on attachment 324567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324567&action=review > Source/WebCore/animation/WebAnimation.h:49 > + void setStartTime(std::optional<double> startTime) { m_startTime = startTime; } I'm sure you're changing this, but no need for the optional in the parameter. If you're setting the value, it clearly exists. Created attachment 324588 [details]
Patch
Comment on attachment 324588 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324588&action=review > Source/WebCore/ChangeLog:18 > + Tests: webanimations/animation-effect-timing.html > + webanimations/animation-effect.html > + webanimations/animation-interface-effect-property.html > + webanimations/animation-interface-start-time-property.html > + webanimations/keyframe-effect-interface-timing-duration.html > + webanimations/keyframe-effect.html Please file a bug to move these to WPT. > Source/WebCore/animation/AnimationEffect.h:44 > + enum ClassType { Use enum class. Then you'll probably need AnimationEffect::Foo elsewhere. > Source/WebCore/animation/WebAnimation.h:50 > + std::optional<double> bindingsStartTime() const; > + void setBindingsStartTime(std::optional<double>); > + std::optional<MonotonicTime> startTime() const { return m_startTime; } > + void setStartTime(MonotonicTime& startTime) { m_startTime = startTime; } You're fixing my bad advice here. Created attachment 324590 [details]
Patch
Created attachment 324591 [details]
Patch
Created attachment 324595 [details]
Patch
Created attachment 324596 [details]
Patch
Created attachment 324644 [details]
Patch
Created attachment 324645 [details]
Patch
Created attachment 324647 [details]
Patch
Created attachment 324648 [details]
Patch
Created attachment 324656 [details]
Patch for landing
Comment on attachment 324656 [details] Patch for landing Clearing flags on attachment: 324656 Committed r223883: <https://trac.webkit.org/changeset/223883> All reviewed patches have been landed. Closing bug. |