servo: Merge #17834 - stylo: Fix the interaction of frame construction restyles with animation restyles (from emilio:animation-fc-crash); r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 24 Jul 2017 01:09:24 -0700
changeset 421646 d39db44301707b46ab4c63889dc777d7af98264b
parent 421645 2dd30b065030bab3f0e2b231822302eda90b6039
child 421647 4698135a21b734ba1b7ba0096222b1cd7641d571
push id1517
push userjlorenzo@mozilla.com
push dateThu, 14 Sep 2017 16:50:54 +0000
treeherdermozilla-release@3b41fd564418 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1383001
milestone56.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
servo: Merge #17834 - stylo: Fix the interaction of frame construction restyles with animation restyles (from emilio:animation-fc-crash); r=heycam Fixes bug 1383001. Source-Repo: https://github.com/servo/servo Source-Revision: 30d6d6024bd0a082424395621f620dc9580970e5
servo/components/style/data.rs
servo/components/style/dom.rs
servo/components/style/style_resolver.rs
servo/components/style/traversal.rs
servo/ports/geckolib/glue.rs
--- a/servo/components/style/data.rs
+++ b/servo/components/style/data.rs
@@ -293,32 +293,26 @@ impl ElementData {
         }
     }
 
     /// Returns true if this element has styles.
     pub fn has_styles(&self) -> bool {
         self.styles.primary.is_some()
     }
 
-    /// Returns whether we have any outstanding style invalidation.
-    pub fn has_invalidations(&self) -> bool {
-        self.restyle.hint.has_self_invalidations()
-    }
-
     /// Returns the kind of restyling that we're going to need to do on this
     /// element, based of the stored restyle hint.
-    pub fn restyle_kind(&self,
-                        shared_context: &SharedStyleContext)
-                        -> RestyleKind {
+    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() {
             return RestyleKind::MatchAndCascade;
         }
 
         let hint = self.restyle.hint;
         if hint.match_self() {
             return RestyleKind::MatchAndCascade;
         }
@@ -330,46 +324,49 @@ impl ElementData {
         }
 
         debug_assert!(hint.has_recascade_self(),
                       "We definitely need to do something!");
         return RestyleKind::CascadeOnly;
     }
 
     /// Returns the kind of restyling for animation-only restyle.
