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

Update cssparser to 0.18 #17820

Merged
merged 1 commit into from Jul 24, 2017
Merged

Update cssparser to 0.18 #17820

merged 1 commit into from Jul 24, 2017

Conversation

SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Jul 21, 2017

Do not merge yet, depends on servo/rust-cssparser#171.


This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/stylesheets/viewport_rule.rs, components/style/properties/longhand/position.mako.rs, components/style/values/specified/color.rs, components/style/properties/longhand/background.mako.rs, components/style/properties/longhand/text.mako.rs and 40 more
  • @canaltinova: components/style/stylesheets/viewport_rule.rs, components/style/properties/longhand/position.mako.rs, components/style/values/specified/color.rs, components/style/properties/longhand/background.mako.rs, components/style/properties/longhand/text.mako.rs and 40 more
  • @KiChjang: components/script/dom/csskeyframesrule.rs, components/script/dom/bindings/str.rs
  • @fitzgen: components/script/dom/csskeyframesrule.rs, components/script/dom/bindings/str.rs
  • @emilio: components/style/stylesheets/viewport_rule.rs, components/style/properties/longhand/position.mako.rs, components/style/values/specified/color.rs, components/style/properties/longhand/background.mako.rs, components/style/properties/longhand/text.mako.rs and 41 more

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 21, 2017
@highfive
Copy link

warning Warning warning

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

@SimonSapin
Copy link
Member Author

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 3a5c6b0 with merge c3738e8...

bors-servo pushed a commit that referenced this pull request Jul 21, 2017
Update cssparser to 0.18

Do not merge yet, depends on servo/rust-cssparser#171.
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Jul 21, 2017
@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Jul 21, 2017
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Jul 21, 2017
@SimonSapin
Copy link
Member Author

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 17b8577 with merge 528c8cc...

bors-servo pushed a commit that referenced this pull request Jul 21, 2017
Update cssparser to 0.18

Do not merge yet, depends on servo/rust-cssparser#171.

<!-- 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/17820)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

Copy link
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good... I don't love the fact that the code looks uglier in many places, but... r=me

@@ -1231,16 +1232,17 @@ fn parse_qualified_name<'i, 't, P, E, Impl>
},
Ok(Token::Delim('*')) => {
let position = input.position();
match input.next_including_whitespace() {
// FIXME: remove clone() when lifetimes are non-lexical
match input.next_including_whitespace().map(|t| t.clone()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: cloned().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cloned exists on Option but not on Result, unfortunately :(

Ok(Token::Delim('|')) => {
explicit_namespace(input, QNamePrefix::ExplicitAnyNamespace)
}
result => {
input.reset(position);
if in_attr_selector {
match result {
Ok(t) => Err(ParseError::Basic(BasicParseError::UnexpectedToken(t))),
Err(e) => Err(ParseError::Basic(e)),
Ok(t) => Err(ParseError::Basic(BasicParseError::UnexpectedToken(t.clone()))),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, I think that means this clone is not needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -1603,27 +1605,28 @@ fn parse_one_simple_selector<'i, 't, P, E, Impl>(parser: &P,
where P: Parser<'i, Impl=Impl, Error=E>, Impl: SelectorImpl
{
let start_position = input.position();
match input.next_including_whitespace() {
// FIXME: remove clone() when lifetimes are non-lexical
match input.next_including_whitespace().map(|t| t.clone()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should be able to use cloned() here too.

let mut flags = 0;
let result = {
let mut feature_name = &*ident;
// FIXME: remove extra indented block when lifetimes are non-lexical
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have an ETA on this? :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s a draft RFC and prototype: http://smallcultfollowing.com/babysteps/blog/2017/07/11/non-lexical-lifetimes-draft-rfc-and-prototype-available/ . I’m hopeful it’ll land in stable in 2018.

@SimonSapin
Copy link
Member Author

@bors-servo
Copy link
Contributor

📌 Commit eb98ae6 has been approved by emilio

@highfive highfive assigned emilio and unassigned asajeffrey Jul 24, 2017
@highfive highfive removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Jul 24, 2017
@highfive highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 24, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit eb98ae6 with merge 4f08211...

bors-servo pushed a commit that referenced this pull request Jul 24, 2017
Update cssparser to 0.18

Do not merge yet, depends on servo/rust-cssparser#171.

<!-- 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/17820)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ 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: emilio
Pushing 4f08211 to master...

@bors-servo bors-servo merged commit eb98ae6 into master Jul 24, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 24, 2017
@SimonSapin SimonSapin deleted the token-cache branch July 24, 2017 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants