Bug 1422225: Error reporting fixes. r=xidorn

Do it so that we always try to evaluate the media expression and the modern
syntax last, so that the most specific error message comes up.

MozReview-Commit-ID: 2tqdAsWh6Kh

dom/locales/en-US/chrome/layout/css.properties

@@ -18,6 +18,8 @@ PEUnknownAtRule=Unrecognized at-rule or error parsing at-rule ‘%1$S’.
 PECharsetRuleEOF=charset string in @charset rule
 PECharsetRuleNotString=Expected charset string but found ‘%1$S’.
 PEGatherMediaEOF=end of media list in @import or @media rule
+PEMQUnexpectedOperator=Unexpected operator in media list.
+PEMQUnexpectedToken=Unexpected token ‘%1$S’ in media list.
 PEGatherMediaNotComma=Expected ‘,’ in media list but found ‘%1$S’.
 PEGatherMediaNotIdent=Expected identifier in media list but found ‘%1$S’.
 PEGatherMediaReservedMediaType=Found reserved keyword ‘%1$S’ when looking for media type.

layout/style/test/test_css_parse_error_smoketest.html

@@ -41,8 +41,9 @@
     { css: "* { -webkit-text-size-adjust: 100% }", error: "Error in parsing value for ‘-webkit-text-size-adjust’.  Declaration dropped." },
 
     { css: "@media (totally-unknown-feature) {}", error: "Expected media feature name but found ‘totally-unknown-feature’." },
-    { css: "@media \"foo\" {}", error: "Expected identifier in media list but found ‘\"foo\"’." },
+    { css: "@media \"foo\" {}", error: "Unexpected token ‘\"foo\"’ in media list." },
     { css: "@media (min-width) {}", error: "Media features with min- or max- must have a value." },
+    { css: "@media (min-width >= 3px) {}", error: "Unexpected operator in media list." },
     { css: "@media (device-height: -1px) {}", error: "Found invalid value for media feature." },
     { css: "@media (min-width: -1px) {}", error: "Found invalid value for media feature." },
     { css: "@media (min-resolution: 2) {}", error: "Found invalid value for media feature." },

servo/components/style/gecko/media_queries.rs

@@ -7,7 +7,7 @@
 use app_units::AU_PER_PX;
 use app_units::Au;
 use context::QuirksMode;
-use cssparser::{BasicParseErrorKind, Parser, RGBA, Token};
+use cssparser::{Parser, RGBA, Token};
 use euclid::Size2D;
 use euclid::TypedScale;
 use gecko::values::{convert_nscolor_to_rgba, convert_rgba_to_nscolor};
@@ -292,8 +292,8 @@ enum RangeOrOperator {
     Operator(Operator),
 }
 
