Auto merge of #17770 - hiikezoe:snapshot-in-animation-only-restyle, r=emilio

Avoid snapshot handling in animation only restyle

<!-- Please describe your changes on the following line: -->
https://bugzilla.mozilla.org/show_bug.cgi?id=1381431

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17770)
<!-- Reviewable:end -->

components/style/data.rs

@@ -241,6 +241,11 @@ impl ElementData {
         element: E,
         shared_context: &SharedStyleContext)
     {
+        // In animation-only restyle we shouldn't touch snapshot at all.
+        if shared_context.traversal_flags.for_animation_only() {
+            return;
+        }
+
         use invalidation::element::invalidator::TreeStyleInvalidator;
 
         debug!("invalidate_style_if_needed: {:?}, flags: {:?}, has_snapshot: {}, \
@@ -278,25 +283,17 @@ impl ElementData {
     pub fn restyle_kind(&self,
                         shared_context: &SharedStyleContext)
                         -> RestyleKind {
+        if shared_context.traversal_flags.for_animation_only() {
+            return self.restyle_kind_for_animation(shared_context);
+        }
+
         debug_assert!(!self.has_styles() || self.has_invalidations(),
                       "Should've stopped earlier");
         if !self.has_styles() {
-            debug_assert!(!shared_context.traversal_flags.for_animation_only(),
-                          "Unstyled element shouldn't be traversed during \
-                           animation-only traversal");
             return RestyleKind::MatchAndCascade;
         }
 
         let hint = self.restyle.hint;
-        if shared_context.traversal_flags.for_animation_only() {
-            // return either CascadeWithReplacements or CascadeOnly in case of
-            // animation-only restyle.
-            if hint.has_animation_hint() {
-                return RestyleKind::CascadeWithReplacements(hint & RestyleHint::for_animations());
-            }
-            return RestyleKind::CascadeOnly;
-        }
-
         if hint.match_self() {
             return RestyleKind::MatchAndCascade;
         }
@@ -312,6 +309,26 @@ impl ElementData {
         return RestyleKind::CascadeOnly;
     }
 
+    /// Returns the kind of restyling for animation-only restyle.
+    pub fn restyle_kind_for_animation(&self,
+                                      shared_context: &SharedStyleContext)
+                                      -> RestyleKind {
+        debug_assert!(shared_context.traversal_flags.for_animation_only());
+        debug_assert!(self.has_styles(),
+                      "Unstyled element shouldn't be traversed during \
+                       animation-only traversal");
+
+        // return either CascadeWithReplacements or CascadeOnly in case of
+        // animation-only restyle. I.e. animation-only restyle never does
+        // selector matching.
+        let hint = self.restyle.hint;
+        if hint.has_animation_hint() {
+            return RestyleKind::CascadeWithReplacements(hint & RestyleHint::for_animations());
+        }
+        return RestyleKind::CascadeOnly;
+
+    }
+
     /// Return true if important rules are different.
     /// We use this to make sure the cascade of off-main thread animations is correct.
     /// Note: Ignore custom properties for now because we only support opacity and transform

components/style/dom.rs

@@ -33,6 +33,7 @@ use std::ops::Deref;
 use stylearc::{Arc, ArcBorrow};
 use stylist::Stylist;
 use thread_state;
+use traversal::TraversalFlags;
 
 pub use style_traits::UnsafeNode;
 
@@ -478,6 +479,29 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone +
         data.has_styles() && !data.has_invalidations()
     }
 
+    /// Returns whether the element's styles are up-to-date for |traversal_flags|.
+    fn has_current_styles_for_traversal(&self,
+                                        data: &ElementData,
+                                        traversal_flags: TraversalFlags) -> bool {
+        if traversal_flags.for_animation_only() {
+            // In animation-only restyle we never touch snapshots and don't
+            // care about them. But we can't assert '!self.handled_snapshot()'
+            // here since there are some cases that a second animation-only
+            // restyle which is a result of normal restyle (e.g. setting
+            // animation-name in normal restyle and creating a new CSS
+            // animation in a SequentialTask) is processed after the normal
+            // traversal in that we had elements that handled snapshot.
+            return data.has_styles() &&
+                   !data.restyle.hint.has_animation_hint_or_recascade();
+        }
+
+        if self.has_snapshot() && !self.handled_snapshot() {
+            return false;
+        }
+
+        data.has_styles() && !data.has_invalidations()
+    }
+
     /// Flags an element and its ancestors with a given `DescendantsBit`.
     ///
     /// TODO(emilio): We call this conservatively from restyle_element_internal

components/style/gecko/generated/bindings.rs

@@ -2733,7 +2733,8 @@ extern "C" {
 }
 extern "C" {
     pub fn Servo_ResolveStyle(element: RawGeckoElementBorrowed,
-                              set: RawServoStyleSetBorrowed)
+                              set: RawServoStyleSetBorrowed,
+                              restyle_behavior: TraversalRestyleBehavior)
      -> ServoStyleContextStrong;
 }
 extern "C" {

components/style/invalidation/element/restyle_hints.rs

@@ -131,6 +131,13 @@ impl RestyleHint {
         self.intersects(Self::for_animations())
     }
 
+    /// Returns whether the hint specifies that an animation cascade level must
+    /// be replaced.
+    #[inline]
+    pub fn has_animation_hint_or_recascade(&self) -> bool {
+        self.intersects(Self::for_animations() | RECASCADE_SELF)
+    }
+
     /// Returns whether the hint specifies some restyle work other than an
     /// animation cascade level replacement.
     #[inline]

components/style/sharing/mod.rs

@@ -675,7 +675,7 @@ impl<E: TElement> StyleSharingCandidateCache<E> {
         }
 
         let data = candidate.element.borrow_data().unwrap();
-        debug_assert!(target.has_current_styles(&data));
+        debug_assert!(target.has_current_styles_for_traversal(&data, shared.traversal_flags));
 
         debug!("Sharing style between {:?} and {:?}",
                target.element, candidate.element);

components/style/traversal.rs

@@ -235,7 +235,9 @@ pub trait DomTraversal<E: TElement> : Sync {
             // Look at whether there has been any attribute or state change, and
             // invalidate our style, and the one of our siblings and descendants as
             // needed.
-            data.invalidate_style_if_needed(root, shared_context);
+            if !flags.for_animation_only() {
+                data.invalidate_style_if_needed(root, shared_context);
+            }
 
             let parent = root.traversal_parent();
             let parent_data = match parent {
@@ -328,8 +330,7 @@ pub trait DomTraversal<E: TElement> : Sync {
         // animation-only restyle hint or recascade.
         if traversal_flags.for_animation_only() {
             return el.has_animation_only_dirty_descendants() ||
-                   data.restyle.hint.has_animation_hint() ||
-                   data.restyle.hint.has_recascade_self();
+                   data.restyle.hint.has_animation_hint_or_recascade();
         }
 
         // If the dirty descendants bit is set, we need to traverse no matter
@@ -341,7 +342,7 @@ pub trait DomTraversal<E: TElement> : Sync {
         // If we have a restyle hint or need to recascade, we need to visit the
         // element.
         //
-        // Note that this is different than checking has_current_styles(),
+        // Note that this is different than checking has_current_styles_for_traversal(),
         // since that can return true even if we have a restyle hint indicating
         // that the element's descendants (but not necessarily the element) need
         // restyling.
@@ -524,10 +525,12 @@ where
 {
     context.thread_local.begin_element(element, data);
     context.thread_local.statistics.elements_traversed += 1;
-    debug_assert!(!element.has_snapshot() || element.handled_snapshot(),
+    debug_assert!(context.shared.traversal_flags.for_animation_only() ||
+                  !element.has_snapshot() || element.handled_snapshot(),
                   "Should've handled snapshots here already");
 
-    let compute_self = !element.has_current_styles(data);
+    let compute_self =
+        !element.has_current_styles_for_traversal(data, context.shared.traversal_flags);
     let mut hint = RestyleHint::empty();
 
     debug!("recalc_style_at: {:?} (compute_self={:?}, \
@@ -580,8 +583,7 @@ where
            propagated_hint,
            data.styles.is_display_none(),
            element.implemented_pseudo_element());
-    debug_assert!(element.has_current_styles(data) ||
-                  context.shared.traversal_flags.for_animation_only(),
+    debug_assert!(element.has_current_styles_for_traversal(data, context.shared.traversal_flags),
                   "Should have computed style or haven't yet valid computed \
                    style in case of animation-only restyle");
 
@@ -811,7 +813,9 @@ where
         // as needed.
         //
         // NB: This will be a no-op if there's no snapshot.
-        child_data.invalidate_style_if_needed(child, &context.shared);
+        if !flags.for_animation_only() {
+            child_data.invalidate_style_if_needed(child, &context.shared);
+        }
 
         if D::element_needs_traversal(child, flags, &*child_data, Some(data)) {
             note_child(child_node);

ports/geckolib/glue.rs

@@ -2799,9 +2799,12 @@ pub extern "C" fn Servo_TakeChangeHint(element: RawGeckoElementBorrowed,
 
 #[no_mangle]
 pub extern "C" fn Servo_ResolveStyle(element: RawGeckoElementBorrowed,
-                                     _raw_data: RawServoStyleSetBorrowed)
+                                     _raw_data: RawServoStyleSetBorrowed,
+                                     restyle_behavior: structs::TraversalRestyleBehavior)
                                      -> ServoStyleContextStrong
 {
+    use self::structs::TraversalRestyleBehavior as Restyle;
+
     let element = GeckoElement(element);
     debug!("Servo_ResolveStyle: {:?}", element);
     let data =
@@ -2809,7 +2812,14 @@ pub extern "C" fn Servo_ResolveStyle(element: RawGeckoElementBorrowed,
 
     // TODO(emilio): Downgrade to debug assertions when close to release.
     assert!(data.has_styles(), "Resolving style on unstyled element");
-    debug_assert!(element.has_current_styles(&*data),
+    // In the case where we process for throttled animation, there remaings
+    // restyle hints other than animation hints.
+    let flags = if restyle_behavior == Restyle::ForThrottledAnimationFlush {
+        ANIMATION_ONLY
+    } else {
+        TraversalFlags::empty()
+    };
+    debug_assert!(element.has_current_styles_for_traversal(&*data, flags),
                   "Resolving style on element without current styles");
     data.styles.primary().clone().into_strong()
 }