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

Jump to bottom

Split length values into their own file #13584

Closed
Manishearth opened this issue Oct 4, 2016 · 6 comments
Closed

Split length values into their own file #13584

Manishearth opened this issue Oct 4, 2016 · 6 comments
Labels
A-content/css C-assigned C-has open PR E-easy I-refactor

Comments

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Oct 4, 2016

components/style/values/computed/mod.rs and components/style/values/specified/mod.rs are huge. Those modules have submodules (position and basic_shape) for groups of CSS values related to particular properties, and we should add more.

A lot of the length-related stuff could be moved into a new module length in values/specified/length.rs (same for computed). Lengths are imported everywhere (and it's convenient to import directly from style::values, so we should reexport the moved types in mod.rs (via pub use length::SomeType).

The types that this would include would basically be anything in those files with Length, Calc, or Percentage in its name, as well as CharacterWidth. The impls related to those would need to be moved too.
@highfive
Copy link

@highfive highfive commented Oct 4, 2016

Please make a comment here if you intend to work on this issue. Thank you!
@shubheksha
Copy link
Contributor

@shubheksha shubheksha commented Oct 4, 2016

I'll take this up!
@KiChjang
Copy link
Member

@KiChjang KiChjang commented Oct 19, 2016

@shubheksha Is this still being worked on?
@jdm jdm removed the C-assigned label Nov 4, 2016
@jdm
Copy link
Member

@jdm jdm commented Nov 4, 2016

Unassigning due to lack of activity.
@kivikakk
Copy link
Contributor

@kivikakk kivikakk commented Nov 5, 2016

I'll do this now. :)
@kivikakk
Copy link
Contributor

@kivikakk kivikakk commented Nov 5, 2016 •
edited

Curious if it makes sense to move BorderRadiusSize along with these. It's not a length as such, so inclined not to.

(Likewise for BorderWidth. I note we're moving CharacterWidth, which makes me think maybe yes.)
kivikakk pushed a commit to kivikakk/servo that referenced this issue Nov 5, 2016
Yuki Izumi
Refactor style lengths per servo#13584
Unverified
No user is associated with the committer email.
Learn about signing commits
10d1e28
kivikakk pushed a commit to kivikakk/servo that referenced this issue Nov 5, 2016
Yuki Izumi
Refactor style lengths per servo#13584
Unverified
No user is associated with the committer email.
Learn about signing commits
e7a94a0
@kivikakk kivikakk mentioned this issue Nov 5, 2016
Merged
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Nov 5, 2016
@bors-servo
Auto merge of #14077 - kivikakk:refactor-style-lengths, r=emilio
Loading status checks…
5be0a31 
Refactor style lengths per #13584

<!-- Please describe your changes on the following line: -->
Refactor `components/style/values/{computed,specified}/mod.rs` into `…/length.rs` per #13584.

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

<!-- Either: -->
- [x] These changes do not require tests because the existing code should be covered by tests

<!-- 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/14077)
<!-- Reviewable:end -->
kivikakk pushed a commit to kivikakk/servo that referenced this issue Nov 6, 2016
Yuki Izumi
Refactor style lengths per servo#13584
Unverified
No user is associated with the committer email.
Learn about signing commits
f33f560
bors-servo added a commit that referenced this issue Nov 6, 2016
@bors-servo
Auto merge of #14077 - kivikakk:refactor-style-lengths, r=emilio
Loading status checks…
8c35f93 
Refactor style lengths per #13584

<!-- Please describe your changes on the following line: -->
Refactor `components/style/values/{computed,specified}/mod.rs` into `…/length.rs` per #13584.

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

<!-- Either: -->
- [x] These changes do not require tests because the existing code should be covered by tests

<!-- 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/14077)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-content/css C-assigned C-has open PR E-easy I-refactor
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Refactor style lengths per #13584
6 participants
@kivikakk @jdm @Manishearth @KiChjang @highfive @shubheksha
You can’t perform that action at this time.