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

Conversation

Projects
None yet
5 participants
@SimonSapin
Copy link
Member

commented Jul 21, 2017

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


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jul 21, 2017

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

This comment has been minimized.

Copy link

commented Jul 21, 2017

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

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

⌛️ Trying commit 3a5c6b0 with merge c3738e8...

bors-servo added a commit that referenced this pull request Jul 21, 2017

Auto merge of #17820 - servo:token-cache, r=<try>
Update cssparser to 0.18

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

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

💔 Test failed - linux-rel-wpt

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

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

@SimonSapin SimonSapin force-pushed the token-cache branch from 3a5c6b0 to 584c994 Jul 21, 2017

@SimonSapin SimonSapin force-pushed the token-cache branch from 584c994 to 17b8577 Jul 21, 2017

@SimonSapin

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

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

bors-servo added a commit that referenced this pull request Jul 21, 2017

Auto merge of #17820 - servo:token-cache, r=<try>
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

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

@SimonSapin SimonSapin force-pushed the token-cache branch from 17b8577 to 80047f9 Jul 22, 2017

@SimonSapin SimonSapin referenced this pull request Jul 23, 2017

Merged

Add a one-token cache #171

@emilio

emilio approved these changes Jul 23, 2017

Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
@emilio

emilio Jul 23, 2017

Member

nit: cloned().

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Jul 23, 2017

Author Member

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

This comment has been minimized.

Copy link
@emilio

emilio Jul 23, 2017

Member

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

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Jul 24, 2017

Author Member

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

This comment has been minimized.

Copy link
@emilio

emilio Jul 23, 2017

Member

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

This comment has been minimized.

Copy link
@emilio

emilio Jul 23, 2017

Member

Do we have an ETA on this? :(

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Jul 23, 2017

Author Member

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 SimonSapin force-pushed the token-cache branch from 80047f9 to 3f7decd Jul 24, 2017

@SimonSapin SimonSapin force-pushed the token-cache branch from 3f7decd to eb98ae6 Jul 24, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

📌 Commit eb98ae6 has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

⌛️ Testing commit eb98ae6 with merge 4f08211...

bors-servo added a commit that referenced this pull request Jul 24, 2017

Auto merge of #17820 - servo:token-cache, r=emilio
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

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

@bors-servo bors-servo merged commit eb98ae6 into master Jul 24, 2017

4 checks passed

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

@SimonSapin SimonSapin deleted the token-cache branch Jul 24, 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.