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

Refactor box longhands #14913

Merged
merged 7 commits into from Jan 8, 2017
Merged

Refactor box longhands #14913

merged 7 commits into from Jan 8, 2017
+220 −395

Conversation

@canova
Copy link
Member

canova commented Jan 8, 2017 •
edited by larsbergstrom

Converted some longhands into vector_longhand to reduce some repeated codes.
Also some of longhands with same SpecifiedValue and computed_value::T changed. Re-exported SpecifiedValue instead of computed_value::T. Normally a computed_value can't have a parse function.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because they're not changing the behavior of the longhands.

This change is Reviewable
canova added 2 commits Jan 7, 2017
@canova
Convert transition-duration longhand into vector_longhand
55ae25c
@canova
Fix SingleSpecifiedValue in vector_longhand
77368ac
@highfive
Copy link

highfive commented Jan 8, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/shorthand/box.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/animation.rs
  • @emilio: components/style/properties/shorthand/box.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/animation.rs
@canova
Copy link
Member Author

canova commented Jan 8, 2017

r? @Manishearth
@highfive highfive assigned Manishearth and unassigned glennw Jan 8, 2017
@canova
Copy link
Member Author

canova commented Jan 8, 2017

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2017

⌛ Trying commit c4d0ee7 with merge 330ae39...
bors-servo added a commit that referenced this pull request Jan 8, 2017
@bors-servo
Auto merge of #14913 - canaltinova:box-longhands, r=<try>
Loading status checks…
330ae39 
Refactor box longhands

<!-- Please describe your changes on the following line: -->
Converted some longhands into `vector_longhand` to reduce some repeated codes.
Also some of longhands with same `SpecifiedValue` and `computed_value::T` changed. Re-exported SpecifiedValue instead of computed_value::T. Normally a computed_value can't have a parse function.

---
<!-- 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

<!-- Either: -->
- [X] These changes do not require tests because they're not changing the behavior of the longhands.

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

bors-servo commented Jan 8, 2017

💔 Test failed - linux-dev
@canova
canova reviewed Jan 8, 2017
View changes
tests/unit/style/properties/serialization.rs Outdated
use style::properties::longhands::transition_duration::SpecifiedValue as DurationContainer;
use style::properties::longhands::transition_property::SpecifiedValue as PropertyContainer;
use style::properties::longhands::transition_timing_function::SpecifiedValue as TimingContainer;
use style::properties::longhands::transition_timing_function::TransitionTimingFunction;
use style::properties::longhands::transition_timing_function::single_value::SpecifiedValue as TransitionTimingFunction;

This comment has been minimized.

Sign in to view
@canova

canova Jan 8, 2017

Author Member

How can I indent this properly?

This comment has been minimized.

Sign in to view
@Manishearth

Manishearth Jan 8, 2017

Member

I don't think you can. use style::properties::longhands::transition_timing_function and then transition_timing_function::whatever in the code might work better (replace all TTF imports if you're doing this)
@Manishearth
Copy link
Member

Manishearth commented Jan 8, 2017

I think empty vectors were allowed for many of these properties. You need to explicitly enable that with allow_empty=True. Aside from that, looks good to me

@bors-servo delegate+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2017

✌️ @canaltinova can now approve this pull request
@canova
Copy link
Member Author

canova commented Jan 8, 2017

I especially looked for allow_empty but it looks like other properties don't allow none keyword. Generally other properties are like this: <time> #
canova added 5 commits Jan 7, 2017
@canova
Convert transition-timing-function longhand into vector_longhand
8e21d65
@canova
Convert transition-property longhand into vector_lonhhand
164e8f5
@canova
Convert animation-name longhand into vector_longhand
3934f50
@canova
Convert animation-iteration-count longhand into vector_longhand
227737c
@canova
Re-export SpecifiedValue instead of computed_value::T on other longhands
Loading status checks…
f169301
@canova canova force-pushed the canova:box-longhands branch from c4d0ee7 to f169301 Jan 8, 2017
@highfive highfive removed the S-tests-failed label Jan 8, 2017
@canova
Copy link
Member Author

canova commented Jan 8, 2017

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

bors-servo commented Jan 8, 2017

📌 Commit f169301 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2017

⌛ Testing commit f169301 with merge 0d5e021...
bors-servo added a commit that referenced this pull request Jan 8, 2017
@bors-servo
Auto merge of #14913 - canaltinova:box-longhands, r=Manishearth
Loading status checks…
0d5e021 
Refactor box longhands

<!-- Please describe your changes on the following line: -->
Converted some longhands into `vector_longhand` to reduce some repeated codes.
Also some of longhands with same `SpecifiedValue` and `computed_value::T` changed. Re-exported SpecifiedValue instead of computed_value::T. Normally a computed_value can't have a parse function.

---
<!-- 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

<!-- Either: -->
- [X] These changes do not require tests because they're not changing the behavior of the longhands.

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

bors-servo commented Jan 8, 2017

💔 Test failed - linux-rel-wpt
@jdm
Copy link
Member

jdm commented Jan 8, 2017

@bors-servo: retry
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 2017

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev are reusable. Rebuilding only linux-rel-wpt...
@bors-servo
Copy link
Contributor

bors-servo commented Jan 8, 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-gnu-dev, windows-msvc-dev
@bors-servo bors-servo merged commit f169301 into servo:master Jan 8, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@hiikezoe hiikezoe mentioned this pull request Mar 1, 2017
Closed
3 of 4 tasks complete
@canova canova deleted the canova:box-longhands branch Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Manishearth Manishearth

Assignees

@Manishearth Manishearth

Labels
S-tests-failed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@canova @highfive @bors-servo @Manishearth @jdm @glennw
You can’t perform that action at this time.