From 1f4f099efe89aacf9d1b520f41644832d560360e Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 9 Feb 2017 16:55:16 -0800 Subject: [PATCH] Clean up and simplify the accumulation of restyle damage. --- components/script/layout_wrapper.rs | 4 +- components/style/dom.rs | 2 +- components/style/gecko/wrapper.rs | 7 +-- components/style/matching.rs | 89 +++++++++++++---------------- 4 files changed, 44 insertions(+), 58 deletions(-) diff --git a/components/script/layout_wrapper.rs b/components/script/layout_wrapper.rs index 43068100b16..000a844bc16 100644 --- a/components/script/layout_wrapper.rs +++ b/components/script/layout_wrapper.rs @@ -393,10 +393,10 @@ impl<'le> TElement for ServoLayoutElement<'le> { #[inline] fn existing_style_for_restyle_damage<'a>(&'a self, - current_cv: Option<&'a Arc>, + current_cv: &'a Arc, _pseudo_element: Option<&PseudoElement>) -> Option<&'a Arc> { - current_cv + Some(current_cv) } fn has_dirty_descendants(&self) -> bool { diff --git a/components/style/dom.rs b/components/style/dom.rs index cf14a98cff3..7a327116746 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -276,7 +276,7 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre /// values as an argument here, but otherwise Servo would crash due to /// double borrows to return it. fn existing_style_for_restyle_damage<'a>(&'a self, - current_computed_values: Option<&'a Arc>, + current_computed_values: &'a Arc, pseudo: Option<&PseudoElement>) -> Option<&'a PreExistingComputedValues>; diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 92b41122b3b..19b8ca7a7dc 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -451,14 +451,9 @@ impl<'le> TElement for GeckoElement<'le> { } fn existing_style_for_restyle_damage<'a>(&'a self, - current_cv: Option<&'a Arc>, + _existing_values: &'a Arc, pseudo: Option<&PseudoElement>) -> Option<&'a nsStyleContext> { - if current_cv.is_none() { - // Don't bother in doing an ffi call to get null back. - return None; - } - unsafe { let atom_ptr = pseudo.map(|p| p.as_atom().as_ptr()) .unwrap_or(ptr::null_mut()); diff --git a/components/style/matching.rs b/components/style/matching.rs index 35daa395d27..2b180f47892 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -13,7 +13,7 @@ use atomic_refcell::AtomicRefMut; use cache::LRUCache; use cascade_info::CascadeInfo; use context::{SequentialTask, SharedStyleContext, StyleContext}; -use data::{ComputedStyle, ElementData, ElementStyles}; +use data::{ComputedStyle, ElementData, ElementStyles, RestyleData}; use dom::{SendElement, TElement, TNode}; use properties::{CascadeFlags, ComputedValues, SHAREABLE, SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP, cascade}; use properties::longhands::display::computed_value as display; @@ -556,13 +556,9 @@ trait PrivateMatchMethods: TElement { } } - // Accumulate restyle damage unless we've already maxed it out. + // Accumulate restyle damage. if let Some(old) = old_values { - let restyle = restyle.expect("Old values but no restyle?"); - if restyle.damage != RestyleDamage::rebuild_and_reflow() { - let d = self.compute_restyle_damage(Some(&old), &new_values, pseudo); - restyle.damage |= d; - } + self.accumulate_damage(restyle.unwrap(), &old, &new_values, pseudo); } // Set the new computed values. @@ -573,6 +569,18 @@ trait PrivateMatchMethods: TElement { } } + /// Computes and applies restyle damage unless we've already maxed it out. + fn accumulate_damage(&self, + restyle: &mut RestyleData, + old_values: &Arc, + new_values: &Arc, + pseudo: Option<&PseudoElement>) { + if restyle.damage != RestyleDamage::rebuild_and_reflow() { + let d = self.compute_restyle_damage(&old_values, &new_values, pseudo); + restyle.damage |= d; + } + } + fn update_animations_for_cascade(&self, context: &SharedStyleContext, style: &mut Arc, @@ -804,21 +812,18 @@ pub trait MatchMethods : TElement { Ok(shared_style) => { // Yay, cache hit. Share the style. - // TODO: add the display: none optimisation here too! Even - // better, factor it out/make it a bit more generic so Gecko - // can decide more easily if it knows that it's a child of - // replaced content, or similar stuff! - let maybe_damage = { - let previous = data.get_styles().map(|x| x.primary.values()); - let existing = self.existing_style_for_restyle_damage(previous, None); - existing.map(|e| RestyleDamage::compute(e, &shared_style.values())) - }; - if let Some(d) = maybe_damage { - data.restyle_mut().damage |= d; + // 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, + 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. + // 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 let styles = ElementStyles::new(shared_style); @@ -901,40 +906,26 @@ pub trait MatchMethods : TElement { /// pseudo-element, compute the restyle damage used to determine which /// kind of layout or painting operations we'll need. fn compute_restyle_damage(&self, - old_style: Option<&Arc>, - new_style: &Arc, + old_values: &Arc, + new_values: &Arc, pseudo: Option<&PseudoElement>) -> RestyleDamage { - match self.existing_style_for_restyle_damage(old_style, pseudo) { - Some(ref source) => RestyleDamage::compute(source, new_style), + match self.existing_style_for_restyle_damage(old_values, pseudo) { + Some(ref source) => RestyleDamage::compute(source, new_values), None => { - // If there's no style source, two things can happen: - // - // 1. This is not an incremental restyle (old_style is none). - // In this case we can't do too much than sending - // rebuild_and_reflow. - // - // 2. This is an incremental restyle, but the old display value - // is none, so there's no effective way for Gecko to get the - // style source (which is the style context). - // - // In this case, we could return either - // RestyleDamage::empty(), in the case both displays are - // none, or rebuild_and_reflow, otherwise. - // - if let Some(old_style) = old_style { - // FIXME(emilio): This should assert the old style is - // display: none, but we still can't get an old style - // context for other stuff that should give us a style - // context source like display: contents, so we fall on the - // safe side here. - if new_style.get_box().clone_display() == display::T::none && - old_style.get_box().clone_display() == display::T::none { - return RestyleDamage::empty(); - } + // If there's no style source, that likely means that Gecko + // couldn't find a style context. This happens with display:none + // elements, and probably a number of other edge cases that + // we don't handle well yet (like display:contents). + if new_values.get_box().clone_display() == display::T::none && + old_values.get_box().clone_display() == display::T::none { + // The style remains display:none. No need for damage. + RestyleDamage::empty() + } else { + // Something else. Be conservative for now. + RestyleDamage::rebuild_and_reflow() } - RestyleDamage::rebuild_and_reflow() } } }