[webkit-reviews] review granted: [Bug 233869] [Web Animations] Add a way to run scripted animations : [Attachment 446016] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 233869: [Web Animations] Add a way to run scripted animations
https://bugs.webkit.org/show_bug.cgi?id=233869

Attachment 446016: Patch

https://bugs.webkit.org/attachment.cgi?id=446016&action=review




--- Comment #4 from Dean Jackson <dino at apple.com> ---
Comment on attachment 446016
  --> https://bugs.webkit.org/attachment.cgi?id=446016
Patch

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

> LayoutTests/ChangeLog:19
> +2021-12-06  Antoine Quint  <graouts at webkit.org>
> +
> +	   [Web Animations] Add a way to run scripted animations
> +	   https://bugs.webkit.org/show_bug.cgi?id=233869
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   * webanimations/custom-effect/custom-effect-expected.txt: Added.
> +	   * webanimations/custom-effect/custom-effect.html: Added.
> +	   *
webanimations/custom-effect/document-timeline-animate-expected.txt: Added.
> +	   * webanimations/custom-effect/document-timeline-animate.html: Added.
> +
> +2021-12-06  Antoine Quint  <graouts at webkit.org>
> +
> +	   [Web Animations] Add a way to run scripted animations
> +	   https://bugs.webkit.org/show_bug.cgi?id=233869
> +	   rdar://85983542
> +
> +	   Reviewed by NOBODY (OOPS!).

oops

> LayoutTests/webanimations/custom-effect/custom-effect.html:30
> +    assert_not_equals(customEffectProgress, 1, "Callback is first fired with
progress = 0.");

Did you want to change this message?

> Source/WTF/ChangeLog:12
> +2021-12-06  Antoine Quint  <graouts at webkit.org>
> +
> +	   [Web Animations] Add a way to run scripted animations
> +	   https://bugs.webkit.org/show_bug.cgi?id=233869
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   * Scripts/Preferences/WebPreferencesExperimental.yaml:
> +
> +2021-12-06  Antoine Quint  <graouts at webkit.org>
> +
> +	   [Web Animations] Add a way to run scripted animations

oops

> Source/WebCore/ChangeLog:39
> +2021-12-06  Antoine Quint  <graouts at webkit.org>
> +
> +	   [Web Animations] Add a way to run scripted animations
> +	   https://bugs.webkit.org/show_bug.cgi?id=233869
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Tests: webanimations/custom-effect/custom-effect.html
> +		  webanimations/custom-effect/document-timeline-animate.html
> +
> +	   * CMakeLists.txt:
> +	   * DerivedSources-input.xcfilelist:
> +	   * DerivedSources-output.xcfilelist:
> +	   * DerivedSources.make:
> +	   * Headers.cmake:
> +	   * Sources.txt:
> +	   * WebCore.xcodeproj/project.pbxproj:
> +	   * animation/AnimationEffect.h:
> +	   (WebCore::AnimationEffect::isCustomEffect const):
> +	   * animation/CustomAnimationOptions.h: Copied from
Source/WebCore/animation/DocumentTimeline.idl.
> +	   * animation/CustomAnimationOptions.idl: Copied from
Source/WebCore/animation/DocumentTimeline.idl.
> +	   * animation/CustomEffect.cpp: Added.
> +	   (WebCore::CustomEffect::create):
> +	   (WebCore::CustomEffect::CustomEffect):
> +	   (WebCore::CustomEffect::animationDidTick):
> +	   * animation/CustomEffect.h: Copied from
Source/WebCore/animation/DocumentTimeline.idl.
> +	   (WebCore::CustomEffect::~CustomEffect):
> +	   * animation/CustomEffect.idl: Copied from
Source/WebCore/animation/DocumentTimeline.idl.
> +	   * animation/CustomEffectCallback.h: Copied from
Source/WebCore/animation/DocumentTimeline.idl.
> +	   * animation/CustomEffectCallback.idl: Copied from
Source/WebCore/animation/DocumentTimeline.idl.
> +	   * animation/DocumentTimeline.cpp:
> +	   (WebCore::DocumentTimeline::animate):
> +	   * animation/DocumentTimeline.h:
> +	   * animation/DocumentTimeline.idl:
> +	   * bindings/js/WebCoreBuiltinNames.h:
> +
> +2021-12-06  Antoine Quint  <graouts at webkit.org>
> +
> +	   [Web Animations] Add a way to run scripted animations

oops

> Source/WebCore/animation/CustomEffect.cpp:43
> +	       std::variant<double, String> duration =
std::get<double>(optionsValue);

We don't have a type for duration?

> Source/WebCore/animation/DocumentTimeline.cpp:516
> +	   std::variant<double, EffectTiming> customEffectOptionsVariant;
> +	   if (std::holds_alternative<double>(*options))
> +	       customEffectOptionsVariant = std::get<double>(*options);
> +	   else {
> +	       auto customEffectOptions =
std::get<CustomAnimationOptions>(*options);
> +	       id = customEffectOptions.id;
> +	       customEffectOptionsVariant = WTFMove(customEffectOptions);
> +	   }
> +	   customEffectOptions = customEffectOptionsVariant;

This might be nicer as a function that returns the variant directly into
customEffectOptions.