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

style: Multiple calc serialization fixes. #18131

Merged
merged 6 commits into from Aug 18, 2017

Conversation

emilio
Copy link
Member

@emilio emilio commented Aug 17, 2017

This puts us in line with the spec as written, except for caveat in w3c/csswg-drafts#1731.

Need to fix computed calc() too, I'm less confident that we won't need to change test expectations.


This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/values/computed/length.rs, components/style/values/computed/percentage.rs, components/style/values/specified/calc.rs
  • @canaltinova: components/style/values/computed/length.rs, components/style/values/computed/percentage.rs, components/style/values/specified/calc.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 17, 2017
@emilio
Copy link
Member Author

emilio commented Aug 17, 2017

r? @canaltinova or @nox

@highfive highfive assigned canova and unassigned jdm Aug 17, 2017
@emilio
Copy link
Member Author

emilio commented Aug 17, 2017

cc w3c/csswg-drafts#1731

Copy link
Contributor

@canova canova left a comment

Choose a reason for hiding this comment

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

It looks good to me. I think we can change the computed value as well.
r=me

//
// calc(10px + -5%)
//
// Need to update and run through try.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should change the computed value as well and test with a try run while we are at it. Is there a reason to prevents to do that right now?

Copy link
Member Author

Choose a reason for hiding this comment

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

The tree is closed.

Copy link
Member Author

Choose a reason for hiding this comment

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

(And I'd need to find a good revision, etc...)

I'll push a try run and if this hasn't landed tomorrow morning I'll just push it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I think this PR can wait, since it doesn't block anything, but either way is ok I guess.

@emilio
Copy link
Member Author

emilio commented Aug 17, 2017

@bors-servo r=canaltinova

  • Will update the computed one ASAP.

@bors-servo
Copy link
Contributor

📌 Commit 52d6838 has been approved by canaltinova

@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 Aug 17, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 52d6838 with merge 548ee68...

bors-servo pushed a commit that referenced this pull request Aug 18, 2017
style: Multiple calc serialization fixes.

This puts us in line with the spec as written, except for caveat in w3c/csswg-drafts#1731.

Need to fix computed calc() too, I'm less confident that we won't need to change test expectations.

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

💔 Test failed - linux-rel-wpt

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 18, 2017
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 18, 2017
@emilio
Copy link
Member Author

emilio commented Aug 18, 2017

@bors-servo r=canaltinova

@bors-servo
Copy link
Contributor

📌 Commit 02f09e1 has been approved by canaltinova

@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 Aug 18, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 02f09e1 with merge e12e555...

bors-servo pushed a commit that referenced this pull request Aug 18, 2017
style: Multiple calc serialization fixes.

This puts us in line with the spec as written, except for caveat in w3c/csswg-drafts#1731.

Need to fix computed calc() too, I'm less confident that we won't need to change test expectations.

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

💔 Test failed - linux-dev

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 18, 2017
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 18, 2017
@emilio
Copy link
Member Author

emilio commented Aug 18, 2017

@bors-servo r=canaltinova p=1

  • The manifest :(

@bors-servo
Copy link
Contributor

📌 Commit 6df597b has been approved by canaltinova

@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 Aug 18, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 6df597b with merge 92176d1...

bors-servo pushed a commit that referenced this pull request Aug 18, 2017
style: Multiple calc serialization fixes.

This puts us in line with the spec as written, except for caveat in w3c/csswg-drafts#1731.

Need to fix computed calc() too, I'm less confident that we won't need to change test expectations.

<!-- 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/18131)
<!-- 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-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: canaltinova
Pushing 92176d1 to master...

@bors-servo bors-servo merged commit 6df597b into servo:master Aug 18, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 18, 2017
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 18, 2017
…#18131. r=bholley

calc() serialization in stylo changed to align to the spec more closely (modulo percentage order).

See the linked issue.

MozReview-Commit-ID: GyzZvdumMSe
bholley pushed a commit to bholley/gecko that referenced this pull request Aug 19, 2017
…#18131. r=bholley

calc() serialization in stylo changed to align to the spec more closely (modulo percentage order).

See the linked issue.

MozReview-Commit-ID: GyzZvdumMSe
@emilio emilio deleted the calc-serialization branch August 20, 2017 07:42
aethanyc pushed a commit to aethanyc/gecko-dev that referenced this pull request Aug 21, 2017
…#18131. r=bholley

calc() serialization in stylo changed to align to the spec more closely (modulo percentage order).

See the linked issue.

MozReview-Commit-ID: GyzZvdumMSe
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…#18131. r=bholley

calc() serialization in stylo changed to align to the spec more closely (modulo percentage order).

See the linked issue.

MozReview-Commit-ID: GyzZvdumMSe

UltraBlame original commit: 7b4da6a65ea1dcc665c9aec416223ea6e70192af
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
…#18131. r=bholley

calc() serialization in stylo changed to align to the spec more closely (modulo percentage order).

See the linked issue.

MozReview-Commit-ID: GyzZvdumMSe

UltraBlame original commit: 7b4da6a65ea1dcc665c9aec416223ea6e70192af
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…#18131. r=bholley

calc() serialization in stylo changed to align to the spec more closely (modulo percentage order).

See the linked issue.

MozReview-Commit-ID: GyzZvdumMSe

UltraBlame original commit: 7b4da6a65ea1dcc665c9aec416223ea6e70192af
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants