Bug 1538101 - Centralize a bit invalid value error reporting. r=jdm
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 08 May 2019 12:44:36 +0000
changeset 473055 ebcb2c853b9ce80debb2a093c3855a16a2f15492
parent 473044 72f13244bf42d994d1a559457c6081b938f8ccb2
child 473056 eee17e1b043502d063b1d609bd98750e91c1bc00
push id35988
push useropoprus@mozilla.com
push dateThu, 09 May 2019 03:32:40 +0000
treeherdermozilla-central@59314da6bb6b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdm
bugs1538101
milestone68.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 1538101 - Centralize a bit invalid value error reporting. r=jdm 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
servo/components/style/parser.rs
servo/components/style/properties/declaration_block.rs
servo/components/style/properties/properties.mako.rs
--- a/servo/components/style/parser.rs
+++ b/servo/components/style/parser.rs
@@ -131,16 +131,22 @@ impl<'a> ParserContext<'a> {
     }
 
     /// Get the rule type, which assumes that one is available.
     pub fn rule_type(&self) -> CssRuleType {
         self.rule_type
             .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 {
             Some(r) => r,
             None => return,
         };
 
         error_reporter.report_error(self.url_data, location, error)
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -1235,36 +1235,47 @@ pub fn parse_one_declaration_into(
         url_data,
         Some(CssRuleType::Style),
         parsing_mode,
         quirks_mode,
         error_reporter,
         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;
-        let error = ContextualParseError::UnsupportedPropertyDeclaration(
-            parser.slice_from(start_position),
-            err,
-            None
-        );
-        context.log_css_error(location, error);
+        if context.error_reporting_enabled() {
+            report_one_css_error(
+                &context,
+                None,
+                None,
+                err,
+                parser.slice_from(start_position),
+                property_id_for_error_reporting,
+            )
+        }
     })
 }
 
 /// A struct to parse property declarations.
 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>,
 }
 
 
 /// Default methods reject all at rules.
 impl<'a, 'b, 'i> AtRuleParser<'i> for PropertyDeclarationParser<'a, 'b> {
     type PreludeNoBlock = ();
     type PreludeBlock = ();
     type AtRule = Importance;
@@ -1291,61 +1302,108 @@ impl<'a, 'b, 'i> DeclarationParser<'i> f
             Err(..) => {
                 return Err(input.new_custom_error(if is_non_mozilla_vendor_identifier(&name) {
                     StyleParseErrorKind::UnknownVendorProperty
                 } else {
                     StyleParseErrorKind::UnknownProperty(name)
                 }));
             }
         };
+        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)
         })?;
         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)
     }
 }
 
+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.
 pub fn parse_property_declaration_list(
     context: &ParserContext,
     input: &mut Parser,
     selectors: Option<&SelectorList<SelectorImpl>>
 ) -> PropertyDeclarationBlock {
     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) => {
                 block.extend(
                     iter.parser.declarations.drain(),
                     importance,
                 );
             }
             Err((error, slice)) => {
                 iter.parser.declarations.clear();
 
-                // 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 {
-                    continue;
+                if context.error_reporting_enabled() {
+                    let property = iter.parser.last_parsed_property_id.take();
+                    errors.push((error, slice, property));
                 }
-
-                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
 }
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -2204,23 +2204,19 @@ impl PropertyDeclaration {
         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(CSSWideKeyword::parse) {
                     Ok(keyword) => CustomDeclarationValue::CSSWideKeyword(keyword),
-                    Err(()) => match crate::custom_properties::SpecifiedValue::parse(input) {
-                        Ok(value) => CustomDeclarationValue::Value(value),
-                        Err(e) => return Err(StyleParseErrorKind::new_invalid(
-                            format!("--{}", property_name),
-                            e,
-                        )),
-                    }
+                    Err(()) => CustomDeclarationValue::Value(
+                        crate::custom_properties::SpecifiedValue::parse(input)?
+                    ),
                 };
                 declarations.push(PropertyDeclaration::Custom(CustomDeclaration {
                     name: property_name,
                     value,
                 }));
                 return Ok(());
             }
             PropertyId::LonghandAlias(id, _) |
@@ -2231,29 +2227,21 @@ impl PropertyDeclaration {
                         WideKeywordDeclaration { id, keyword },
                     )
                 }).or_else(|()| {
                     input.look_for_var_or_env_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_or_env_functions() {
-                            return Err(StyleParseErrorKind::new_invalid(
-                                non_custom_id.unwrap().name(),
-                                err,
-                            ));
+                            return Err(err);
                         }
                         input.reset(&start);
                         let (first_token_type, css) =
-                            crate::custom_properties::parse_non_custom_with_var(input).map_err(|e| {
-                                StyleParseErrorKind::new_invalid(
-                                    non_custom_id.unwrap().name(),
-                                    e,
-                                )
-                            })?;
+                            crate::custom_properties::parse_non_custom_with_var(input)?;
                         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,
                             }),
@@ -2282,30 +2270,22 @@ impl PropertyDeclaration {
                 } else {
                     input.look_for_var_or_env_functions();
                     // 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_or_env_functions() {
-                            return Err(StyleParseErrorKind::new_invalid(
-                                non_custom_id.unwrap().name(),
-                                err,
-                            ));
+                            return Err(err);
                         }
 
                         input.reset(&start);
                         let (first_token_type, css) =
-                            crate::custom_properties::parse_non_custom_with_var(input).map_err(|e| {
-                                StyleParseErrorKind::new_invalid(
-                                    non_custom_id.unwrap().name(),
-                                    e,
-                                )
-                            })?;
+                            crate::custom_properties::parse_non_custom_with_var(input)?;
                         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 {
                             declarations.all_shorthand = AllShorthand::WithVariables(unparsed)