style: Centralize a bit invalid value error reporting.

Also, buffer the errors, since we're going to want to look at the whole
declaration block to skip reporting them.

This shouldn't change behavior, just moves some work to the caller, and defers a
bit the work so that it happens only when error reporting is enabled.

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

components/style/parser.rs

@@ -136,6 +136,12 @@ impl<'a> ParserContext<'a> {
             .expect("Rule type expected, but none was found.")
     }
 
+    /// Returns whether CSS error reporting is enabled.
+    #[inline]
+    pub fn error_reporting_enabled(&self) -> bool {
+        self.error_reporter.is_some()
+    }
+
     /// Record a CSS parse error with this context’s error reporting.
     pub fn log_css_error(&self, location: SourceLocation, error: ContextualParseError) {
         let error_reporter = match self.error_reporter {

components/style/properties/declaration_block.rs

@@ -1240,19 +1240,28 @@ pub fn parse_one_declaration_into(
         None,
     );
 
+    let property_id_for_error_reporting = if context.error_reporting_enabled() {
+        Some(id.clone())
+    } else {
+        None
+    };
+
     let mut input = ParserInput::new(input);
     let mut parser = Parser::new(&mut input);
     let start_position = parser.position();
     parser.parse_entirely(|parser| {
         PropertyDeclaration::parse_into(declarations, id, &context, parser)
     }).map_err(|err| {
-        let location = err.location;
+        if context.error_reporting_enabled() {
-        let error = ContextualParseError::UnsupportedPropertyDeclaration(
+            report_one_css_error(
-            parser.slice_from(start_position),
+                &context,
-            err,
+                None,
-            None
+                None,
-        );
+                err,
-        context.log_css_error(location, error);
+                parser.slice_from(start_position),
+                property_id_for_error_reporting,
+            )
+        }
     })
 }
 
@@ -1260,6 +1269,8 @@ pub fn parse_one_declaration_into(
 struct PropertyDeclarationParser<'a, 'b: 'a> {
     context: &'a ParserContext<'b>,
     declarations: &'a mut SourcePropertyDeclaration,
+    /// The last parsed property id if non-custom, and if any.
+    last_parsed_property_id: Option<PropertyId>,
 }
 
 
@@ -1296,6 +1307,9 @@ impl<'a, 'b, 'i> DeclarationParser<'i> for PropertyDeclarationParser<'a, 'b> {
                 }));
             }
         };
+        if self.context.error_reporting_enabled() {
+            self.last_parsed_property_id = Some(id.clone());
+        }
         input.parse_until_before(Delimiter::Bang, |input| {
             PropertyDeclaration::parse_into(self.declarations, id, self.context, input)
         })?;
@@ -1309,6 +1323,48 @@ impl<'a, 'b, 'i> DeclarationParser<'i> for PropertyDeclarationParser<'a, 'b> {
     }
 }
 
+type SmallParseErrorVec<'i> = SmallVec<[(ParseError<'i>, &'i str, Option<PropertyId>); 2]>;
+
+#[cold]
+fn report_one_css_error<'i>(
+    context: &ParserContext,
+    _block: Option<&PropertyDeclarationBlock>,
+    selectors: Option<&SelectorList<SelectorImpl>>,
+    mut error: ParseError<'i>,
+    slice: &str,
+    property: Option<PropertyId>,
+) {
+    debug_assert!(context.error_reporting_enabled());
+
+    // If the unrecognized property looks like a vendor-specific property,
+    // silently ignore it instead of polluting the error output.
+    if let ParseErrorKind::Custom(StyleParseErrorKind::UnknownVendorProperty) = error.kind {
+        return;
+    }
+
+    if let Some(ref property) = property {
+        error = match *property {
+            PropertyId::Custom(ref c) => StyleParseErrorKind::new_invalid(format!("--{}", c), error),
+            _ => StyleParseErrorKind::new_invalid(property.non_custom_id().unwrap().name(), error),
+        };
+    }
+
+    let location = error.location;
+    let error = ContextualParseError::UnsupportedPropertyDeclaration(slice, error, selectors);
+    context.log_css_error(location, error);
+}
+
+#[cold]
+fn report_css_errors(
+    context: &ParserContext,
+    block: &PropertyDeclarationBlock,
+    selectors: Option<&SelectorList<SelectorImpl>>,
+    errors: &mut SmallParseErrorVec,
+) {
+    for (mut error, slice, property) in errors.drain() {
+        report_one_css_error(context, Some(block), selectors, error, slice, property)
+    }
+}
 
 /// Parse a list of property declarations and return a property declaration
 /// block.
