diff --git a/components/selectors/visitor.rs b/components/selectors/visitor.rs index 07f121fe82f..51d335c6a74 100644 --- a/components/selectors/visitor.rs +++ b/components/selectors/visitor.rs @@ -37,7 +37,7 @@ pub trait SelectorVisitor { /// Visits a complex selector. /// /// Gets the combinator to the right of the selector, or `None` if the - /// selector is the leftmost one. + /// selector is the rightmost one. fn visit_complex_selector(&mut self, _: SelectorIter, _combinator_to_right: Option) diff --git a/components/style/selector_map.rs b/components/style/selector_map.rs index 497baf2f914..846d7972811 100644 --- a/components/style/selector_map.rs +++ b/components/style/selector_map.rs @@ -159,6 +159,12 @@ impl SelectorMap { |block| (block.specificity, block.source_order)); } + /// Check whether we have rules for the given id + #[inline] + pub fn has_rules_for_id(&self, id: &Atom) -> bool { + self.id_hash.get(id).is_some() + } + /// Append to `rule_list` all universal Rules (rules with selector `*|*`) in /// `self` sorted by specificity and source order. pub fn get_universal_rules(&self, diff --git a/components/style/sharing/checks.rs b/components/style/sharing/checks.rs index d8ce6565e62..581022392d6 100644 --- a/components/style/sharing/checks.rs +++ b/components/style/sharing/checks.rs @@ -6,6 +6,7 @@ //! quickly whether it's worth to share style, and whether two different //! elements can indeed share the same style. +use Atom; use bloom::StyleBloom; use context::{SelectorFlagsMap, SharedStyleContext}; use dom::TElement; @@ -123,3 +124,28 @@ pub fn revalidate(target: &mut StyleSharingTarget, for_element == for_candidate } + +/// Checks whether we might have rules for either of the two ids. +#[inline] +pub fn may_have_rules_for_ids(shared_context: &SharedStyleContext, + element_id: Option<&Atom>, + candidate_id: Option<&Atom>) -> bool +{ + // We shouldn't be called unless the ids are different. + debug_assert!(element_id.is_some() || candidate_id.is_some()); + let stylist = &shared_context.stylist; + + let may_have_rules_for_element = match element_id { + Some(id) => stylist.may_have_rules_for_id(id), + None => false + }; + + if may_have_rules_for_element { + return true; + } + + match candidate_id { + Some(id) => stylist.may_have_rules_for_id(id), + None => false + } +} diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index 5a884bef138..a6598ec6769 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -509,11 +509,6 @@ impl StyleSharingCandidateCache { return StyleSharingResult::CannotShare; } - if target.get_id().is_some() { - debug!("{:?} Cannot share style: element has id", target.element); - return StyleSharingResult::CannotShare - } - let mut should_clear_cache = false; for (i, candidate) in self.iter_mut().enumerate() { let sharing_result = @@ -632,8 +627,14 @@ impl StyleSharingCandidateCache { miss!(State) } - if target.get_id() != candidate.element.get_id() { - miss!(IdAttr) + let element_id = target.element.get_id(); + let candidate_id = candidate.element.get_id(); + if element_id != candidate_id { + // It's possible that there are no styles for either id. + if checks::may_have_rules_for_ids(shared, element_id.as_ref(), + candidate_id.as_ref()) { + miss!(IdAttr) + } } if !checks::have_same_style_attribute(target, candidate) { diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 5e8948c23fc..5df0bf2717b 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -146,6 +146,15 @@ pub struct Stylist { /// when an irrelevant element state bit changes. state_dependencies: ElementState, + /// The ids that appear in the rightmost complex selector of selectors (and + /// hence in our selector maps). Used to determine when sharing styles is + /// safe: we disallow style sharing for elements whose id matches this + /// filter, and hence might be in one of our selector maps. + /// + /// FIXME(bz): This doesn't really need to be a counting Blooom filter. + #[cfg_attr(feature = "servo", ignore_heap_size_of = "just an array")] + mapped_ids: BloomFilter, + /// Selectors that require explicit cache revalidation (i.e. which depend /// on state that is not otherwise visible to the cache, like attributes or /// tree-structural state like child index and pseudos). @@ -249,6 +258,7 @@ impl Stylist { attribute_dependencies: BloomFilter::new(), style_attribute_dependency: false, state_dependencies: ElementState::empty(), + mapped_ids: BloomFilter::new(), selectors_for_cache_revalidation: SelectorMap::new(), num_selectors: 0, num_declarations: 0, @@ -323,6 +333,7 @@ impl Stylist { self.attribute_dependencies.clear(); self.style_attribute_dependency = false; self.state_dependencies = ElementState::empty(); + self.mapped_ids.clear(); self.selectors_for_cache_revalidation = SelectorMap::new(); self.num_selectors = 0; self.num_declarations = 0; @@ -468,6 +479,9 @@ impl Stylist { style_attribute_dependency: &mut self.style_attribute_dependency, state_dependencies: &mut self.state_dependencies, }); + selector.visit(&mut MappedIdVisitor { + mapped_ids: &mut self.mapped_ids, + }); } self.rules_source_order += 1; } @@ -1069,6 +1083,13 @@ impl Stylist { debug!("push_applicable_declarations: shareable: {:?}", context.relations); } + /// Given an id, returns whether there might be any rules for that id in any + /// of our rule maps. + #[inline] + pub fn may_have_rules_for_id(&self, id: &Atom) -> bool { + self.mapped_ids.might_contain(id) + } + /// Return whether the device is dirty, that is, whether the screen size or /// media type have changed (for now). #[inline] @@ -1232,6 +1253,37 @@ impl<'a> SelectorVisitor for AttributeAndStateDependencyVisitor<'a> { } } +/// Visitor to collect ids that appear in the rightmost portion of selectors. +struct MappedIdVisitor<'a> { + mapped_ids: &'a mut BloomFilter, +} + +impl<'a> SelectorVisitor for MappedIdVisitor<'a> { + type Impl = SelectorImpl; + + /// We just want to insert all the ids we find into mapped_ids. + fn visit_simple_selector(&mut self, s: &Component) -> bool { + if let Component::ID(ref id) = *s { + self.mapped_ids.insert(id); + } + true + } + + /// We want to stop as soon as we've moved off the rightmost ComplexSelector + /// that is not a psedo-element. That can be detected by a + /// visit_complex_selector call with a combinator other than None and + /// PseudoElement. Importantly, this call happens before we visit any of + /// the simple selectors in that ComplexSelector. + fn visit_complex_selector(&mut self, + _: SelectorIter, + combinator: Option) -> bool { + match combinator { + None | Some(Combinator::PseudoElement) => true, + _ => false, + } + } +} + /// Visitor determine whether a selector requires cache revalidation. /// /// Note that we just check simple selectors and eagerly return when the first