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

Be consistent about where ToComputedValue implementations live. #19670

Open
emilio opened this issue Jan 1, 2018 · 3 comments
Open

Be consistent about where ToComputedValue implementations live. #19670

emilio opened this issue Jan 1, 2018 · 3 comments
Labels
A-content/css Interacting with CSS from web content (parsing, serializing, introspection)

Comments

@emilio
Copy link
Member

emilio commented Jan 1, 2018

Right now:

$ cd components/style/values/specified
$ git grep 'impl ToComputedValue' | wc -l
33
$ ../computed
$ git grep 'impl ToComputedValue' | wc -l
14

So that's... not very consistent.

@emilio emilio added the A-content/css Interacting with CSS from web content (parsing, serializing, introspection) label Jan 1, 2018
@emilio
Copy link
Member Author

emilio commented Jan 1, 2018

We should decide whether they should live in computed or specified. I don't have a particularly strong preference, and the only argument I've heard (from @mauricioc and @nox IIRC) is that ideally specified modules shouldn't depend on computed ones, and making complex cases depend on that makes the code intrincate.

I think that's a valid point, so lacking other arguments I'd be fine moving all them to computed. But I bet there can be other arguments in the other direction, so given this isn't particularly urgent we can wait and discuss for as long as needed :)

@CYBAI
Copy link
Member

CYBAI commented Jan 2, 2018

IMO, I'll vote for impl ToComputedValue inside specified modules because I think it's reasonable to make associated types of impl ToComputedValue with its computed module.
We can see that we'd like to implement ToComputedValue trait and make its associated type with its own computed module.

I'm also fine for moving all of them in specified modules but just share my opinion.

@nox
Copy link
Contributor

nox commented Jan 2, 2018

IIRC, I prefer to put them in specified, but I don't remember it well and I'm still on holidays. We can discuss about that next Monday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/css Interacting with CSS from web content (parsing, serializing, introspection)
Projects
None yet
Development

No branches or pull requests

3 participants