Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stylo: Error reporting for unknown media features #18341

Merged
merged 1 commit into from Sep 8, 2017

Conversation

Projects
None yet
6 participants
@ferjm
Copy link
Member

commented Sep 1, 2017

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

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Sep 1, 2017

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

This comment has been minimized.

Copy link

commented Sep 1, 2017

warning Warning warning

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

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2017

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

@highfive highfive assigned jdm and unassigned KiChjang Sep 1, 2017

@@ -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,

This comment has been minimized.

Copy link
@emilio

emilio Sep 1, 2017

Member

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,
{
media_queries.push(MediaQuery::never_matching());
let error = ContextualParseError::InvalidMediaFeature(

This comment has been minimized.

Copy link
@emilio

emilio Sep 1, 2017

Member

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.

This comment has been minimized.

Copy link
@jdm

jdm Sep 1, 2017

Member

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

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

@ferjm ferjm closed this Sep 4, 2017

@ferjm ferjm force-pushed the ferjm:bug1384225.media.errors branch from 3877856 to bcddb19 Sep 4, 2017

@ferjm ferjm reopened this Sep 4, 2017

@ferjm ferjm force-pushed the ferjm:bug1384225.media.errors branch from ddf0033 to 5a8aa19 Sep 4, 2017

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

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2017

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

@ferjm ferjm force-pushed the ferjm:bug1384225.media.errors branch from 5a8aa19 to 8c332c8 Sep 5, 2017

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

@ferjm ferjm removed the S-needs-rebase label Sep 5, 2017

@ferjm ferjm force-pushed the ferjm:bug1384225.media.errors branch from 8c332c8 to fd52218 Sep 5, 2017

@ferjm

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2017

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

This comment has been minimized.

Copy link
@emilio

emilio Sep 5, 2017

Member

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...

This comment has been minimized.

Copy link
@ferjm

ferjm Sep 5, 2017

Author Member

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

This comment has been minimized.

Copy link
@jdm

jdm Sep 5, 2017

Member

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

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

This comment has been minimized.

Copy link
@emilio

emilio Sep 5, 2017

Member

nit: use a block here.

@emilio

This comment has been minimized.

Copy link
Member

commented Sep 5, 2017

r? @jdm

@jdm
Copy link
Member

left a comment

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.

@@ -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())

This comment has been minimized.

Copy link
@jdm

jdm Sep 5, 2017

Member

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.

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

This comment has been minimized.

Copy link
@jdm

jdm Sep 5, 2017

Member

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

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

This comment has been minimized.

Copy link
@jdm

jdm Sep 5, 2017

Member

These will look like:

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

@ferjm ferjm force-pushed the ferjm:bug1384225.media.errors branch from fd52218 to 3533cfb Sep 7, 2017

@ferjm ferjm force-pushed the ferjm:bug1384225.media.errors branch from 3533cfb to 1fabc6e Sep 7, 2017

@ferjm ferjm force-pushed the ferjm:bug1384225.media.errors branch from 1fabc6e to c709490 Sep 7, 2017

@ferjm

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2017

r? @jdm

@jdm
Copy link
Member

left a comment

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

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

This comment has been minimized.

Copy link
@jdm

jdm Sep 7, 2017

Member

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

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 comment has been minimized.

Copy link
@jdm

jdm Sep 7, 2017

Member

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

@@ -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(|_|

This comment has been minimized.

Copy link
@jdm

jdm Sep 7, 2017

Member

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

This comment has been minimized.

Copy link
@ferjm

ferjm Sep 8, 2017

Author Member

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

@ferjm ferjm force-pushed the ferjm:bug1384225.media.errors branch from c709490 to 337a903 Sep 8, 2017

@ferjm

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2017

r? @jdm

@jdm

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

@bors-servo: r+
Thanks!

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

📌 Commit 337a903 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

⌛️ Testing commit 337a903 with merge fef2cfd...

bors-servo added a commit that referenced this pull request Sep 8, 2017

Auto merge of #18341 - ferjm:bug1384225.media.errors, r=jdm
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)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18341)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

💔 Test failed - linux-rel-wpt

@ferjm

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2017

@bors-servo: retry

I think these are intermittents

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

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

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@KiChjang

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

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

@jdm

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.