Bug 1351535 - Part 6: Handle TraversalRestyleBehavior::ForReconstruct in the Servo restyle. r?bholley draft
authorCameron McCormack <cam@mcc.id.au>
Tue, 04 Apr 2017 19:32:47 +0800
changeset 558866 f18318047e51375be45e6ea0a6473413918e7105
parent 558865 716619146c4e1a15b5c42ad0d24ba3f4f228a580
child 558867 fa352fe30aba2422470927dbe39ec228ca6b1156
push id52975
push userbmo:cam@mcc.id.au
push dateSat, 08 Apr 2017 09:24:07 +0000
reviewersbholley
bugs1351535
milestone55.0a1
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;
         }
@@ -1094,17 +1101,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,
 }
@@ -133,16 +140,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 {
@@ -150,22 +161,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,
         }
     }
@@ -180,16 +197,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 cfg!(feature = "servo") && opts::get().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() {
@@ -249,17 +270,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
             },
         }
@@ -327,21 +352,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
@@ -542,30 +571,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);