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

Fix supports rule parsing issues with <any-value> #17792

Merged
merged 1 commit into from Jul 20, 2017

Conversation

@upsuper
Copy link
Member

commented Jul 20, 2017

This eventually fixes #15482, as well as several reftests in mozilla-central which were added for bug 883987.

The new function should probably be moved into cssparser crate at some point.


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jul 20, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/stylesheets/supports_rule.rs
  • @canaltinova: components/style/stylesheets/supports_rule.rs
  • @emilio: components/style/stylesheets/supports_rule.rs
@highfive

This comment has been minimized.

Copy link

commented Jul 20, 2017

warning Warning warning

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

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2017

@highfive highfive assigned SimonSapin and unassigned metajack Jul 20, 2017

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

⌛️ Trying commit 1b58392 with merge fd1e558...

bors-servo added a commit that referenced this pull request Jul 20, 2017
Auto merge of #17792 - upsuper:supports-any-value, r=<try>
Fix supports rule parsing issues with <any-value>

This eventually fixes #15482, as well as several reftests in mozilla-central which were added for [bug 883987](https://bugzilla.mozilla.org/show_bug.cgi?id=883987).

The new function should probably be moved into cssparser crate at some point.

<!-- 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/17792)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

💔 Test failed - linux-dev

@upsuper upsuper force-pushed the upsuper:supports-any-value branch from 1b58392 to 4978f6c Jul 20, 2017

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

⌛️ Trying commit 4978f6c with merge 35d4e45...

bors-servo added a commit that referenced this pull request Jul 20, 2017
Auto merge of #17792 - upsuper:supports-any-value, r=<try>
Fix supports rule parsing issues with <any-value>

This eventually fixes #15482, as well as several reftests in mozilla-central which were added for [bug 883987](https://bugzilla.mozilla.org/show_bug.cgi?id=883987).

The new function should probably be moved into cssparser crate at some point.

<!-- 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/17792)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

Ok(SupportsCondition::FutureSyntax(input.slice_from(pos).to_owned()))
Token::Function(name) => {
let content = input.parse_nested_block(|i| parse_any_value(i))?;
Ok(SupportsCondition::FutureSyntax(format!("{}({})", name, content)))

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Jul 20, 2017

Member

This needs to use serialize_identifier for the function name. Why not keep using slice_from with a wider range, though?

This comment has been minimized.

Copy link
@upsuper

upsuper Jul 20, 2017

Author Member

hmm, I thought it would be useful to have a parse_any_value returning the value parsed... but yeah, probably in this case it is better to just keep using slice_from in the callsites.

@upsuper upsuper force-pushed the upsuper:supports-any-value branch from 4978f6c to 4022957 Jul 20, 2017

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2017

@SimonSapin updated

Ok(SupportsCondition::FutureSyntax(input.slice_from(pos).to_owned()))
})
})
input.try(|i| parse_condition_or_declaration(i).map(|_| ()))

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Jul 20, 2017

Member

.map(|_| ()) here drops a SupportsCondition::Parenthesized or SupportsCondition::Declaration, and replaces it with () that is eventually replaced with SupportsCondition::FutureSyntax. Is that the desired behavior?

This comment has been minimized.

Copy link
@upsuper

upsuper Jul 20, 2017

Author Member

No. Fixed...

@upsuper upsuper force-pushed the upsuper:supports-any-value branch from 4022957 to 5eb0613 Jul 20, 2017

@SimonSapin

This comment has been minimized.

Copy link
Member

commented Jul 20, 2017

Thanks!

@bors-servo r+

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

📌 Commit 5eb0613 has been approved by SimonSapin

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

🌲 The tree is currently closed for pull requests below priority 9000, this pull request will be tested once the tree is reopened

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

⌛️ Testing commit 5eb0613 with merge e19fefc...

bors-servo added a commit that referenced this pull request Jul 20, 2017
Auto merge of #17792 - upsuper:supports-any-value, r=SimonSapin
Fix supports rule parsing issues with <any-value>

This eventually fixes #15482, as well as several reftests in mozilla-central which were added for [bug 883987](https://bugzilla.mozilla.org/show_bug.cgi?id=883987).

The new function should probably be moved into cssparser crate at some point.

<!-- 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/17792)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

@bors-servo bors-servo merged commit 5eb0613 into servo:master Jul 20, 2017

3 checks passed

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

@upsuper upsuper deleted the upsuper:supports-any-value branch Jul 20, 2017

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