-    pub fn restyle_kind_for_animation(&self,
-                                      shared_context: &SharedStyleContext)
-                                      -> RestyleKind {
+    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
     ///       properties for animations running on compositor. Actually, we only care about opacity
     ///       and transform for now, but it's fine to compare all properties and let the user
     ///       the check which properties do they want.
     ///       If it costs too much, get_properties_overriding_animations() should return a set
     ///       containing only opacity and transform properties.
-    pub fn important_rules_are_different(&self,
-                                         rules: &StrongRuleNode,
-                                         guards: &StylesheetGuards) -> bool {
+    pub fn important_rules_are_different(
+        &self,
+        rules: &StrongRuleNode,
+        guards: &StylesheetGuards
+    ) -> bool {
         debug_assert!(self.has_styles());
         let (important_rules, _custom) =
             self.styles.primary().rules().get_properties_overriding_animations(&guards);
         let (other_important_rules, _custom) = rules.get_properties_overriding_animations(&guards);
         important_rules != other_important_rules
     }
 
     /// Drops any restyle state from the element.
--- a/servo/components/style/dom.rs
+++ b/servo/components/style/dom.rs
@@ -465,46 +465,39 @@ pub trait TElement : Eq + PartialEq + De
     fn has_snapshot(&self) -> bool;
 
     /// Returns whether the current snapshot if present has been handled.
     fn handled_snapshot(&self) -> bool;
 
     /// Flags this element as having handled already its snapshot.
     unsafe fn set_handled_snapshot(&self);
 
-    /// Returns whether the element's styles are up-to-date.
-    fn has_current_styles(&self, data: &ElementData) -> bool {
-        if self.has_snapshot() && !self.handled_snapshot() {
-            return false;
-        }
-
-        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 {
+    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()
+        data.has_styles() && !data.restyle.hint.has_non_animation_hint()
     }
 
     /// Flags an element and its ancestors with a given `DescendantsBit`.
     ///
     /// TODO(emilio): We call this conservatively from restyle_element_internal
     /// because we never flag unstyled stuff. A different setup for this may be
     /// a bit cleaner, but it's probably not worth to invest on it right now
     /// unless necessary.
--- a/servo/components/style/style_resolver.rs
+++ b/servo/components/style/style_resolver.rs
@@ -47,27 +47,17 @@ pub struct PrimaryStyle {
 
 fn with_default_parent_styles<E, F, R>(element: E, f: F) -> R
 where
     E: TElement,
     F: FnOnce(Option<&ComputedValues>, Option<&ComputedValues>) -> R,
 {
     let parent_el = element.inheritance_parent();
     let parent_data = parent_el.as_ref().and_then(|e| e.borrow_data());
-    let parent_style = parent_data.as_ref().map(|d| {
-        // Sometimes Gecko eagerly styles things without processing
-        // pending restyles first. In general we'd like to avoid this,
-        // but there can be good reasons (for example, needing to
-        // construct a frame for some small piece of newly-added
-        // content in order to do something specific with that frame,
-        // but not wanting to flush all of layout).
-        debug_assert!(cfg!(feature = "gecko") ||
-                      parent_el.unwrap().has_current_styles(d));
-        d.styles.primary()
-    });
+    let parent_style = parent_data.as_ref().map(|d| d.styles.primary());
 
     let mut layout_parent_el = parent_el.clone();
     let layout_parent_data;
     let mut layout_parent_style = parent_style;
     if parent_style.map_or(false, |s| s.is_display_contents()) {
         layout_parent_el = Some(layout_parent_el.unwrap().layout_parent());
         layout_parent_data = layout_parent_el.as_ref().unwrap().borrow_data().unwrap();
         layout_parent_style = Some(layout_parent_data.styles.primary());
--- a/servo/components/style/traversal.rs
+++ b/servo/components/style/traversal.rs
@@ -369,19 +369,18 @@ pub trait DomTraversal<E: TElement> : Sy
 
     /// Returns true if we want to cull this subtree from the travesal.
     fn should_cull_subtree(
         &self,
         context: &mut StyleContext<E>,
         parent: E,
         parent_data: &ElementData,
     ) -> bool {
-        // See the comment on `cascade_node` for why we allow this on Gecko.
         debug_assert!(cfg!(feature = "gecko") ||
-                      parent.has_current_styles(parent_data));
+                      parent.has_current_styles_for_traversal(parent_data, context.shared.traversal_flags));
 
         // If the parent computed display:none, we don't style the subtree.
         if parent_data.styles.is_display_none() {
             debug!("Parent {:?} is display:none, culling traversal", parent);
             return true;
         }
 
         // Gecko-only XBL handling.
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -270,31 +270,36 @@ pub extern "C" fn Servo_TraverseSubtree(
     debug!("Servo_TraverseSubtree: {:?} {:?}", element, restyle_behavior);
 
     let traversal_flags = match (root_behavior, restyle_behavior) {
         (Root::Normal, Restyle::Normal) |
         (Root::Normal, Restyle::ForNewlyBoundElement) |
         (Root::Normal, Restyle::ForThrottledAnimationFlush)
             => TraversalFlags::empty(),
         (Root::UnstyledChildrenOnly, Restyle::Normal) |
-        (Root::UnstyledChildrenOnly, Restyle::ForNewlyBoundElement) |
-        (Root::UnstyledChildrenOnly, Restyle::ForThrottledAnimationFlush)
+        (Root::UnstyledChildrenOnly, Restyle::ForNewlyBoundElement)
             => UNSTYLED_CHILDREN_ONLY,
         (Root::Normal, Restyle::ForCSSRuleChanges) => FOR_CSS_RULE_CHANGES,
         (Root::Normal, Restyle::ForReconstruct) => FOR_RECONSTRUCT,
         _ => panic!("invalid combination of TraversalRootBehavior and TraversalRestyleBehavior"),
     };
 
-    let needs_animation_only_restyle = element.has_animation_only_dirty_descendants() ||
-                                       element.has_animation_restyle_hints();
-    if needs_animation_only_restyle {
-        traverse_subtree(element,
-                         raw_data,
-                         traversal_flags | ANIMATION_ONLY,
-                         unsafe { &*snapshots });
+    // It makes no sense to do an animation restyle when we're restyling
+    // newly-inserted content.
+    if !traversal_flags.contains(UNSTYLED_CHILDREN_ONLY) {
+        let needs_animation_only_restyle =
+            element.has_animation_only_dirty_descendants() ||
+            element.has_animation_restyle_hints();
+
+        if needs_animation_only_restyle {
+            traverse_subtree(element,
+                             raw_data,
+                             traversal_flags | ANIMATION_ONLY,
+                             unsafe { &*snapshots });
+        }
     }
 
     if restyle_behavior == Restyle::ForThrottledAnimationFlush {
         return element.has_animation_only_dirty_descendants() ||
                element.borrow_data().unwrap().restyle.is_restyle();
     }
 
     traverse_subtree(element,
@@ -2809,17 +2814,17 @@ pub extern "C" fn Servo_ResolveStyle(ele
     // 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");
+                  "Resolving style on {:?} without current styles: {:?}", element, data);
     data.styles.primary().clone().into()
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_ResolveStyleLazily(element: RawGeckoElementBorrowed,
                                            pseudo_type: CSSPseudoElementType,
                                            rule_inclusion: StyleRuleInclusion,
                                            snapshots: *const ServoElementSnapshotTable,