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

Cleanup various modules and introduce generic types #16444

Merged
merged 10 commits into from Apr 25, 2017

Conversation

@wafflespeanut
Copy link
Member

commented Apr 13, 2017

Almost all the types in values/specified and values/computed share their ToCss implementations. The only reason they have duplicated code hanging around is a result of different specified and computed forms for types like LengthOrPercentage. This PR makes some of these types generic so that we could have a common definition and ToCss impl (Parse and ToComputedValue impls too, if possible).


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Apr 13, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/gecko.mako.rs, components/style/values/specified/basic_shape.rs, components/style/gecko/conversions.rs, components/style/values/mod.rs, components/style/values/computed/length.rs, components/style/values/specified/mod.rs, components/style/values/computed/basic_shape.rs, components/style/values/generics/basic_shape.rs, components/style/values/computed/mod.rs, components/style/properties/helpers/animated_properties.mako.rs, components/style/values/specified/length.rs, components/style/gecko/values.rs, components/style/values/generics/mod.rs
  • @emilio: components/style/properties/gecko.mako.rs, components/style/values/specified/basic_shape.rs, components/style/gecko/conversions.rs, components/style/values/mod.rs, components/style/values/computed/length.rs, components/style/values/specified/mod.rs, components/style/values/computed/basic_shape.rs, components/style/values/generics/basic_shape.rs, components/style/values/computed/mod.rs, components/style/properties/helpers/animated_properties.mako.rs, components/style/values/specified/length.rs, components/style/gecko/values.rs, components/style/values/generics/mod.rs
@highfive

This comment has been minimized.

Copy link

commented Apr 13, 2017

warning Warning warning

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

@wafflespeanut wafflespeanut force-pushed the wafflespeanut:generics branch 4 times, most recently from 24b1a01 to 935986e Apr 13, 2017

@wafflespeanut wafflespeanut changed the title WIP: Extract generic types out into a new module Cleanup various modules and introduce generic types Apr 16, 2017

@wafflespeanut wafflespeanut force-pushed the wafflespeanut:generics branch 3 times, most recently from 421fad0 to 1bc602b Apr 16, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2017

☔️ The latest upstream changes (presumably #16494) made this pull request unmergeable. Please resolve the merge conflicts.

@wafflespeanut wafflespeanut force-pushed the wafflespeanut:generics branch from 2d4b3fc to 6230db9 Apr 17, 2017

@wafflespeanut

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2017

Ready for review! Anyone? (cc @Manishearth @emilio)

@wafflespeanut wafflespeanut force-pushed the wafflespeanut:generics branch 3 times, most recently from 4a436fa to 60819c0 Apr 17, 2017

@@ -188,6 +188,13 @@ pub enum LengthOrPercentage {
Calc(CalcLengthOrPercentage),
}

impl Default for LengthOrPercentage {

This comment has been minimized.

Copy link
@emilio

emilio Apr 18, 2017

Member

Why? I don't think this is necessarily a good default. Ultimately defaults should be per-property, so I think if something doesn't have a clear default we shouldn't implement it.

This comment has been minimized.

Copy link
@emilio

emilio Apr 18, 2017

Member

And actually, what you want is a Zero trait. We already depend on something similar for euclid (num_traits?), so let's use that.

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Apr 20, 2017

Author Member

We don't seem to use this a lot, so I've moved it to a type-specific impl.

impl Default for LengthOrPercentage {
#[inline]
fn default() -> Self {
LengthOrPercentage::zero()

This comment has been minimized.

Copy link
@emilio

emilio Apr 18, 2017

Member

(ditto)

&self.bottom_right.0, &self.bottom_left.0)
}
}
impl Copy for BorderRadius {}

This comment has been minimized.

Copy link
@emilio

emilio Apr 18, 2017

Member

(as an aside, since it was pre-existing, I think we should avoid deriving copy on such big types. We've had memmove traffic before, and if we can avoid accidental memcpys it'd be lovely)

/// https://drafts.csswg.org/css-backgrounds-3/#border-radius
#[derive(Clone, PartialEq, Debug)]
#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
#[allow(missing_docs)]

This comment has been minimized.

Copy link
@emilio

emilio Apr 18, 2017

Member

no allow(missing_docs), please


#[derive(Clone, PartialEq, Debug)]
#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
#[allow(missing_docs)]

This comment has been minimized.

Copy link
@emilio

emilio Apr 18, 2017

Member

no missing_docs? now this is not duplicating anything, so let's just document the fields.

}

