Bug 1466645: Remove PropertyId::name. r?xidorn draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 04 Jun 2018 20:57:49 +0200
changeset 804323 b3c86fc7e7870868df0344dba7db08c3c5dfbe86
parent 804322 11efb6ce849c9dd0d38b38e7d8a06742041e724c
push id112345
push userbmo:emilio@crisal.io
push dateTue, 05 Jun 2018 19:10:58 +0000
reviewersxidorn
bugs1466645
milestone62.0a1
Bug 1466645: Remove PropertyId::name. r?xidorn It's only used for the error path in property parsing, so most of the time is not useful. Use the just-introduced NonCustomPropertyId::name to preserve the alias name, which we were doing by passing the name around. MozReview-Commit-ID: 46xxZKCoeBB
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_traits/lib.rs
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -1111,19 +1111,17 @@ where
         parsing_mode,
         quirks_mode,
     );
 
     let mut input = ParserInput::new(input);
     let mut parser = Parser::new(&mut input);
     let start_position = parser.position();
     parser.parse_entirely(|parser| {
-        let name = id.name().into();
-        PropertyDeclaration::parse_into(declarations, id, name, &context, parser)
-            .map_err(|e| e.into())
+        PropertyDeclaration::parse_into(declarations, id, &context, parser)
     }).map_err(|err| {
         let location = err.location;
         let error = ContextualParseError::UnsupportedPropertyDeclaration(
             parser.slice_from(start_position), err);
         let error_context = ParserErrorContext { error_reporter: error_reporter };
         context.log_css_error(&error_context, location, error);
     })
 }
@@ -1164,17 +1162,17 @@ impl<'a, 'b, 'i> DeclarationParser<'i> f
                 return Err(input.new_custom_error(if is_non_mozilla_vendor_identifier(&name) {
                     StyleParseErrorKind::UnknownVendorProperty
                 } else {
                     StyleParseErrorKind::UnknownProperty(name)
                 }));
             }
         };
         input.parse_until_before(Delimiter::Bang, |input| {
-            PropertyDeclaration::parse_into(self.declarations, id, name, self.context, input)
+            PropertyDeclaration::parse_into(self.declarations, id, self.context, input)
         })?;
         let importance = match input.try(parse_important) {
             Ok(()) => Importance::Important,
             Err(_) => Importance::Normal,
         };
         // In case there is still unparsed text in the declaration, we should roll back.
         input.expect_exhausted()?;
         Ok(importance)
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -18,17 +18,17 @@ use servo_arc::{Arc, UniqueArc};
 use smallbitvec::SmallBitVec;
 use std::borrow::Cow;
 use std::{ops, ptr};
 use std::cell::RefCell;
 use std::fmt::{self, Write};
 use std::mem::{self, ManuallyDrop};
 
 #[cfg(feature = "servo")] use cssparser::RGBA;
-use cssparser::{CowRcStr, Parser, TokenSerializationType, serialize_identifier};
+use cssparser::{Parser, TokenSerializationType};
 use cssparser::ParserInput;
 #[cfg(feature = "servo")] use euclid::SideOffsets2D;
 use context::QuirksMode;
 use font_metrics::FontMetricsProvider;
 #[cfg(feature = "gecko")] use gecko_bindings::bindings;
 #[cfg(feature = "gecko")] use gecko_bindings::structs::{self, nsCSSPropertyID};
 #[cfg(feature = "servo")] use logical_geometry::LogicalMargin;
 #[cfg(feature = "servo")] use computed_values;
@@ -1760,31 +1760,16 @@ impl PropertyId {
             PropertyId::ShorthandAlias(id, _) |
             PropertyId::Shorthand(id) => Ok(id),
             PropertyId::LonghandAlias(id, _) |
             PropertyId::Longhand(id) => Err(PropertyDeclarationId::Longhand(id)),
             PropertyId::Custom(ref name) => Err(PropertyDeclarationId::Custom(name)),
         }
     }
 
