From 2e9da9004eb2170ad2f77261e54f6a8fb098bbeb Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 15 Aug 2017 17:07:20 -0500 Subject: [PATCH] servo: Merge #18094 - Avoid leaving stale ANCESTOR_WAS_RECONSTRUCTED bits in the tree (from bholley:ancestor_reconstruct); r=emilio https://bugzilla.mozilla.org/show_bug.cgi?id=1390179 Source-Repo: https://github.com/servo/servo Source-Revision: 99b4b7e9602ea1440d2d2ae6c32be30bd0c4f721 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : 81f873ffd15c3cf538c5450eff5530fe49cd3c44 --- servo/components/style/data.rs | 12 ++++--- servo/components/style/dom.rs | 7 ++++ servo/components/style/gecko/wrapper.rs | 7 ++++ servo/components/style/traversal.rs | 43 +++++++++++++------------ 4 files changed, 45 insertions(+), 24 deletions(-) diff --git a/servo/components/style/data.rs b/servo/components/style/data.rs index 096f09f00def..35f75b10b3dd 100644 --- a/servo/components/style/data.rs +++ b/servo/components/style/data.rs @@ -106,10 +106,14 @@ impl RestyleData { } /// Sets the flag that tells us whether we've reconstructed an ancestor. - pub fn set_reconstructed_ancestor(&mut self) { - // If it weren't for animation-only traversals, we could assert - // `!self.reconstructed_ancestor()` here. - self.flags.insert(ANCESTOR_WAS_RECONSTRUCTED); + pub fn set_reconstructed_ancestor(&mut self, reconstructed: bool) { + if reconstructed { + // If it weren't for animation-only traversals, we could assert + // `!self.reconstructed_ancestor()` here. + self.flags.insert(ANCESTOR_WAS_RECONSTRUCTED); + } else { + self.flags.remove(ANCESTOR_WAS_RECONSTRUCTED); + } } /// Mark this element as restyled, which is useful to know whether we need diff --git a/servo/components/style/dom.rs b/servo/components/style/dom.rs index 7df9c850fd42..8673a50e2033 100644 --- a/servo/components/style/dom.rs +++ b/servo/components/style/dom.rs @@ -513,6 +513,13 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone + unsafe fn unset_animation_only_dirty_descendants(&self) { } + /// Clear all bits related to dirty descendant. + /// + /// In Gecko, this corresponds to the regular dirty descendants bit, the + /// animation-only dirty descendants bit, and the lazy frame construction + /// descendants bit. + unsafe fn clear_descendants_bits(&self) { self.unset_dirty_descendants(); } + /// Returns true if this element is a visited link. /// /// Servo doesn't support visited styles yet. diff --git a/servo/components/style/gecko/wrapper.rs b/servo/components/style/gecko/wrapper.rs index 80ab02ae5185..e4ccac1a2ba4 100644 --- a/servo/components/style/gecko/wrapper.rs +++ b/servo/components/style/gecko/wrapper.rs @@ -61,6 +61,7 @@ use gecko_bindings::structs::ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SE use gecko_bindings::structs::ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO; use gecko_bindings::structs::ELEMENT_HAS_SNAPSHOT; use gecko_bindings::structs::EffectCompositor_CascadeLevel as CascadeLevel; +use gecko_bindings::structs::NODE_DESCENDANTS_NEED_FRAMES; use gecko_bindings::structs::NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE; use gecko_bindings::structs::NODE_IS_NATIVE_ANONYMOUS; use gecko_bindings::structs::nsChangeHint; @@ -1052,6 +1053,12 @@ impl<'le> TElement for GeckoElement<'le> { self.unset_flags(ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO as u32) } + unsafe fn clear_descendants_bits(&self) { + self.unset_flags(ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32 | + ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO as u32 | + NODE_DESCENDANTS_NEED_FRAMES as u32) + } + fn is_visited_link(&self) -> bool { use element_state::IN_VISITED_STATE; self.get_state().intersects(IN_VISITED_STATE) diff --git a/servo/components/style/traversal.rs b/servo/components/style/traversal.rs index 546e254adf42..37abedfc2375 100644 --- a/servo/components/style/traversal.rs +++ b/servo/components/style/traversal.rs @@ -170,6 +170,12 @@ pub trait DomTraversal : Sync { // Invalidate our style, and the one of our siblings and descendants // as needed. data.invalidate_style_if_needed(root, shared_context); + + // Make sure we don't have any stale RECONSTRUCTED_ANCESTOR bits from + // the last traversal (at a potentially-higher root). From the + // perspective of this traversal, the root cannot have reconstructed + // ancestors. + data.restyle.set_reconstructed_ancestor(false); }; let parent = root.traversal_parent(); @@ -591,18 +597,10 @@ where data.clear_restyle_flags_and_damage(); } - // Optionally clear the descendants bit for the traversal type we're in. - if flags.for_animation_only() { - if flags.contains(ClearAnimationOnlyDirtyDescendants) { - unsafe { element.unset_animation_only_dirty_descendants(); } - } - } else { - // There are two cases when we want to clear the dity descendants bit here - // after styling this element. The first case is when we were explicitly - // asked to clear the bit by the caller. - // - // The second 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, + // Optionally clear the descendants bits. + if data.styles.is_display_none() { + // When this element is the root of a display:none subtree, we want to clear + // the bits 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 @@ -610,10 +608,18 @@ where // moderately expensive). Instead, DOM implementations can unconditionally // set the dirty descendants bit on any styled parent, and let the traversal // sort it out. - if flags.contains(ClearDirtyDescendants) || - data.styles.is_display_none() { - unsafe { element.unset_dirty_descendants(); } + // + // Note that the NODE_DESCENDANTS_NEED_FRAMES bit should generally only be set + // when appending content beneath an element with a frame (i.e. not + // display:none), so clearing it here isn't strictly necessary, but good + // belt-and-suspenders. + unsafe { element.clear_descendants_bits(); } + } else if flags.for_animation_only() { + if flags.contains(ClearAnimationOnlyDirtyDescendants) { + unsafe { element.unset_animation_only_dirty_descendants(); } } + } else if flags.contains(ClearDirtyDescendants) { + unsafe { element.unset_dirty_descendants(); } } context.thread_local.end_element(element); @@ -793,9 +799,7 @@ where if let Some(ref mut child_data) = child_data { // Propagate the parent restyle hint, that may make us restyle the whole // subtree. - if reconstructed_ancestor { - child_data.restyle.set_reconstructed_ancestor(); - } + child_data.restyle.set_reconstructed_ancestor(reconstructed_ancestor); child_data.restyle.hint.insert(propagated_hint); @@ -845,7 +849,6 @@ where } unsafe { - el.unset_dirty_descendants(); - el.unset_animation_only_dirty_descendants(); + el.clear_descendants_bits(); } }