impl<L: Parse> Polygon<L> {
#[allow(missing_docs)]

This comment has been minimized.

Copy link
@emilio

emilio Apr 18, 2017

Member

please if you could document this while you're here, it'd be lovely.

pub fn parse_function_arguments(context: &ParserContext, input: &mut Parser) -> Result<Self, ()> {
let fill = input.try(|i| {
let fill = FillRule::parse(i);
i.expect_comma()?; // only eat the comma if there is something before it

This comment has been minimized.

Copy link
@emilio

emilio Apr 18, 2017

Member

What guarantees that input doesn't start with a comma? That'd be incorrect in that case wouldn't it? Well, it'd rewind if there isn't anything, so perhaps it's fine, but I find it more confusing than:

let fill = FillRule::parse(i)?;
i.expect_comma()?;
Ok(fill)

impl<L: Parse> Parse for Polygon<L> {
fn parse(context: &ParserContext, input: &mut Parser) -> Result<Self, ()> {
match input.try(|i| i.expect_function()) {

This comment has been minimized.

Copy link
@emilio

emilio Apr 18, 2017

Member

Why do you need input.try here? Please remove while you're here.

pos = Some(l);
}

if let Ok(k) = input.try(|i| Keyword::parse(i)) {

This comment has been minimized.

Copy link
@emilio

emilio Apr 18, 2017

Member

nit: input.try(Keyword::parse)

dest.write_str(" ")?;
self.vertical.to_css(dest)
}
}

This comment has been minimized.

Copy link
@emilio

emilio Apr 18, 2017

Member

(I'm not sure how worth is it to differentiate between HorizontalPosition and VerticalPosition, could you elaborate on why did you make this change?)

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Apr 20, 2017

Author Member

I've added a doc comment on generic Position about it.

dest.write_str(" ")?;
self.vertical.to_css(dest)
}
}

This comment has been minimized.

Copy link
@emilio

emilio Apr 18, 2017

Member

(I'm not sure how worth is it to differentiate between HorizontalPosition and VerticalPosition, could you elaborate on why did you make this change?)

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2017

☔️ The latest upstream changes (presumably #16231) made this pull request unmergeable. Please resolve the merge conflicts.

@wafflespeanut wafflespeanut force-pushed the wafflespeanut:generics branch from 60819c0 to 2864c0e Apr 20, 2017

@wafflespeanut

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2017

Addressed all review comments, and added documentation for everything I've touched :)

#[inline]
/// Check whether this is a keyword for horizontal position.
pub fn is_horizontal(&self) -> bool {
*self == Keyword::Left || *self == Keyword::Right

This comment has been minimized.

Copy link
@emilio

emilio Apr 20, 2017

Member

nit: use matches! instead?

This comment has been minimized.

Copy link
@emilio

emilio Apr 20, 2017

Member

(here and below)

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Apr 22, 2017

Author Member

Nice!


computed_position::VerticalPosition(vertical)
GenericVerticalPosition(self.0.computed_value(context, |keyword| {
match keyword {

This comment has been minimized.

Copy link
@emilio

emilio Apr 20, 2017

Member

matches!

This comment has been minimized.

Copy link
@emilio

emilio Apr 20, 2017

Member

Though this function is actually the same as the previous one, and these seem to be the only callsites. Is this right? If not, it seems we could get rid of this function...

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Apr 22, 2017

Author Member

Yeah, it's wrong, it should've been different. Good catch! But, I realize that we don't really need a function at all. Since we already parse keywords properly, I've added a function to Keyword to check for the opposite sides.

#[inline]
fn to_computed_value(&self, context: &Context) -> computed_position::HorizontalPosition {
GenericHorizontalPosition(self.0.computed_value(context, |keyword| {
match keyword {

This comment has been minimized.

Copy link
@emilio

emilio Apr 20, 2017

Member

matches!

@wafflespeanut wafflespeanut force-pushed the wafflespeanut:generics branch from 2864c0e to 60a5448 Apr 22, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2017

☔️ The latest upstream changes (presumably #16587) made this pull request unmergeable. Please resolve the merge conflicts.

@wafflespeanut wafflespeanut force-pushed the wafflespeanut:generics branch from c544856 to d56aec4 Apr 25, 2017

@wafflespeanut

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

📌 Commit d56aec4 has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

⌛️ Testing commit d56aec4 with merge 3c7c960...

bors-servo added a commit that referenced this pull request Apr 25, 2017
Auto merge of #16444 - Wafflespeanut:generics, r=emilio
Cleanup various modules and introduce generic types

Almost all the types in `values/specified` and `values/computed` share their `ToCss` implementations. The only reason they have duplicated code hanging around is a result of different specified and computed forms for types like `LengthOrPercentage`. This PR makes some of these types *generic* so that we could have a common definition and `ToCss` impl (`Parse` and `ToComputedValue` impls too, if possible).

<!-- 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/16444)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

💔 Test failed - linux-rel-wpt

@wafflespeanut

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

⚡️ Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-wpt1, windows-msvc-dev are reusable. Rebuilding only linux-rel-css, linux-rel-wpt, mac-rel-css, mac-rel-wpt2...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: emilio
Pushing 3c7c960 to master...

@bors-servo bors-servo merged commit d56aec4 into servo:master Apr 25, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@wafflespeanut wafflespeanut deleted the wafflespeanut:generics branch Apr 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.