Stop setting Animation id to transition property and animation name.

Update devtools to no longer require the id property as identifying the
animation name or transition property and instead use the proper IDL
hierarchy and properties.

Bug: 849927
Change-Id: I85a1c6eda7f176ce23906ee64b4c9c2d3374c57c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1740647
Auto-Submit: George Steel <gtsteel@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: George Steel <gtsteel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686500}

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

@@ -99,6 +99,9 @@ class CORE_EXPORT Animation : public EventTargetWithInlineData,
   ~Animation() override;
   void Dispose();
 
+  virtual bool IsCSSAnimation() const { return false; }
+  virtual bool IsCSSTransition() const { return false; }
+
   // Returns whether the animation is finished.
   bool Update(TimingUpdateReason);
 

third_party/blink/renderer/core/animation/css/css_animation.cc

@@ -21,12 +21,6 @@ CSSAnimation::CSSAnimation(ExecutionContext* execution_context,
                            AnimationEffect* content,
                            const String& animation_name)
     : Animation(execution_context, timeline, content),
-      animation_name_(animation_name) {
-  setId(animation_name);
-}
-
-const String& CSSAnimation::animationName() const {
-  return animation_name_;
-}
+      animation_name_(animation_name) {}
 
 }  // namespace blink

third_party/blink/renderer/core/animation/css/css_animation.h

@@ -24,12 +24,20 @@ class CORE_EXPORT CSSAnimation : public Animation {
                AnimationEffect*,
                const String& animation_name);
 
-  const String& animationName() const;
+  bool IsCSSAnimation() const final { return true; }
+
+  const String& animationName() const { return animation_name_; }
 
  private:
   String animation_name_;
 };
 
+DEFINE_TYPE_CASTS(CSSAnimation,
+                  Animation,
+                  animation,
+                  animation->IsCSSAnimation(),
+                  animation.IsCSSAnimation());
+
 }  // namespace blink
 
 #endif  // THIRD_PARTY_BLINK_RENDERER_CORE_ANIMATION_CSS_CSS_ANIMATION_H_

third_party/blink/renderer/core/animation/css/css_animations.cc

