Bug 1351535 - Part 6: Handle TraversalRestyleBehavior::ForReconstruct in the Servo restyle. r=bholley
☠☠ backed out by 8f21c09df004 ☠ ☠
authorCameron McCormack <cam@mcc.id.au>
Tue, 04 Apr 2017 19:32:47 +0800
changeset 351943 b1a425c54b7905e8758e32b73ff62e2d017e44c5
parent 351942 d44f633899ce8d2d4621174464cb7e104cb9542d
child 351944 c1be168c4c6341d51497de57eda9c8086e576624
push id40349
push usercmccormack@mozilla.com
push dateSat, 08 Apr 2017 14:47:16 +0000
treeherderautoland@0410ff898157 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs1351535
milestone55.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 1351535 - Part 6: Handle TraversalRestyleBehavior::ForReconstruct in the Servo restyle. r=bholley MozReview-Commit-ID: 6vz9gHgzK87
servo/components/style/data.rs
servo/components/style/matching.rs
servo/components/style/traversal.rs
servo/ports/geckolib/glue.rs
--- a/servo/components/style/data.rs
+++ b/servo/components/style/data.rs
@@ -218,16 +218,23 @@ impl StoredRestyleHint {
     }
 
     /// Creates a restyle hint that forces the whole subtree to be restyled,
     /// including the element.
     pub fn subtree() -> Self {
         StoredRestyleHint(RESTYLE_SELF | RESTYLE_DESCENDANTS)
     }
 
