[BGPT] move composited element check down.

Move the composited element check from CanStartAnimationOnCompositor to
CheckCanStartEffectOnCompositor because the effect has sufficient context
to use the correct namespace to map the object ID to the compositor
element ID, whereas the animation does not.

Bug: 855702
Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ia490fb07780586ddd5b5059671c3611139a25ce8
Reviewed-on: https://chromium-review.googlesource.com/1170238
Commit-Queue: Peter Mayo <petermayo@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587124}

third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-gen-property-trees

@@ -149,8 +149,6 @@ crbug.com/855688 transitions/opacity-transform-transitions-inside-iframe.html [
 crbug.com/855688 transitions/transition-end-event-destroy-iframe.html [ Timeout ]
 crbug.com/855688 virtual/threaded/transitions/opacity-transform-transitions-inside-iframe.html [ Timeout ]
 crbug.com/855688 virtual/threaded/transitions/transition-end-event-destroy-iframe.html [ Timeout ]
-crbug.com/855702 virtual/threaded/animations/composited-filter-webkit-filter.html [ Failure ]
-crbug.com/855702 virtual/threaded/animations/img-element-transform.html [ Timeout Failure Pass ]
 
 crbug.com/861824 virtual/threaded/fast/animationworklet/animation-worklet-inside-iframe.html [ Failure ]
 

third_party/WebKit/LayoutTests/animations/responsive/filter-responsive-neutral-keyframe.html

@@ -24,6 +24,7 @@ if (window.testRunner)
   testRunner.waitUntilDone();
 
 function waitForCompositor() {
+  var target = document.getElementById("green");
   return target.animate({transform: ['none', 'none']}, 1).ready;
 }
 

third_party/WebKit/LayoutTests/transitions/resources/opacity-transform-transitions-inside-iframe-inner.html

@@ -64,6 +64,7 @@
     }
 
     function waitForCompositor() {
+      var background = document.getElementById("background");
       return background.animate({opacity: ['1', '1']}, 1).ready;
     }
 

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

@@ -797,18 +797,17 @@ CompositorAnimations::FailureCode Animation::CheckCanStartAnimationOnCompositor(
     const base::Optional<CompositorElementIdSet>& composited_element_ids)
     const {
   CompositorAnimations::FailureCode code =
-      CheckCanStartAnimationOnCompositorInternal(composited_element_ids);
+      CheckCanStartAnimationOnCompositorInternal();
   if (!code.Ok()) {
     return code;
   }
   return ToKeyframeEffect(content_.Get())
-      ->CheckCanStartAnimationOnCompositor(playback_rate_);
+      ->CheckCanStartAnimationOnCompositor(composited_element_ids,
+                                           playback_rate_);
 }
 
 CompositorAnimations::FailureCode
