Bug 1500260 - Expire keyframes animations when no longer referenced by the style. r=emilio
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sat, 05 May 2018 19:15:59 +0200
changeset 497837 7037075929099a8d88ef702257e88d15cbe3ab59
parent 497836 6d0c84abdfa1d1b5e64e78a1826762e110c7ea57
child 497838 99cc3c6f76ef81913aea6d0a7d785e02e55f4648
push id10002
push userarchaeopteryx@coole-files.de
push dateFri, 19 Oct 2018 23:09:29 +0000
treeherdermozilla-beta@01378c910610 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
bugs1500260, 20731, 20757
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1500260 - Expire keyframes animations when no longer referenced by the style. r=emilio It's a long way to make this sound in general... Fixes #20731 This cherry-picks part of servo/servo#20757.
servo/components/style/animation.rs
servo/components/style/matching.rs
--- a/servo/components/style/animation.rs
+++ b/servo/components/style/animation.rs
@@ -625,38 +625,62 @@ pub fn update_style_for_animation_frame(
 
     frame
         .property_animation
         .update(Arc::make_mut(&mut new_style), progress);
 
     true
 }
 
+/// Returns the kind of animation update that happened.
+pub enum AnimationUpdate {
+    /// The style was successfully updated, the animation is still running.
+    Regular,
+    /// A style change canceled this animation.
+    AnimationCanceled,
+}
+
 /// Updates a single animation and associated style based on the current time.