+    /// Creates a restyle hint that forces the element and all its later
+    /// siblings to have their whole subtrees restyled, including the elements
+    /// themselves.
+    pub fn subtree_and_later_siblings() -> Self {
+        StoredRestyleHint(RESTYLE_SELF | RESTYLE_DESCENDANTS | RESTYLE_LATER_SIBLINGS)
+    }
+
     /// Returns true if the hint indicates that our style may be invalidated.
     pub fn has_self_invalidations(&self) -> bool {
         self.0.intersects(RestyleHint::for_self())
     }
 
     /// Returns true if the hint indicates that our sibling's style may be
     /// invalidated.
     pub fn has_sibling_invalidations(&self) -> bool {
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -552,17 +552,17 @@ trait PrivateMatchMethods: TElement {
             self.process_animations(context,
                                     &mut old_values,
                                     &mut new_values,
                                     pseudo);
         }
 
         // Accumulate restyle damage.
         if let Some(old) = old_values {
-            self.accumulate_damage(restyle.unwrap(), &old, &new_values, pseudo);
+            self.accumulate_damage(&context.shared, restyle.unwrap(), &old, &new_values, pseudo);
         }
 
         // Set the new computed values.
         if let Some((_, ref mut style)) = pseudo_style {
             style.values = Some(new_values);
         } else {
             primary_style.values = Some(new_values);
         }
@@ -672,20 +672,26 @@ trait PrivateMatchMethods: TElement {
                 &shared_context.timer,
                 &possibly_expired_animations);
         }
     }
 
     /// Computes and applies non-redundant damage.
     #[cfg(feature = "gecko")]
     fn accumulate_damage(&self,
+                         shared_context: &SharedStyleContext,
                          restyle: &mut RestyleData,
                          old_values: &Arc<ComputedValues>,
                          new_values: &Arc<ComputedValues>,
                          pseudo: Option<&PseudoElement>) {
+        // Don't accumulate damage if we're in a restyle for reconstruction.
+        if shared_context.traversal_flags.for_reconstruct() {
+            return;
+        }
+
         // If an ancestor is already getting reconstructed by Gecko's top-down
         // frame constructor, no need to apply damage.
         if restyle.damage_handled.contains(RestyleDamage::reconstruct()) {
             restyle.damage = RestyleDamage::empty();
             return;
         }
 
         // Add restyle damage, but only the bits that aren't redundant with respect
@@ -700,16 +706,17 @@ trait PrivateMatchMethods: TElement {
                 restyle.damage |= new_damage;
             }
         }
     }
 
     /// Computes and applies restyle damage unless we've already maxed it out.
     #[cfg(feature = "servo")]
     fn accumulate_damage(&self,
+                         _shared_context: &SharedStyleContext,
                          restyle: &mut RestyleData,
                          old_values: &Arc<ComputedValues>,
                          new_values: &Arc<ComputedValues>,
                          pseudo: Option<&PseudoElement>) {
         if restyle.damage != RestyleDamage::rebuild_and_reflow() {
             let d = self.compute_restyle_damage(&old_values, &new_values, pseudo);
             restyle.damage |= d;
         }
@@ -1104,17 +1111,17 @@ pub trait MatchMethods : TElement {
                 Ok(shared_style) => {
                     // Yay, cache hit. Share the style.
 
                     // Accumulate restyle damage.
                     debug_assert_eq!(data.has_styles(), data.has_restyle());
                     let old_values = data.get_styles_mut()
                                          .and_then(|s| s.primary.values.take());
                     if let Some(old) = old_values {
-                        self.accumulate_damage(data.restyle_mut(), &old,
+                        self.accumulate_damage(shared_context, data.restyle_mut(), &old,
                                                shared_style.values(), None);
                     }
 
                     // We never put elements with pseudo style into the style
                     // sharing cache, so we can just mint an ElementStyles
                     // directly here.
                     //
                     // See https://bugzilla.mozilla.org/show_bug.cgi?id=1329361
--- a/servo/components/style/traversal.rs
+++ b/servo/components/style/traversal.rs
@@ -26,35 +26,42 @@ pub struct PerLevelTraversalData {
     /// The current dom depth, if known, or `None` otherwise.
     ///
     /// This is kept with cooperation from the traversal code and the bloom
     /// filter.
     pub current_dom_depth: Option<usize>,
 }
 
 bitflags! {
-    /// Represents that target elements of the traversal.
+    /// Flags that control the traversal process.
     pub flags TraversalFlags: u8 {
         /// Traverse only unstyled children.
         const UNSTYLED_CHILDREN_ONLY = 0x01,
-        /// Traverse only elements for animation restyles
+        /// Traverse only elements for animation restyles.
         const ANIMATION_ONLY = 0x02,
+        /// Traverse without generating any change hints.
+        const FOR_RECONSTRUCT = 0x04,
     }
 }
 
 impl TraversalFlags {
     /// Returns true if the traversal is for animation-only restyles.
     pub fn for_animation_only(&self) -> bool {
         self.contains(ANIMATION_ONLY)
     }
 
     /// Returns true if the traversal is for unstyled children.
     pub fn for_unstyled_children_only(&self) -> bool {
         self.contains(UNSTYLED_CHILDREN_ONLY)
     }
+
+    /// Returns true if the traversal is for a frame reconstruction.
+    pub fn for_reconstruct(&self) -> bool {
+        self.contains(FOR_RECONSTRUCT)
+    }
 }
 
 /// This structure exists to enforce that callers invoke pre_traverse, and also
 /// to pass information from the pre-traversal into the primary traversal.
 pub struct PreTraverseToken {
     traverse: bool,
     unstyled_children_only: bool,
 }
@@ -143,16 +150,20 @@ pub trait DomTraversal<E: TElement> : Sy
     /// The traversal_flag is used in Gecko.
     /// If traversal_flag::UNSTYLED_CHILDREN_ONLY is specified, style newly-
     /// appended children without restyling the parent.
     /// If traversal_flag::ANIMATION_ONLY is specified, style only elements for
     /// animations.
     fn pre_traverse(root: E, stylist: &Stylist, traversal_flags: TraversalFlags)
                     -> PreTraverseToken
     {
+        debug_assert!(!(traversal_flags.for_reconstruct() &&
+                        traversal_flags.for_unstyled_children_only()),
+                      "must not specify FOR_RECONSTRUCT in combination with UNSTYLED_CHILDREN_ONLY");
+
         if traversal_flags.for_unstyled_children_only() {
             if root.borrow_data().map_or(true, |d| d.has_styles() && d.styles().is_display_none()) {
                 return PreTraverseToken {
                     traverse: false,
                     unstyled_children_only: false,
                 };
             }
             return PreTraverseToken {
@@ -160,22 +171,28 @@ pub trait DomTraversal<E: TElement> : Sy
                 unstyled_children_only: true,
             };
         }
 
         // Expand the snapshot, if any. This is normally handled by the parent, so
         // we need a special case for the root.
         //
         // Expanding snapshots here may create a LATER_SIBLINGS restyle hint, which
-        // we will drop on the floor. To prevent missed restyles, we assert against
-        // restyling a root with later siblings.
+        // we propagate to the next sibling element.
         if let Some(mut data) = root.mutate_data() {
             if let Some(r) = data.get_restyle_mut() {
-                debug_assert!(root.next_sibling_element().is_none());
-                let _later_siblings = r.compute_final_hint(root, stylist);
+                let later_siblings = r.compute_final_hint(root, stylist);
+                if later_siblings {
+                    if let Some(next) = root.next_sibling_element() {
+                        if let Some(mut next_data) = next.mutate_data() {
+                            let hint = StoredRestyleHint::subtree_and_later_siblings();
+                            next_data.ensure_restyle().hint.insert(&hint);
+                        }
+                    }
+                }
             }
         }
 
         PreTraverseToken {
             traverse: Self::node_needs_traversal(root.as_node(), traversal_flags),
             unstyled_children_only: false,
         }
     }
@@ -190,16 +207,20 @@ pub trait DomTraversal<E: TElement> : Sy
 
     /// Returns true if traversal is needed for the given node and subtree.
     fn node_needs_traversal(node: E::ConcreteNode, traversal_flags: TraversalFlags) -> bool {
         // Non-incremental layout visits every node.
         if is_servo_nonincremental_layout() {
             return true;
         }
 
+        if traversal_flags.for_reconstruct() {
+            return true;
+        }
+
         match node.as_element() {
             None => Self::text_node_needs_traversal(node),
             Some(el) => {
                 // If the element is native-anonymous and an ancestor frame will
                 // be reconstructed, the child and all its descendants will be
                 // destroyed. In that case, we don't need to traverse the subtree.
                 if el.is_native_anonymous() {
                     if let Some(parent) = el.parent_element() {
@@ -259,17 +280,21 @@ pub trait DomTraversal<E: TElement> : Sy
                     if !r.hint.is_empty() || r.recascade {
                         return true;
                     }
                 }
 
                 // Servo uses the post-order traversal for flow construction, so
                 // we need to traverse any element with damage so that we can perform
                 // fixup / reconstruction on our way back up the tree.
-                if cfg!(feature = "servo") &&
+                //
+                // We also need to traverse nodes with explicit damage and no other
+                // restyle data, so that this damage can be cleared.
+                if (cfg!(feature = "servo") ||
+                    traversal_flags.for_reconstruct()) &&
                    data.get_restyle().map_or(false, |r| r.damage != RestyleDamage::empty())
                 {
                     return true;
                 }
 
                 false
             },
         }
@@ -337,21 +362,25 @@ pub trait DomTraversal<E: TElement> : Sy
                                           &parent.borrow_data().unwrap(), MayLog);
         thread_local.borrow_mut().end_element(parent);
         if !should_traverse {
             return;
         }
 
         for kid in parent.as_node().children() {
             if Self::node_needs_traversal(kid, self.shared_context().traversal_flags) {
-                let el = kid.as_element();
-                if el.as_ref().and_then(|el| el.borrow_data())
-                              .map_or(false, |d| d.has_styles())
-                {
-                    unsafe { parent.set_dirty_descendants(); }
+                // If we are in a restyle for reconstruction, there is no need to
+                // perform a post-traversal, so we don't need to set the dirty
+                // descendants bit on the parent.
+                if !self.shared_context().traversal_flags.for_reconstruct() {
+                    let el = kid.as_element();
+                    if el.as_ref().and_then(|el| el.borrow_data())
+                                  .map_or(false, |d| d.has_styles()) {
+                        unsafe { parent.set_dirty_descendants(); }
+                    }
                 }
                 f(thread_local, kid);
             }
         }
     }
 
     /// Ensures the existence of the ElementData, and returns it. This can't live
     /// on TNode because of the trait-based separation between Servo's script
@@ -552,30 +581,44 @@ pub fn recalc_style_at<E, D>(traversal: 
 
         preprocess_children(traversal,
                             element,
                             propagated_hint,
                             damage_handled,
                             inherited_style_changed);
     }
 
+    // If we are in a restyle for reconstruction, drop the existing restyle
+    // data here, since we won't need to perform a post-traversal to pick up
+    // any change hints.
+    if context.shared.traversal_flags.for_reconstruct() {
+        data.clear_restyle();
+    }
+
     if context.shared.traversal_flags.for_animation_only() {
         unsafe { element.unset_animation_only_dirty_descendants(); }
     }
 
-    // Make sure the dirty descendants bit is not set for the root of a
-    // display:none subtree, even if the style didn't change (since, if
-    // the style did change, we'd have already cleared it above).
+    // There are two cases when we want to clear the dity descendants bit
+    // here after styling this element.
+    //
+    // The first case is when this element is the root of a display:none
+    // subtree, even if the style didn't change (since, if the style did
+    // change, we'd have already cleared it above).
     //
     // This keeps the tree in a valid state without requiring the DOM to
     // check display:none on the parent when inserting new children (which
     // can be moderately expensive). Instead, DOM implementations can
     // unconditionally set the dirty descendants bit on any styled parent,
     // and let the traversal sort it out.
-    if data.styles().is_display_none() {
+    //
+    // The second case is when we are in a restyle for reconstruction,
+    // where we won't need to perform a post-traversal to pick up any
+    // change hints.
+    if data.styles().is_display_none() || context.shared.traversal_flags.for_reconstruct() {
         unsafe { element.unset_dirty_descendants(); }
     }
 }
 
 fn compute_style<E, D>(_traversal: &D,
                        traversal_data: &mut PerLevelTraversalData,
                        context: &mut StyleContext<E>,
                        element: E,
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -80,17 +80,17 @@ use style::sequential;
 use style::shared_lock::{SharedRwLock, SharedRwLockReadGuard, StylesheetGuards, ToCssWithGuard, Locked};
 use style::string_cache::Atom;
 use style::stylesheets::{CssRule, CssRules, ImportRule, MediaRule, NamespaceRule};
 use style::stylesheets::{Origin, Stylesheet, StyleRule};
 use style::stylesheets::StylesheetLoader as StyleStylesheetLoader;
 use style::supports::parse_condition_or_declaration;
 use style::thread_state;
 use style::timer::Timer;
-use style::traversal::{ANIMATION_ONLY, UNSTYLED_CHILDREN_ONLY};
+use style::traversal::{ANIMATION_ONLY, FOR_RECONSTRUCT, UNSTYLED_CHILDREN_ONLY};
 use style::traversal::{resolve_style, DomTraversal, TraversalDriver, TraversalFlags};
 use style_traits::ToCss;
 use super::stylesheet_loader::StylesheetLoader;
 
 /*
  * For Gecko->Servo function calls, we need to redeclare the same signature that was declared in
  * the C header in Gecko. In order to catch accidental mismatches, we run rust-bindgen against
  * those signatures as well, giving us a second declaration of all the Servo_* functions in this
@@ -202,23 +202,30 @@ fn traverse_subtree(element: GeckoElemen
 }
 
 /// Traverses the subtree rooted at `root` for restyling.  Returns whether a
 /// Gecko post-traversal (to perform lazy frame construction, or consume any
 /// RestyleData, or drop any ElementData) is required.
 #[no_mangle]
 pub extern "C" fn Servo_TraverseSubtree(root: RawGeckoElementBorrowed,
                                         raw_data: RawServoStyleSetBorrowed,
-                                        behavior: structs::TraversalRootBehavior) -> bool {
+                                        root_behavior: structs::TraversalRootBehavior,
+                                        restyle_behavior: structs::TraversalRestyleBehavior)
+                                        -> bool {
+    use self::structs::TraversalRootBehavior as Root;
+    use self::structs::TraversalRestyleBehavior as Restyle;
+
     let element = GeckoElement(root);
     debug!("Servo_TraverseSubtree: {:?}", element);
 
-    let traversal_flags = match behavior {
-        structs::TraversalRootBehavior::UnstyledChildrenOnly => UNSTYLED_CHILDREN_ONLY,
-        _ => TraversalFlags::empty(),
+    let traversal_flags = match (root_behavior, restyle_behavior) {
+        (Root::Normal, Restyle::Normal) => TraversalFlags::empty(),
+        (Root::UnstyledChildrenOnly, Restyle::Normal) => UNSTYLED_CHILDREN_ONLY,
+        (Root::Normal, Restyle::ForReconstruct) => FOR_RECONSTRUCT,
+        _ => panic!("invalid combination of TraversalRootBehavior and TraversalRestyleBehavior"),
     };
 
     if element.has_animation_only_dirty_descendants() ||
        element.has_animation_restyle_hints() {
         traverse_subtree(element, raw_data, traversal_flags | ANIMATION_ONLY);
     }
 
     traverse_subtree(element, raw_data, traversal_flags);