Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upMake the style crate more concrete #12515
Conversation
|
I kinda skimmed parts of this because it's so enormous, but it's mechanical enough that this shouldn't be fine. I'm a little worried about throwing away such a painstakingly-built abstraction layer, especially around the style structs traits. But I also agree that it's causing problems, and as long as we're building both configurations on CI, it's probably ok. Thanks for cleaning this stuff up! @bors-servo r+ |
|
|
|
|
|
Yes, the only "problem" this brings up IMO is that is too easy to forget that you're working with two different sets of types when you're touching style (i.e., rust won't blame you if you use the style structs fields directly when touching Servo, but then everything will break for Gecko, and vice versa). I think the cleanup might be worth it though, those kind of errors would be caught on CI, and this should work just fine for new features. In case it doesn't, worst case is we could just learn from some mistakes we did (like putting the same type in two different traits, forcing to use a lot of extra where clauses), and make it better, but I can't find anything why this shouldn't work :) So yeah, r=me too. |
Doing this in a separate commit avoids mixups with the ComputedValues trait that the previous commit removed.
It is conditionally compiled to one implementation or the other (Gecko or Servo) with `#[cfg(…)]`.
|
About that abstraction, I’m skeptical that it ever could support a third user without significant amount of work. We were already using conditional compilation for some things anyway. Generics-everything can get hairy pretty quickly. In some of the code I touched there were more code in @emilio That’s a fair point. I considered keeping the traits around to formally encode what API is supposed to be shared, but then decided to go with inherent methods that don’t need to be imported. Rebased. @bors-servo r=bholley |
|
|
|
@bors-servo p=1 Large change somewhat likely to conflict with other PRs. |
Make the style crate more concrete Background: The changes to Servo code to support Stylo began in the `selectors` crate with making pseudo-elements generic, defined be the user, so that different users (such as Servo and Gecko/Stylo) could have a different set of pseudo-elements supported and parsed. Adding a trait makes sense there since `selectors` is in its own repository and has others users (or at least [one](https://github.com/SimonSapin/kuchiki)). Then we kind of kept going with the same pattern and added a bunch of traits in the `style` crate to make everything generic, allowing Servo and Gecko/Stylo to do things differently. But we’ve also added a `gecko` Cargo feature to do conditional compilation, at first to enable or disable some CSS properties and values in the Mako templates. Since we’re doing conditional compilation anyway, it’s often easier and simpler to do it more (with `#[cfg(feature = "gecko")]` and `#[cfg(feature = "servo")]`) that to keep adding traits and making everything generic. When a type is generic, any method that we want to call on it needs to be part of some trait. ---- The first several commits move some code around, mostly from `geckolib` to `style` (with `#[cfg(feature = "gecko")]`) but otherwise don’t change much. The following commits remove some traits and many type parameters through the `style` crate, replacing them with pairs of conditionally-compiled API-compatible items (types, methods, …). Simplifying code is nice to make it more maintainable, but this is motivated by another change described in #12391 (comment). (Porting Servo for that change proved difficult because some code in the `style` crate was becoming generic over `String` vs `Atom`, and this PR will help make that concrete. That change, in turn, is motivated by removing geckolib’s `[replace]` override for string-cache, in order to enable using a single Cargo "workspace" in this repository.) r? @bholley --- <!-- 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 #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [x] These changes do not require new tests because refactoring <!-- 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/12515) <!-- Reviewable:end -->
|
|
SimonSapin commentedJul 19, 2016
•
edited by larsbergstrom
Background:
The changes to Servo code to support Stylo began in the
selectorscrate with making pseudo-elements generic, defined be the user, so that different users (such as Servo and Gecko/Stylo) could have a different set of pseudo-elements supported and parsed. Adding a trait makes sense there sinceselectorsis in its own repository and has others users (or at least one).Then we kind of kept going with the same pattern and added a bunch of traits in the
stylecrate to make everything generic, allowing Servo and Gecko/Stylo to do things differently. But we’ve also added ageckoCargo feature to do conditional compilation, at first to enable or disable some CSS properties and values in the Mako templates. Since we’re doing conditional compilation anyway, it’s often easier and simpler to do it more (with#[cfg(feature = "gecko")]and#[cfg(feature = "servo")]) that to keep adding traits and making everything generic. When a type is generic, any method that we want to call on it needs to be part of some trait.The first several commits move some code around, mostly from
geckolibtostyle(with#[cfg(feature = "gecko")]) but otherwise don’t change much.The following commits remove some traits and many type parameters through the
stylecrate, replacing them with pairs of conditionally-compiled API-compatible items (types, methods, …).Simplifying code is nice to make it more maintainable, but this is motivated by another change described in #12391 (comment). (Porting Servo for that change proved difficult because some code in the
stylecrate was becoming generic overStringvsAtom, and this PR will help make that concrete. That change, in turn, is motivated by removing geckolib’s[replace]override for string-cache, in order to enable using a single Cargo "workspace" in this repository.)r? @bholley
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is