Skip to content

Implement column-count in stylo #13674

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.

Sign up for GitHub

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
Merged
merged 1 commit into from
Oct 10, 2016
Merged

Implement column-count in stylo #13674

merged 1 commit into from
Oct 10, 2016

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Oct 10, 2016 •
edited
Loading

Unblocks column-fill easy property
r? @emilio

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/gecko.mako.rs, components/style/properties/longhand/box.mako.rs, components/style/stylesheets.rs, components/style/properties/properties.mako.rs

@highfive
Copy link

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 10, 2016
emilio
emilio approved these changes Oct 10, 2016
View reviewed changes
components/style/properties/gecko.mako.rs Outdated
${impl_coord_copy('column_width', 'mColumnWidth')}

pub fn set_column_count(&mut self, v: longhands::column_count::computed_value::T) {
use std::cmp::min;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already import std::cmp at the top of the file. I think it looks cleaner just using cmp::min and dropping this import.

Also, a match might be a better bet for this instead of if let, but that's arguable:

self.gecko.mColumnCount = match v.0 {
    Some(number) => cmp::min(number, nsStyleColumn_kMaxColumnCount),
    None => NS_STYLE_COLUMN_COUNT_AUTO,
};

r=me with the first, and optionally with the second suggestion.

@Manishearth
Copy link
Member Author

@bors-servo r=emilio

@bors-servo
Copy link
Contributor

📌 Commit 4271167 has been approved by emilio

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 10, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit 4271167 with merge aef6054...

bors-servo pushed a commit that referenced this pull request Oct 10, 2016
Auto merge of #13674 - Manishearth:column_count, r=emilio
aef6054 
Implement column-count in stylo

Unblocks column-fill easy property
r? @emilio

<!-- 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/13674)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

@bors-servo bors-servo merged commit 4271167 into servo:master Oct 10, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 10, 2016
@Manishearth Manishearth deleted the column_count branch October 10, 2016 15:38
@upsuper
Copy link
Contributor

upsuper commented Oct 14, 2016

Oh, hmmm, eventually we have something breaks Windows build because of use of static constant from binding. What should we do then?

@Manishearth
Copy link
Member Author

Perhaps we should manually run bindgen on all platforms, extract the link names for just these constants, and use them?

@upsuper
Copy link
Contributor

upsuper commented Oct 14, 2016

I guess only Windows needs to be handled specially. I'm concertned about that manually running bindgen on different platforms would add too much work for people who do the merge.

Probably for this case we can just make kMaxColumnCount a macro or enum value in the Gecko side so we don't rely on the linkage.

And we should probably prioritize build-time bindgen I guess?

@upsuper
Copy link
Contributor

upsuper commented Oct 14, 2016

Actually it adds burden to patch writers as well.

@Manishearth
Copy link
Member Author

I'm concertned about that manually running bindgen on different platforms would add too much work for people who do the merge.

We only need to do that whenever a new static starts being used, no? I don't think the mangled name of kmaxcolumncount is going to change often, is it?

But yes, making it a macro or const is a better solution.

@upsuper
Copy link
Contributor

upsuper commented Oct 14, 2016

Probably macro works better currently. Adding an anonymous enum is not quite friendly to bindgen.

@upsuper
Copy link
Contributor

upsuper commented Oct 14, 2016

I'll open a bug and submit a patch... later today or next week. It seems to be a blocker for me.

@upsuper
Copy link
Contributor

upsuper commented Oct 14, 2016

(Or if anyone wants to fix that, feel free to.)

@upsuper upsuper mentioned this pull request Oct 14, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio emilio approved these changes

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Manishearth @highfive @bors-servo @upsuper @emilio