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.

Sign up for GitHub

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

Jump to bottom

Don't reject out of bound calc() values at parsing-time #16382

Merged
merged 2 commits into from Apr 12, 2017
Merged

Don't reject out of bound calc() values at parsing-time #16382

merged 2 commits into from Apr 12, 2017

Conversation

nox
Copy link
Contributor

@nox nox commented Apr 12, 2017 •
edited by larsbergstrom

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/gecko.mako.rs, components/style/values/specified/mod.rs, components/style/values/specified/length.rs, components/style/properties/longhand/box.mako.rs, components/style/gecko_bindings/sugar/ns_timing_function.rs
  • @emilio: components/style/properties/gecko.mako.rs, components/style/values/specified/mod.rs, components/style/values/specified/length.rs, components/style/properties/longhand/box.mako.rs, components/style/gecko_bindings/sugar/ns_timing_function.rs

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

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!

emilio
emilio approved these changes Apr 12, 2017
View reviewed changes
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 awesome! r=me % nits

components/style/values/specified/mod.rs Outdated
@@ -690,11 +698,11 @@ impl ToCss for Time {
#[allow(missing_docs)]
pub struct Number {
/// The numeric value itself.
pub value: CSSFloat,
value: CSSFloat,
/// Whether this came from a `calc()` expression. This is needed for
/// serialization purposes, since `calc(1)` should still serialize to
/// `calc(1)`, not just `1`.
Copy link
Member

Choose a reason for hiding this comment

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

Update the comment?

components/style_traits/values.rs Outdated
match *self {
AllowedNumericType::NonNegative if val < 0. => 0.,
AllowedNumericType::AtLeastOne if val < 1. => 1.,
_ => val,
Copy link
Member

Choose a reason for hiding this comment

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

I'd just add AllowedNumericType::All here.

emilio
emilio reviewed Apr 12, 2017
View reviewed changes
components/style/values/specified/mod.rs Outdated
parse_number_with_clamping_mode(input, AllowedNumericType::All)
}

#[allow(missing_docs)]
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we'd have something here, but I guess I won't complain

@emilio
Copy link
Member

emilio commented Apr 12, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 3dd4e88 has been approved by emilio

@highfive highfive assigned emilio and unassigned glennw Apr 12, 2017
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 12, 2017
@bors-servo
Copy link
Contributor

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

@highfive highfive added S-needs-rebase There are merge conflict errors. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 12, 2017
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 12, 2017
@nox
Copy link
Contributor Author

nox commented Apr 12, 2017

@bors-servo r=emilio

@bors-servo
Copy link
Contributor

📌 Commit dfbd898 has been approved by emilio

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

⌛ Testing commit dfbd898 with merge 294f44e...

bors-servo pushed a commit that referenced this pull request Apr 12, 2017
Auto merge of #16382 - nox:calc-clamping, r=emilio
294f44e 
Don't reject out of bound calc() values at parsing-time

<!-- 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/16382)
<!-- 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-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: emilio
Pushing 294f44e to master...

@bors-servo bors-servo merged commit dfbd898 into servo:master Apr 12, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 12, 2017
@nox nox mentioned this pull request Apr 13, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio emilio approved these changes

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nox @highfive @emilio @bors-servo @glennw