-/// A range expression for gecko contains a reference to the media feature, the
-/// value the media query contained, and the range to evaluate.
+/// A feature expression for gecko contains a reference to the media feature,
+/// the value the media query contained, and the range to evaluate.
 #[derive(Clone, Debug, MallocSizeOf)]
 pub struct MediaFeatureExpression {
     feature: &'static nsMediaFeature,
@@ -641,21 +641,13 @@ impl MediaFeatureExpression {
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
-        input.expect_parenthesis_block().map_err(|err| {
-            err.location.new_custom_error(match err.kind {
-                BasicParseErrorKind::UnexpectedToken(t) => {
-                    StyleParseErrorKind::ExpectedIdentifier(t)
-                },
-                _ => StyleParseErrorKind::UnspecifiedError,
-            })
-        })?;
-
+        input.expect_parenthesis_block()?;
         input.parse_nested_block(|input| {
             Self::parse_in_parenthesis_block(context, input)
         })
     }
 
-    /// Parse a media range expression where we've already consumed the
+    /// Parse a media feature expression where we've already consumed the
     /// parenthesis.
     pub fn parse_in_parenthesis_block<'i, 't>(
         context: &ParserContext,
@@ -666,14 +658,7 @@ impl MediaFeatureExpression {
         let range;
         {
             let location = input.current_source_location();
-            let ident = input.expect_ident().map_err(|err| {
-                err.location.new_custom_error(match err.kind {
-                    BasicParseErrorKind::UnexpectedToken(t) => {
-                        StyleParseErrorKind::ExpectedIdentifier(t)
-                    },
-                    _ => StyleParseErrorKind::UnspecifiedError,
-                })
-            })?;
+            let ident = input.expect_ident()?;
 
             let mut flags = 0;
 
@@ -768,7 +753,7 @@ impl MediaFeatureExpression {
             Some(range) => {
                 if operator.is_some() {
                     return Err(input.new_custom_error(
-                        StyleParseErrorKind::MediaQueryExpectedFeatureValue
+                        StyleParseErrorKind::MediaQueryUnexpectedOperator
                     ));
                 }
                 Some(RangeOrOperator::Range(range))
@@ -778,7 +763,7 @@ impl MediaFeatureExpression {
                     Some(operator) => {
                         if !feature_allows_ranges {
                             return Err(input.new_custom_error(
-                                StyleParseErrorKind::MediaQueryExpectedFeatureValue
+                                StyleParseErrorKind::MediaQueryUnexpectedOperator
                             ));
                         }
                         Some(RangeOrOperator::Operator(operator))

servo/components/style/media_queries/media_condition.rs

@@ -163,12 +163,11 @@ impl MediaCondition {
     ) -> Result<Self, ParseError<'i>> {
         input.parse_nested_block(|input| {
             // Base case.
-            if let Ok(expr) = input.try(|i| MediaFeatureExpression::parse_in_parenthesis_block(context, i)) {
-                return Ok(MediaCondition::Feature(expr));
+            if let Ok(inner) = input.try(|i| Self::parse(context, i)) {
+                return Ok(MediaCondition::InParens(Box::new(inner)))
             }
-
-            let inner = Self::parse(context, input)?;
-            Ok(MediaCondition::InParens(Box::new(inner)))
+            let expr = MediaFeatureExpression::parse_in_parenthesis_block(context, input)?;
+            Ok(MediaCondition::Feature(expr))
         })
     }
 

servo/components/style/media_queries/media_query.rs

@@ -9,7 +9,6 @@
 use Atom;
 use cssparser::Parser;
 use parser::ParserContext;
-use selectors::parser::SelectorParseErrorKind;
 use std::fmt::{self, Write};
 use str::string_as_ascii_lowercase;
 use style_traits::{CssWriter, ParseError, ToCss};
@@ -125,34 +124,23 @@ impl MediaQuery {
     pub fn parse<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
-    ) -> Result<MediaQuery, ParseError<'i>> {
-        if let Ok(condition) = input.try(|i| MediaCondition::parse(context, i)) {
-            return Ok(Self {
-                qualifier: None,
-                media_type: MediaQueryType::All,
-                condition: Some(condition),
-            })
-        }
+    ) -> Result<Self, ParseError<'i>> {
+        let (qualifier, explicit_media_type) = input.try(|input| -> Result<_, ()> {
+            let qualifier = input.try(Qualifier::parse).ok();
+            let ident = input.expect_ident().map_err(|_| ())?;
+            let media_type = MediaQueryType::parse(&ident)?;
+            Ok((qualifier, Some(media_type)))
+        }).unwrap_or_default();
 
-        let qualifier = input.try(Qualifier::parse).ok();
-        let media_type = {
-            let location = input.current_source_location();
-            let ident = input.expect_ident()?;
-            match MediaQueryType::parse(&ident) {
-                Ok(t) => t,
-                Err(..) => return Err(location.new_custom_error(
-                    SelectorParseErrorKind::UnexpectedIdent(ident.clone())
-                ))
-            }
+        let condition = if explicit_media_type.is_none() {
+            Some(MediaCondition::parse(context, input)?)
+        } else if input.try(|i| i.expect_ident_matching("and")).is_ok() {
+            Some(MediaCondition::parse_disallow_or(context, input)?)
+        } else {
+            None
         };
 
-        let condition =
-            if input.try(|i| i.expect_ident_matching("and")).is_ok() {
-                Some(MediaCondition::parse_disallow_or(context, input)?)
-            } else {
-                None
-            };
-
+        let media_type = explicit_media_type.unwrap_or(MediaQueryType::All);
         Ok(Self { qualifier, media_type, condition })
     }
 }

servo/components/style_traits/lib.rs

@@ -106,12 +106,12 @@ pub enum StyleParseErrorKind<'i> {
     PropertyDeclarationValueNotExhausted,
     /// An unexpected dimension token was encountered.
     UnexpectedDimension(CowRcStr<'i>),
-    /// Expected identifier not found.
-    ExpectedIdentifier(Token<'i>),
     /// Missing or invalid media feature name.
     MediaQueryExpectedFeatureName(CowRcStr<'i>),
     /// Missing or invalid media feature value.
     MediaQueryExpectedFeatureValue,
+    /// A media feature range operator was not expected.
+    MediaQueryUnexpectedOperator,
     /// min- or max- properties must have a value.
     RangedExpressionWithNoValue,
     /// A function was encountered that was not expected.

servo/ports/geckolib/error_reporter.rs

@@ -145,9 +145,7 @@ fn extract_error_params<'a>(err: ErrorKind<'a>) -> Option<ErrorParams<'a>> {
             (Some(ErrorString::Ident(ident)), None)
         }
 
-        ParseErrorKind::Custom(
-            StyleParseErrorKind::ExpectedIdentifier(token)
-        ) |
+        ParseErrorKind::Basic(BasicParseErrorKind::UnexpectedToken(token)) |
         ParseErrorKind::Custom(
             StyleParseErrorKind::ValueError(ValueParseErrorKind::InvalidColor(token))
         ) => {
@@ -342,20 +340,20 @@ impl<'a> ErrorHelpers<'a> for ContextualParseError<'a> {
             }
             ContextualParseError::InvalidMediaRule(_, ref err) => {
                 let err: &CStr = match err.kind {
-                    ParseErrorKind::Custom(StyleParseErrorKind::ExpectedIdentifier(..)) => {
-                        cstr!("PEGatherMediaNotIdent")
-                    },
                     ParseErrorKind::Custom(StyleParseErrorKind::MediaQueryExpectedFeatureName(..)) => {
                         cstr!("PEMQExpectedFeatureName")
                     },
                     ParseErrorKind::Custom(StyleParseErrorKind::MediaQueryExpectedFeatureValue) => {
                         cstr!("PEMQExpectedFeatureValue")
                     },
+                    ParseErrorKind::Custom(StyleParseErrorKind::MediaQueryUnexpectedOperator) => {
+                        cstr!("PEMQUnexpectedOperator")
+                    },
                     ParseErrorKind::Custom(StyleParseErrorKind::RangedExpressionWithNoValue) => {
                         cstr!("PEMQNoMinMaxWithoutValue")
                     },
                     _ => {
-                        cstr!("PEDeclDropped")
+                        cstr!("PEMQUnexpectedToken")
                     },
                 };
                 (err, Action::Nothing)