From 095265affec010d2ecb42fa633e46929820b4894 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 8 Feb 2017 12:47:45 -0800 Subject: [PATCH 1/5] Hoist some work out of the tail of compute_style into the caller. This is really part of the "recalc" rather than the "compute" work. But more to the point, it lets us early-return on the style-sharing stuff (see the next patch). --- components/style/traversal.rs | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/components/style/traversal.rs b/components/style/traversal.rs index f6e75a2efe8..4c608212653 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -428,7 +428,18 @@ pub fn recalc_style_at(traversal: &D, // Compute style for this element if necessary. if compute_self { - inherited_style_changed = compute_style(traversal, traversal_data, context, element, &mut data); + compute_style(traversal, traversal_data, context, element, &mut data); + + // If we're restyling this element to display:none, throw away all style + // data in the subtree, notify the caller to early-return. + let display_none = data.styles().is_display_none(); + if display_none { + debug!("New element style is display:none - clearing data from descendants."); + clear_descendant_data(element, &|e| unsafe { D::clear_element_data(&e) }); + } + + // FIXME(bholley): Compute this accurately from the call to CalcStyleDifference. + inherited_style_changed = true; } // Now that matching and cascading is done, clear the bits corresponding to @@ -452,7 +463,7 @@ pub fn recalc_style_at(traversal: &D, // 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 in compute_style). + // 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 @@ -464,13 +475,11 @@ pub fn recalc_style_at(traversal: &D, } } -// Computes style, returning true if the inherited styles changed for this -// element. fn compute_style(_traversal: &D, traversal_data: &mut PerLevelTraversalData, context: &mut StyleContext, element: E, - mut data: &mut AtomicRefMut) -> bool + mut data: &mut AtomicRefMut) where E: TElement, D: DomTraversal, { @@ -546,19 +555,6 @@ fn compute_style(_traversal: &D, match_results.relations); } } - - // If we're restyling this element to display:none, throw away all style data - // in the subtree, notify the caller to early-return. - let display_none = data.styles().is_display_none(); - if display_none { - debug!("New element style is display:none - clearing data from descendants."); - clear_descendant_data(element, &|e| unsafe { D::clear_element_data(&e) }); - } - - // FIXME(bholley): Compute this accurately from the call to CalcStyleDifference. - let inherited_styles_changed = true; - - inherited_styles_changed } fn preprocess_children(traversal: &D, From 1c530f927971cde1dcfa00eee1e237e7041c217e Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 8 Feb 2017 12:55:10 -0800 Subject: [PATCH 2/5] Hoist style sharing cache handling to the top of compute_style. --- components/style/traversal.rs | 112 +++++++++++++++++----------------- 1 file changed, 56 insertions(+), 56 deletions(-) diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 4c608212653..6f2e25f18d5 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -8,9 +8,9 @@ use atomic_refcell::{AtomicRefCell, AtomicRefMut}; use context::{SharedStyleContext, StyleContext, ThreadLocalStyleContext}; -use data::{ElementData, ElementStyles, RestyleKind, StoredRestyleHint}; +use data::{ElementData, ElementStyles, StoredRestyleHint}; use dom::{NodeInfo, TElement, TNode}; -use matching::{MatchMethods, StyleSharingResult}; +use matching::MatchMethods; use restyle_hints::{RESTYLE_DESCENDANTS, RESTYLE_SELF}; use selector_parser::RestyleDamage; use servo_config::opts; @@ -483,77 +483,77 @@ fn compute_style(_traversal: &D, where E: TElement, D: DomTraversal, { + use data::RestyleKind::*; + use matching::StyleSharingResult::*; + context.thread_local.statistics.elements_styled += 1; let shared_context = context.shared; + let kind = data.restyle_kind(); + + // First, try the style sharing cache. If we get a match we can skip the rest + // of the work. + if let MatchAndCascade = kind { + let sharing_result = unsafe { + let cache = &mut context.thread_local.style_sharing_candidate_cache; + element.share_style_if_possible(cache, shared_context, &mut data) + }; + if let StyleWasShared(index) = sharing_result { + context.thread_local.statistics.styles_shared += 1; + context.thread_local.style_sharing_candidate_cache.touch(index); + return; + } + } // TODO(emilio): Make cascade_input less expensive to compute in the cases // we don't need to run selector matching. - let cascade_input = match data.restyle_kind() { - RestyleKind::MatchAndCascade => { - // Check to see whether we can share a style with someone. - let sharing_result = unsafe { - element.share_style_if_possible(&mut context.thread_local.style_sharing_candidate_cache, - shared_context, - &mut data) - }; + let match_results = match kind { + MatchAndCascade => { + // Ensure the bloom filter is up to date. + let dom_depth = + context.thread_local.bloom_filter + .insert_parents_recovering(element, + traversal_data.current_dom_depth); - match sharing_result { - StyleSharingResult::StyleWasShared(index) => { - context.thread_local.statistics.styles_shared += 1; - context.thread_local.style_sharing_candidate_cache.touch(index); - None - } - StyleSharingResult::CannotShare => { - // Ensure the bloom filter is up to date. - let dom_depth = - context.thread_local.bloom_filter - .insert_parents_recovering(element, - traversal_data.current_dom_depth); + // Update the dom depth with the up-to-date dom depth. + // + // Note that this is always the same than the pre-existing depth, + // but it can change from unknown to known at this step. + traversal_data.current_dom_depth = Some(dom_depth); - // Update the dom depth with the up-to-date dom depth. - // - // Note that this is always the same than the pre-existing depth, - // but it can change from unknown to known at this step. - traversal_data.current_dom_depth = Some(dom_depth); + context.thread_local.bloom_filter.assert_complete(element); - context.thread_local.bloom_filter.assert_complete(element); + // Perform the CSS selector matching. + context.thread_local.statistics.elements_matched += 1; - // Perform the CSS selector matching. - context.thread_local.statistics.elements_matched += 1; - - Some(element.match_element(context)) - } - } + element.match_element(context) } - RestyleKind::CascadeWithReplacements(hint) => { - Some(element.cascade_with_replacements(hint, context, &mut data)) + CascadeWithReplacements(hint) => { + element.cascade_with_replacements(hint, context, &mut data) } - RestyleKind::CascadeOnly => { + CascadeOnly => { // TODO(emilio): Stop doing this work, and teach cascade_node about // the current style instead. - Some(element.match_results_from_current_style(&*data)) + element.match_results_from_current_style(&*data) } }; - if let Some(match_results) = cascade_input { - // Perform the CSS cascade. - let shareable = match_results.primary_is_shareable(); - unsafe { - element.cascade_node(context, &mut data, - element.parent_element(), - match_results.primary, - match_results.per_pseudo, - shareable); - } + // Perform the CSS cascade. + let shareable = match_results.primary_is_shareable(); + unsafe { + element.cascade_node(context, &mut data, + element.parent_element(), + match_results.primary, + match_results.per_pseudo, + shareable); + } - if shareable { - // Add ourselves to the LRU cache. - context.thread_local - .style_sharing_candidate_cache - .insert_if_possible(&element, - &data.styles().primary.values, - match_results.relations); - } + if shareable { + // Add ourselves to the LRU cache. + context.thread_local + .style_sharing_candidate_cache + .insert_if_possible(&element, + &data.styles().primary.values, + match_results.relations); } } From 5873de3fb670bdcf38af0375a217b888d92220b2 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 8 Feb 2017 15:13:21 -0800 Subject: [PATCH 3/5] Allow the ComputedValues in ComputedStyle to be null. This is necessary to start synthesizing the styles in match_element and avoid round-tripping them through the caller. --- components/script/layout_wrapper.rs | 2 +- .../script_layout_interface/wrapper_traits.rs | 14 ++++----- components/style/data.rs | 29 ++++++++++++++++--- components/style/dom.rs | 2 +- components/style/matching.rs | 18 ++++++------ components/style/stylist.rs | 2 +- components/style/traversal.rs | 4 +-- ports/geckolib/glue.rs | 14 ++++----- 8 files changed, 53 insertions(+), 32 deletions(-) diff --git a/components/script/layout_wrapper.rs b/components/script/layout_wrapper.rs index a5baec25745..43068100b16 100644 --- a/components/script/layout_wrapper.rs +++ b/components/script/layout_wrapper.rs @@ -777,7 +777,7 @@ impl<'ln> ThreadSafeLayoutNode for ServoThreadSafeLayoutNode<'ln> { debug_assert!(self.is_text_node()); let parent = self.node.parent_node().unwrap().as_element().unwrap(); let parent_data = parent.get_data().unwrap().borrow(); - parent_data.styles().primary.values.clone() + parent_data.styles().primary.values().clone() } fn debug_id(self) -> usize { diff --git a/components/script_layout_interface/wrapper_traits.rs b/components/script_layout_interface/wrapper_traits.rs index 59ad5c4128a..5930538eac2 100644 --- a/components/script_layout_interface/wrapper_traits.rs +++ b/components/script_layout_interface/wrapper_traits.rs @@ -389,7 +389,7 @@ pub trait ThreadSafeLayoutElement: Clone + Copy + Sized + Debug + fn style(&self, context: &SharedStyleContext) -> Arc { match self.get_pseudo_element_type() { PseudoElementType::Normal => self.get_style_data().unwrap().borrow() - .styles().primary.values.clone(), + .styles().primary.values().clone(), other => { // Precompute non-eagerly-cascaded pseudo-element styles if not // cached before. @@ -406,7 +406,7 @@ pub trait ThreadSafeLayoutElement: Clone + Copy + Sized + Debug + let new_style = context.stylist.precomputed_values_for_pseudo( &style_pseudo, - Some(&data.styles().primary.values), + Some(data.styles().primary.values()), &context.default_computed_values, false); data.styles_mut().pseudos @@ -424,7 +424,7 @@ pub trait ThreadSafeLayoutElement: Clone + Copy + Sized + Debug + .lazily_compute_pseudo_element_style( unsafe { &self.unsafe_get() }, &style_pseudo, - &data.styles().primary.values, + data.styles().primary.values(), &context.default_computed_values); data.styles_mut().pseudos .insert(style_pseudo.clone(), new_style.unwrap()); @@ -434,7 +434,7 @@ pub trait ThreadSafeLayoutElement: Clone + Copy + Sized + Debug + self.get_style_data().unwrap().borrow() .styles().pseudos.get(&style_pseudo) - .unwrap().values.clone() + .unwrap().values().clone() } } } @@ -445,7 +445,7 @@ pub trait ThreadSafeLayoutElement: Clone + Copy + Sized + Debug + data.styles().pseudos .get(&PseudoElement::Selection).map(|s| s) .unwrap_or(&data.styles().primary) - .values.clone() + .values().clone() } /// Returns the already resolved style of the node. @@ -460,10 +460,10 @@ pub trait ThreadSafeLayoutElement: Clone + Copy + Sized + Debug + let data = self.get_style_data().unwrap().borrow(); match self.get_pseudo_element_type() { PseudoElementType::Normal - => data.styles().primary.values.clone(), + => data.styles().primary.values().clone(), other => data.styles().pseudos - .get(&other.style_pseudo_element()).unwrap().values.clone(), + .get(&other.style_pseudo_element()).unwrap().values().clone(), } } } diff --git a/components/style/data.rs b/components/style/data.rs index c2807a2036b..a6bcc344208 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -30,8 +30,9 @@ pub struct ComputedStyle { pub rules: StrongRuleNode, /// The computed values for each property obtained by cascading the - /// matched rules. - pub values: Arc, + /// matched rules. This can only be none during a transient interval of + /// the styling algorithm, and callers can safely unwrap it. + pub values: Option>, } impl ComputedStyle { @@ -39,9 +40,29 @@ impl ComputedStyle { pub fn new(rules: StrongRuleNode, values: Arc) -> Self { ComputedStyle { rules: rules, - values: values, + values: Some(values), } } + + /// Constructs a partial ComputedStyle, whose ComputedVaues will be filled + /// in later. + pub fn new_partial(rules: StrongRuleNode) -> Self { + ComputedStyle { + rules: rules, + values: None, + } + } + + /// Returns a reference to the ComputedValues. The values can only be null during + /// the styling algorithm, so this is safe to call elsewhere. + pub fn values(&self) -> &Arc { + self.values.as_ref().unwrap() + } + + /// Mutable version of the above. + pub fn values_mut(&mut self) -> &mut Arc { + self.values.as_mut().unwrap() + } } // We manually implement Debug for ComputedStyle so that we can avoid the @@ -121,7 +142,7 @@ impl ElementStyles { /// Whether this element `display` value is `none`. pub fn is_display_none(&self) -> bool { - self.primary.values.get_box().clone_display() == display::T::none + self.primary.values().get_box().clone_display() == display::T::none } } diff --git a/components/style/dom.rs b/components/style/dom.rs index f6bcf76f68e..cf14a98cff3 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -197,7 +197,7 @@ fn fmt_with_data_and_primary_values(f: &mut fmt::Formatter, n: N) -> f let dd = el.has_dirty_descendants(); let data = el.borrow_data(); let styles = data.as_ref().and_then(|d| d.get_styles()); - let values = styles.map(|s| &s.primary.values); + let values = styles.map(|s| s.primary.values()); write!(f, "{:?} dd={} data={:?} values={:?}", el, dd, &data, values) } else { write!(f, "{:?}", n) diff --git a/components/style/matching.rs b/components/style/matching.rs index 652feba7264..08d87f27b02 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -737,9 +737,9 @@ pub trait MatchMethods : TElement { // 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 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)) + existing.map(|e| RestyleDamage::compute(e, &shared_style.values())) }; if let Some(d) = maybe_damage { data.restyle_mut().damage |= d; @@ -887,7 +887,7 @@ pub trait MatchMethods : TElement { // specific with that frame, but not wanting to flush all of // layout). debug_assert!(cfg!(feature = "gecko") || d.has_current_styles()); - &d.styles().primary.values + d.styles().primary.values() }); let mut new_styles; @@ -901,9 +901,9 @@ pub trait MatchMethods : TElement { // Update animations before the cascade. This may modify the // value of the old primary style. self.update_animations_for_cascade(&context.shared, - &mut previous.primary.values, + previous.primary.values_mut(), &mut possibly_expired_animations); - (Some(&previous.primary.values), Some(&mut previous.pseudos)) + (Some(previous.primary.values()), Some(&mut previous.pseudos)) } }; @@ -924,7 +924,7 @@ pub trait MatchMethods : TElement { let damage = self.compute_damage_and_cascade_pseudos(old_primary, old_pseudos, - &new_styles.primary.values, + &new_styles.primary.values(), &mut new_styles.pseudos, context, pseudo_rule_nodes, @@ -987,7 +987,7 @@ pub trait MatchMethods : TElement { // Update animations before the cascade. This may modify // the value of old_pseudo_style. self.update_animations_for_cascade(&context.shared, - &mut old_pseudo_style.values, + old_pseudo_style.values_mut(), possibly_expired_animations); } } @@ -996,7 +996,7 @@ pub trait MatchMethods : TElement { self.cascade_node_pseudo_element(context, Some(new_primary), maybe_old_pseudo_style.as_ref() - .map(|s| &s.values), + .map(|s| s.values()), &new_rule_node, &possibly_expired_animations, CascadeBooleans { @@ -1008,7 +1008,7 @@ pub trait MatchMethods : TElement { if damage != rebuild_and_reflow { damage = damage | match maybe_old_pseudo_style { None => rebuild_and_reflow, - Some(ref old) => self.compute_restyle_damage(Some(&old.values), + Some(ref old) => self.compute_restyle_damage(Some(old.values()), &new_pseudo_values, Some(&pseudo)), }; diff --git a/components/style/stylist.rs b/components/style/stylist.rs index d1a47011def..f5127d5b15b 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -349,7 +349,7 @@ impl Stylist { } }; self.precomputed_values_for_pseudo(&pseudo, Some(parent_style), default_style, inherit_all) - .values + .values.unwrap() } /// Computes a pseudo-element style lazily during layout. diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 6f2e25f18d5..141637b8471 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -246,7 +246,7 @@ pub trait DomTraversal : Sync { // recursively drops Servo ElementData when the XBL insertion parent of // an Element is changed. if cfg!(feature = "gecko") && thread_local.is_initial_style() && - parent_data.styles().primary.values.has_moz_binding() { + parent_data.styles().primary.values().has_moz_binding() { if log.allow() { debug!("Parent {:?} has XBL binding, deferring traversal", parent); } return false; } @@ -552,7 +552,7 @@ fn compute_style(_traversal: &D, context.thread_local .style_sharing_candidate_cache .insert_if_possible(&element, - &data.styles().primary.values, + data.styles().primary.values(), match_results.relations); } } diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 9369a83ee89..5bd12939548 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -683,7 +683,7 @@ pub extern "C" fn Servo_ComputedValues_GetForAnonymousBox(parent_style_or_null: let maybe_parent = ComputedValues::arc_from_borrowed(&parent_style_or_null); data.stylist.precomputed_values_for_pseudo(&pseudo, maybe_parent, data.default_computed_values(), false) - .values + .values.unwrap() .into_strong() } @@ -709,7 +709,7 @@ pub extern "C" fn Servo_ResolvePseudoStyle(element: RawGeckoElementBorrowed, match get_pseudo_style(element, pseudo_tag, data.styles(), doc_data) { Some(values) => values.into_strong(), - None if !is_probe => data.styles().primary.values.clone().into_strong(), + None if !is_probe => data.styles().primary.values().clone().into_strong(), None => Strong::null(), } } @@ -720,13 +720,13 @@ fn get_pseudo_style(element: GeckoElement, pseudo_tag: *mut nsIAtom, { let pseudo = PseudoElement::from_atom_unchecked(Atom::from(pseudo_tag), false); match SelectorImpl::pseudo_element_cascade_type(&pseudo) { - PseudoElementCascadeType::Eager => styles.pseudos.get(&pseudo).map(|s| s.values.clone()), + PseudoElementCascadeType::Eager => styles.pseudos.get(&pseudo).map(|s| s.values().clone()), PseudoElementCascadeType::Precomputed => unreachable!("No anonymous boxes"), PseudoElementCascadeType::Lazy => { let d = doc_data.borrow_mut(); - let base = &styles.primary.values; + let base = styles.primary.values(); d.stylist.lazily_compute_pseudo_element_style(&element, &pseudo, base, &d.default_computed_values()) - .map(|s| s.values.clone()) + .map(|s| s.values().clone()) }, } } @@ -1167,7 +1167,7 @@ pub extern "C" fn Servo_ResolveStyle(element: RawGeckoElementBorrowed, return per_doc_data.default_computed_values().clone().into_strong(); } - data.styles().primary.values.clone().into_strong() + data.styles().primary.values().clone().into_strong() } #[no_mangle] @@ -1184,7 +1184,7 @@ pub extern "C" fn Servo_ResolveStyleLazily(element: RawGeckoElementBorrowed, } else { None }; - maybe_pseudo.unwrap_or_else(|| styles.primary.values.clone()) + maybe_pseudo.unwrap_or_else(|| styles.primary.values().clone()) }; // In the common case we already have the style. Check that before setting From 0e3aeac9222262ce33d20ad0f2311e49d7dd59af Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 8 Feb 2017 15:38:35 -0800 Subject: [PATCH 4/5] Stop returning rule nodes from match_element and overhaul style calculation logic. --- components/style/data.rs | 29 +- components/style/matching.rs | 452 ++++++++++++++---------------- components/style/rule_tree/mod.rs | 13 +- components/style/traversal.rs | 45 ++- 4 files changed, 242 insertions(+), 297 deletions(-) diff --git a/components/style/data.rs b/components/style/data.rs index a6bcc344208..ddf9370418a 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -76,13 +76,6 @@ impl fmt::Debug for ComputedStyle { type PseudoStylesInner = HashMap>; -/// The rule nodes for each of the pseudo-elements of an element. -/// -/// TODO(emilio): Probably shouldn't be a `HashMap` by default, but a smaller -/// array. -pub type PseudoRuleNodes = HashMap>; - /// A set of styles for a given element's pseudo-elements. /// /// This is a map from pseudo-element to `ComputedStyle`. @@ -97,19 +90,6 @@ impl PseudoStyles { pub fn empty() -> Self { PseudoStyles(HashMap::with_hasher(Default::default())) } - - /// Gets the rules that the different pseudo-elements matched. - /// - /// FIXME(emilio): We could in theory avoid creating these when we have - /// support for just re-cascading an element. Then the input to - /// `cascade_node` could be `MatchResults` or just `UseExistingStyle`. - pub fn get_rules(&self) -> PseudoRuleNodes { - let mut rules = HashMap::with_hasher(Default::default()); - for (pseudo, style) in &self.0 { - rules.insert(pseudo.clone(), style.rules.clone()); - } - rules - } } impl Deref for PseudoStyles { @@ -467,7 +447,14 @@ impl ElementData { /// Gets a mutable reference to the element styles. Panic if the element has /// never been styled. pub fn styles_mut(&mut self) -> &mut ElementStyles { - self.styles.as_mut().expect("Caling styles_mut() on unstyled ElementData") + self.styles.as_mut().expect("Calling styles_mut() on unstyled ElementData") + } + + /// Borrows both styles and restyle mutably at the same time. + pub fn styles_and_restyle_mut(&mut self) -> (&mut ElementStyles, + Option<&mut RestyleData>) { + (self.styles.as_mut().unwrap(), + self.restyle.as_mut().map(|r| &mut **r)) } /// Sets the computed element styles. diff --git a/components/style/matching.rs b/components/style/matching.rs index 08d87f27b02..35daa395d27 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, PseudoRuleNodes, PseudoStyles}; +use data::{ComputedStyle, ElementData, ElementStyles}; 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; @@ -22,11 +22,11 @@ use rule_tree::{CascadeLevel, StrongRuleNode}; use selector_parser::{PseudoElement, RestyleDamage, SelectorImpl}; use selectors::MatchAttr; use selectors::bloom::BloomFilter; -use selectors::matching::{AFFECTED_BY_PSEUDO_ELEMENTS, AFFECTED_BY_STYLE_ATTRIBUTE}; use selectors::matching::{ElementSelectorFlags, StyleRelations}; +use selectors::matching::AFFECTED_BY_PSEUDO_ELEMENTS; use servo_config::opts; use sink::ForgetfulSink; -use std::collections::HashMap; +use std::collections::hash_map::Entry; use std::slice::IterMut; use std::sync::Arc; use stylist::ApplicableDeclarationBlock; @@ -61,22 +61,21 @@ fn create_common_style_affecting_attributes_from_element(element: & flags } -/// The results of selector matching on an element. +/// The results returned from running selector matching on an element. pub struct MatchResults { - /// The rule node reference that represents the rules matched by the - /// element. - pub primary: StrongRuleNode, /// A set of style relations (different hints about what rules matched or - /// could have matched). - pub relations: StyleRelations, - /// The results of selector-matching the pseudo-elements. - pub per_pseudo: PseudoRuleNodes, + /// could have matched). This is necessary if the style will be shared. + /// If None, the style will not be shared. + pub primary_relations: Option, + /// Whether the rule nodes changed during selector matching. + pub rule_nodes_changed: bool, } impl MatchResults { /// Returns true if the primary rule node is shareable with other nodes. pub fn primary_is_shareable(&self) -> bool { - relations_are_shareable(&self.relations) + self.primary_relations.as_ref() + .map_or(false, relations_are_shareable) } } @@ -434,7 +433,7 @@ pub enum StyleSharingResult { StyleWasShared(usize), } -/// Callers need to pass several boolean flags to cascade_node_pseudo_element. +/// Callers need to pass several boolean flags to cascade_primary_or_pseudo. /// We encapsulate them in this struct to avoid mixing them up. /// /// FIXME(pcwalton): Unify with `CascadeFlags`, perhaps? @@ -444,18 +443,12 @@ struct CascadeBooleans { } trait PrivateMatchMethods: TElement { - /// Actually cascades style for a node or a pseudo-element of a node. - /// - /// Note that animations only apply to nodes or ::before or ::after - /// pseudo-elements. - fn cascade_node_pseudo_element<'a>(&self, - context: &StyleContext, - parent_style: Option<&Arc>, - old_style: Option<&Arc>, - rule_node: &StrongRuleNode, - possibly_expired_animations: &[PropertyAnimation], - booleans: CascadeBooleans) - -> Arc { + fn cascade_internal(&self, + context: &StyleContext, + primary_style: &ComputedStyle, + pseudo_style: &mut Option<(&PseudoElement, &mut ComputedStyle)>, + booleans: &CascadeBooleans) + -> Arc { let shared_context = context.shared; let mut cascade_info = CascadeInfo::new(); let mut cascade_flags = CascadeFlags::empty(); @@ -466,53 +459,118 @@ trait PrivateMatchMethods: TElement { cascade_flags.insert(SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP) } - let this_style = match parent_style { - Some(ref parent_style) => { - cascade(shared_context.viewport_size, - rule_node, - Some(&***parent_style), - &shared_context.default_computed_values, - Some(&mut cascade_info), - shared_context.error_reporter.clone(), - cascade_flags) - } - None => { - cascade(shared_context.viewport_size, - rule_node, - None, - &shared_context.default_computed_values, - Some(&mut cascade_info), - shared_context.error_reporter.clone(), - cascade_flags) + // Grab the rule node. + let rule_node = &pseudo_style.as_ref().map_or(primary_style, |p| &*p.1).rules; + + // Grab the inherited values. + let parent_el; + let parent_data; + let inherited_values_ = if pseudo_style.is_none() { + parent_el = self.parent_element(); + parent_data = parent_el.as_ref().and_then(|x| x.borrow_data()); + let parent_values = 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") || d.has_current_styles()); + d.styles().primary.values() + }); + + // Propagate the "can be fragmented" bit. It would be nice to + // encapsulate this better. + if let Some(ref p) = parent_values { + let can_be_fragmented = + p.is_multicol() || parent_el.unwrap().as_node().can_be_fragmented(); + unsafe { self.as_node().set_can_be_fragmented(can_be_fragmented); } } + + parent_values + } else { + Some(primary_style.values()) }; + let inherited_values = inherited_values_.map(|x| &**x); + + // Invoke the cascade algorithm. + let values = + Arc::new(cascade(shared_context.viewport_size, + rule_node, + inherited_values, + &shared_context.default_computed_values, + Some(&mut cascade_info), + shared_context.error_reporter.clone(), + cascade_flags)); + cascade_info.finish(&self.as_node()); + values + } - let mut this_style = Arc::new(this_style); + /// Computes values and damage for the primary or pseudo style of an element, + /// setting them on the ElementData. + fn cascade_primary_or_pseudo<'a>(&self, + context: &StyleContext, + data: &mut ElementData, + pseudo: Option<&PseudoElement>, + possibly_expired_animations: &mut Vec, + booleans: CascadeBooleans) { + // Collect some values. + let shared_context = context.shared; + let (mut styles, restyle) = data.styles_and_restyle_mut(); + let mut primary_style = &mut styles.primary; + let pseudos = &mut styles.pseudos; + let mut pseudo_style = pseudo.map(|p| (p, pseudos.get_mut(p).unwrap())); + let mut old_values = + pseudo_style.as_mut().map_or_else(|| primary_style.values.take(), |p| p.1.values.take()); + // Compute the new values. + let mut new_values = self.cascade_internal(context, primary_style, + &mut pseudo_style, &booleans); + + // Handle animations. if booleans.animate { + if let Some(ref mut old) = old_values { + self.update_animations_for_cascade(shared_context, old, + possibly_expired_animations); + } + let new_animations_sender = &context.thread_local.new_animations_sender; let this_opaque = self.as_node().opaque(); // Trigger any present animations if necessary. animation::maybe_start_animations(&shared_context, new_animations_sender, - this_opaque, &this_style); + this_opaque, &new_values); - // Trigger transitions if necessary. This will reset `this_style` back + // Trigger transitions if necessary. This will reset `new_values` back // to its old value if it did trigger a transition. - if let Some(ref style) = old_style { + if let Some(ref values) = old_values { animation::start_transitions_if_applicable( new_animations_sender, this_opaque, self.as_node().to_unsafe(), - &**style, - &mut this_style, + &**values, + &mut new_values, &shared_context.timer, &possibly_expired_animations); } } - this_style + // Accumulate restyle damage unless we've already maxed it out. + 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; + } + } + + // 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); + } } fn update_animations_for_cascade(&self, @@ -576,9 +634,10 @@ impl PrivateMatchMethods for E {} /// The public API that elements expose for selector matching. pub trait MatchMethods : TElement { - /// Runs selector matching of this element, and returns the result. + /// Runs selector matching to (re)compute rule nodes for this element. fn match_element(&self, - context: &mut StyleContext) + context: &mut StyleContext, + data: &mut ElementData) -> MatchResults { let mut applicable_declarations = @@ -588,6 +647,7 @@ pub trait MatchMethods : TElement { let style_attribute = self.style_attribute(); let animation_rules = self.get_animation_rules(None); let mut flags = ElementSelectorFlags::empty(); + let mut rule_nodes_changed = false; // Compute the primary rule node. let mut primary_relations = @@ -599,10 +659,18 @@ pub trait MatchMethods : TElement { &mut applicable_declarations, &mut flags); let primary_rule_node = compute_rule_node(context, &mut applicable_declarations); + if !data.has_styles() { + data.set_styles(ElementStyles::new(ComputedStyle::new_partial(primary_rule_node))); + rule_nodes_changed = true; + } else if data.styles().primary.rules != primary_rule_node { + data.styles_mut().primary.rules = primary_rule_node; + rule_nodes_changed = true; + } - // Compute the pseudo rule nodes. - let mut per_pseudo: PseudoRuleNodes = HashMap::with_hasher(Default::default()); + // Compute rule nodes for eagerly-cascaded pseudo-elements. + let mut matches_different_pseudos = false; SelectorImpl::each_eagerly_cascaded_pseudo_element(|pseudo| { + let mut per_pseudo = &mut data.styles_mut().pseudos; debug_assert!(applicable_declarations.is_empty()); let pseudo_animation_rules = self.get_animation_rules(Some(&pseudo)); stylist.push_applicable_declarations(self, @@ -613,13 +681,35 @@ pub trait MatchMethods : TElement { &mut flags); if !applicable_declarations.is_empty() { - let rule_node = compute_rule_node(context, &mut applicable_declarations); - per_pseudo.insert(pseudo, rule_node); + let new_rules = compute_rule_node(context, &mut applicable_declarations); + match per_pseudo.entry(pseudo) { + Entry::Occupied(mut e) => { + if e.get().rules != new_rules { + e.get_mut().rules = new_rules; + rule_nodes_changed = true; + } + }, + Entry::Vacant(e) => { + e.insert(ComputedStyle::new_partial(new_rules)); + matches_different_pseudos = true; + } + } + } else if per_pseudo.remove(&pseudo).is_some() { + matches_different_pseudos = true; } }); + if matches_different_pseudos { + rule_nodes_changed = true; + if let Some(r) = data.get_restyle_mut() { + // Any changes to the matched pseudo-elements trigger + // reconstruction. + r.damage |= RestyleDamage::rebuild_and_reflow(); + } + } + // If we have any pseudo elements, indicate so in the primary StyleRelations. - if !per_pseudo.is_empty() { + if !data.styles().pseudos.is_empty() { primary_relations |= AFFECTED_BY_PSEUDO_ELEMENTS; } @@ -640,63 +730,45 @@ pub trait MatchMethods : TElement { } MatchResults { - primary: primary_rule_node, - relations: primary_relations, - per_pseudo: per_pseudo, + primary_relations: Some(primary_relations), + rule_nodes_changed: rule_nodes_changed, } } - /// Get the appropriate MatchResults from the current styles, to perform a - /// recascade. - /// - /// TODO(emilio): Stop using `MachResults`, use an enum, or something. - fn match_results_from_current_style(&self, data: &ElementData) -> MatchResults { - let rule_node = data.styles().primary.rules.clone(); - MatchResults { - primary: rule_node, - // FIXME(emilio): Same concern as below. - relations: StyleRelations::empty(), - // The per-pseudo rule-nodes haven't changed, but still need to be - // recascaded. - per_pseudo: data.styles().pseudos.get_rules(), - } - - } - /// Updates the rule nodes without re-running selector matching, using just - /// the rule tree. + /// the rule tree. Returns true if the rule nodes changed. fn cascade_with_replacements(&self, hint: RestyleHint, context: &StyleContext, data: &mut AtomicRefMut) - -> MatchResults { - let mut rule_node = data.styles().primary.rules.clone(); + -> bool { + let primary_rules = &mut data.styles_mut().primary.rules; + let mut rule_node_changed = false; if hint.contains(RESTYLE_STYLE_ATTRIBUTE) { let style_attribute = self.style_attribute(); - rule_node = context.shared.stylist.rule_tree + let new_node = context.shared.stylist.rule_tree .update_rule_at_level(CascadeLevel::StyleAttributeNormal, style_attribute, - rule_node); + primary_rules); + if let Some(n) = new_node { + *primary_rules = n; + rule_node_changed = true; + } - rule_node = context.shared.stylist.rule_tree + let new_node = context.shared.stylist.rule_tree .update_rule_at_level(CascadeLevel::StyleAttributeImportant, style_attribute, - rule_node); + primary_rules); + if let Some(n) = new_node { + *primary_rules = n; + rule_node_changed = true; + } } - MatchResults { - primary: rule_node, - // FIXME(emilio): This is ok, for now, we shouldn't need to fake - // this. - relations: AFFECTED_BY_STYLE_ATTRIBUTE, - // The per-pseudo rule-nodes haven't changed, but still need to be - // recomputed. - // - // TODO(emilio): We could probably optimize this quite a bit. - per_pseudo: data.styles().pseudos.get_rules(), - } + // The per-pseudo rule nodes never change in this path. + rule_node_changed } /// Attempts to share a style with another node. This method is unsafe @@ -867,165 +939,53 @@ pub trait MatchMethods : TElement { } } - /// Given the results of selector matching, run the CSS cascade and style - /// the node, potentially starting any new transitions or animations. - fn cascade_node(&self, - context: &StyleContext, - mut data: &mut AtomicRefMut, - parent: Option, - primary_rule_node: StrongRuleNode, - pseudo_rule_nodes: PseudoRuleNodes, - primary_is_shareable: bool) + /// Run the CSS cascade and compute values for the element, potentially + /// starting any new transitions or animations. + fn cascade_element(&self, + context: &StyleContext, + mut data: &mut AtomicRefMut, + primary_is_shareable: bool) { - // Get our parent's style. - let parent_data = parent.as_ref().map(|x| x.borrow_data().unwrap()); - 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") || d.has_current_styles()); - d.styles().primary.values() - }); - - let mut new_styles; let mut possibly_expired_animations = vec![]; - let damage = { - debug_assert!(!data.has_current_styles()); - let (old_primary, old_pseudos) = match data.get_styles_mut() { - None => (None, None), - Some(previous) => { - // Update animations before the cascade. This may modify the - // value of the old primary style. - self.update_animations_for_cascade(&context.shared, - previous.primary.values_mut(), - &mut possibly_expired_animations); - (Some(previous.primary.values()), Some(&mut previous.pseudos)) - } - }; + // Cascade the primary style. + self.cascade_primary_or_pseudo(context, data, None, + &mut possibly_expired_animations, + CascadeBooleans { + shareable: primary_is_shareable, + animate: true, + }); - let new_style = - self.cascade_node_pseudo_element(context, - parent_style, - old_primary, - &primary_rule_node, - &possibly_expired_animations, - CascadeBooleans { - shareable: primary_is_shareable, - animate: true, - }); + // Check whether the primary style is display:none. + let display_none = data.styles().primary.values().get_box().clone_display() == + display::T::none; - let primary = ComputedStyle::new(primary_rule_node, new_style); - new_styles = ElementStyles::new(primary); - - let damage = - self.compute_damage_and_cascade_pseudos(old_primary, - old_pseudos, - &new_styles.primary.values(), - &mut new_styles.pseudos, - context, - pseudo_rule_nodes, - &mut possibly_expired_animations); - - unsafe { - self.as_node().set_can_be_fragmented(parent.map_or(false, |p| { - p.as_node().can_be_fragmented() || - parent_style.unwrap().is_multicol() - })); + // Cascade each pseudo-element. + // + // Note that we've already set up the map of matching pseudo-elements + // in match_element (and handled the damage implications of changing + // which pseudos match), so now we can just iterate the map. This does + // mean collecting the keys, so that the borrow checker will let us pass + // the mutable |data| to the inner cascade function. + let matched_pseudos: Vec = + data.styles().pseudos.keys().cloned().collect(); + for pseudo in matched_pseudos { + // If the new primary style is display:none, we don't need pseudo + // styles, but we still need to clear any stale values. + if display_none { + data.styles_mut().pseudos.get_mut(&pseudo).unwrap().values = None; + continue; } - damage - }; - - if data.has_styles() { - data.restyle_mut().damage |= damage; + // Only ::before and ::after are animatable. + let animate = ::Impl::pseudo_is_before_or_after(&pseudo); + self.cascade_primary_or_pseudo(context, data, Some(&pseudo), + &mut possibly_expired_animations, + CascadeBooleans { + shareable: false, + animate: animate, + }); } - data.set_styles(new_styles); - } - - /// Given the old and new styling results, compute the final restyle damage. - fn compute_damage_and_cascade_pseudos( - &self, - old_primary: Option<&Arc>, - mut old_pseudos: Option<&mut PseudoStyles>, - new_primary: &Arc, - new_pseudos: &mut PseudoStyles, - context: &StyleContext, - mut pseudo_rule_nodes: PseudoRuleNodes, - possibly_expired_animations: &mut Vec) - -> RestyleDamage - { - // Compute the damage and sum up the damage related to pseudo-elements. - let mut damage = - self.compute_restyle_damage(old_primary, new_primary, None); - - // If the new style is display:none, we don't need pseudo-elements styles. - if new_primary.get_box().clone_display() == display::T::none { - return damage; - } - - let rebuild_and_reflow = RestyleDamage::rebuild_and_reflow(); - - debug_assert!(new_pseudos.is_empty()); - ::Impl::each_eagerly_cascaded_pseudo_element(|pseudo| { - let maybe_rule_node = pseudo_rule_nodes.remove(&pseudo); - - // Grab the old pseudo style for analysis. - let mut maybe_old_pseudo_style = - old_pseudos.as_mut().and_then(|x| x.remove(&pseudo)); - - if maybe_rule_node.is_some() { - let new_rule_node = maybe_rule_node.unwrap(); - - // We have declarations, so we need to cascade. Compute parameters. - let animate = ::Impl::pseudo_is_before_or_after(&pseudo); - if animate { - if let Some(ref mut old_pseudo_style) = maybe_old_pseudo_style { - // Update animations before the cascade. This may modify - // the value of old_pseudo_style. - self.update_animations_for_cascade(&context.shared, - old_pseudo_style.values_mut(), - possibly_expired_animations); - } - } - - let new_pseudo_values = - self.cascade_node_pseudo_element(context, - Some(new_primary), - maybe_old_pseudo_style.as_ref() - .map(|s| s.values()), - &new_rule_node, - &possibly_expired_animations, - CascadeBooleans { - shareable: false, - animate: animate, - }); - - // Compute restyle damage unless we've already maxed it out. - if damage != rebuild_and_reflow { - damage = damage | match maybe_old_pseudo_style { - None => rebuild_and_reflow, - Some(ref old) => self.compute_restyle_damage(Some(old.values()), - &new_pseudo_values, - Some(&pseudo)), - }; - } - - // Insert the new entry into the map. - let new_pseudo_style = ComputedStyle::new(new_rule_node, new_pseudo_values); - let existing = new_pseudos.insert(pseudo, new_pseudo_style); - debug_assert!(existing.is_none()); - } else { - if maybe_old_pseudo_style.is_some() { - damage = rebuild_and_reflow; - } - } - }); - - damage } } diff --git a/components/style/rule_tree/mod.rs b/components/style/rule_tree/mod.rs index c93ff94a841..77cf7d17697 100644 --- a/components/style/rule_tree/mod.rs +++ b/components/style/rule_tree/mod.rs @@ -183,12 +183,13 @@ impl RuleTree { /// Replaces a rule in a given level (if present) for another rule. /// - /// Returns the resulting node that represents the new path. + /// Returns the resulting node that represents the new path, or None if + /// the old path is still valid. pub fn update_rule_at_level(&self, level: CascadeLevel, pdb: Option<&Arc>>, - path: StrongRuleNode) - -> StrongRuleNode { + path: &StrongRuleNode) + -> Option { debug_assert!(level.is_unique_per_element()); // TODO(emilio): Being smarter with lifetimes we could avoid a bit of // the refcount churn. @@ -231,7 +232,7 @@ impl RuleTree { if is_here_already { debug!("Picking the fast path in rule replacement"); - return path; + return None; } } current = current.parent().unwrap().clone(); @@ -262,7 +263,7 @@ impl RuleTree { // Now the rule is in the relevant place, push the children as // necessary. - self.insert_ordered_rules_from(current, children.into_iter().rev()) + Some(self.insert_ordered_rules_from(current, children.into_iter().rev())) } } @@ -501,7 +502,7 @@ struct WeakRuleNode { } /// A strong reference to a rule node. -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub struct StrongRuleNode { ptr: *mut RuleNode, } diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 141637b8471..f49a2042ec7 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -10,7 +10,7 @@ use atomic_refcell::{AtomicRefCell, AtomicRefMut}; use context::{SharedStyleContext, StyleContext, ThreadLocalStyleContext}; use data::{ElementData, ElementStyles, StoredRestyleHint}; use dom::{NodeInfo, TElement, TNode}; -use matching::MatchMethods; +use matching::{MatchMethods, MatchResults}; use restyle_hints::{RESTYLE_DESCENDANTS, RESTYLE_SELF}; use selector_parser::RestyleDamage; use servo_config::opts; @@ -339,12 +339,9 @@ fn resolve_style_internal(context: &mut StyleContext, element: E, ensur } // Compute our style. - let match_results = element.match_element(context); - let shareable = match_results.primary_is_shareable(); - element.cascade_node(context, &mut data, element.parent_element(), - match_results.primary, - match_results.per_pseudo, - shareable); + let match_results = element.match_element(context, &mut data); + element.cascade_element(context, &mut data, + match_results.primary_is_shareable()); // Conservatively mark us as having dirty descendants, since there might // be other unstyled siblings we miss when walking straight up the parent @@ -504,8 +501,6 @@ fn compute_style(_traversal: &D, } } - // TODO(emilio): Make cascade_input less expensive to compute in the cases - // we don't need to run selector matching. let match_results = match kind { MatchAndCascade => { // Ensure the bloom filter is up to date. @@ -522,38 +517,40 @@ fn compute_style(_traversal: &D, context.thread_local.bloom_filter.assert_complete(element); - // Perform the CSS selector matching. - context.thread_local.statistics.elements_matched += 1; - element.match_element(context) + // Perform CSS selector matching. + context.thread_local.statistics.elements_matched += 1; + element.match_element(context, &mut data) } CascadeWithReplacements(hint) => { - element.cascade_with_replacements(hint, context, &mut data) + let rule_nodes_changed = + element.cascade_with_replacements(hint, context, &mut data); + MatchResults { + primary_relations: None, + rule_nodes_changed: rule_nodes_changed, + } } CascadeOnly => { - // TODO(emilio): Stop doing this work, and teach cascade_node about - // the current style instead. - element.match_results_from_current_style(&*data) + MatchResults { + primary_relations: None, + rule_nodes_changed: false, + } } }; - // Perform the CSS cascade. + // Cascade properties and compute values. let shareable = match_results.primary_is_shareable(); unsafe { - element.cascade_node(context, &mut data, - element.parent_element(), - match_results.primary, - match_results.per_pseudo, - shareable); + element.cascade_element(context, &mut data, shareable); } + // If the style is shareable, add it to the LRU cache. if shareable { - // Add ourselves to the LRU cache. context.thread_local .style_sharing_candidate_cache .insert_if_possible(&element, data.styles().primary.values(), - match_results.relations); + match_results.primary_relations.unwrap()); } } From 1f4f099efe89aacf9d1b520f41644832d560360e Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 9 Feb 2017 16:55:16 -0800 Subject: [PATCH 5/5] 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() } } }