From f1cc225e970a882aa68fa44e67931e1f040dcc8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 15 Oct 2017 00:50:28 +0200 Subject: [PATCH] style: Use left-to-right indices in the invalidator. This will make easier to create external invalidations, and also makes reasoning about the invalidator a bit easier. --- components/selectors/matching.rs | 19 +++++----- components/selectors/parser.rs | 35 +++++++++++-------- .../style/invalidation/element/collector.rs | 6 ++-- .../invalidation/element/invalidation_map.rs | 2 +- .../style/invalidation/element/invalidator.rs | 23 ++++++------ 5 files changed, 46 insertions(+), 39 deletions(-) diff --git a/components/selectors/matching.rs b/components/selectors/matching.rs index 711bec885d46..596fa8afe9f6 100644 --- a/components/selectors/matching.rs +++ b/components/selectors/matching.rs @@ -299,11 +299,10 @@ where /// Whether a compound selector matched, and whether it was the rightmost /// selector inside the complex selector. pub enum CompoundSelectorMatchingResult { + /// The selector was fully matched. + FullyMatched, /// The compound selector matched, and the next combinator offset is /// `next_combinator_offset`. - /// - /// If the next combinator offset is zero, it means that it's the rightmost - /// selector. Matched { next_combinator_offset: usize, }, /// The selector didn't match. NotMatched, @@ -325,8 +324,9 @@ pub fn matches_compound_selector( where E: Element { + debug_assert_ne!(from_offset, 0); if cfg!(debug_assertions) { - selector.combinator_at(from_offset); // This asserts. + selector.combinator_at_parse_order(from_offset - 1); // This asserts. } let mut local_context = LocalMatchingContext { @@ -334,10 +334,11 @@ where matches_hover_and_active_quirk: false, }; - for component in selector.iter_raw_parse_order_from(from_offset - 1) { + for component in selector.iter_raw_parse_order_from(from_offset) { if matches!(*component, Component::Combinator(..)) { + debug_assert_ne!(from_offset, 0, "Selector started with a combinator?"); return CompoundSelectorMatchingResult::Matched { - next_combinator_offset: from_offset - 1, + next_combinator_offset: from_offset, } } @@ -350,12 +351,10 @@ where return CompoundSelectorMatchingResult::NotMatched; } - from_offset -= 1; + from_offset += 1; } - return CompoundSelectorMatchingResult::Matched { - next_combinator_offset: 0, - } + CompoundSelectorMatchingResult::FullyMatched } /// Matches a complex selector. diff --git a/components/selectors/parser.rs b/components/selectors/parser.rs index 6b87ed96720a..093ccf366e17 100644 --- a/components/selectors/parser.rs +++ b/components/selectors/parser.rs @@ -440,12 +440,11 @@ impl Selector { } } - /// Returns the combinator at index `index` (one-indexed from the right), + /// Returns the combinator at index `index` (zero-indexed from the right), /// or panics if the component is not a combinator. - /// - /// FIXME(bholley): Use more intuitive indexing. - pub fn combinator_at(&self, index: usize) -> Combinator { - match self.0.slice[index - 1] { + #[inline] + pub fn combinator_at_match_order(&self, index: usize) -> Combinator { + match self.0.slice[index] { Component::Combinator(c) => c, ref other => { panic!("Not a combinator: {:?}, {:?}, index: {}", @@ -460,16 +459,24 @@ impl Selector { self.0.slice.iter() } + /// Returns the combinator at index `index` (zero-indexed from the left), + /// or panics if the component is not a combinator. + #[inline] + pub fn combinator_at_parse_order(&self, index: usize) -> Combinator { + match self.0.slice[self.len() - index - 1] { + Component::Combinator(c) => c, + ref other => { + panic!("Not a combinator: {:?}, {:?}, index: {}", + other, self, index) + } + } + } + /// Returns an iterator over the sequence of simple selectors and - /// combinators, in parse order (from left to right), _starting_ - /// 'offset_from_right' entries from the past-the-end sentinel on - /// the right. So "0" panics,. "1" iterates nothing, and "len" - /// iterates the entire sequence. - /// - /// FIXME(bholley): This API is rather unintuive, and should really - /// be changed to accept an offset from the left. Same for combinator_at. - pub fn iter_raw_parse_order_from(&self, offset_from_right: usize) -> Rev>> { - self.0.slice[..offset_from_right].iter().rev() + /// combinators, in parse order (from left to right), starting from + /// `offset`. + pub fn iter_raw_parse_order_from(&self, offset: usize) -> Rev>> { + self.0.slice[..self.len() - offset].iter().rev() } /// Creates a Selector from a vec of Components, specified in parse order. Used in tests. diff --git a/components/style/invalidation/element/collector.rs b/components/style/invalidation/element/collector.rs index 82d99c025dcc..b1971bfab196 100644 --- a/components/style/invalidation/element/collector.rs +++ b/components/style/invalidation/element/collector.rs @@ -463,16 +463,18 @@ where if dependency.affects_descendants() { debug_assert_ne!(dependency.selector_offset, 0); + debug_assert_ne!(dependency.selector_offset, dependency.selector.len()); debug_assert!(!dependency.affects_later_siblings()); self.descendant_invalidations.push(Invalidation::new( dependency.selector.clone(), - dependency.selector_offset, + dependency.selector.len() - dependency.selector_offset + 1, )); } else if dependency.affects_later_siblings() { debug_assert_ne!(dependency.selector_offset, 0); + debug_assert_ne!(dependency.selector_offset, dependency.selector.len()); self.sibling_invalidations.push(Invalidation::new( dependency.selector.clone(), - dependency.selector_offset, + dependency.selector.len() - dependency.selector_offset + 1, )); } } diff --git a/components/style/invalidation/element/invalidation_map.rs b/components/style/invalidation/element/invalidation_map.rs index 2e4816aad348..bcc597330f10 100644 --- a/components/style/invalidation/element/invalidation_map.rs +++ b/components/style/invalidation/element/invalidation_map.rs @@ -79,7 +79,7 @@ impl Dependency { return None; } - Some(self.selector.combinator_at(self.selector_offset)) + Some(self.selector.combinator_at_match_order(self.selector_offset - 1)) } /// Whether this dependency affects the style of the element. diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs index 8b269569af93..97edd7a42032 100644 --- a/components/style/invalidation/element/invalidator.rs +++ b/components/style/invalidation/element/invalidator.rs @@ -114,14 +114,13 @@ enum InvalidationKind { /// An `Invalidation` is a complex selector that describes which elements, /// relative to a current element we are processing, must be restyled. -/// -/// When `offset` points to the right-most compound selector in `selector`, -/// then the Invalidation `represents` the fact that the current element -/// must be restyled if the compound selector matches. Otherwise, if -/// describes which descendants (or later siblings) must be restyled. #[derive(Clone)] pub struct Invalidation { selector: Selector, + /// The offset of the selector pointing to a compound selector. + /// + /// This order is a "parse order" offset, that is, zero is the leftmost part + /// of the selector written as parsed / serialized. offset: usize, /// Whether the invalidation was already matched by any previous sibling or /// ancestor. @@ -149,7 +148,7 @@ impl Invalidation { // for the weird pseudos in . // // We should be able to do better here! - match self.selector.combinator_at(self.offset) { + match self.selector.combinator_at_parse_order(self.offset - 1) { Combinator::NextSibling | Combinator::Child => false, _ => true, @@ -157,7 +156,7 @@ impl Invalidation { } fn kind(&self) -> InvalidationKind { - if self.selector.combinator_at(self.offset).is_ancestor() { + if self.selector.combinator_at_parse_order(self.offset - 1).is_ancestor() { InvalidationKind::Descendant } else { InvalidationKind::Sibling @@ -170,7 +169,7 @@ impl fmt::Debug for Invalidation { use cssparser::ToCss; f.write_str("Invalidation(")?; - for component in self.selector.iter_raw_parse_order_from(self.offset - 1) { + for component in self.selector.iter_raw_parse_order_from(self.offset) { if matches!(*component, Component::Combinator(..)) { break; } @@ -624,14 +623,14 @@ where let mut invalidated_self = false; let mut matched = false; match matching_result { - CompoundSelectorMatchingResult::Matched { next_combinator_offset: 0 } => { + CompoundSelectorMatchingResult::FullyMatched => { debug!(" > Invalidation matched completely"); matched = true; invalidated_self = true; } CompoundSelectorMatchingResult::Matched { next_combinator_offset } => { let next_combinator = - invalidation.selector.combinator_at(next_combinator_offset); + invalidation.selector.combinator_at_parse_order(next_combinator_offset); matched = true; if matches!(next_combinator, Combinator::PseudoElement) { @@ -640,7 +639,7 @@ where // around, so there could also be state pseudo-classes. let pseudo_selector = invalidation.selector - .iter_raw_parse_order_from(next_combinator_offset - 1) + .iter_raw_parse_order_from(next_combinator_offset + 1) .skip_while(|c| matches!(**c, Component::NonTSPseudoClass(..))) .next() .unwrap(); @@ -681,7 +680,7 @@ where let next_invalidation = Invalidation { selector: invalidation.selector.clone(), - offset: next_combinator_offset, + offset: next_combinator_offset + 1, matched_by_any_previous: false, };