From 90290eb985940a6fe29ee71d7bece905d402c698 Mon Sep 17 00:00:00 2001 From: Matthew Olsson Date: Thu, 7 Mar 2024 11:11:27 -0700 Subject: [PATCH] LibWeb: Store Animations in Animatable instead of AnimationEffects This is closer to what the spec instructs us to do, and matches how associations are maintained in the timelines. Also note that the removed destructor logic is not necessary since we visit the associated animations anyways. --- .../LibWeb/Animations/Animatable.cpp | 22 +++++++++---------- .../Libraries/LibWeb/Animations/Animatable.h | 6 ++--- .../Libraries/LibWeb/Animations/Animation.cpp | 2 +- .../LibWeb/Animations/KeyframeEffect.cpp | 16 +++++--------- .../LibWeb/Animations/KeyframeEffect.h | 2 +- 5 files changed, 22 insertions(+), 26 deletions(-) diff --git a/Userland/Libraries/LibWeb/Animations/Animatable.cpp b/Userland/Libraries/LibWeb/Animations/Animatable.cpp index 64512d41b8..ec63978846 100644 --- a/Userland/Libraries/LibWeb/Animations/Animatable.cpp +++ b/Userland/Libraries/LibWeb/Animations/Animatable.cpp @@ -63,9 +63,9 @@ Vector> Animatable::get_animations(Web::Animations:: // The returned list is sorted using the composite order described for the associated animations of effects in // ยง5.4.2 The effect stack. if (!m_is_sorted_by_composite_order) { - quick_sort(m_associated_effects, [](JS::NonnullGCPtr& a, JS::NonnullGCPtr& b) { - auto& a_effect = verify_cast(*a); - auto& b_effect = verify_cast(*b); + quick_sort(m_associated_animations, [](JS::NonnullGCPtr& a, JS::NonnullGCPtr& b) { + auto& a_effect = verify_cast(*a->effect()); + auto& b_effect = verify_cast(*b->effect()); return KeyframeEffect::composite_order(a_effect, b_effect) < 0; }); m_is_sorted_by_composite_order = true; @@ -75,29 +75,29 @@ Vector> Animatable::get_animations(Web::Animations:: (void)options; Vector> relevant_animations; - for (auto& effect : m_associated_effects) { - if (auto animation = effect->associated_animation(); animation && animation->is_relevant()) + for (auto const& animation : m_associated_animations) { + if (animation->is_relevant()) relevant_animations.append(*animation); } return relevant_animations; } -void Animatable::associate_with_effect(JS::NonnullGCPtr effect) +void Animatable::associate_with_animation(JS::NonnullGCPtr animation) { - m_associated_effects.append(effect); + m_associated_animations.append(animation); m_is_sorted_by_composite_order = false; } -void Animatable::disassociate_with_effect(JS::NonnullGCPtr effect) +void Animatable::disassociate_with_animation(JS::NonnullGCPtr animation) { - m_associated_effects.remove_first_matching([&](auto element) { return effect == element; }); + m_associated_animations.remove_first_matching([&](auto element) { return animation == element; }); } void Animatable::visit_edges(JS::Cell::Visitor& visitor) { - for (auto const& effect : m_associated_effects) - visitor.visit(effect); + for (auto const& animation : m_associated_animations) + visitor.visit(animation); visitor.visit(m_cached_animation_name_source); visitor.visit(m_cached_animation_name_animation); } diff --git a/Userland/Libraries/LibWeb/Animations/Animatable.h b/Userland/Libraries/LibWeb/Animations/Animatable.h index 0ba8c2afdc..b38d02d892 100644 --- a/Userland/Libraries/LibWeb/Animations/Animatable.h +++ b/Userland/Libraries/LibWeb/Animations/Animatable.h @@ -30,8 +30,8 @@ public: WebIDL::ExceptionOr> animate(Optional> keyframes, Variant options = {}); Vector> get_animations(GetAnimationsOptions options = {}); - void associate_with_effect(JS::NonnullGCPtr effect); - void disassociate_with_effect(JS::NonnullGCPtr effect); + void associate_with_animation(JS::NonnullGCPtr); + void disassociate_with_animation(JS::NonnullGCPtr); JS::GCPtr cached_animation_name_source() const { return m_cached_animation_name_source; } void set_cached_animation_name_source(JS::GCPtr value) { m_cached_animation_name_source = value; } @@ -43,7 +43,7 @@ protected: void visit_edges(JS::Cell::Visitor&); private: - Vector> m_associated_effects; + Vector> m_associated_animations; bool m_is_sorted_by_composite_order { true }; JS::GCPtr m_cached_animation_name_source; JS::GCPtr m_cached_animation_name_animation; diff --git a/Userland/Libraries/LibWeb/Animations/Animation.cpp b/Userland/Libraries/LibWeb/Animations/Animation.cpp index 863a99c72c..2ebec92058 100644 --- a/Userland/Libraries/LibWeb/Animations/Animation.cpp +++ b/Userland/Libraries/LibWeb/Animations/Animation.cpp @@ -352,7 +352,7 @@ void Animation::set_replace_state(Bindings::AnimationReplaceState value) if (value == Bindings::AnimationReplaceState::Removed) { // Remove the associated effect from its target, if applicable if (m_effect && m_effect->target()) - m_effect->target()->disassociate_with_effect(*m_effect); + m_effect->target()->disassociate_with_animation(*this); // Remove this animation from its timeline m_timeline->disassociate_with_animation(*this); diff --git a/Userland/Libraries/LibWeb/Animations/KeyframeEffect.cpp b/Userland/Libraries/LibWeb/Animations/KeyframeEffect.cpp index 68d11b81d1..9fdc1a6522 100644 --- a/Userland/Libraries/LibWeb/Animations/KeyframeEffect.cpp +++ b/Userland/Libraries/LibWeb/Animations/KeyframeEffect.cpp @@ -723,11 +723,13 @@ WebIDL::ExceptionOr> KeyframeEffect::construct_ void KeyframeEffect::set_target(DOM::Element* target) { - if (m_target_element) - m_target_element->disassociate_with_effect(*this); + if (auto animation = this->associated_animation()) { + if (m_target_element) + m_target_element->disassociate_with_animation(*animation); + if (target) + target->associate_with_animation(*animation); + } m_target_element = target; - if (m_target_element) - m_target_element->associate_with_effect(*this); } WebIDL::ExceptionOr KeyframeEffect::set_pseudo_element(Optional pseudo_element) @@ -853,12 +855,6 @@ KeyframeEffect::KeyframeEffect(JS::Realm& realm) { } -KeyframeEffect::~KeyframeEffect() -{ - if (m_target_element) - m_target_element->disassociate_with_effect(*this); -} - void KeyframeEffect::initialize(JS::Realm& realm) { Base::initialize(realm); diff --git a/Userland/Libraries/LibWeb/Animations/KeyframeEffect.h b/Userland/Libraries/LibWeb/Animations/KeyframeEffect.h index fd2dfd689d..5b1707962e 100644 --- a/Userland/Libraries/LibWeb/Animations/KeyframeEffect.h +++ b/Userland/Libraries/LibWeb/Animations/KeyframeEffect.h @@ -105,7 +105,7 @@ public: private: KeyframeEffect(JS::Realm&); - virtual ~KeyframeEffect() override; + virtual ~KeyframeEffect() override = default; virtual void initialize(JS::Realm&) override; virtual void visit_edges(Cell::Visitor&) override;