From 7bdeeaa70253a23d64e236bb0ac59bc27d6f2f69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 11 Jan 2018 17:39:47 +0100 Subject: [PATCH] style: Fix slotted invalidation. This is a partial revert of https://github.com/servo/servo/commit/ce1d8cd232dfbc9e0a52f9467ba2bc209087ea63 If you're in a shadow tree, you may not be slotted but you still need to look at the slotted rules, since a could be a descendant of yours. Just use the same invalidation map everywhere, and remove complexity. This means that we can do some extra work while trying to gather invalidation if there are slotted rules, but I don't think it's a problem. The test is ported from https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/css/invalidation/slotted.html?l=1&rcl=58d68fdf783d7edde1c82a642e037464861f2787 Curiously, Blink fails the test as written, presumably because they don't flush styles from getComputedStyle correctly (in their test they do via updateStyleAndReturnAffectedElementCount), due to s not being in the flat tree in their implementation. Bug: 1429846 Reviewed-by: heycam MozReview-Commit-ID: 6b7BQ6bGMgd --- components/style/dom.rs | 16 +- .../invalidation/element/document_state.rs | 6 +- .../element/state_and_attributes.rs | 10 +- components/style/stylist.rs | 326 +++++++----------- 4 files changed, 139 insertions(+), 219 deletions(-) diff --git a/components/style/dom.rs b/components/style/dom.rs index 668f54e4bdd..bfa0b6078ac 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -31,7 +31,7 @@ use std::fmt; use std::fmt::Debug; use std::hash::Hash; use std::ops::Deref; -use stylist::{StyleRuleCascadeData, Stylist}; +use stylist::{CascadeData, Stylist}; use traversal_flags::TraversalFlags; /// An opaque handle to a node, which, unlike UnsafeNode, cannot be transformed @@ -772,12 +772,12 @@ pub trait TElement fn each_applicable_non_document_style_rule_data<'a, F>(&self, mut f: F) -> bool where Self: 'a, - F: FnMut(AtomicRef<'a, StyleRuleCascadeData>, QuirksMode), + F: FnMut(AtomicRef<'a, CascadeData>, QuirksMode), { let cut_off_inheritance = self.each_xbl_stylist(|stylist| { let quirks_mode = stylist.quirks_mode(); f( - AtomicRef::map(stylist, |stylist| stylist.normal_author_cascade_data()), + AtomicRef::map(stylist, |stylist| stylist.author_cascade_data()), quirks_mode, ) }); @@ -786,12 +786,10 @@ pub trait TElement while let Some(slot) = current { slot.each_xbl_stylist(|stylist| { let quirks_mode = stylist.quirks_mode(); - if stylist.slotted_author_cascade_data().is_some() { - f( - AtomicRef::map(stylist, |stylist| stylist.slotted_author_cascade_data().unwrap()), - quirks_mode, - ) - } + f( + AtomicRef::map(stylist, |stylist| stylist.author_cascade_data()), + quirks_mode, + ) }); current = slot.assigned_slot(); diff --git a/components/style/invalidation/element/document_state.rs b/components/style/invalidation/element/document_state.rs index cf7a164ebdb..77b0034531a 100644 --- a/components/style/invalidation/element/document_state.rs +++ b/components/style/invalidation/element/document_state.rs @@ -10,7 +10,7 @@ use invalidation::element::invalidator::{DescendantInvalidationLists, Invalidati use invalidation::element::invalidator::{Invalidation, InvalidationProcessor}; use invalidation::element::state_and_attributes; use selectors::matching::{MatchingContext, MatchingMode, QuirksMode, VisitedHandlingMode}; -use stylist::StyleRuleCascadeData; +use stylist::CascadeData; /// A struct holding the members necessary to invalidate document state /// selectors. @@ -34,7 +34,7 @@ pub struct DocumentStateInvalidationProcessor<'a, E: TElement> { // TODO(emilio): We might want to just run everything for every possible // binding along with the document data, or just apply the XBL stuff to the // bound subtrees. - rules: &'a StyleRuleCascadeData, + rules: &'a CascadeData, matching_context: MatchingContext<'a, E::Impl>, document_states_changed: DocumentState, } @@ -43,7 +43,7 @@ impl<'a, E: TElement> DocumentStateInvalidationProcessor<'a, E> { /// Creates a new DocumentStateInvalidationProcessor. #[inline] pub fn new( - rules: &'a StyleRuleCascadeData, + rules: &'a CascadeData, document_states_changed: DocumentState, quirks_mode: QuirksMode, ) -> Self { diff --git a/components/style/invalidation/element/state_and_attributes.rs b/components/style/invalidation/element/state_and_attributes.rs index fd93062eadd..a2dc18fb28c 100644 --- a/components/style/invalidation/element/state_and_attributes.rs +++ b/components/style/invalidation/element/state_and_attributes.rs @@ -24,7 +24,7 @@ use selectors::matching::{MatchingContext, MatchingMode, VisitedHandlingMode}; use selectors::matching::matches_selector; use smallvec::SmallVec; use stylesheets::origin::{Origin, OriginSet}; -use stylist::StyleRuleCascadeData; +use stylist::CascadeData; #[derive(Debug, PartialEq)] enum VisitedDependent { @@ -57,7 +57,7 @@ where /// changes. pub struct StateAndAttrInvalidationProcessor<'a, 'b: 'a, E: TElement> { shared_context: &'a SharedStyleContext<'b>, - shadow_rule_datas: &'a [(AtomicRef<'b, StyleRuleCascadeData>, QuirksMode)], + shadow_rule_datas: &'a [(AtomicRef<'b, CascadeData>, QuirksMode)], cut_off_inheritance: bool, element: E, data: &'a mut ElementData, @@ -68,7 +68,7 @@ impl<'a, 'b: 'a, E: TElement> StateAndAttrInvalidationProcessor<'a, 'b, E> { /// Creates a new StateAndAttrInvalidationProcessor. pub fn new( shared_context: &'a SharedStyleContext<'b>, - shadow_rule_datas: &'a [(AtomicRef<'b, StyleRuleCascadeData>, QuirksMode)], + shadow_rule_datas: &'a [(AtomicRef<'b, CascadeData>, QuirksMode)], cut_off_inheritance: bool, element: E, data: &'a mut ElementData, @@ -255,11 +255,11 @@ where OriginSet::all() }; - self.shared_context.stylist.each_normal_rule_cascade_data(|cascade_data, origin| { + for (cascade_data, origin) in self.shared_context.stylist.iter_origins() { if document_origins.contains(origin.into()) { collector.collect_dependencies_in_invalidation_map(cascade_data.invalidation_map()); } - }); + } for &(ref data, quirks_mode) in self.shadow_rule_datas { // FIXME(emilio): Replace with assert / remove when we figure diff --git a/components/style/stylist.rs b/components/style/stylist.rs index ee767010c65..8234a0c7b3b 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -188,7 +188,8 @@ struct DocumentCascadeData { per_origin: PerOrigin<()>, } -struct DocumentCascadeDataIter<'a> { +/// An iterator over the cascade data of a given document. +pub struct DocumentCascadeDataIter<'a> { iter: PerOriginIter<'a, ()>, cascade_data: &'a DocumentCascadeData, } @@ -436,6 +437,18 @@ impl Stylist { } } + /// Returns the cascade data for the author level. + #[inline] + pub fn author_cascade_data(&self) -> &CascadeData { + &self.cascade_data.author + } + + /// Iterate through all the cascade datas from the document. + #[inline] + pub fn iter_origins(&self) -> DocumentCascadeDataIter { + self.cascade_data.iter_origins() + } + /// Iterate over the extra data in origin order. #[inline] pub fn iter_extra_data_origins(&self) -> ExtraStyleDataIterator { @@ -466,30 +479,22 @@ impl Stylist { /// Returns the number of revalidation_selectors. pub fn num_revalidation_selectors(&self) -> usize { self.cascade_data.iter_origins() - .map(|(data, _)| { - data.normal_rule_data.selectors_for_cache_revalidation.len() + - data.slotted_rule_data.as_ref().map_or(0, |d| { - d.selectors_for_cache_revalidation.len() - }) - }).sum() + .map(|(data, _)| data.selectors_for_cache_revalidation.len()) + .sum() } /// Returns the number of entries in invalidation maps. pub fn num_invalidations(&self) -> usize { self.cascade_data.iter_origins() - .map(|(data, _)| { - data.normal_rule_data.invalidation_map.len() + - data.slotted_rule_data.as_ref().map_or(0, |d| d.invalidation_map.len()) - }).sum() + .map(|(data, _)| data.invalidation_map.len()) + .sum() } /// Returns whether the given DocumentState bit is relied upon by a selector /// of some rule. pub fn has_document_state_dependency(&self, state: DocumentState) -> bool { self.cascade_data.iter_origins() - .any(|(d, _)| { - d.normal_rule_data.has_document_state_dependency(state) - }) + .any(|(d, _)| d.document_state_dependencies.intersects(state)) } /// Flush the list of stylesheets if they changed, ensuring the stylist is @@ -605,24 +610,14 @@ impl Stylist { self.stylesheets.remove_stylesheet(Some(&self.device), sheet, guard) } - /// Executes `f` on each of the normal rule cascade datas in this styleset. - pub fn each_normal_rule_cascade_data<'a, F>(&'a self, mut f: F) - where - F: FnMut(&'a StyleRuleCascadeData, Origin), - { - for (data, origin) in self.cascade_data.iter_origins() { - f(&data.normal_rule_data, origin); - } - } - /// Returns whether for any of the applicable style rule data a given /// condition is true. pub fn any_applicable_rule_data(&self, element: E, mut f: F) -> bool where E: TElement, - F: FnMut(&StyleRuleCascadeData, QuirksMode) -> bool, + F: FnMut(&CascadeData, QuirksMode) -> bool, { - if f(&self.cascade_data.user_agent.cascade_data.normal_rule_data, self.quirks_mode()) { + if f(&self.cascade_data.user_agent.cascade_data, self.quirks_mode()) { return true; } @@ -636,8 +631,8 @@ impl Stylist { return maybe; } - f(&self.cascade_data.author.normal_rule_data, self.quirks_mode()) || - f(&self.cascade_data.user.normal_rule_data, self.quirks_mode()) + f(&self.cascade_data.author, self.quirks_mode()) || + f(&self.cascade_data.user, self.quirks_mode()) } /// Computes the style for a given "precomputed" pseudo-element, taking the @@ -1446,18 +1441,6 @@ impl Stylist { }) } - /// Returns the cascade data for the normal rules. - #[inline] - pub fn normal_author_cascade_data(&self) -> &StyleRuleCascadeData { - &self.cascade_data.author.normal_rule_data - } - - /// Returns the cascade data for the slotted rules in this scope, if any. - #[inline] - pub fn slotted_author_cascade_data(&self) -> Option<&StyleRuleCascadeData> { - self.cascade_data.author.slotted_rule_data.as_ref().map(|d| &**d) - } - /// Returns the registered `@keyframes` animation for the specified name. /// /// FIXME(emilio): This needs to account for the element rules. @@ -1498,7 +1481,7 @@ impl Stylist { // this in the caller by asserting that the bitvecs are same-length. let mut results = SmallBitVec::new(); for (data, _) in self.cascade_data.iter_origins() { - data.normal_rule_data.selectors_for_cache_revalidation.lookup( + data.selectors_for_cache_revalidation.lookup( element, self.quirks_mode, |selector_and_hashes| { @@ -1918,7 +1901,7 @@ impl ElementAndPseudoRules { } #[inline] - fn borrow_for_pseudo(&self, pseudo: Option<&PseudoElement>) -> Option<&SelectorMap> { + fn rules(&self, pseudo: Option<&PseudoElement>) -> Option<&SelectorMap> { match pseudo { Some(pseudo) => self.pseudos_map.get(&pseudo.canonical()).map(|p| &**p), None => Some(&self.element_map), @@ -1938,12 +1921,26 @@ impl ElementAndPseudoRules { } } -/// Cascade data generated from style rules. -#[derive(Debug)] +/// Data resulting from performing the CSS cascade that is specific to a given +/// origin. +/// +/// FIXME(emilio): Consider renaming and splitting in `CascadeData` and +/// `InvalidationData`? That'd make `clear_cascade_data()` clearer. #[cfg_attr(feature = "servo", derive(MallocSizeOf))] -pub struct StyleRuleCascadeData { - /// The actual style rules. - rules: ElementAndPseudoRules, +#[derive(Debug)] +pub struct CascadeData { + /// The data coming from normal style rules that apply to elements at this + /// cascade level. + normal_rules: ElementAndPseudoRules, + + /// The data coming from ::slotted() pseudo-element rules. + /// + /// We need to store them separately because an element needs to match + /// ::slotted() pseudo-element rules in different shadow roots. + /// + /// In particular, we need to go through all the style data in all the + /// containing style scopes starting from the closest assigned slot. + slotted_rules: Option>, /// The invalidation map for these rules. invalidation_map: InvalidationMap, @@ -1985,142 +1982,6 @@ pub struct StyleRuleCascadeData { /// tree-structural state like child index and pseudos). #[cfg_attr(feature = "servo", ignore_malloc_size_of = "Arc")] selectors_for_cache_revalidation: SelectorMap, -} - -impl StyleRuleCascadeData { - #[inline(always)] - fn insert( - &mut self, - rule: Rule, - pseudo_element: Option<&PseudoElement>, - quirks_mode: QuirksMode, - rebuild_kind: SheetRebuildKind, - ) -> Result<(), FailedAllocationError> { - if rebuild_kind.should_rebuild_invalidation() { - self.invalidation_map.note_selector(&rule.selector, quirks_mode)?; - let mut visitor = StylistSelectorVisitor { - needs_revalidation: false, - passed_rightmost_selector: false, - attribute_dependencies: &mut self.attribute_dependencies, - style_attribute_dependency: &mut self.style_attribute_dependency, - state_dependencies: &mut self.state_dependencies, - document_state_dependencies: &mut self.document_state_dependencies, - mapped_ids: &mut self.mapped_ids, - }; - - rule.selector.visit(&mut visitor); - - if visitor.needs_revalidation { - self.selectors_for_cache_revalidation.insert( - RevalidationSelectorAndHashes::new( - rule.selector.clone(), - rule.hashes.clone(), - ), - quirks_mode - )?; - } - } - - self.rules.insert(rule, pseudo_element, quirks_mode) - } - - /// Returns the invalidation map. - #[inline] - pub fn invalidation_map(&self) -> &InvalidationMap { - &self.invalidation_map - } - - #[cfg(feature = "gecko")] - fn add_size_of(&self, ops: &mut MallocSizeOfOps, sizes: &mut ServoStyleSetSizes) { - self.rules.add_size_of(ops, sizes); - sizes.mInvalidationMap += self.invalidation_map.size_of(ops); - sizes.mRevalidationSelectors += self.selectors_for_cache_revalidation.size_of(ops); - } - - fn clear_cascade_data(&mut self) { - self.rules.clear(); - } - - fn clear(&mut self) { - self.clear_cascade_data(); - self.invalidation_map.clear(); - self.attribute_dependencies.clear(); - self.style_attribute_dependency = false; - self.state_dependencies = ElementState::empty(); - self.document_state_dependencies = DocumentState::empty(); - self.mapped_ids.clear(); - self.selectors_for_cache_revalidation.clear(); - } - - /// Returns whether the given attribute might appear in an attribute - /// selector of some rule. - #[inline] - pub fn might_have_attribute_dependency( - &self, - local_name: &LocalName, - ) -> bool { - if *local_name == local_name!("style") { - return self.style_attribute_dependency - } - - self.attribute_dependencies.might_contain_hash(local_name.get_hash()) - } - - /// Returns whether the given ElementState bit is relied upon by a selector - /// of some rule. - #[inline] - pub fn has_state_dependency(&self, state: ElementState) -> bool { - self.state_dependencies.intersects(state) - } - - /// Returns whether the given DocumentState bit is relied upon by a selector - /// of some rule in the stylist. - #[inline] - fn has_document_state_dependency(&self, state: DocumentState) -> bool { - self.document_state_dependencies.intersects(state) - } -} - -impl StyleRuleCascadeData { - fn new() -> Self { - Self { - rules: ElementAndPseudoRules::default(), - invalidation_map: InvalidationMap::new(), - attribute_dependencies: NonCountingBloomFilter::new(), - style_attribute_dependency: false, - state_dependencies: ElementState::empty(), - document_state_dependencies: DocumentState::empty(), - mapped_ids: NonCountingBloomFilter::new(), - selectors_for_cache_revalidation: SelectorMap::new(), - } - } - - #[inline] - fn rules(&self, pseudo: Option<&PseudoElement>) -> Option<&SelectorMap> { - self.rules.borrow_for_pseudo(pseudo) - } -} - -/// Data resulting from performing the CSS cascade that is specific to a given -/// origin. -/// -/// FIXME(emilio): Consider renaming and splitting in `CascadeData` and -/// `InvalidationData`? That'd make `clear_cascade_data()` clearer. -#[cfg_attr(feature = "servo", derive(MallocSizeOf))] -#[derive(Debug)] -struct CascadeData { - /// The data coming from normal style rules that apply to elements at this - /// cascade level. - normal_rule_data: StyleRuleCascadeData, - - /// The data coming from ::slotted() pseudo-element rules. - /// - /// We need to store them separately because an element needs to match - /// ::slotted() pseudo-element rules in different shadow roots. - /// - /// In particular, we need to go through all the style data in all the - /// containing style scopes starting from the closest assigned slot. - slotted_rule_data: Option>, /// A map with all the animations at this `CascadeData`'s origin, indexed /// by name. @@ -2146,8 +2007,15 @@ struct CascadeData { impl CascadeData { fn new() -> Self { Self { - normal_rule_data: StyleRuleCascadeData::new(), - slotted_rule_data: None, + normal_rules: ElementAndPseudoRules::default(), + slotted_rules: None, + invalidation_map: InvalidationMap::new(), + attribute_dependencies: NonCountingBloomFilter::new(), + style_attribute_dependency: false, + state_dependencies: ElementState::empty(), + document_state_dependencies: DocumentState::empty(), + mapped_ids: NonCountingBloomFilter::new(), + selectors_for_cache_revalidation: SelectorMap::new(), animations: Default::default(), extra_data: ExtraStyleData::default(), effective_media_query_results: EffectiveMediaQueryResults::new(), @@ -2157,14 +2025,39 @@ impl CascadeData { } } + /// Returns the invalidation map. + pub fn invalidation_map(&self) -> &InvalidationMap { + &self.invalidation_map + } + + /// Returns whether the given ElementState bit is relied upon by a selector + /// of some rule. + #[inline] + pub fn has_state_dependency(&self, state: ElementState) -> bool { + self.state_dependencies.intersects(state) + } + + /// Returns whether the given attribute might appear in an attribute + /// selector of some rule. + #[inline] + pub fn might_have_attribute_dependency( + &self, + local_name: &LocalName, + ) -> bool { + if *local_name == local_name!("style") { + return self.style_attribute_dependency + } + + self.attribute_dependencies.might_contain_hash(local_name.get_hash()) + } #[inline] fn normal_rules(&self, pseudo: Option<&PseudoElement>) -> Option<&SelectorMap> { - self.normal_rule_data.rules(pseudo) + self.normal_rules.rules(pseudo) } #[inline] fn slotted_rules(&self, pseudo: Option<&PseudoElement>) -> Option<&SelectorMap> { - self.slotted_rule_data.as_ref().and_then(|d| d.rules(pseudo)) + self.slotted_rules.as_ref().and_then(|d| d.rules(pseudo)) } /// Collects all the applicable media query results into `results`. @@ -2270,19 +2163,43 @@ impl CascadeData { self.rules_source_order ); - let style_rule_cascade_data = if selector.is_slotted() { - self.slotted_rule_data.get_or_insert_with(|| { - Box::new(StyleRuleCascadeData::new()) + if rebuild_kind.should_rebuild_invalidation() { + self.invalidation_map.note_selector(&rule.selector, quirks_mode)?; + let mut visitor = StylistSelectorVisitor { + needs_revalidation: false, + passed_rightmost_selector: false, + attribute_dependencies: &mut self.attribute_dependencies, + style_attribute_dependency: &mut self.style_attribute_dependency, + state_dependencies: &mut self.state_dependencies, + document_state_dependencies: &mut self.document_state_dependencies, + mapped_ids: &mut self.mapped_ids, + }; + + rule.selector.visit(&mut visitor); + + if visitor.needs_revalidation { + self.selectors_for_cache_revalidation.insert( + RevalidationSelectorAndHashes::new( + rule.selector.clone(), + rule.hashes.clone(), + ), + quirks_mode + )?; + } + } + + let rules = if selector.is_slotted() { + self.slotted_rules.get_or_insert_with(|| { + Box::new(Default::default()) }) } else { - &mut self.normal_rule_data + &mut self.normal_rules }; - style_rule_cascade_data.insert( + rules.insert( rule, pseudo_element, quirks_mode, - rebuild_kind, )?; } self.rules_source_order += 1; @@ -2435,9 +2352,9 @@ impl CascadeData { /// Clears the cascade data, but not the invalidation data. fn clear_cascade_data(&mut self) { - self.normal_rule_data.clear_cascade_data(); - if let Some(ref mut slotted_rule_data) = self.slotted_rule_data { - slotted_rule_data.clear_cascade_data(); + self.normal_rules.clear(); + if let Some(ref mut slotted_rules) = self.slotted_rules { + slotted_rules.clear(); } self.animations.clear(); self.extra_data.clear(); @@ -2448,20 +2365,25 @@ impl CascadeData { fn clear(&mut self) { self.clear_cascade_data(); - self.normal_rule_data.clear(); - if let Some(ref mut slotted_rule_data) = self.slotted_rule_data { - slotted_rule_data.clear(); - } + self.invalidation_map.clear(); + self.attribute_dependencies.clear(); + self.style_attribute_dependency = false; + self.state_dependencies = ElementState::empty(); + self.document_state_dependencies = DocumentState::empty(); + self.mapped_ids.clear(); + self.selectors_for_cache_revalidation.clear(); self.effective_media_query_results.clear(); } /// Measures heap usage. #[cfg(feature = "gecko")] fn add_size_of(&self, ops: &mut MallocSizeOfOps, sizes: &mut ServoStyleSetSizes) { - self.normal_rule_data.add_size_of(ops, sizes); - if let Some(ref slotted_rules) = self.slotted_rule_data { + self.normal_rules.add_size_of(ops, sizes); + if let Some(ref slotted_rules) = self.slotted_rules { slotted_rules.add_size_of(ops, sizes); } + sizes.mInvalidationMap += self.invalidation_map.size_of(ops); + sizes.mRevalidationSelectors += self.selectors_for_cache_revalidation.size_of(ops); sizes.mOther += self.animations.size_of(ops); sizes.mOther += self.effective_media_query_results.size_of(ops); sizes.mOther += self.extra_data.size_of(ops);