Bug 1487582 - Use an Atom to represent Direction values in pseudo-classes. r=xidorn
authorCameron McCormack <cam@mcc.id.au>
Fri, 31 Aug 2018 15:18:59 +1000
changeset 482508 60f2fed43451da188478d6a29672c69776731e3e
parent 482507 3ec600e74178311e39182f2de58dda01534cdb5b
child 482509 d929a08b7cce07e1a902403cd71328b4a8ac448a
push id232
push userfmarier@mozilla.com
push dateWed, 05 Sep 2018 20:45:54 +0000
reviewersxidorn
bugs1487582
milestone63.0a1
Bug 1487582 - Use an Atom to represent Direction values in pseudo-classes. r=xidorn Differential Revision: https://phabricator.services.mozilla.com/D4730
servo/components/style/gecko/selector_parser.rs
servo/components/style/gecko/wrapper.rs
servo/components/style/invalidation/element/element_wrapper.rs
servo/components/style/invalidation/element/invalidation_map.rs
servo/components/style/selector_parser.rs
--- a/servo/components/style/gecko/selector_parser.rs
+++ b/servo/components/style/gecko/selector_parser.rs
@@ -49,24 +49,24 @@ macro_rules! pseudo_class_name {
                 #[doc = $css]
                 $name,
             )*
             $(
                 #[doc = $s_css]
                 $s_name(PseudoClassStringArg),
             )*
             /// The `:dir` pseudo-class.
-            Dir(Box<Direction>),
+            Dir(Direction),
             /// The non-standard `:-moz-any` pseudo-class.
             ///
             /// TODO(emilio): We disallow combinators and pseudos here, so we
             /// should use SimpleSelector instead
             MozAny(ThinBoxedSlice<Selector<SelectorImpl>>),
             /// The non-standard `:-moz-locale-dir` pseudo-class.
