Bug 1466095: Make PropertyId::parse less of a footgun. r=xidorn
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 01 Jun 2018 14:00:57 +0200
changeset 420890 23e1f479bfd60730b949bad388ee238599d555ee
parent 420889 ff4fa69beec83326b08a45b8abc0277e2972b4f4
child 420891 178ac5165152f8a0978bf0740735da03122052d5
push id34083
push userapavel@mozilla.com
push dateSat, 02 Jun 2018 23:03:25 +0000
treeherdermozilla-central@1f62ecdf59b6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersxidorn
bugs1466095
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1466095: Make PropertyId::parse less of a footgun. r=xidorn MozReview-Commit-ID: 2BmtSDPmHj9
servo/components/style/properties/declaration_block.rs
servo/components/style/properties/properties.mako.rs
servo/components/style/stylesheets/keyframes_rule.rs
servo/components/style/stylesheets/supports_rule.rs
servo/components/style/values/specified/box.rs
servo/ports/geckolib/glue.rs
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -1153,17 +1153,17 @@ impl<'a, 'b, 'i> DeclarationParser<'i> f
     type Declaration = Importance;
     type Error = StyleParseErrorKind<'i>;
 
     fn parse_value<'t>(
         &mut self,
         name: CowRcStr<'i>,
         input: &mut Parser<'i, 't>,
     ) -> Result<Importance, ParseError<'i>> {
-        let id = match PropertyId::parse(&name) {
+        let id = match PropertyId::parse(&name, self.context) {
             Ok(id) => id,
             Err(..) => {
                 return Err(input.new_custom_error(if is_non_mozilla_vendor_identifier(&name) {
                     StyleParseErrorKind::UnknownVendorProperty
                 } else {
                     StyleParseErrorKind::UnknownProperty(name)
                 }));
             }
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -1590,17 +1590,17 @@ impl PropertyId {
             PropertyId::LonghandAlias(id, _) => id,
             _ => return None,
         })
     }
 
     /// Returns a given property from the string `s`.
     ///
     /// Returns Err(()) for unknown non-custom properties.
-    pub fn parse(property_name: &str) -> Result<Self, ()> {
+    fn parse_unchecked(property_name: &str) -> Result<Self, ()> {
         // FIXME(https://github.com/rust-lang/rust/issues/33156): remove this
         // enum and use PropertyId when stable Rust allows destructors in
         // statics.
         //
         // ShorthandAlias is not used in the Servo build.
         // That's why we need to allow dead_code.
         #[allow(dead_code)]
         pub enum StaticId {
@@ -1634,23 +1634,49 @@ impl PropertyId {
             },
             Some(&StaticId::LonghandAlias(id, alias)) => {
                 PropertyId::LonghandAlias(id, alias)
             },
             Some(&StaticId::ShorthandAlias(id, alias)) => {
                 PropertyId::ShorthandAlias(id, alias)
             },
             None => {
-                return ::custom_properties::parse_name(property_name).map(|name| {
-                    PropertyId::Custom(::custom_properties::Name::from(name))
-                })
+                let name = ::custom_properties::parse_name(property_name)?;
+                PropertyId::Custom(::custom_properties::Name::from(name))
             },
         })
     }
 