-    /// Returns the name of the property without CSS escaping.
-    pub fn name(&self) -> Cow<'static, str> {
-        match *self {
-            PropertyId::ShorthandAlias(id, _) |
-            PropertyId::Shorthand(id) => id.name().into(),
-            PropertyId::LonghandAlias(id, _) |
-            PropertyId::Longhand(id) => id.name().into(),
-            PropertyId::Custom(ref name) => {
-                let mut s = String::new();
-                write!(&mut s, "--{}", name).unwrap();
-                s.into()
-            }
-        }
-    }
-
     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(),
         })
@@ -2037,34 +2022,37 @@ impl PropertyDeclaration {
     /// > but does accept the `animation-play-state` property and interprets it specially.
     ///
     /// This will not actually parse Importance values, and will always set things
     /// to Importance::Normal. Parsing Importance values is the job of PropertyDeclarationParser,
     /// we only set them here so that we don't have to reallocate
     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());
         debug_assert!(id.allowed_in(context), "{:?}", id);
 
+        let non_custom_id = id.non_custom_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)) {
                     Ok(keyword) => DeclaredValueOwned::CSSWideKeyword(keyword),
                     Err(()) => match ::custom_properties::SpecifiedValue::parse(input) {
                         Ok(value) => DeclaredValueOwned::Value(value),
-                        Err(e) => return Err(StyleParseErrorKind::new_invalid(name, e)),
+                        Err(e) => return Err(StyleParseErrorKind::new_invalid(
+                            format!("--{}", property_name),
+                            e,
+                        )),
                     }
                 };
                 declarations.push(PropertyDeclaration::Custom(CustomDeclaration {
                     name: property_name,
                     value,
                 }));
                 Ok(())
             }
@@ -2079,29 +2067,35 @@ impl PropertyDeclaration {
                     input.look_for_var_functions();
                     input.parse_entirely(|input| id.parse_value(context, input))
                     .or_else(|err| {
                         while let Ok(_) = input.next() {}  // Look for var() after the error.
                         if input.seen_var_functions() {
                             input.reset(&start);
                             let (first_token_type, css) =
                                 ::custom_properties::parse_non_custom_with_var(input).map_err(|e| {
-                                    StyleParseErrorKind::new_invalid(name, e)
+                                    StyleParseErrorKind::new_invalid(
+                                        non_custom_id.unwrap().name(),
+                                        e,
+                                    )
                                 })?;
                             Ok(PropertyDeclaration::WithVariables(VariableDeclaration {
                                 id,
                                 value: Arc::new(UnparsedValue {
                                 css: css.into_owned(),
                                 first_token_type: first_token_type,
                                 url_data: context.url_data.clone(),
                                 from_shorthand: None,
                                 }),
                             }))
                         } else {
-                            Err(StyleParseErrorKind::new_invalid(name, err))
+                            Err(StyleParseErrorKind::new_invalid(
+                                non_custom_id.unwrap().name(),
+                                err,
+                            ))
                         }
                     })
                 }).map(|declaration| {
                     declarations.push(declaration)
                 })
             }
             PropertyId::ShorthandAlias(id, _) |
             PropertyId::Shorthand(id) => {
@@ -2125,17 +2119,20 @@ impl PropertyDeclaration {
                     // Not using parse_entirely here: each ${shorthand.ident}::parse_into function
                     // needs to do so *before* pushing to `declarations`.
                     id.parse_into(declarations, context, input).or_else(|err| {
                         while let Ok(_) = input.next() {}  // Look for var() after the error.
                         if input.seen_var_functions() {
                             input.reset(&start);
                             let (first_token_type, css) =
                                 ::custom_properties::parse_non_custom_with_var(input).map_err(|e| {
-                                    StyleParseErrorKind::new_invalid(name, e)
+                                    StyleParseErrorKind::new_invalid(
+                                        non_custom_id.unwrap().name(),
+                                        e,
+                                    )
                                 })?;
                             let unparsed = Arc::new(UnparsedValue {
                                 css: css.into_owned(),
                                 first_token_type: first_token_type,
                                 url_data: context.url_data.clone(),
                                 from_shorthand: Some(id),
                             });
                             if id == ShorthandId::All {
@@ -2147,17 +2144,20 @@ impl PropertyDeclaration {
                                             id,
                                             value: unparsed.clone(),
                                         })
                                     )
                                 }
                             }
                             Ok(())
                         } else {
-                            Err(StyleParseErrorKind::new_invalid(name, err))
+                            Err(StyleParseErrorKind::new_invalid(
+                                non_custom_id.unwrap().name(),
+                                err,
+                            ))
                         }
                     })
                 }
             }
         }
     }
 }
 
