Skip to content

/ servo

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

style: Multiple cleanups around parsing code. #16283

Merged
merged 3 commits into from Apr 6, 2017
Merged

style: Multiple cleanups around parsing code. #16283

merged 3 commits into from Apr 6, 2017

Conversation

@emilio
Copy link
Member

emilio commented Apr 6, 2017 •
edited by larsbergstrom

This change is Reviewable
@highfive
Copy link

highfive commented Apr 6, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/longhand/background.mako.rs, components/style/stylesheets.rs, components/style/properties/shorthand/position.mako.rs, components/style/properties/longhand/font.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/values/specified/length.rs, components/style/viewport.rs, components/style/values/specified/grid.rs
@highfive
Copy link

highfive commented Apr 6, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@emilio
Copy link
Member Author

emilio commented Apr 6, 2017

r? @wafflespeanut or @Manishearth or @canaltinova
@highfive highfive assigned wafflespeanut and unassigned jdm Apr 6, 2017
@wafflespeanut
wafflespeanut approved these changes Apr 6, 2017
View changes
Copy link
Member

wafflespeanut left a comment

Nice cleanup! r=me after addressing the nits
components/style/values/specified/grid.rs
}

try!(input.expect_function_matching("fit-content"));
// FIXME(emilio): This needs a parse_nested_block, doesn't it?

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut Apr 6, 2017

Member

I've fixed this as a part of #16067 - could this be removed?

This comment has been minimized.

Sign in to view
@emilio

emilio Apr 6, 2017

Author Member

Given that hasn't merged yet, and this needs a rebase anyway, I'd rather leave it there until that merges (btw, let me know when is it ready for re-review! I just saw a bunch of comments addressed that I wouldn't have noticed otherwise).
components/style/values/specified/grid.rs Outdated
let inflexible_breadth =
match input.try(LengthOrPercentage::parse_non_negative) {
Ok(lop) => TrackBreadth::Breadth(lop),
Err(..) => TrackBreadth::Keyword(try!(TrackKeyword::parse(input))),

This comment has been minimized.

Sign in to view
@wafflespeanut

wafflespeanut Apr 6, 2017

Member

Nit: Clearly TrackKeyword::parse(input).map(TrackBreadth::Keyword) is better, right?

This comment has been minimized.

Sign in to view
@emilio

emilio Apr 6, 2017

Author Member

I don't strictly think so, it creates an unnecessary TrackBreadth when parsing fails, but I could split it like:

let keyword = try!(TrackKeyword::parse(..));
TrackBreadth::Keyword(keyword)

which is a bit nicer to read.
@emilio emilio force-pushed the emilio:cleanup-parse-non-negative branch 2 times, most recently from 06a1376 to d947d98 Apr 6, 2017
@emilio
Copy link
Member Author

emilio commented Apr 6, 2017

@bors-servo r=WafflesPeanut
@bors-servo
Copy link
Contributor

bors-servo commented Apr 6, 2017

📌 Commit d947d98 has been approved by WafflesPeanut
@bors-servo
Copy link
Contributor

bors-servo commented Apr 6, 2017

⌛ Testing commit d947d98 with merge 9fb8192...
bors-servo added a commit that referenced this pull request Apr 6, 2017
@bors-servo
Auto merge of #16283 - emilio:cleanup-parse-non-negative, r=WafflesPe…
Loading status checks…
9fb8192 
…anut

style: Multiple cleanups around parsing code.

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

bors-servo commented Apr 6, 2017

💔 Test failed - mac-dev-unit
@wafflespeanut
Copy link
Member

wafflespeanut commented Apr 6, 2017

Looks like the unit tests need to be updated (just an import).
emilio added 3 commits Apr 6, 2017
@emilio
style: Cleanup viewport rule parsing.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
726f918
@emilio
style: Multiple style nits across parsing code.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
705ecb4
@emilio
style: More cleanups around length parsing and viewport rule parsing.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
Loading status checks…
846c950
@emilio emilio force-pushed the emilio:cleanup-parse-non-negative branch from d947d98 to 846c950 Apr 6, 2017
@emilio
Copy link
Member Author

emilio commented Apr 6, 2017

@bors-servo r=Wafflespeanut
@bors-servo
Copy link
Contributor

bors-servo commented Apr 6, 2017

📌 Commit 846c950 has been approved by Wafflespeanut
@bors-servo
Copy link
Contributor

bors-servo commented Apr 6, 2017

⌛ Testing commit 846c950 with merge 208d5db...
bors-servo added a commit that referenced this pull request Apr 6, 2017
@bors-servo
Auto merge of #16283 - emilio:cleanup-parse-non-negative, r=Wafflespe…
Loading status checks…
208d5db 
…anut

style: Multiple cleanups around parsing code.

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

bors-servo commented Apr 6, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: Wafflespeanut
Pushing 208d5db to master...
@bors-servo bors-servo merged commit 846c950 into servo:master Apr 6, 2017
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Apr 6, 2017
Merged
4 of 4 tasks complete
@emilio emilio deleted the emilio:cleanup-parse-non-negative branch Apr 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wafflespeanut wafflespeanut

Assignees

@wafflespeanut wafflespeanut

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@emilio @highfive @bors-servo @wafflespeanut @jdm
You can’t perform that action at this time.