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

Add some CSS keywords as static atoms. #21

Merged
merged 1 commit into from Oct 4, 2014
Merged

Conversation

@glennw
Copy link
Member

glennw commented Oct 2, 2014

@SimonSapin
Copy link
Member

SimonSapin commented Oct 2, 2014

Why is this needed? So far we codegen a custom enum (with data for the values) for CSS properties, so we don’t store their name as a string.

@glennw
Copy link
Member Author

glennw commented Oct 2, 2014

The strings are stored here (in python as temps) - https://github.com/servo/servo/blob/master/components/style/properties/mod.rs.mako#L55. I am prototyping getComputedStyle, building on jdm's cssom prototype branch. It has functions like https://github.com/jdm/servo/blob/cssom/components/style/properties/mod.rs.mako#L2421, which can be more efficient if we store the names as static atoms.

Ping me on IRC if you want to discuss more or if there's a better way to achieve this, I'm still finding my way around the DOM / script code.

@SimonSapin
Copy link
Member

SimonSapin commented Oct 2, 2014

I am prototyping getComputedStyle, building on jdm's cssom prototype branch. It has functions like https://github.com/jdm/servo/blob/cssom/components/style/properties/mod.rs.mako#L2421, which can be more efficient if we store the names as static atoms.

Ok.

It just seems unfortunate to have a list of CSS properties not just in a different file, but in a different repository. I’m pretty sure we’ll (at least) occasionally fail to update string-cache when adding new properties to Servo. This wouldn’t affect correctness, but still.

I’ve filed #22 about making string-cache generic over the list of static strings. If that happens, Servo could build its list from multiple sources, including the style crate’s codegen.

@glennw
Copy link
Member Author

glennw commented Oct 2, 2014

Actually, once the code lands in master (if it works out the way I'm hoping it will, and if you're happy with the change) that won't be possible. If you declare a static atom that doesn't exist in that table, it's a compile time error due to the way @kmcallister implemented it :)

@SimonSapin
Copy link
Member

SimonSapin commented Oct 2, 2014

What does “declare a static atom” mean, and does the style crate do it?

@glennw
Copy link
Member Author

glennw commented Oct 2, 2014

It doesn't currently do that, but I'm hoping it will do in the future, if this stuff works out.

If you write atom!("color") that creates a static atom (which is an integer, created with a perfect hash function) that causes a compile time error if it doesn't exist in the static atoms table.

@SimonSapin
Copy link
Member

SimonSapin commented Oct 3, 2014

Ok, that’s good.

@kmcallister
Copy link
Contributor

kmcallister commented Oct 4, 2014

r+

kmcallister added a commit that referenced this pull request Oct 4, 2014
Add some CSS keywords as static atoms.
@kmcallister kmcallister merged commit 124b891 into servo:master Oct 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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