stylo: Error reporting for unknown media features #18341

ferjm · 2017-09-01T10:56:55Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix bug 1384225

This change is

highfive · 2017-09-01T10:56:59Z

Heads up! This PR modifies the following files:

@bholley: components/style/error_reporting.rs, components/style/media_queries.rs, components/style/stylesheets/rule_parser.rs
@canaltinova: components/style/error_reporting.rs, components/style/media_queries.rs, components/style/stylesheets/rule_parser.rs
@KiChjang: components/script/dom/htmllinkelement.rs, components/script/dom/cssmediarule.rs, components/script/dom/medialist.rs, components/script/dom/htmlstyleelement.rs, components/script/dom/window.rs
@fitzgen: components/script/dom/htmllinkelement.rs, components/script/dom/cssmediarule.rs, components/script/dom/medialist.rs, components/script/dom/htmlstyleelement.rs, components/script/dom/window.rs
@emilio: ports/geckolib/glue.rs, components/style/error_reporting.rs, components/style/media_queries.rs, components/style/stylesheets/rule_parser.rs, ports/geckolib/error_reporter.rs

highfive · 2017-09-01T10:57:01Z

Warning

These commits modify style and script code, but no tests are modified. Please consider adding a test!

ferjm · 2017-09-01T10:58:11Z

r? @jdm I believe you wrote most part of css error reporting.

emilio · 2017-09-01T12:09:17Z

components/style/media_queries.rs

