Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upinitial-letter property #16166
initial-letter property #16166
Conversation
highfive
commented
Mar 28, 2017
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon. |
highfive
commented
Mar 28, 2017
| match *self { | ||
| SpecifiedValue::Normal => try!(dest.write_str("normal")), | ||
| SpecifiedValue::Specified(size, Some(sink)) => | ||
| try!(write!(dest, "{} {}", size.to_css_string(), sink.to_css_string())), |
This comment has been minimized.
This comment has been minimized.
emilio
Mar 28, 2017
Member
let's do instead:
SpecifiedValue::Specified(size, sink) => {
try!(size.to_css(dest));
if let Some(sink) = sink {
try!(dest.write_str(" "));
try!(sink.to_css(dest));
}
}And remove the branch below.
| size = value; | ||
| } else { | ||
| return Err(()); | ||
| } |
This comment has been minimized.
This comment has been minimized.
emilio
Mar 28, 2017
Member
instead of this, just:
let size = try!(Number::parse_at_least_one(input));| return Err(()); | ||
| } | ||
|
|
||
| if let Ok(value) = Integer::parse(context, input) { |
This comment has been minimized.
This comment has been minimized.
emilio
Mar 28, 2017
Member
This needs to use input.try to avoid inadvertently treating invalid input as valid. For example, I suspect with your patch 1.5 5px would parse as 1.5. Could you add a test for this?
Let's write this instead as:
match input.try(|input| Integer::parse(context, input)) {
Ok(number) => {
if number.value() < 1 {
return Err(());
}
Ok(SpecifiedValue::Specified(size, Some(value))
}
Err(()) => Ok(SpecifiedValue::Specified(size, None)),
}|
Looks good to me with that bit addressed, thanks! |
|
|
||
| match input.try(|input| Integer::parse(context, input)) { | ||
| Ok(number) => { | ||
| if number.0 < 1 { |
This comment has been minimized.
This comment has been minimized.
emilio
Mar 28, 2017
Member
You need to rebase to account for #16144, which makes this not compile (you need to use number.value() instead of number.0).
|
Also, if you could squash, that'd be great! :) |
… property
|
@bors-servo r=emilio |
|
|
initial-letter property <!-- Please describe your changes on the following line: --> Implemented parsing and serialization for the initial-letter property. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15959 (github issue number if applicable). <!-- Either: --> - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- 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/16166) <!-- Reviewable:end -->
|
|
streichgeorg commentedMar 28, 2017
•
edited by larsbergstrom
Implemented parsing and serialization for the initial-letter property.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is