-            MozLocaleDir(Box<Direction>),
+            MozLocaleDir(Direction),
         }
     }
 }
 apply_non_ts_list!(pseudo_class_name);
 
 impl ToCss for NonTSPseudoClass {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result
     where
@@ -406,24 +406,20 @@ impl<'a, 'i> ::selectors::Parser<'i> for
                     $($s_css => {
                         let name = parser.expect_ident_or_string()?;
                         // convert to null terminated utf16 string
                         // since that's what Gecko deals with
                         let utf16: Vec<u16> = name.encode_utf16().chain(Some(0u16)).collect();
                         NonTSPseudoClass::$s_name(utf16.into_boxed_slice().into())
                     }, )*
                     "-moz-locale-dir" => {
-                        NonTSPseudoClass::MozLocaleDir(
-                            Box::new(Direction::parse(parser)?)
-                        )
+                        NonTSPseudoClass::MozLocaleDir(Direction::parse(parser)?)
                     },
                     "dir" => {
-                        NonTSPseudoClass::Dir(
-                            Box::new(Direction::parse(parser)?)
-                        )
+                        NonTSPseudoClass::Dir(Direction::parse(parser)?)
                     },
                     "-moz-any" => {
                         NonTSPseudoClass::MozAny(
                             selector_parser::parse_compound_selector_list(
                                 self,
                                 parser,
                             )?.into()
                         )
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -64,17 +64,17 @@ use gecko_bindings::sugar::ownership::{H
 use hash::FxHashMap;
 use logical_geometry::WritingMode;
 use media_queries::Device;
 use properties::{ComputedValues, LonghandId};
 use properties::{Importance, PropertyDeclaration, PropertyDeclarationBlock};
 use properties::animated_properties::{AnimationValue, AnimationValueMap};
 use properties::style_structs::Font;
 use rule_tree::CascadeLevel as ServoCascadeLevel;
-use selector_parser::{AttrValue, Direction, PseudoClassStringArg};
+use selector_parser::{AttrValue, HorizontalDirection, PseudoClassStringArg};
 use selectors::{Element, OpaqueElement};
 use selectors::attr::{AttrSelectorOperation, AttrSelectorOperator};
 use selectors::attr::{CaseSensitivity, NamespaceConstraint};
 use selectors::matching::{ElementSelectorFlags, MatchingContext};
 use selectors::matching::VisitedHandlingMode;
 use selectors::sink::Push;
 use servo_arc::{Arc, ArcBorrow, RawOffsetArc};
 use shared_lock::Locked;
@@ -2232,34 +2232,32 @@ impl<'le> ::selectors::Element for Gecko
             NonTSPseudoClass::MozAny(ref sels) => context.nest(|context| {
                 sels.iter()
                     .any(|s| matches_complex_selector(s.iter(), self, context, flags_setter))
             }),
             NonTSPseudoClass::Lang(ref lang_arg) => self.match_element_lang(None, lang_arg),
             NonTSPseudoClass::MozLocaleDir(ref dir) => {
                 let state_bit = DocumentState::NS_DOCUMENT_STATE_RTL_LOCALE;
                 if context.extra_data.document_state.intersects(state_bit) {
-                    // NOTE(emilio): We could still return false for
-                    // Direction::Other(..), but we don't bother.
+                    // NOTE(emilio): We could still return false for values
+                    // other than "ltr" and "rtl", but we don't bother.
                     return !context.in_negation();
                 }
 
                 let doc_is_rtl = self.document_state().contains(state_bit);
 
-                match **dir {
-                    Direction::Ltr => !doc_is_rtl,
-                    Direction::Rtl => doc_is_rtl,
-                    Direction::Other(..) => false,
+                match dir.as_horizontal_direction() {
+                    Some(HorizontalDirection::Ltr) => !doc_is_rtl,
+                    Some(HorizontalDirection::Rtl) => doc_is_rtl,
+                    None => false,
                 }
             },
-            NonTSPseudoClass::Dir(ref dir) => match **dir {
-                Direction::Ltr => self.state().intersects(ElementState::IN_LTR_STATE),
-                Direction::Rtl => self.state().intersects(ElementState::IN_RTL_STATE),
-                Direction::Other(..) => false,
-            },
+            NonTSPseudoClass::Dir(ref dir) => {
+                self.state().intersects(dir.element_state())
+            }
         }
     }
 
     fn match_pseudo_element(
         &self,
         pseudo_element: &PseudoElement,
         _context: &mut MatchingContext<Self::Impl>,
     ) -> bool {
--- a/servo/components/style/invalidation/element/element_wrapper.rs
+++ b/servo/components/style/invalidation/element/element_wrapper.rs
@@ -183,18 +183,17 @@ where
             // just add its state flags to the NonTSPseudoClass, because if we
             // added all of them there, and tested via intersects() here, we'd
             // get incorrect behavior for :not(:dir()) cases.
             //
             // FIXME(bz): How can I set this up so once Servo adds :dir()
             // support we don't forget to update this code?
             #[cfg(feature = "gecko")]
             NonTSPseudoClass::Dir(ref dir) => {
-                use invalidation::element::invalidation_map::dir_selector_to_state;
-                let selector_flag = dir_selector_to_state(dir);
+                let selector_flag = dir.element_state();
                 if selector_flag.is_empty() {
                     // :dir() with some random argument; does not match.
                     return false;
                 }
                 let state = match self.snapshot().and_then(|s| s.state()) {
                     Some(snapshot_state) => snapshot_state,
                     None => self.element.state(),
                 };
--- a/servo/components/style/invalidation/element/invalidation_map.rs
+++ b/servo/components/style/invalidation/element/invalidation_map.rs
@@ -5,39 +5,23 @@
 //! Code for invalidations due to state or attribute changes.
 
 use {Atom, LocalName, Namespace};
 use context::QuirksMode;
 use element_state::{DocumentState, ElementState};
 use fallible::FallibleVec;
 use hashglobe::FailedAllocationError;
 use selector_map::{MaybeCaseInsensitiveHashMap, SelectorMap, SelectorMapEntry};
-#[cfg(feature = "gecko")]
-use selector_parser::Direction;
 use selector_parser::SelectorImpl;
 use selectors::attr::NamespaceConstraint;
 use selectors::parser::{Combinator, Component};
 use selectors::parser::{Selector, SelectorIter, Visit};
 use selectors::visitor::SelectorVisitor;
 use smallvec::SmallVec;
 
-#[cfg(feature = "gecko")]
-/// Gets the element state relevant to the given `:dir` pseudo-class selector.
-pub fn dir_selector_to_state(dir: &Direction) -> ElementState {
-    match *dir {
-        Direction::Ltr => ElementState::IN_LTR_STATE,
-        Direction::Rtl => ElementState::IN_RTL_STATE,
-        Direction::Other(_) => {
-            // :dir(something-random) is a valid selector, but shouldn't
-            // match anything.
-            ElementState::empty()
-        },
-    }
-}
-
 /// Mapping between (partial) CompoundSelectors (and the combinator to their
 /// right) and the states and attributes they depend on.
 ///
 /// In general, for all selectors in all applicable stylesheets of the form:
 ///
 /// |a _ b _ c _ d _ e|
 ///
 /// Where:
@@ -377,17 +361,17 @@ impl<'a> SelectorVisitor for CompoundSel
             },
             Component::Class(ref class) => {
                 self.classes.push(class.clone());
             },
             Component::NonTSPseudoClass(ref pc) => {
                 self.other_attributes |= pc.is_attr_based();
                 self.state |= match *pc {
                     #[cfg(feature = "gecko")]
-                    NonTSPseudoClass::Dir(ref dir) => dir_selector_to_state(dir),
+                    NonTSPseudoClass::Dir(ref dir) => dir.element_state(),
                     _ => pc.state_flag(),
                 };
                 *self.document_state |= pc.document_state_flag();
             },
             _ => {},
         }
 
         true
--- a/servo/components/style/selector_parser.rs
+++ b/servo/components/style/selector_parser.rs
@@ -2,20 +2,23 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 //! The pseudo-classes and pseudo-elements supported by the style system.
 
 #![deny(missing_docs)]
 
 use cssparser::{Parser as CssParser, ParserInput};
+use element_state::ElementState;
 use selectors::parser::SelectorList;
 use std::fmt::{self, Debug, Write};
+use string_cache::Atom;
 use style_traits::{CssWriter, ParseError, ToCss};
 use stylesheets::{Namespaces, Origin, UrlExtraData};
+use values::serialize_atom_identifier;
 
 /// A convenient alias for the type that represents an attribute value used for
 /// selector parser implementation.
 pub type AttrValue = <SelectorImpl as ::selectors::SelectorImpl>::AttrValue;
 
 #[cfg(feature = "servo")]
 pub use servo::selector_parser::*;
 
@@ -167,46 +170,62 @@ impl<T> PerPseudoElementMap<T> {
 
     /// Get an iterator for the entries.
     pub fn iter(&self) -> ::std::slice::Iter<Option<T>> {
         self.entries.iter()
     }
 }
 
 /// Values for the :dir() pseudo class
+///
+/// "ltr" and "rtl" values are normalized to lowercase.
 #[derive(Clone, Debug, Eq, MallocSizeOf, PartialEq)]
-pub enum Direction {
-    /// left-to-right semantic directionality
+pub struct Direction(pub Atom);
+
+/// Horizontal values for the :dir() pseudo class
+#[derive(Clone, Debug, Eq, PartialEq)]
+pub enum HorizontalDirection {
+    /// :dir(ltr)
     Ltr,
-    /// right-to-left semantic directionality
+    /// :dir(rtl)
     Rtl,
-    /// Some other provided directionality value
-    ///
-    /// TODO(emilio): If we atomize we can then unbox in NonTSPseudoClass.
-    Other(Box<str>),
 }
 
 impl Direction {
     /// Parse a direction value.
     pub fn parse<'i, 't>(parser: &mut CssParser<'i, 't>) -> Result<Self, ParseError<'i>> {
         let ident = parser.expect_ident()?;
-        Ok(match_ignore_ascii_case! { &ident,
-            "rtl" => Direction::Rtl,
-            "ltr" => Direction::Ltr,
-            _ => Direction::Other(Box::from(ident.as_ref())),
-        })
+        Ok(Direction(match_ignore_ascii_case! { &ident,
+            "rtl" => atom!("rtl"),
+            "ltr" => atom!("ltr"),
+            _ => Atom::from(ident.as_ref()),
+        }))
+    }
+
+    /// Convert this Direction into a HorizontalDirection, if applicable
+    pub fn as_horizontal_direction(&self) -> Option<HorizontalDirection> {
+        if self.0 == atom!("ltr") {
+            Some(HorizontalDirection::Ltr)
+        } else if self.0 == atom!("rtl") {
+            Some(HorizontalDirection::Rtl)
+        } else {
+            None
+        }
+    }
+
+    /// Gets the element state relevant to this :dir() selector.
+    pub fn element_state(&self) -> ElementState {
+        match self.as_horizontal_direction() {
+            Some(HorizontalDirection::Ltr) => ElementState::IN_LTR_STATE,
+            Some(HorizontalDirection::Rtl) => ElementState::IN_RTL_STATE,
+            None => ElementState::empty(),
+        }
     }
 }
 
 impl ToCss for Direction {
     fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
     where
         W: Write,
     {
-        let dir_str = match *self {
-            Direction::Rtl => "rtl",
-            Direction::Ltr => "ltr",
-            // FIXME: This should be escaped as an identifier; see #19231
-            Direction::Other(ref other) => other,
-        };
-        dest.write_str(dir_str)
+        serialize_atom_identifier(&self.0, dest)
     }
 }