@@ -240,19 +241,29 @@ impl MediaQuery {
 /// media query list is only filled with the equivalent of "not all", see:
 ///
 /// https://drafts.csswg.org/mediaqueries/#error-handling
-pub fn parse_media_query_list(context: &ParserContext, input: &mut Parser) -> MediaList {
+pub fn parse_media_query_list<R>(context: &ParserContext,


Let's format this as rustfmt would:

pub fn parse_media_query_list<R>( context: &ParserContext, input: &mut Parser, error_reporter: &R, ) -> MediaList where R: ParseErrorReporter, {

emilio · 2017-09-01T12:14:40Z

components/style/media_queries.rs

                media_queries.push(MediaQuery::never_matching());
+                let error = ContextualParseError::InvalidMediaFeature(


So this would report Expected media feature name but found..., but there are definitely more errors than can happen instead... What does gecko do for those? Does it report them? I suspect not.

Yes; we will want to add a new variant to StyleParseError that is only returned when an invalid media feature is encountered, and only report if that variant is encountered here.

jdm · 2017-09-01T17:06:44Z

Can you add a new test to https://dxr.mozilla.org/mozilla-central/source/layout/style/test/test_css_parse_error_smoketest.html for this as well?

bors-servo · 2017-09-04T19:28:27Z

☔ The latest upstream changes (presumably #18355) made this pull request unmergeable. Please resolve the merge conflicts.

ferjm · 2017-09-05T13:59:48Z

Feedback addressed and test added https://bugzilla.mozilla.org/show_bug.cgi?id=1384225#c5

emilio · 2017-09-05T14:09:30Z

components/style/media_queries.rs

+                    CssParseError::Custom(SelectorParseError::Custom(
+                            StyleParseError::MediaQueryExpectedFeatureValue)) =>
+                        ContextualParseError::MediaQueryExpectedFeatureValue,
+                    _ => ContextualParseError::UnknownAtRule(input.slice_from(start_position), err),


UnknownAtRule is not necessarily correct, is it? We can try to parse media query lists from a whole set of different places, like @import rules, stylesheet attributes...

Ah, right. I guess something like a new UnknownMediaQuery will do then?

Rather than matching on known errors here, we should simply have a InvalidMediaRule(ParseError) variant.

emilio · 2017-09-05T14:10:10Z

components/style/gecko/media_queries.rs

-                    }
-                    Err(()) => return Err(SelectorParseError::UnexpectedIdent(ident.clone()).into()),
+                    },
+                    Err(()) =>


nit: use a block here.

emilio · 2017-09-05T14:10:28Z

r? @jdm

jdm

Other testcase:
<style>@media "foo" {}</style> should yield Expected identifier in media list but found ‘"foo"’.
The use of RangedExpressionWithNoValue should also be an instance of MediaQueryExpectedFeatureValue, I believe. The use of UnexpectedIdent should be replaced by MediaQueryExpectedFeatureName instead.

jdm · 2017-09-05T16:57:09Z

components/style/gecko/media_queries.rs

@@ -564,7 +565,7 @@ impl Expression {
                    // for parity with gecko. We should remove this check when we want
                    // to support it.
                    if let Length::Calc(_) = length {
-                        return Err(StyleParseError::UnspecifiedError.into())
+                        return Err(StyleParseError::MediaQueryExpectedFeatureValue.into())


There a bunch of errors that can be returned in this feature value code that aren't caught (like let length = Length::parse_non_negative(context, input)?;). Let's extract all of this parsing code into a separate function and use map_err to translate any error that is returned into StyleParseError::MediaQueryExpectedFeatureValue instead.

jdm · 2017-09-05T17:07:59Z

components/style/media_queries.rs

+                    CssParseError::Custom(SelectorParseError::Custom(
+                            StyleParseError::MediaQueryExpectedFeatureValue)) =>
+                        ContextualParseError::MediaQueryExpectedFeatureValue,
+                    _ => ContextualParseError::UnknownAtRule(input.slice_from(start_position), err),


Rather than matching on known errors here, we should simply have a InvalidMediaRule(ParseError) variant.

jdm · 2017-09-05T17:13:05Z

ports/geckolib/error_reporter.rs

+            ContextualParseError::MediaQueryExpectedFeatureValue =>
+                (b"PEMQExpectedFeatureValue\0", Action::Nothing),
+            ContextualParseError::MediaQueryNoMinMaxWithoutValue =>
+                (b"PEMQNoMinMaxWithoutValue\0", Action::Nothing),


These will look like:

ContextualPArseError::InvalidMediaQuery( CssParseError::Custom(SelectorParseError::Custom(StyleParseError::MediaQueryExpectedFeatureName(..)))) => (b"PEMQExpectedFeatureName\0", Action::Nothing),

ferjm · 2017-09-07T16:31:31Z

r? @jdm

jdm

Almost there! We can get better error messages by explicitly adding the token or identifier to the error variants, however.

jdm · 2017-09-07T16:51:21Z

components/style/gecko/media_queries.rs

-                    Err(()) => return Err(SelectorParseError::UnexpectedIdent(ident.clone()).into()),
+                    },
+                    Err(()) => {
+                        return Err(StyleParseError::MediaQueryExpectedFeatureName.into())


We should pass the ident that was not recognized as a parameter of this error variant.

jdm · 2017-09-07T16:54:53Z

components/style/gecko/media_queries.rs

        input.parse_nested_block(|input| {
            // FIXME: remove extra indented block when lifetimes are non-lexical
            let feature;
            let range;
            {
-                let ident = input.expect_ident()?;
+                let ident = input.expect_ident().map_err(|_|
+                    StyleParseError::MediaQueryExpectedFeatureName


This should be ExpectedIdentifier, instead. We can match an UnexpectedToken error, extract it and add it as a parameter of the error variant.

jdm · 2017-09-07T16:55:19Z

components/style/gecko/media_queries.rs

@@ -488,13 +570,17 @@ impl Expression {
    /// ```
    pub fn parse<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>)
                         -> Result<Self, ParseError<'i>> {
-        input.expect_parenthesis_block()?;
+        input.expect_parenthesis_block().map_err(|_|


Gecko does not report an error here, so we can revert this change.

If I revert this, @media "foo" {} would only report "Declaration dropped" instead of "Expected identifier in media list but found ‘ "foo" ’"

ferjm · 2017-09-08T11:25:11Z

r? @jdm

jdm · 2017-09-08T16:14:47Z

@bors-servo: r+
Thanks!

bors-servo · 2017-09-08T16:14:48Z

📌 Commit 337a903 has been approved by jdm

bors-servo · 2017-09-08T16:14:53Z

⌛ Testing commit 337a903 with merge fef2cfd...

stylo: Error reporting for unknown media features - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [bug 1384225](https://bugzilla.mozilla.org/show_bug.cgi?id=1384225)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18341)

bors-servo · 2017-09-08T16:48:59Z

💔 Test failed - linux-rel-wpt

ferjm · 2017-09-08T16:57:56Z

@bors-servo: retry

I think these are intermittents

bors-servo · 2017-09-08T17:22:07Z

⚡ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only linux-rel-css, linux-rel-wpt...

bors-servo · 2017-09-08T18:04:21Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing fef2cfd to master...

KiChjang · 2017-09-08T18:14:16Z

@ferjm I'm not in front of a computer, could you please open new issues filing the intermittents you see here?

jdm · 2017-09-08T23:14:24Z

#18424 and #18273

highfive assigned KiChjang Sep 1, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 1, 2017

highfive assigned jdm and unassigned KiChjang Sep 1, 2017

emilio reviewed Sep 1, 2017

View reviewed changes

ferjm closed this Sep 4, 2017

ferjm force-pushed the bug1384225.media.errors branch from 3877856 to bcddb19 Compare September 4, 2017 13:58

ferjm reopened this Sep 4, 2017

ferjm force-pushed the bug1384225.media.errors branch from ddf0033 to 5a8aa19 Compare September 4, 2017 14:05

ferjm changed the title ~~stylo: Error reporting for unknown media features~~ [WIP] stylo: Error reporting for unknown media features Sep 4, 2017

highfive added the S-needs-rebase There are merge conflict errors. label Sep 4, 2017

ferjm force-pushed the bug1384225.media.errors branch from 5a8aa19 to 8c332c8 Compare September 5, 2017 13:53

ferjm changed the title ~~[WIP] stylo: Error reporting for unknown media features~~ stylo: Error reporting for unknown media features Sep 5, 2017

ferjm removed the S-needs-rebase There are merge conflict errors. label Sep 5, 2017

ferjm force-pushed the bug1384225.media.errors branch from 8c332c8 to fd52218 Compare September 5, 2017 13:58

emilio reviewed Sep 5, 2017

View reviewed changes

jdm requested changes Sep 5, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 5, 2017

ferjm force-pushed the bug1384225.media.errors branch from fd52218 to 3533cfb Compare September 7, 2017 09:58

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Sep 7, 2017

ferjm force-pushed the bug1384225.media.errors branch from 3533cfb to 1fabc6e Compare September 7, 2017 11:32

ferjm force-pushed the bug1384225.media.errors branch from 1fabc6e to c709490 Compare September 7, 2017 11:41

jdm requested changes Sep 7, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 7, 2017

stylo: Error reporting for unknown media features

337a903

ferjm force-pushed the bug1384225.media.errors branch from c709490 to 337a903 Compare September 8, 2017 10:50

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Sep 8, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 8, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Sep 8, 2017

bors-servo merged commit 337a903 into servo:master Sep 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stylo: Error reporting for unknown media features #18341

stylo: Error reporting for unknown media features #18341

ferjm commented Sep 1, 2017 •

edited by SimonSapin

Loading

highfive commented Sep 1, 2017

highfive commented Sep 1, 2017

ferjm commented Sep 1, 2017

emilio Sep 1, 2017

emilio Sep 1, 2017

jdm Sep 1, 2017

jdm commented Sep 1, 2017

bors-servo commented Sep 4, 2017

ferjm commented Sep 5, 2017

emilio Sep 5, 2017

ferjm Sep 5, 2017

jdm Sep 5, 2017

emilio Sep 5, 2017

emilio commented Sep 5, 2017

jdm left a comment

jdm Sep 5, 2017

jdm Sep 5, 2017

jdm Sep 5, 2017

ferjm commented Sep 7, 2017

jdm left a comment

jdm Sep 7, 2017

jdm Sep 7, 2017

jdm Sep 7, 2017

ferjm Sep 8, 2017

ferjm commented Sep 8, 2017

jdm commented Sep 8, 2017

bors-servo commented Sep 8, 2017

bors-servo commented Sep 8, 2017

bors-servo commented Sep 8, 2017

ferjm commented Sep 8, 2017

bors-servo commented Sep 8, 2017

bors-servo commented Sep 8, 2017

KiChjang commented Sep 8, 2017

jdm commented Sep 8, 2017

		media_queries.push(MediaQuery::never_matching());
		let error = ContextualParseError::InvalidMediaFeature(

stylo: Error reporting for unknown media features #18341

stylo: Error reporting for unknown media features #18341

Conversation

ferjm commented Sep 1, 2017 • edited by SimonSapin Loading

highfive commented Sep 1, 2017

highfive commented Sep 1, 2017

ferjm commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Sep 1, 2017

bors-servo commented Sep 4, 2017

ferjm commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Sep 5, 2017

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferjm commented Sep 7, 2017

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferjm commented Sep 8, 2017

jdm commented Sep 8, 2017

bors-servo commented Sep 8, 2017

bors-servo commented Sep 8, 2017

bors-servo commented Sep 8, 2017

ferjm commented Sep 8, 2017

bors-servo commented Sep 8, 2017

bors-servo commented Sep 8, 2017

KiChjang commented Sep 8, 2017

jdm commented Sep 8, 2017

ferjm commented Sep 1, 2017 •

edited by SimonSapin

Loading