De-inline KeyframeEffect::Create() from animate()

Move metrics from KeyframeEffect::Create() body to IDL [Measure]
and merge the two keys into a new one with the proper name.

This in preparation for changing it in the next CL.
Split from crrev.com/c/1921343

Bug: 981894
Change-Id: I7f42597e507e1c4354804a907474aeb0be6bc840
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1966501
Commit-Queue: George Steel <gtsteel@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726211}

third_party/blink/public/mojom/web_feature/web_feature.mojom

@@ -176,8 +176,6 @@ enum WebFeature {
   kDocumentXPathCreateExpression = 295,
   kDocumentXPathCreateNSResolver = 296,
   kDocumentXPathEvaluate = 297,
-  kAnimationConstructorKeyframeListEffectObjectTiming = 300,
-  kAnimationConstructorKeyframeListEffectNoTiming = 302,
   kPrefixedCancelAnimationFrame = 304,
   kNamedNodeMapGetNamedItem = 306,
   kNamedNodeMapSetNamedItem = 307,
@@ -2494,6 +2492,7 @@ enum WebFeature {
   kWebkitBoxPackJustifyDoesSomething = 3123,
   kWebkitBoxPackCenterDoesSomething = 3124,
   kWebkitBoxPackEndDoesSomething = 3125,
+  kV8KeyframeEffect_Constructor = 3126,
 
   // Add new features immediately above this line. Don't change assigned
   // numbers of any item, and don't reuse removed slots.

third_party/blink/renderer/core/animation/animatable.cc

@@ -5,6 +5,7 @@
 #include "third_party/blink/renderer/core/animation/animatable.h"
 
 #include "third_party/blink/renderer/bindings/core/v8/unrestricted_double_or_keyframe_animation_options.h"
+#include "third_party/blink/renderer/bindings/core/v8/unrestricted_double_or_keyframe_effect_options.h"
 #include "third_party/blink/renderer/core/animation/animation.h"
 #include "third_party/blink/renderer/core/animation/document_animations.h"
 #include "third_party/blink/renderer/core/animation/document_timeline.h"
@@ -41,6 +42,17 @@ void ReportFeaturePolicyViolationsIfNecessary(
   }
 }
 
+UnrestrictedDoubleOrKeyframeEffectOptions CoerceEffectOptions(
+    UnrestrictedDoubleOrKeyframeAnimationOptions options) {
+  if (options.IsKeyframeAnimationOptions()) {
+    return UnrestrictedDoubleOrKeyframeEffectOptions::FromKeyframeEffectOptions(
+        options.GetAsKeyframeAnimationOptions());
+  } else {
+    return UnrestrictedDoubleOrKeyframeEffectOptions::FromUnrestrictedDouble(
+        options.GetAsUnrestrictedDouble());
+  }
+}
+
 }  // namespace
 
 Animation* Animatable::animate(
@@ -48,25 +60,16 @@ Animation* Animatable::animate(
     const ScriptValue& keyframes,
     const UnrestrictedDoubleOrKeyframeAnimationOptions& options,
     ExceptionState& exception_state) {
-  EffectModel::CompositeOperation composite = EffectModel::kCompositeReplace;
-  if (options.IsKeyframeAnimationOptions()) {
-    composite = EffectModel::StringToCompositeOperation(
-                    options.GetAsKeyframeAnimationOptions()->composite())
-                    .value();
-  }
-
   Element* element = GetAnimationTarget();
-  KeyframeEffectModelBase* effect = EffectInput::Convert(
-      element, keyframes, composite, script_state, exception_state);
+  KeyframeEffect* effect =
+      KeyframeEffect::Create(script_state, element, keyframes,
+                             CoerceEffectOptions(options), exception_state);
   if (exception_state.HadException())
     return nullptr;
 
-  Timing timing =
-      TimingInput::Convert(options, &element->GetDocument(), exception_state);
-  if (exception_state.HadException())
-    return nullptr;
-
-  Animation* animation = animateInternal(*element, effect, timing);
+  ReportFeaturePolicyViolationsIfNecessary(element->GetDocument(),
+                                           *effect->Model());
+  Animation* animation = element->GetDocument().Timeline().Play(effect);
   if (options.IsKeyframeAnimationOptions())
     animation->setId(options.GetAsKeyframeAnimationOptions()->id());
   return animation;
@@ -76,12 +79,14 @@ Animation* Animatable::animate(ScriptState* script_state,
                                const ScriptValue& keyframes,
                                ExceptionState& exception_state) {
   Element* element = GetAnimationTarget();
-  KeyframeEffectModelBase* effect =
-      EffectInput::Convert(element, keyframes, EffectModel::kCompositeReplace,
-                           script_state, exception_state);
+  KeyframeEffect* effect =
+      KeyframeEffect::Create(script_state, element, keyframes, exception_state);
   if (exception_state.HadException())
     return nullptr;
-  return animateInternal(*element, effect, Timing());
+
+  ReportFeaturePolicyViolationsIfNecessary(element->GetDocument(),
+                                           *effect->Model());
+  return element->GetDocument().Timeline().Play(effect);
 }
 
 HeapVector<Member<Animation>> Animatable::getAnimations(
@@ -113,13 +118,4 @@ HeapVector<Member<Animation>> Animatable::getAnimations(
   return animations;
 }
 
-Animation* Animatable::animateInternal(Element& element,
-                                       KeyframeEffectModelBase* effect,
-                                       const Timing& timing) {
-  ReportFeaturePolicyViolationsIfNecessary(element.GetDocument(), *effect);
-  auto* keyframe_effect =
-      MakeGarbageCollected<KeyframeEffect>(&element, effect, timing);
-  return element.GetDocument().Timeline().Play(keyframe_effect);
-}
-
 }  // namespace blink

third_party/blink/renderer/core/animation/animatable.h

@@ -41,11 +41,9 @@ namespace blink {
 class Animation;
 class ExceptionState;
 class Element;
-class KeyframeEffectModelBase;
 class ScriptState;
 class ScriptValue;
 class UnrestrictedDoubleOrKeyframeAnimationOptions;
-struct Timing;
 
 // https://drafts.csswg.org/web-animations-1/#the-animatable-interface-mixin
 class CORE_EXPORT Animatable {
@@ -63,13 +61,6 @@ class CORE_EXPORT Animatable {
 
   HeapVector<Member<Animation>> getAnimations(
       GetAnimationsOptions* options = nullptr);
-
- private:
-  FRIEND_TEST_ALL_PREFIXES(AnimationSimTest, CustomPropertyBaseComputedStyle);
-
-  static Animation* animateInternal(Element&,
-                                    KeyframeEffectModelBase*,
-                                    const Timing&);
 };
 
 }  // namespace blink

third_party/blink/renderer/core/animation/animation_sim_test.cc

@@ -3,7 +3,8 @@
 // found in the LICENSE file.
 
 #include "third_party/blink/public/web/web_script_source.h"
-#include "third_party/blink/renderer/core/animation/animatable.h"
+#include "third_party/blink/renderer/core/animation/document_timeline.h"
+#include "third_party/blink/renderer/core/animation/keyframe_effect.h"
 #include "third_party/blink/renderer/core/animation/keyframe_effect_model.h"
 #include "third_party/blink/renderer/core/animation/string_keyframe.h"
 #include "third_party/blink/renderer/core/css/css_style_sheet.h"
@@ -68,9 +69,11 @@ TEST_F(AnimationSimTest, CustomPropertyBaseComputedStyle) {
   keyframes.push_back(keyframe);
   Timing timing;
   timing.iteration_duration = AnimationTimeDelta::FromSecondsD(1);
-  Animatable::animateInternal(
-      *target, MakeGarbageCollected<StringKeyframeEffectModel>(keyframes),
+
+  auto* keyframe_effect = MakeGarbageCollected<KeyframeEffect>(
+      target, MakeGarbageCollected<StringKeyframeEffectModel>(keyframes),
       timing);
+  target->GetDocument().Timeline().Play(keyframe_effect);
 
   // This sets the baseComputedStyle on the animation exit frame.
   Compositor().BeginFrame(1);
@@ -90,9 +93,11 @@ TEST_F(AnimationSimTest, CustomPropertyBaseComputedStyle) {
   keyframes.push_back(std::move(keyframe));
   timing = Timing();
   timing.iteration_duration = AnimationTimeDelta::FromSecondsD(1);
-  Animatable::animateInternal(
-      *target, MakeGarbageCollected<StringKeyframeEffectModel>(keyframes),
+
+  keyframe_effect = MakeGarbageCollected<KeyframeEffect>(
+      target, MakeGarbageCollected<StringKeyframeEffectModel>(keyframes),
       timing);
+  target->GetDocument().Timeline().Play(keyframe_effect);
 
   // This (previously) would not clear the existing baseComputedStyle and would
   // crash on the equality assertion in the exit frame when it tried to update

third_party/blink/renderer/core/animation/keyframe_effect.cc

@@ -45,7 +45,6 @@
 #include "third_party/blink/renderer/core/svg/svg_element.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
 #include "third_party/blink/renderer/platform/heap/heap.h"
-#include "third_party/blink/renderer/platform/instrumentation/use_counter.h"
 #include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 
 namespace blink {
@@ -56,11 +55,6 @@ KeyframeEffect* KeyframeEffect::Create(
     const ScriptValue& keyframes,
     const UnrestrictedDoubleOrKeyframeEffectOptions& options,
     ExceptionState& exception_state) {
-  if (element) {
-    UseCounter::Count(
-        element->GetDocument(),
-        WebFeature::kAnimationConstructorKeyframeListEffectObjectTiming);
-  }
   Document* document = element ? &element->GetDocument() : nullptr;
   Timing timing = TimingInput::Convert(options, document, exception_state);
   if (exception_state.HadException())
@@ -84,11 +78,6 @@ KeyframeEffect* KeyframeEffect::Create(ScriptState* script_state,
                                        Element* element,
                                        const ScriptValue& keyframes,
                                        ExceptionState& exception_state) {
-  if (element) {
-    UseCounter::Count(
-        element->GetDocument(),
-        WebFeature::kAnimationConstructorKeyframeListEffectNoTiming);
-  }
   KeyframeEffectModelBase* model =
       EffectInput::Convert(element, keyframes, EffectModel::kCompositeReplace,
                            script_state, exception_state);

third_party/blink/renderer/core/animation/keyframe_effect.idl

@@ -33,7 +33,8 @@
 enum CompositeOperation { "replace", "add", "accumulate" };
 
 [
-    Exposed=Window
+    Exposed=Window,
+    Measure
 ] interface KeyframeEffect : AnimationEffect {
   [CallWith=ScriptState, RaisesException] constructor(Element? target, object? keyframes, optional (unrestricted double or KeyframeEffectOptions) options);
   [CallWith=ScriptState, RaisesException] constructor(KeyframeEffect source);

tools/metrics/histograms/enums.xml

@@ -25767,6 +25767,7 @@ to ensure that the crash string is shown properly on the user-facing crash UI.
   <int value="3123" label="WebkitBoxPackJustifyDoesSomething"/>
   <int value="3124" label="WebkitBoxPackCenterDoesSomething"/>
   <int value="3125" label="WebkitBoxPackEndDoesSomething"/>
+  <int value="3126" label="V8KeyframeEffect_Constructor"/>
 </enum>
 
 <enum name="FeaturePolicyAllowlistType">