+    /// Parses a property name, and returns an error if it's unknown or isn't
+    /// enabled for all content.
+    #[inline]
+    pub fn parse_enabled_for_all_content(name: &str) -> Result<Self, ()> {
+        let id = Self::parse_unchecked(name)?;
+
+        if !id.enabled_for_all_content() {
+            return Err(());
+        }
+
+        Ok(id)
+    }
+
+
+    /// Parses a property name, and returns an error if it's unknown or isn't
+    /// allowed in this context.
+    #[inline]
+    pub fn parse(name: &str, context: &ParserContext) -> Result<Self, ()> {
+        let id = Self::parse_unchecked(name)?;
+
+        if !id.allowed_in(context) {
+            return Err(());
+        }
+
+        Ok(id)
+    }
+
     /// Returns a property id from Gecko's nsCSSPropertyID.
     #[cfg(feature = "gecko")]
     #[allow(non_upper_case_globals)]
     pub fn from_nscsspropertyid(id: nsCSSPropertyID) -> Result<Self, ()> {
         use gecko_bindings::structs::*;
         match id {
             % for property in data.longhands:
                 ${property.nscsspropertyid()} => {
@@ -1710,18 +1736,17 @@ impl PropertyId {
             PropertyId::Custom(ref name) => {
                 let mut s = String::new();
                 write!(&mut s, "--{}", name).unwrap();
                 s.into()
             }
         }
     }
 
-    /// Returns the non-custom property id for this property.
-    pub fn non_custom_id(&self) -> Option<NonCustomPropertyId> {
+    fn non_custom_id(&self) -> Option<NonCustomPropertyId> {
         Some(match *self {
             PropertyId::Custom(_) => return None,
             PropertyId::Shorthand(shorthand_id) => shorthand_id.into(),
             PropertyId::Longhand(longhand_id) => longhand_id.into(),
             PropertyId::ShorthandAlias(_, alias_id) => alias_id.into(),
             PropertyId::LonghandAlias(_, alias_id) => alias_id.into(),
         })
     }
@@ -1746,18 +1771,17 @@ impl PropertyId {
             // Custom properties are allowed everywhere
             None => return true,
             Some(id) => id,
         };
 
         id.enabled_for_all_content()
     }
 
-    /// Returns whether the property is allowed in a given context.
-    pub fn allowed_in(&self, context: &ParserContext) -> bool {
+    fn allowed_in(&self, context: &ParserContext) -> bool {
         let id = match self.non_custom_id() {
             // Custom properties are allowed everywhere
             None => return true,
             Some(id) => id,
         };
         id.allowed_in(context)
     }
 
@@ -1969,22 +1993,17 @@ impl PropertyDeclaration {
     pub fn parse_into<'i, 't>(
         declarations: &mut SourcePropertyDeclaration,
         id: PropertyId,
         name: CowRcStr<'i>,
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<(), ParseError<'i>> {
         assert!(declarations.is_empty());
-
-        if !id.allowed_in(context) {
-            return Err(input.new_custom_error(
-                StyleParseErrorKind::UnknownProperty(name)
-            ));
-        }
+        debug_assert!(id.allowed_in(context), "{:?}", id);
 
         let start = input.state();
         match id {
             PropertyId::Custom(property_name) => {
                 // FIXME: fully implement https://github.com/w3c/csswg-drafts/issues/774
                 // before adding skip_whitespace here.
                 // This probably affects some test results.
                 let value = match input.try(|i| CSSWideKeyword::parse(i)) {
--- a/servo/components/style/stylesheets/keyframes_rule.rs
+++ b/servo/components/style/stylesheets/keyframes_rule.rs
@@ -615,19 +615,22 @@ impl<'a, 'b, 'i> DeclarationParser<'i> f
     type Declaration = ();
     type Error = StyleParseErrorKind<'i>;
 
     fn parse_value<'t>(
         &mut self,
         name: CowRcStr<'i>,
         input: &mut Parser<'i, 't>,
     ) -> Result<(), ParseError<'i>> {
-        let id = PropertyId::parse(&name).map_err(|()| {
-            input.new_custom_error(StyleParseErrorKind::UnknownProperty(name.clone()))
-        })?;
+        let id = match PropertyId::parse(&name, self.context) {
+            Ok(id) => id,
+            Err(()) => return Err(input.new_custom_error(
+                StyleParseErrorKind::UnknownProperty(name.clone())
+            )),
+        };
 
         // TODO(emilio): Shouldn't this use parse_entirely?
         PropertyDeclaration::parse_into(self.declarations, id, name, self.context, input)?;
 
         // In case there is still unparsed text in the declaration, we should
         // roll back.
         input.expect_exhausted()?;
 
--- a/servo/components/style/stylesheets/supports_rule.rs
+++ b/servo/components/style/stylesheets/supports_rule.rs
@@ -310,22 +310,22 @@ impl Declaration {
     /// Determine if a declaration parses
     ///
     /// <https://drafts.csswg.org/css-conditional-3/#support-definition>
     pub fn eval(&self, context: &ParserContext) -> bool {
         debug_assert_eq!(context.rule_type(), CssRuleType::Style);
 
         let mut input = ParserInput::new(&self.0);
         let mut input = Parser::new(&mut input);
-        input
-            .parse_entirely(|input| -> Result<(), CssParseError<()>> {
+        input.parse_entirely(|input| -> Result<(), CssParseError<()>> {
                 let prop = input.expect_ident_cloned().unwrap();
                 input.expect_colon().unwrap();
 
-                let id = PropertyId::parse(&prop).map_err(|_| input.new_custom_error(()))?;
+                let id = PropertyId::parse(&prop, context)
+                    .map_err(|_| input.new_custom_error(()))?;
 
                 let mut declarations = SourcePropertyDeclaration::new();
                 input.parse_until_before(Delimiter::Bang, |input| {
                     PropertyDeclaration::parse_into(&mut declarations, id, prop, &context, input)
                         .map_err(|_| input.new_custom_error(()))
                 })?;
                 let _ = input.try(parse_important);
                 Ok(())
--- a/servo/components/style/values/specified/box.rs
+++ b/servo/components/style/values/specified/box.rs
@@ -439,25 +439,21 @@ fn change_bits_for_longhand(longhand: Lo
     }
     if property_flags.contains(PropertyFlags::ABSPOS_CB) {
         flags |= WillChangeBits::ABSPOS_CB;
     }
     flags
 }
 
 fn change_bits_for_maybe_property(ident: &str, context: &ParserContext) -> WillChangeBits {
-    let id = match PropertyId::parse(ident) {
+    let id = match PropertyId::parse(ident, context) {
         Ok(id) => id,
         Err(..) => return WillChangeBits::empty(),
     };
 
-    if !id.allowed_in(context) {
-        return WillChangeBits::empty();
-    }
-
     match id.as_shorthand() {
         Ok(shorthand) => {
             shorthand.longhands().fold(WillChangeBits::empty(), |flags, p| {
                 flags | change_bits_for_longhand(p)
             })
         }
         Err(PropertyDeclarationId::Longhand(longhand)) => change_bits_for_longhand(longhand),
         Err(PropertyDeclarationId::Custom(..)) => WillChangeBits::empty(),
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -901,30 +901,22 @@ pub extern "C" fn Servo_ComputedValues_E
         Some(v) => Arc::new(v).into_strong(),
         None => Strong::null(),
     }
 }
 
 macro_rules! parse_enabled_property_name {
     ($prop_name:ident, $found:ident, $default:expr) => {{
         let prop_name = $prop_name.as_ref().unwrap().as_str_unchecked();
-        // XXX This can be simplified once Option::filter is stable.
-        let prop_id = PropertyId::parse(prop_name).ok().and_then(|p| {
-            if p.enabled_for_all_content() {
-                Some(p)
-            } else {
-                None
-            }
-        });
-        match prop_id {
-            Some(p) => {
+        match PropertyId::parse_enabled_for_all_content(prop_name) {
+            Ok(p) => {
                 *$found = true;
                 p
             }
-            None => {
+            Err(..) => {
                 *$found = false;
                 return $default;
             }
         }
     }}
 }
 
 #[no_mangle]
@@ -936,17 +928,17 @@ pub unsafe extern "C" fn Servo_Property_
     prop_id.is_shorthand()
 }
 
 #[no_mangle]
 pub unsafe extern "C" fn Servo_Property_IsInherited(
     prop_name: *const nsACString,
 ) -> bool {
     let prop_name = prop_name.as_ref().unwrap().as_str_unchecked();
-    let prop_id = match PropertyId::parse(prop_name) {
+    let prop_id = match PropertyId::parse_enabled_for_all_content(prop_name) {
         Ok(id) => id,
         Err(_) => return false,
     };
     let longhand_id = match prop_id {
         PropertyId::Custom(_) => return true,
         PropertyId::Longhand(id) |
         PropertyId::LonghandAlias(id, _) => id,
         PropertyId::Shorthand(id) |
@@ -3471,19 +3463,19 @@ pub extern "C" fn Servo_DeclarationBlock
             false
         }
     })
 }
 
 macro_rules! get_property_id_from_property {
     ($property: ident, $ret: expr) => {{
         let property = $property.as_ref().unwrap().as_str_unchecked();
-        match PropertyId::parse(property.into()) {
+        match PropertyId::parse_enabled_for_all_content(property) {
             Ok(property_id) => property_id,
-            Err(_) => { return $ret; }
+            Err(_) => return $ret,
         }
     }}
 }
 
 unsafe fn get_property_value(
     declarations: RawServoDeclarationBlockBorrowed,
     property_id: PropertyId,
     value: *mut nsAString,