+///
+/// FIXME(emilio): This doesn't handle any kind of dynamic change to the
+/// animation or transition properties in any reasonable way.
+///
+/// This should probably be split in two, one from updating animations and
+/// transitions in response to a style change (that is,
+/// consider_starting_transitions + maybe_start_animations, but handling
+/// canceled animations, duration changes, etc, there instead of here), and this
+/// function should be only about the style update in response of a transition.
 pub fn update_style_for_animation<E>(
     context: &SharedStyleContext,
     animation: &Animation,
     style: &mut Arc<ComputedValues>,
     font_metrics_provider: &FontMetricsProvider,
-) where
+) -> AnimationUpdate
+where
     E: TElement,
 {
     debug!("update_style_for_animation: entering");
     debug_assert!(!animation.is_expired());
 
     match *animation {
         Animation::Transition(_, start_time, ref frame) => {
             debug!("update_style_for_animation: transition found");
             let now = context.timer.seconds();
             let mut new_style = (*style).clone();
             let updated_style =
                 update_style_for_animation_frame(&mut new_style, now, start_time, frame);
             if updated_style {
                 *style = new_style
             }
+            // FIXME(emilio): Should check before updating the style that the
+            // transition_property still transitions this, or bail out if not.
+            //
+            // Or doing it in process_animations, only if transition_property
+            // changed somehow (even better).
+            AnimationUpdate::Regular
         },
         Animation::Keyframes(_, ref animation, ref name, ref state) => {
             debug!(
                 "update_style_for_animation: animation found: \"{}\", {:?}",
                 name, state
             );
             let duration = state.duration;
             let started_at = state.started_at;
@@ -670,38 +694,28 @@ pub fn update_style_for_animation<E>(
 
             let maybe_index = style
                 .get_box()
                 .animation_name_iter()
                 .position(|animation_name| Some(name) == animation_name.as_atom());
 
             let index = match maybe_index {
                 Some(index) => index,
-                None => {
-                    warn!(
-                        "update_style_for_animation: Animation {:?} not found in style",
-                        name
-                    );
-                    return;
-                },
+                None => return AnimationUpdate::AnimationCanceled,
             };
 
             let total_duration = style.get_box().animation_duration_mod(index).seconds() as f64;
             if total_duration == 0. {
-                debug!(
-                    "update_style_for_animation: zero duration for animation {:?}",
-                    name
-                );
-                return;
+                return AnimationUpdate::AnimationCanceled;
             }
 
             let mut total_progress = (now - started_at) / total_duration;
             if total_progress < 0. {
                 warn!("Negative progress found for animation {:?}", name);
-                return;
+                return AnimationUpdate::Regular;
             }
             if total_progress > 1. {
                 total_progress = 1.;
             }
 
             debug!(
                 "update_style_for_animation: anim \"{}\", steps: {:?}, state: {:?}, progress: {}",
                 name, animation.steps, state, total_progress
@@ -743,21 +757,17 @@ pub fn update_style_for_animation<E>(
 
             debug!(
                 "update_style_for_animation: keyframe from {:?} to {:?}",
                 last_keyframe_position, target_keyframe_position
             );
 
             let target_keyframe = match target_keyframe_position {
                 Some(target) => &animation.steps[target],
-                None => {
-                    warn!("update_style_for_animation: No current keyframe found for animation \"{}\" at progress {}",
-                          name, total_progress);
-                    return;
-                },
+                None => return AnimationUpdate::Regular,
             };
 
             let last_keyframe = &animation.steps[last_keyframe_position];
 
             let relative_timespan =
                 (target_keyframe.start_percentage.0 - last_keyframe.start_percentage.0).abs();
             let relative_duration = relative_timespan as f64 * duration;
             let last_keyframe_ended_at = match state.current_direction {
@@ -831,16 +841,17 @@ pub fn update_style_for_animation<E>(
                 }
             }
 
             debug!(
                 "update_style_for_animation: got style change in animation \"{}\"",
                 name
             );
             *style = new_style;
+            AnimationUpdate::Regular
         },
     }
 }
 
 /// Update the style in the node when it finishes.
 #[cfg(feature = "servo")]
 pub fn complete_expired_transitions(
     node: OpaqueNode,
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -580,17 +580,17 @@ trait PrivateMatchMethods: TElement {
     #[cfg(feature = "servo")]
     fn update_animations_for_cascade(
         &self,
         context: &SharedStyleContext,
         style: &mut Arc<ComputedValues>,
         possibly_expired_animations: &mut Vec<::animation::PropertyAnimation>,
         font_metrics: &::font_metrics::FontMetricsProvider,
     ) {
-        use animation::{self, Animation};
+        use animation::{self, Animation, AnimationUpdate};
         use dom::TNode;
 
         // Finish any expired transitions.
         let this_opaque = self.as_node().opaque();
         animation::complete_expired_transitions(this_opaque, style, context);
 
         // Merge any running animations into the current style, and cancel them.
         let had_running_animations = context
@@ -598,40 +598,50 @@ trait PrivateMatchMethods: TElement {
             .read()
             .get(&this_opaque)
             .is_some();
         if !had_running_animations {
             return;
         }
 
         let mut all_running_animations = context.running_animations.write();
-        for running_animation in all_running_animations.get_mut(&this_opaque).unwrap() {
+        for mut running_animation in all_running_animations.get_mut(&this_opaque).unwrap() {
             // This shouldn't happen frequently, but under some circumstances
             // mainly huge load or debug builds, the constellation might be
             // delayed in sending the `TickAllAnimations` message to layout.
             //
             // Thus, we can't assume all the animations have been already
             // updated by layout, because other restyle due to script might be
             // triggered by layout before the animation tick.
             //
             // See #12171 and the associated PR for an example where this
             // happened while debugging other release panic.
             if running_animation.is_expired() {
                 continue;
             }
 
-            animation::update_style_for_animation::<Self>(
+            let update = animation::update_style_for_animation::<Self>(
                 context,
-                running_animation,
+                &mut running_animation,
                 style,
                 font_metrics,
             );
 
-            if let Animation::Transition(_, _, ref frame) = *running_animation {
-                possibly_expired_animations.push(frame.property_animation.clone())
+            match *running_animation {
+                Animation::Transition(_, _, ref frame) => {
+                    possibly_expired_animations.push(frame.property_animation.clone())
+                }
+                Animation::Keyframes(_, _, _, ref mut state) => {
+                    match update {
+                        AnimationUpdate::Regular => {},
+                        AnimationUpdate::AnimationCanceled => {
+                            state.expired = true;
+                        }
+                    }
+                }
             }
         }
     }
 }
 
 impl<E: TElement> PrivateMatchMethods for E {}
 
 /// The public API that elements expose for selector matching.