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

Track only the number of important declarations in a declaration block #12959

Closed
emilio opened this issue Aug 21, 2016 · 4 comments
Closed

Track only the number of important declarations in a declaration block #12959

emilio opened this issue Aug 21, 2016 · 4 comments

Comments

@emilio
Copy link
Member

@emilio emilio commented Aug 21, 2016

Once #12943 lands, DeclarationBlock will have two fields, normal_count and important_count that track the amount of declarations with a given priority are in that block.

We should be able to remove one of the two fields, and compute the other one in terms of the length of the declarations vector and the other, simplifying the code that computes it.

Code:

  • components/style/properties/properties.mako.rs

Tests:

  • No need to write new ones, the existing tests should cover it. You'll have to modify test/units/style to take the change into account.
@highfive
Copy link

@highfive highfive commented Aug 21, 2016

Please make a comment here if you intend to work on this issue. Thank you!

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Aug 21, 2016

Run ./mach test-unit to try changes to tests/unit.

@cers
Copy link
Contributor

@cers cers commented Aug 21, 2016

I'd like to give it a go, but this would be my first foray into servo (and indeed rust).

Should the field be removed completely, or should the calculation of it just be moved to the end of the parsing, when it can be calculated by subtracting the important ones from the total?

If completely removed, then I guess some consideration is needed to components/script/dom/element.rs and components/style/selector_matching.rs, which both seem to reference the normal_count of a style rule/block.

@emilio
Copy link
Member Author

@emilio emilio commented Aug 21, 2016

Sure, it's yours! :)

Yes, the idea is removing it completely, and yes, the calculations in element.rs and selector_matching.rs should be modified appropriately.

If you have any more doubts, don't hesitate in reaching out :)

@emilio emilio added the C-assigned label Aug 21, 2016
bors-servo added a commit that referenced this issue Aug 22, 2016
Track only the number of important declarations in a declaration block

<!-- Please describe your changes on the following line: -->
Track only the number of important declarations in a declaration block

---
<!-- 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 #12959 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because "the existing tests should cover it"

<!-- 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/12969)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.