-Animation::CheckCanStartAnimationOnCompositorInternal(
-    const base::Optional<CompositorElementIdSet>& composited_element_ids)
-    const {
+Animation::CheckCanStartAnimationOnCompositorInternal() const {
   if (is_composited_animation_disabled_for_testing_) {
     return CompositorAnimations::FailureCode::NonActionable(
         "Accelerated animations disabled for testing");
@@ -849,35 +848,6 @@ Animation::CheckCanStartAnimationOnCompositorInternal(
         "Animation effect is not keyframe-based");
   }
 
-  // If the optional element id set has no value we must be in SPv1 mode in
-  // which case we trust the compositing logic will create a layer if needed.
-  if (composited_element_ids) {
-    DCHECK(RuntimeEnabledFeatures::BlinkGenPropertyTreesEnabled() ||
-           RuntimeEnabledFeatures::SlimmingPaintV2Enabled());
-    Element* target_element = ToKeyframeEffect(content_.Get())->target();
-    if (!target_element) {
-      return CompositorAnimations::FailureCode::Actionable(
-          "Animation is not attached to an element");
-    }
-
-    bool has_own_layer_id = false;
-    if (target_element->GetLayoutObject() &&
-        target_element->GetLayoutObject()->IsBoxModelObject() &&
-        target_element->GetLayoutObject()->HasLayer()) {
-      CompositorElementId target_element_id =
-          CompositorElementIdFromUniqueObjectId(
-              target_element->GetLayoutObject()->UniqueId(),
-              CompositorElementIdNamespace::kPrimary);
-      if (composited_element_ids->Contains(target_element_id)) {
-        has_own_layer_id = true;
-      }
-    }
-    if (!has_own_layer_id) {
-      return CompositorAnimations::FailureCode::NonActionable(
-          "Target element does not have its own compositing layer");
-    }
-  }
-
   if (!Playing()) {
     return CompositorAnimations::FailureCode::Actionable(
         "Animation is not playing");
@@ -907,6 +877,7 @@ void Animation::StartAnimationOnCompositor(
 
   DCHECK(!start_time || !IsNull(start_time.value()));
   DCHECK_NE(compositor_group_, 0);
+  DCHECK(ToKeyframeEffect(content_.Get()));
   ToKeyframeEffect(content_.Get())
       ->StartAnimationOnCompositor(compositor_group_, start_time, time_offset,
                                    playback_rate_);

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

@@ -253,8 +253,8 @@ class CORE_EXPORT Animation final : public EventTargetWithInlineData,
   void BeginUpdatingState();
   void EndUpdatingState();
 
-  CompositorAnimations::FailureCode CheckCanStartAnimationOnCompositorInternal(
-      const base::Optional<CompositorElementIdSet>&) const;
+  CompositorAnimations::FailureCode CheckCanStartAnimationOnCompositorInternal()
+      const;
   void CreateCompositorAnimation();
   void DestroyCompositorAnimation();
   void AttachCompositorTimeline();

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

@@ -32,7 +32,9 @@
 
 #include <memory>
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/core/animation/animatable/animatable_double.h"
 #include "third_party/blink/renderer/core/animation/animation_clock.h"
+#include "third_party/blink/renderer/core/animation/css_number_interpolation_type.h"
 #include "third_party/blink/renderer/core/animation/document_timeline.h"
 #include "third_party/blink/renderer/core/animation/element_animations.h"
 #include "third_party/blink/renderer/core/animation/keyframe_effect.h"
@@ -73,6 +75,34 @@ class AnimationAnimationTest : public RenderingTest {
 
   void StartTimeline() { SimulateFrame(0); }
 
+  KeyframeEffectModelBase* MakeSimpleEffectModel() {
+    PropertyHandle PropertyHandleOpacity(GetCSSPropertyOpacity());
+    TransitionKeyframe* start_keyframe =
+        TransitionKeyframe::Create(PropertyHandleOpacity);
+    start_keyframe->SetValue(TypedInterpolationValue::Create(
+        CSSNumberInterpolationType(PropertyHandleOpacity),
+        InterpolableNumber::Create(1.0)));
+    start_keyframe->SetOffset(0.0);
+    // Egregious hack: Sideload the compositor value.
+    // This is usually set in a part of the rendering process SimulateFrame
+    // doesn't call.
+    start_keyframe->SetCompositorValue(AnimatableDouble::Create(1.0));
+    TransitionKeyframe* end_keyframe =
+        TransitionKeyframe::Create(PropertyHandleOpacity);
+    end_keyframe->SetValue(TypedInterpolationValue::Create(
+        CSSNumberInterpolationType(PropertyHandleOpacity),
+        InterpolableNumber::Create(0.0)));
+    end_keyframe->SetOffset(1.0);
+    // Egregious hack: Sideload the compositor value.
+    end_keyframe->SetCompositorValue(AnimatableDouble::Create(0.0));
+
+    TransitionKeyframeVector keyframes;
+    keyframes.push_back(start_keyframe);
+    keyframes.push_back(end_keyframe);
+
+    return TransitionKeyframeEffectModel::Create(keyframes);
+  }
+
   void ResetWithCompositedAnimation() {
     // Get rid of the default animation.
     animation->cancel();
@@ -859,23 +889,23 @@ TEST_F(AnimationAnimationTest, NoCompositeWithoutCompositedElementId) {
   Timing timing;
   timing.iteration_duration = 30;
   KeyframeEffect* keyframe_effect_composited = KeyframeEffect::Create(
-      ToElement(object_composited->GetNode()), MakeEmptyEffectModel(), timing);
+      ToElement(object_composited->GetNode()), MakeSimpleEffectModel(), timing);
   Animation* animation_composited = timeline->Play(keyframe_effect_composited);
   KeyframeEffect* keyframe_effect_not_composited =
       KeyframeEffect::Create(ToElement(object_not_composited->GetNode()),
-                             MakeEmptyEffectModel(), timing);
+                             MakeSimpleEffectModel(), timing);
   Animation* animation_not_composited =
       timeline->Play(keyframe_effect_not_composited);
 
   SimulateFrame(0, composited_element_ids);
   EXPECT_TRUE(
-      animation_composited
-          ->CheckCanStartAnimationOnCompositorInternal(composited_element_ids)
-          .Ok());
-  EXPECT_FALSE(
-      animation_not_composited
-          ->CheckCanStartAnimationOnCompositorInternal(composited_element_ids)
-          .Ok());
+      animation_composited->CheckCanStartAnimationOnCompositorInternal().Ok());
+  EXPECT_TRUE(animation_composited
+                  ->CheckCanStartAnimationOnCompositor(composited_element_ids)
+                  .Ok());
+  EXPECT_FALSE(animation_not_composited
+                   ->CheckCanStartAnimationOnCompositor(composited_element_ids)
+                   .Ok());
 }
 
 // Regression test for http://crbug.com/819591 . If a compositable animation is

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

@@ -151,6 +151,7 @@ CompositorAnimations::CheckCanStartEffectOnCompositor(
     const Element& target_element,
     const Animation* animation_to_add,
     const EffectModel& effect,
+    const base::Optional<CompositorElementIdSet>& composited_element_ids,
     double animation_playback_rate) {
   const KeyframeEffectModelBase& keyframe_effect =
       ToKeyframeEffectModelBase(effect);
@@ -160,6 +161,16 @@ CompositorAnimations::CheckCanStartEffectOnCompositor(
     return FailureCode::Actionable("Animation does not affect any properties");
   }
 
+  if (composited_element_ids) {
+    // If we are going to check that we can animate these below, we need
+    // to have the UniqueID to compute the target ID.  Let's check it
+    // once in common in advance.
+    if (!target_element.GetLayoutObject() ||
+        !target_element.GetLayoutObject()->UniqueId()) {
+      return FailureCode::NonActionable("Target element has no layout");
+    }
+  }
+
   unsigned transform_property_count = 0;
   for (const auto& property : properties) {
     if (!property.IsCSSProperty()) {
@@ -167,6 +178,7 @@ CompositorAnimations::CheckCanStartEffectOnCompositor(
     }
 
     if (IsTransformRelatedCSSProperty(property)) {
+      // We use this later in computing element IDs too.
       if (target_element.GetLayoutObject() &&
           !target_element.GetLayoutObject()->IsTransformApplicable()) {
         return FailureCode::Actionable(
@@ -192,6 +204,8 @@ CompositorAnimations::CheckCanStartEffectOnCompositor(
             "Accelerated keyframe value could not be computed");
       }
 
+      CompositorElementIdNamespace property_namespace =
+          CompositorElementIdNamespace::kPrimary;
       // FIXME: Determine candidacy based on the CSSValue instead of a snapshot
       // AnimatableValue.
       switch (property.GetCSSProperty().PropertyID()) {
@@ -218,6 +232,7 @@ CompositorAnimations::CheckCanStartEffectOnCompositor(
             return FailureCode::Actionable(
                 "Filter-related property may affect surrounding pixels");
           }
+          property_namespace = CompositorElementIdNamespace::kEffectFilter;
           break;
         }
         default:
@@ -231,6 +246,17 @@ CompositorAnimations::CheckCanStartEffectOnCompositor(
           }
           return FailureCode::Actionable(builder.ToString());
       }
+      if (composited_element_ids) {
+        CompositorElementId target_element_id =
+            CompositorElementIdFromUniqueObjectId(
+                target_element.GetLayoutObject()->UniqueId(),
+                property_namespace);
+        DCHECK(target_element_id);
+        if (!composited_element_ids->Contains(target_element_id)) {
+          return FailureCode::NonActionable(
+              "Target element does not have its own compositing layer");
+        }
+      }
     }
   }
 
@@ -274,6 +300,7 @@ CompositorAnimations::CheckCanStartElementOnCompositor(
     // the DCHECK below.
     // DCHECK(document().lifecycle().state() >=
     // DocumentLifecycle::PrePaintClean);
+    DCHECK(target_element.GetLayoutObject());
     if (const auto* paint_properties = target_element.GetLayoutObject()
                                            ->FirstFragment()
                                            .PaintProperties()) {
@@ -309,10 +336,11 @@ CompositorAnimations::CheckCanStartAnimationOnCompositor(
     const Element& target_element,
     const Animation* animation_to_add,
     const EffectModel& effect,
+    const base::Optional<CompositorElementIdSet>& composited_element_ids,
     double animation_playback_rate) {
-  FailureCode code =
-      CheckCanStartEffectOnCompositor(timing, target_element, animation_to_add,
-                                      effect, animation_playback_rate);
+  FailureCode code = CheckCanStartEffectOnCompositor(
+      timing, target_element, animation_to_add, effect, composited_element_ids,
+      animation_playback_rate);
   if (!code.Ok()) {
     return code;
   }
@@ -370,8 +398,11 @@ void CompositorAnimations::StartAnimationOnCompositor(
     Vector<int>& started_keyframe_model_ids,
     double animation_playback_rate) {
   DCHECK(started_keyframe_model_ids.IsEmpty());
-  DCHECK(CheckCanStartAnimationOnCompositor(timing, element, animation, effect,
-                                            animation_playback_rate)
+  // TODO(petermayo): Find and pass the set of valid compositor elements before
+  // BlinkGenPropertyTrees is always on.
+  DCHECK(CheckCanStartAnimationOnCompositor(
+             timing, element, animation, effect,
+             base::Optional<CompositorElementIdSet>(), animation_playback_rate)
              .Ok());
 
   const KeyframeEffectModelBase& keyframe_effect =

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

@@ -36,6 +36,7 @@
 #include "third_party/blink/renderer/core/animation/timing.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/platform/animation/timing_function.h"
+#include "third_party/blink/renderer/platform/graphics/compositor_element_id.h"
 #include "third_party/blink/renderer/platform/wtf/allocator.h"
 #include "third_party/blink/renderer/platform/wtf/vector.h"
 
@@ -85,6 +86,7 @@ class CORE_EXPORT CompositorAnimations {
       const Element&,
       const Animation*,
       const EffectModel&,
+      const base::Optional<CompositorElementIdSet>& composited_element_ids,
       double animation_playback_rate);
   static void CancelIncompatibleAnimationsOnCompositor(const Element&,
                                                        const Animation&,
@@ -140,6 +142,7 @@ class CORE_EXPORT CompositorAnimations {
       const Element&,
       const Animation*,
       const EffectModel&,
+      const base::Optional<CompositorElementIdSet>& composited_element_ids,
       double animation_playback_rate);
   static FailureCode CheckCanStartElementOnCompositor(const Element&);
 

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

@@ -117,7 +117,7 @@ class CORE_EXPORT Keyframe : public GarbageCollectedFinalized<Keyframe> {
 
   // Represents a property-specific keyframe as defined in the spec. Refer to
   // the Keyframe class-level documentation for more details.
-  class PropertySpecificKeyframe
+  class CORE_EXPORT PropertySpecificKeyframe
       : public GarbageCollectedFinalized<PropertySpecificKeyframe> {
    public:
     virtual ~PropertySpecificKeyframe() = default;

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

@@ -216,6 +216,7 @@ void KeyframeEffect::NotifySampledEffectRemovedFromEffectStack() {
 
 CompositorAnimations::FailureCode
 KeyframeEffect::CheckCanStartAnimationOnCompositor(
+    const base::Optional<CompositorElementIdSet>& composited_element_ids,
     double animation_playback_rate) const {
   if (!model_->HasFrames()) {
     return CompositorAnimations::FailureCode::Actionable(
@@ -242,7 +243,7 @@ KeyframeEffect::CheckCanStartAnimationOnCompositor(
 
   return CompositorAnimations::CheckCanStartAnimationOnCompositor(
       SpecifiedTiming(), *target_, GetAnimation(), *Model(),
-      animation_playback_rate);
+      composited_element_ids, animation_playback_rate);
 }
 
 void KeyframeEffect::StartAnimationOnCompositor(
@@ -252,11 +253,15 @@ void KeyframeEffect::StartAnimationOnCompositor(
     double animation_playback_rate,
     CompositorAnimation* compositor_animation) {
   DCHECK(!HasActiveAnimationsOnCompositor());
-  DCHECK(CheckCanStartAnimationOnCompositor(animation_playback_rate).Ok());
+  // TODO(petermayo): Maybe we should recheck that we can start on the
+  // compositor if we have the compositable IDs somewhere.
 
   if (!compositor_animation)
     compositor_animation = GetAnimation()->GetCompositorAnimation();
+
   DCHECK(compositor_animation);
+  DCHECK(target_);
+  DCHECK(Model());
 
   CompositorAnimations::StartAnimationOnCompositor(
       *target_, group, start_time, current_time, SpecifiedTiming(),

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

@@ -99,6 +99,7 @@ class CORE_EXPORT KeyframeEffect final : public AnimationEffect {
   void NotifySampledEffectRemovedFromEffectStack();
 
   CompositorAnimations::FailureCode CheckCanStartAnimationOnCompositor(
+      const base::Optional<CompositorElementIdSet>& composited_element_ids,
       double animation_playback_rate) const;
   // Must only be called once.
   void StartAnimationOnCompositor(int group,

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

@@ -165,13 +165,14 @@ class CORE_EXPORT StringKeyframe : public Keyframe {
     String value_;
   };
 
- private:
+ protected:
   StringKeyframe()
       : css_property_map_(
             MutableCSSPropertyValueSet::Create(kHTMLStandardMode)),
         presentation_attribute_map_(
             MutableCSSPropertyValueSet::Create(kHTMLStandardMode)) {}
 
+ private:
   StringKeyframe(const StringKeyframe& copy_from);
 
   Keyframe* Clone() const override;

third_party/blink/renderer/core/layout/layout_object.h

@@ -235,6 +235,9 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
 
   // DisplayItemClient methods.
 
+ protected:
+  void EnsureIdForTesting() { fragment_.EnsureIdForTesting(); };
+
   // Do not call VisualRect directly outside of the DisplayItemClient
   // interface, use a per-fragment one on FragmentData instead.
  private:

third_party/blink/renderer/core/paint/fragment_data.h

@@ -141,6 +141,7 @@ class CORE_EXPORT FragmentData {
     if (rare_data_)
       rare_data_->paint_properties = nullptr;
   }
+  void EnsureIdForTesting() { EnsureRareData(); }
 
   // This is a complete set of property nodes that should be used as a
   // starting point to paint a LayoutObject. This data is cached because some

third_party/blink/renderer/modules/animationworklet/worklet_animation.cc

@@ -406,7 +406,8 @@ bool WorkletAnimation::StartOnCompositor(String* failure_message) {
 
   double playback_rate = 1;
   CompositorAnimations::FailureCode failure_code =
-      GetEffect()->CheckCanStartAnimationOnCompositor(playback_rate);
+      GetEffect()->CheckCanStartAnimationOnCompositor(
+          base::Optional<CompositorElementIdSet>(), playback_rate);
 
   if (!failure_code.Ok()) {
     play_state_ = Animation::kIdle;