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

Port to the new cssparser. #4689

Merged
merged 1 commit into from Jan 21, 2015

Conversation

Projects
None yet
5 participants
@SimonSapin
Copy link
Member

SimonSapin commented Jan 20, 2015

@hoppipolla-critic-bot

This comment has been minimized.

Copy link

hoppipolla-critic-bot commented Jan 20, 2015

Critic review: https://critic.hoppipolla.co.uk/r/3796

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@pcwalton

This comment has been minimized.

Copy link
Contributor

pcwalton commented Jan 20, 2015

@larsbergstrom How would you like to split this up?

@larsbergstrom

This comment has been minimized.

Copy link
Contributor

larsbergstrom commented Jan 20, 2015

@pcwalton I was just going to review it since you are on PTO today and am happy to either let you review it or divide it up however you would like.

@SimonSapin

This comment has been minimized.

Copy link
Member Author

SimonSapin commented Jan 20, 2015

I just realized I forgot to write any docs. Sorry!

I’ll write more down tomorrow. In the meantime, the main important point is the convention for parsing functions:

  • Take (at least) a input: &mut cssparser::Parser parameter
  • Return Result<_, ()>
  • When returning Ok(_), the function must have consume exactly the amount of input that represents the parsed value.
  • When returning Err(()), any amount of input may have been consumed.

As a consequence, when calling another parsing function, either:

  • Any Err(()) return value must be propagated. This happens by definition for tail calls, and can otherwise be done with the try! macro.
  • Or the call must be wrapped in a cssparser::Parser::try call. try takes a closure that takes a Parser and returns a Result, calls it once, and returns itself that same result. If the result is Err, it restores the position inside the input to the one saved before calling the closure.

Examples:

// 'none' | <image>
fn parse_background_image(context: &ParserContext, input: &mut Parser) 
                                    -> Result<Option<Image>, ()> {
    if input.try(|input| input.expect_ident_matching("none")).is_ok() {
        Ok(None)
    } else {
        Image::parse(context, input).map(Some)  // tail call
    }
}
// [ <length> | <percentage> ] [ <length> | <percentage> ]?
fn parse_border_spacing(_context: &ParserContext, input: &mut Parser) 
                          -> Result<(LengthOrPercentage, LengthOrPercentage), ()> {
    let first = try!(LengthOrPercentage::parse);
    let second = input.try(LengthOrPercentage::parse).unwrap_or(first);
    (first, second)
}

@SimonSapin SimonSapin force-pushed the newnewnewcss branch from 2c9567f to d034a6c Jan 21, 2015

@SimonSapin

This comment has been minimized.

Copy link
Member Author

SimonSapin commented on d034a6c Jan 21, 2015

r=larsbergstrom

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 21, 2015

saw approval from larsbergstrom
at d034a6c

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 21, 2015

merging servo/servo/newnewnewcss = d034a6c into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 21, 2015

servo/servo/newnewnewcss = d034a6c merged ok, testing candidate = 59bca29

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 21, 2015

fast-forwarding master to auto = 59bca29

@SimonSapin

This comment has been minimized.

Copy link
Member Author

SimonSapin commented on d034a6c Jan 21, 2015

r=larsbergstrom

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 21, 2015

saw approval from larsbergstrom
at d034a6c

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 21, 2015

merging servo/servo/newnewnewcss = d034a6c into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 21, 2015

servo/servo/newnewnewcss = d034a6c merged ok, testing candidate = 59bca29

This comment has been minimized.

Copy link
Contributor

bors-servo replied Jan 21, 2015

fast-forwarding master to auto = 59bca29

bors-servo pushed a commit that referenced this pull request Jan 21, 2015

@bors-servo bors-servo closed this Jan 21, 2015

@bors-servo bors-servo merged commit d034a6c into master Jan 21, 2015

1 check passed

default all tests passed

@SimonSapin SimonSapin deleted the newnewnewcss branch Jan 21, 2015

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.