@@ -225,24 +225,6 @@ double StartTimeFromDelay(double start_delay) {
 
 CSSAnimations::CSSAnimations() = default;
 
-bool CSSAnimations::IsAnimationForInspector(const Animation& animation) {
-  for (const auto& running_animation : running_animations_) {
-    if (running_animation->animation->SequenceNumber() ==
-        animation.SequenceNumber())
-      return true;
-  }
-  return false;
-}
-
-bool CSSAnimations::IsTransitionAnimationForInspector(
-    const Animation& animation) const {
-  for (const auto& it : transitions_) {
-    if (it.value.animation->SequenceNumber() == animation.SequenceNumber())
-      return true;
-  }
-  return false;
-}
-
 namespace {
 
 const KeyframeEffectModelBase* GetKeyframeEffectModelBase(

third_party/blink/renderer/core/animation/css/css_animations.h

@@ -57,9 +57,6 @@ class CORE_EXPORT CSSAnimations final {
  public:
   CSSAnimations();
 
-  bool IsAnimationForInspector(const Animation&);
-  bool IsTransitionAnimationForInspector(const Animation&) const;
-
   static const StylePropertyShorthand& PropertiesForTransitionAll();
   static bool IsAnimationAffectingProperty(const CSSProperty&);
   static bool IsAffectedByKeyframesFromScope(const Element&, const TreeScope&);

third_party/blink/renderer/core/animation/css/css_transition.cc

@@ -19,9 +19,7 @@ CSSTransition::CSSTransition(ExecutionContext* execution_context,
                              AnimationEffect* content,
                              const PropertyHandle& transition_property)
     : Animation(execution_context, timeline, content),
-      transition_property_(transition_property) {
-  setId(transitionProperty());
-}
+      transition_property_(transition_property) {}
 
 AtomicString CSSTransition::transitionProperty() const {
   return transition_property_.GetCSSPropertyName().ToAtomicString();

third_party/blink/renderer/core/animation/css/css_transition.h

@@ -23,12 +23,23 @@ class CORE_EXPORT CSSTransition : public Animation {
                 AnimationEffect*,
                 const PropertyHandle& transition_property);
 
+  bool IsCSSTransition() const final { return true; }
+
   AtomicString transitionProperty() const;
+  const CSSProperty& TransitionCSSProperty() const {
+    return transition_property_.GetCSSProperty();
+  }
 
  private:
   PropertyHandle transition_property_;
 };
 
+DEFINE_TYPE_CASTS(CSSTransition,
+                  Animation,
+                  animation,
+                  animation->IsCSSTransition(),
+                  animation.IsCSSTransition());
+
 }  // namespace blink
 
 #endif  // THIRD_PARTY_BLINK_RENDERER_CORE_ANIMATION_CSS_CSS_TRANSITION_H_

third_party/blink/renderer/core/inspector/inspector_animation_agent.cc

@@ -10,7 +10,9 @@
 #include "third_party/blink/renderer/core/animation/animation.h"
 #include "third_party/blink/renderer/core/animation/animation_effect.h"
 #include "third_party/blink/renderer/core/animation/computed_effect_timing.h"
+#include "third_party/blink/renderer/core/animation/css/css_animation.h"
 #include "third_party/blink/renderer/core/animation/css/css_animations.h"
+#include "third_party/blink/renderer/core/animation/css/css_transition.h"
 #include "third_party/blink/renderer/core/animation/effect_model.h"
 #include "third_party/blink/renderer/core/animation/element_animations.h"
 #include "third_party/blink/renderer/core/animation/keyframe_effect.h"
@@ -34,6 +36,21 @@
 
 namespace blink {
 
+namespace {
+
+String AnimationDisplayName(const Animation& animation) {
+  if (!animation.id().IsEmpty())
+    return animation.id();
+  else if (animation.IsCSSAnimation())
+    return ToCSSAnimation(animation).animationName();
+  else if (animation.IsCSSTransition())
+    return ToCSSTransition(animation).transitionProperty();
+  else
+    return animation.id();
+}
+
+}  // namespace
+
 using protocol::Response;
 
 InspectorAnimationAgent::InspectorAnimationAgent(
@@ -65,7 +82,6 @@ Response InspectorAnimationAgent::disable() {
   enabled_.Clear();
   instrumenting_agents_->RemoveInspectorAnimationAgent(this);
   id_to_animation_.clear();
-  id_to_animation_type_.clear();
   id_to_animation_clone_.clear();
   cleared_animations_.clear();
   return Response::OK();
@@ -74,7 +90,6 @@ Response InspectorAnimationAgent::disable() {
 void InspectorAnimationAgent::DidCommitLoadForLocalFrame(LocalFrame* frame) {
   if (frame == inspected_frames_->Root()) {
     id_to_animation_.clear();
-    id_to_animation_type_.clear();
     id_to_animation_clone_.clear();
     cleared_animations_.clear();
   }
@@ -145,47 +160,32 @@ BuildObjectForAnimationKeyframes(const KeyframeEffect* effect) {
 
 std::unique_ptr<protocol::Animation::Animation>
 InspectorAnimationAgent::BuildObjectForAnimation(blink::Animation& animation) {
-  String animation_type;
+  String animation_type = AnimationType::WebAnimation;
   std::unique_ptr<protocol::Animation::AnimationEffect> animation_effect_object;
 
-  if (!animation.effect()) {
-    animation_type = AnimationType::WebAnimation;
-  } else {
-    const Element* element = ToKeyframeEffect(animation.effect())->target();
-    std::unique_ptr<protocol::Animation::KeyframesRule> keyframe_rule;
+  if (animation.effect()) {
+    animation_effect_object =
+        BuildObjectForAnimationEffect(ToKeyframeEffect(animation.effect()));
 
-    if (!element) {
-      animation_type = AnimationType::WebAnimation;
+    if (animation.IsCSSTransition()) {
+      animation_type = AnimationType::CSSTransition;
     } else {
-      CSSAnimations& css_animations =
-          element->GetElementAnimations()->CssAnimations();
+      animation_effect_object->setKeyframesRule(
+          BuildObjectForAnimationKeyframes(
+              ToKeyframeEffect(animation.effect())));
 
-      if (css_animations.IsTransitionAnimationForInspector(animation)) {
-        // CSS Transitions
-        animation_type = AnimationType::CSSTransition;
-      } else {
-        // Keyframe based animations
-        keyframe_rule = BuildObjectForAnimationKeyframes(
-            ToKeyframeEffect(animation.effect()));
-        animation_type = css_animations.IsAnimationForInspector(animation)
-                             ? AnimationType::CSSAnimation
-                             : AnimationType::WebAnimation;
-      }
+      if (animation.IsCSSAnimation())
+        animation_type = AnimationType::CSSAnimation;
     }
-
-    animation_effect_object =
-        BuildObjectForAnimationEffect(ToKeyframeEffect(animation.effect()));
-    animation_effect_object->setKeyframesRule(std::move(keyframe_rule));
   }
 
   String id = String::Number(animation.SequenceNumber());
   id_to_animation_.Set(id, &animation);
-  id_to_animation_type_.Set(id, animation_type);
 
   std::unique_ptr<protocol::Animation::Animation> animation_object =
       protocol::Animation::Animation::create()
           .setId(id)
-          .setName(animation.id())
+          .setName(AnimationDisplayName(animation))
           .setPausedState(animation.Paused())
           .setPlayState(animation.playState())
           .setPlaybackRate(animation.playbackRate())
@@ -343,7 +343,6 @@ Response InspectorAnimationAgent::releaseAnimations(
       clone->cancel();
     id_to_animation_clone_.erase(animation_id);
     id_to_animation_.erase(animation_id);
-    id_to_animation_type_.erase(animation_id);
     cleared_animations_.insert(animation_id);
   }
   return Response::OK();
@@ -417,26 +416,28 @@ String InspectorAnimationAgent::CreateCSSId(blink::Animation& animation) {
       &GetCSSPropertyTransitionProperty(),
       &GetCSSPropertyTransitionTimingFunction(),
   };
-  String type =
-      id_to_animation_type_.at(String::Number(animation.SequenceNumber()));
-  DCHECK_NE(type, AnimationType::WebAnimation);
 
   KeyframeEffect* effect = ToKeyframeEffect(animation.effect());
   Vector<const CSSProperty*> css_properties;
-  if (type == AnimationType::CSSAnimation) {
+  if (animation.IsCSSAnimation()) {
     for (const CSSProperty* property : g_animation_properties)
       css_properties.push_back(property);
-  } else {
+  } else if (animation.IsCSSTransition()) {
     for (const CSSProperty* property : g_transition_properties)
       css_properties.push_back(property);
-    css_properties.push_back(&CSSProperty::Get(cssPropertyID(animation.id())));
+    css_properties.push_back(
+        &ToCSSTransition(animation).TransitionCSSProperty());
+  } else {
+    NOTREACHED();
   }
 
   Element* element = effect->target();
   HeapVector<Member<CSSStyleDeclaration>> styles =
       css_agent_->MatchingStyles(element);
   Digestor digestor(kHashAlgorithmSha1);
-  digestor.UpdateUtf8(type);
+  digestor.UpdateUtf8(animation.IsCSSTransition()
+                          ? AnimationType::CSSTransition
+                          : AnimationType::CSSAnimation);
   digestor.UpdateUtf8(animation.id());
   for (const CSSProperty* property : css_properties) {
     CSSStyleDeclaration* style =

third_party/blink/renderer/core/inspector/inspector_animation_agent.h

@@ -87,7 +87,6 @@ class CORE_EXPORT InspectorAnimationAgent final
   v8_inspector::V8InspectorSession* v8_session_;
   HeapHashMap<String, Member<blink::Animation>> id_to_animation_;
   HeapHashMap<String, Member<blink::Animation>> id_to_animation_clone_;
-  HashMap<String, String> id_to_animation_type_;
   bool is_cloning_;
   HashSet<String> cleared_animations_;
   InspectorAgentState::Boolean enabled_;

third_party/blink/web_tests/external/wpt/css/css-animations/CSSAnimation-id.tentative-expected.txt

-This is a testharness.js-based test.
-FAIL Animation.id for CSS Animations assert_equals: id for CSS Animation is initially empty expected "" but got "abc"
-Harness: the test ran to completion.
-