style: Minor clean-ups and fixes on top of the previous patch

This makes the matching / parsing more self-contained, and I believe
it's generally easier to follow. Also addresses review comments from the
previous patch.

Differential Revision: https://phabricator.services.mozilla.com/D165861

components/selectors/matching.rs

@@ -8,7 +8,7 @@ use crate::attr::{
 };
 use crate::bloom::{BloomFilter, BLOOM_HASH_MASK};
 use crate::nth_index_cache::NthIndexCacheInner;
-use crate::parser::{AncestorHashes, Combinator, Component, LocalName, NthType};
+use crate::parser::{AncestorHashes, Combinator, Component, LocalName, NthSelectorData};
 use crate::parser::{NonTSPseudoClass, Selector, SelectorImpl, SelectorIter, SelectorList};
 use crate::tree::Element;
 use bitflags::bitflags;
@@ -800,46 +800,8 @@ where
             Some(ref scope_element) => element.opaque() == *scope_element,
             None => element.is_root(),
         },
-        Component::Nth(nth_data) => {
+        Component::Nth(ref nth_data) => {
-            if nth_data.is_function ||
+            matches_generic_nth_child(element, context.shared, nth_data)
-                (match nth_data.ty {
-                    NthType::Child => return matches_first_child(element, context.shared),
-                    NthType::LastChild => return matches_last_child(element, context.shared),
-                    NthType::OnlyChild => {
-                        return matches_first_child(element, context.shared) &&
-                            matches_last_child(element, context.shared)
-                    },
-                    NthType::OnlyOfType => {
-                        return matches_generic_nth_child(
-                            element,
-                            context.shared,
-                            nth_data.a,
-                            nth_data.b,
-                            true,
-                            false,
-                        ) && matches_generic_nth_child(
-                            element,
-                            context.shared,
-                            nth_data.a,
-                            nth_data.b,
-                            true,
-                            true,
-                        )
-                    },
-                    _ => true,
-                })
-            {
-                matches_generic_nth_child(
-                    element,
-                    context.shared,
-                    nth_data.a,
-                    nth_data.b,
-                    nth_data.ty == NthType::OfType || nth_data.ty == NthType::LastOfType,
-                    nth_data.ty == NthType::LastChild || nth_data.ty == NthType::LastOfType,
-                )
-            } else {
-                unreachable!()
-            }
         },
         Component::Is(ref list) | Component::Where(ref list) => context.shared.nest(|context| {
             for selector in &**list {
@@ -899,14 +861,10 @@ fn to_unconditional_case_sensitivity<'a, E: Element>(
     }
 }
 
-#[inline]
 fn matches_generic_nth_child<E>(
     element: &E,
     context: &mut MatchingContext<E::Impl>,
-    a: i32,
+    nth_data: &NthSelectorData,
-    b: i32,
-    is_of_type: bool,
-    is_from_end: bool,
 ) -> bool
 where
     E: Element,
@@ -915,14 +873,40 @@ where
         return false;
     }
 
+    let NthSelectorData { ty, a, b, .. } = *nth_data;
+    let is_of_type = ty.is_of_type();
+    if ty.is_only() {
+        return matches_generic_nth_child(element, context, &NthSelectorData::first(is_of_type)) &&
+            matches_generic_nth_child(element, context, &NthSelectorData::last(is_of_type));
+    }
+
+    let is_from_end = ty.is_from_end();
+
+    // It's useful to know whether this can only select the first/last element
+    // child for optimization purposes, see the `HAS_EDGE_CHILD_SELECTOR` flag.
+    let is_edge_child_selector = a == 0 && b == 1 && !is_of_type;
+
     if context.needs_selector_flags() {
-        element.apply_selector_flags(if is_from_end {
+        element.apply_selector_flags(if is_edge_child_selector {
+            ElementSelectorFlags::HAS_EDGE_CHILD_SELECTOR
+        } else if is_from_end {
             ElementSelectorFlags::HAS_SLOW_SELECTOR
         } else {
             ElementSelectorFlags::HAS_SLOW_SELECTOR_LATER_SIBLINGS
         });
     }
 
+    // :first/last-child are rather trivial to match, don't bother with the
+    // cache.
+    if is_edge_child_selector {
+        return if is_from_end {
+            element.next_sibling_element()
+        } else {
+            element.prev_sibling_element()
+        }
+        .is_none();
+    }
+
     // Grab a reference to the appropriate cache.
     let mut cache = context
         .nth_index_cache
@@ -1013,25 +997,3 @@ where
 
     index
 }
-
-#[inline]
-fn matches_first_child<E>(element: &E, context: &MatchingContext<E::Impl>) -> bool
-where
-    E: Element,
-{
-    if context.needs_selector_flags() {
-        element.apply_selector_flags(ElementSelectorFlags::HAS_EDGE_CHILD_SELECTOR);
-    }
-    element.prev_sibling_element().is_none()
-}
-
-#[inline]
-fn matches_last_child<E>(element: &E, context: &MatchingContext<E::Impl>) -> bool
-where
-    E: Element,
-{
-    if context.needs_selector_flags() {
-        element.apply_selector_flags(ElementSelectorFlags::HAS_EDGE_CHILD_SELECTOR);
-    }
-    element.next_sibling_element().is_none()
-}

components/selectors/parser.rs

@@ -1048,6 +1048,20 @@ pub enum NthType {
     OnlyOfType,
 }
 
+impl NthType {
+    pub fn is_only(self) -> bool {
+        self == Self::OnlyChild || self == Self::OnlyOfType
+    }
+
+    pub fn is_of_type(self) -> bool {
+        self == Self::OfType || self == Self::LastOfType || self == Self::OnlyOfType
+    }
+
+    pub fn is_from_end(self) -> bool {
+        self == Self::LastChild || self == Self::LastOfType
+    }
+}
+
 /// The properties that comprise an :nth- pseudoclass as of Selectors 3 (e.g.,
 /// nth-child(An+B)).
 /// https://www.w3.org/TR/selectors-3/#nth-child-pseudo
@@ -1060,6 +1074,53 @@ pub struct NthSelectorData {
     pub b: i32,
 }
 
+impl NthSelectorData {
+    /// Returns selector data for :only-{child,of-type}
+    #[inline]
+    pub const fn only(of_type: bool) -> Self {
+        Self {
+            ty: if of_type {
+                NthType::OnlyOfType
+            } else {
+                NthType::OnlyChild
+            },
+            is_function: false,
+            a: 0,
+            b: 1,
+        }
+    }
+
+    /// Returns selector data for :first-{child,of-type}
+    #[inline]
+    pub const fn first(of_type: bool) -> Self {
+        Self {
+            ty: if of_type {
+                NthType::OfType
+            } else {
+                NthType::Child
+            },
+            is_function: false,
+            a: 0,
+            b: 1,
+        }
+    }
+
+    /// Returns selector data for :last-{child,of-type}
+    #[inline]
+    pub const fn last(of_type: bool) -> Self {
+        Self {
+            ty: if of_type {
+                NthType::LastOfType
+            } else {
+                NthType::LastChild
+            },
+            is_function: false,
+            a: 0,
+            b: 1,
+        }
+    }
+}
+
 /// A CSS simple selector or combinator. We store both in the same enum for
 /// optimal packing and cache performance, see [1].
 ///
@@ -1607,28 +1668,24 @@ impl<Impl: SelectorImpl> ToCss for Component<Impl> {
                 }
                 Ok(())
             },
-            Nth(nth_data) => {
+            Nth(ref nth_data) => {
+                dest.write_str(match nth_data.ty {
+                    NthType::Child if nth_data.is_function => ":nth-child(",
+                    NthType::Child => ":first-child",
+                    NthType::LastChild if nth_data.is_function => ":nth-last-child(",
+                    NthType::LastChild => ":last-child",
+                    NthType::OfType if nth_data.is_function => ":nth-of-type(",
+                    NthType::OfType => ":first-of-type",
+                    NthType::LastOfType if nth_data.is_function => ":nth-last-of-type(",
+                    NthType::LastOfType => ":last-of-type",
+                    NthType::OnlyChild => ":only-child",
+                    NthType::OnlyOfType => ":only-of-type",
+                })?;
                 if nth_data.is_function {
-                    match nth_data.ty {
-                        NthType::Child => dest.write_str(":nth-child("),
-                        NthType::LastChild => dest.write_str(":nth-last-child("),
-                        NthType::OfType => dest.write_str(":nth-of-type("),
-                        NthType::LastOfType => dest.write_str(":nth-last-of-type("),
-                        _ => unreachable!(),
-                    }?;
                     write_affine(dest, nth_data.a, nth_data.b)?;
-                } else {
+                    dest.write_char(')')?;
-                    match nth_data.ty {
-                        NthType::Child => dest.write_str(":first-child"),
-
-                        NthType::LastChild => dest.write_str(":last-child"),
-                        NthType::OnlyChild => dest.write_str(":only-child"),
-                        NthType::OfType => dest.write_str(":first-of-type"),
-                        NthType::LastOfType => dest.write_str(":last-of-type"),
-                        NthType::OnlyOfType => dest.write_str(":only-of-type"),
-                    }?;
                 }
-                dest.write_char(')')
+                Ok(())
             },
             Is(ref list) | Where(ref list) | Negation(ref list) | Has(ref list) => {
                 match *self {
@@ -2543,16 +2600,16 @@ where
 
     if state.allows_tree_structural_pseudo_classes() {
         match_ignore_ascii_case! { &name,
-            "first-child" => return Ok(Component::Nth(NthSelectorData{ ty: NthType:: Child, is_function: false, a: 0, b: 1})),
+            "first-child" => return Ok(Component::Nth(NthSelectorData::first(/* of_type = */ false))),
-            "last-child" => return Ok(Component::Nth(NthSelectorData{ ty: NthType:: LastChild, is_function: false, a: 0, b: 1})),
+            "last-child" => return Ok(Component::Nth(NthSelectorData::last(/* of_type = */ false))),
-            "only-child" => return Ok(Component::Nth(NthSelectorData{ ty: NthType:: OnlyChild, is_function: false, a: 0, b: 1})),
+            "only-child" => return Ok(Component::Nth(NthSelectorData::only(/* of_type = */ false))),
             "root" => return Ok(Component::Root),
             "empty" => return Ok(Component::Empty),
             "scope" => return Ok(Component::Scope),
             "host" if P::parse_host(parser) => return Ok(Component::Host(None)),
-            "first-of-type" => return Ok(Component::Nth(NthSelectorData{ ty: NthType:: OfType, is_function: false, a: 0, b: 1})),
+            "first-of-type" => return Ok(Component::Nth(NthSelectorData::first(/* of_type = */ true))),
-            "last-of-type" => return Ok(Component::Nth(NthSelectorData{ ty: NthType:: LastOfType, is_function: false, a: 0, b: 1})),
+            "last-of-type" => return Ok(Component::Nth(NthSelectorData::last(/* of_type = */ true))),
-            "only-of-type" => return Ok(Component::Nth(NthSelectorData{ ty: NthType:: OnlyOfType, is_function: false, a: 0, b: 1})),
+            "only-of-type" => return Ok(Component::Nth(NthSelectorData::only(/* of_type = */ true))),
             _ => {},
         }
     }