Bug 1466095: Make PropertyId::parse less of a footgun. r?xidorn draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 01 Jun 2018 14:00:57 +0200
changeset 802782 4d78b08babc39f58a1f52e8ef8f7040f5c937146
parent 802780 30d8106f32a4d446d65ae4ab49731ae15ff40449
push id111954
push userbmo:emilio@crisal.io
push dateFri, 01 Jun 2018 12:15:07 +0000
reviewersxidorn
bugs1466095
milestone62.0a1
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_without_enabledness_check(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_without_enabledness_check(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_without_enabledness_check(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,18 @@ 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 {
+    #[inline]
+    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 +1994,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) => {
             let mut flags = WillChangeBits::empty();
             for longhand in shorthand.longhands() {
                 flags |= change_bits_for_longhand(longhand);
             }
             flags
         }
--- 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,