@@ -1320,10 +1376,12 @@ pub fn parse_property_declaration_list(
     let mut declarations = SourcePropertyDeclaration::new();
     let mut block = PropertyDeclarationBlock::new();
     let parser = PropertyDeclarationParser {
-        context: context,
+        context,
+        last_parsed_property_id: None,
         declarations: &mut declarations,
     };
     let mut iter = DeclarationListParser::new(input, parser);
+    let mut errors = SmallParseErrorVec::new();
     while let Some(declaration) = iter.next() {
         match declaration {
             Ok(importance) => {
@@ -1335,17 +1393,17 @@ pub fn parse_property_declaration_list(
             Err((error, slice)) => {
                 iter.parser.declarations.clear();
 
-                // If the unrecognized property looks like a vendor-specific property,
+                if context.error_reporting_enabled() {
-                // silently ignore it instead of polluting the error output.
+                    let property = iter.parser.last_parsed_property_id.take();
-                if let ParseErrorKind::Custom(StyleParseErrorKind::UnknownVendorProperty) = error.kind {
+                    errors.push((error, slice, property));
-                    continue;
                 }
-
-                let location = error.location;
-                let error = ContextualParseError::UnsupportedPropertyDeclaration(slice, error, selectors);
-                context.log_css_error(location, error);
             }
         }
     }
+
+    if !errors.is_empty() {
+        report_css_errors(context, &block, selectors, &mut errors)
+    }
+
     block
 }

components/style/properties/properties.mako.rs

@@ -2209,13 +2209,9 @@ impl PropertyDeclaration {
                 // This probably affects some test results.
                 let value = match input.try(CSSWideKeyword::parse) {
                     Ok(keyword) => CustomDeclarationValue::CSSWideKeyword(keyword),
-                    Err(()) => match crate::custom_properties::SpecifiedValue::parse(input) {
+                    Err(()) => CustomDeclarationValue::Value(
-                        Ok(value) => CustomDeclarationValue::Value(value),
+                        crate::custom_properties::SpecifiedValue::parse(input)?
-                        Err(e) => return Err(StyleParseErrorKind::new_invalid(
+                    ),
-                            format!("--{}", property_name),
-                            e,
-                        )),
-                    }
                 };
                 declarations.push(PropertyDeclaration::Custom(CustomDeclaration {
                     name: property_name,
@@ -2236,19 +2232,11 @@ impl PropertyDeclaration {
                     .or_else(|err| {
                         while let Ok(_) = input.next() {}  // Look for var() after the error.
                         if !input.seen_var_or_env_functions() {
-                            return Err(StyleParseErrorKind::new_invalid(
+                            return Err(err);
-                                non_custom_id.unwrap().name(),
-                                err,
-                            ));
                         }
                         input.reset(&start);
                         let (first_token_type, css) =
-                            crate::custom_properties::parse_non_custom_with_var(input).map_err(|e| {
+                            crate::custom_properties::parse_non_custom_with_var(input)?;
-                                StyleParseErrorKind::new_invalid(
-                                    non_custom_id.unwrap().name(),
-                                    e,
-                                )
-                            })?;
                         Ok(PropertyDeclaration::WithVariables(VariableDeclaration {
                             id,
                             value: Arc::new(UnparsedValue {
@@ -2287,20 +2275,12 @@ impl PropertyDeclaration {
                     id.parse_into(declarations, context, input).or_else(|err| {
                         while let Ok(_) = input.next() {}  // Look for var() after the error.
                         if !input.seen_var_or_env_functions() {
-                            return Err(StyleParseErrorKind::new_invalid(
+                            return Err(err);
-                                non_custom_id.unwrap().name(),
-                                err,
-                            ));
                         }
 
                         input.reset(&start);
                         let (first_token_type, css) =
-                            crate::custom_properties::parse_non_custom_with_var(input).map_err(|e| {
+                            crate::custom_properties::parse_non_custom_with_var(input)?;
-                                StyleParseErrorKind::new_invalid(
-                                    non_custom_id.unwrap().name(),
-                                    e,
-                                )
-                            })?;
                         let unparsed = Arc::new(UnparsedValue {
                             css: css.into_owned(),
                             first_token_type: first_token_type,