--- a/servo/components/style/stylesheets/keyframes_rule.rs
+++ b/servo/components/style/stylesheets/keyframes_rule.rs
@@ -618,22 +618,22 @@ impl<'a, 'b, 'i> DeclarationParser<'i> f
     fn parse_value<'t>(
         &mut self,
         name: CowRcStr<'i>,
         input: &mut Parser<'i, 't>,
     ) -> Result<(), ParseError<'i>> {
         let id = match PropertyId::parse(&name, self.context) {
             Ok(id) => id,
             Err(()) => return Err(input.new_custom_error(
-                StyleParseErrorKind::UnknownProperty(name.clone())
+                StyleParseErrorKind::UnknownProperty(name)
             )),
         };
 
         // TODO(emilio): Shouldn't this use parse_entirely?
-        PropertyDeclaration::parse_into(self.declarations, id, name, self.context, input)?;
+        PropertyDeclaration::parse_into(self.declarations, id, self.context, input)?;
 
         // In case there is still unparsed text in the declaration, we should
         // roll back.
         input.expect_exhausted()?;
 
         Ok(())
     }
 }
--- a/servo/components/style/stylesheets/supports_rule.rs
+++ b/servo/components/style/stylesheets/supports_rule.rs
@@ -311,25 +311,28 @@ impl Declaration {
     ///
     /// <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<()>> {
-                let prop = input.expect_ident_cloned().unwrap();
-                input.expect_colon().unwrap();
+            let prop = input.expect_ident_cloned().unwrap();
+            input.expect_colon().unwrap();
 
-                let id = PropertyId::parse(&prop, context)
-                    .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(())
-            })
-            .is_ok()
+            let mut declarations = SourcePropertyDeclaration::new();
+            input.parse_until_before(Delimiter::Bang, |input| {
+                PropertyDeclaration::parse_into(
+                    &mut declarations,
+                    id,
+                    &context,
+                    input,
+                ).map_err(|_| input.new_custom_error(()))
+            })?;
+            let _ = input.try(parse_important);
+            Ok(())
+        }).is_ok()
     }
 }
--- a/servo/components/style_traits/lib.rs
+++ b/servo/components/style_traits/lib.rs
@@ -172,17 +172,21 @@ pub enum ValueParseErrorKind<'i> {
     /// An invalid token was encountered while parsing a color value.
     InvalidColor(Token<'i>),
     /// An invalid filter value was encountered.
     InvalidFilter(Token<'i>),
 }
 
 impl<'i> StyleParseErrorKind<'i> {
     /// Create an InvalidValue parse error
-    pub fn new_invalid(name: CowRcStr<'i>, value_error: ParseError<'i>) -> ParseError<'i> {
+    pub fn new_invalid<S>(name: S, value_error: ParseError<'i>) -> ParseError<'i>
+    where
+        S: Into<CowRcStr<'i>>,
+    {
+        let name = name.into();
         let variant = match value_error.kind {
             cssparser::ParseErrorKind::Custom(StyleParseErrorKind::ValueError(e)) => {
                 match e {
                     ValueParseErrorKind::InvalidColor(token) => {
                         StyleParseErrorKind::InvalidColor(name, token)
                     }
                     ValueParseErrorKind::InvalidFilter(token) => {
                         StyleParseErrorKind::InvalidFilter(name, token)