Skip to content

Commit

Permalink
Auto merge of #15115 - Wafflespeanut:lop, r=emilio
Browse files Browse the repository at this point in the history
Introduce the `NoCalcLength`

<!-- Please describe your changes on the following line: -->

I began this for making the `CalcLengthOrPercentage` represent `LengthOrPercentage` (instead of the enum we already have), but only later did I realize that it will make `LengthOrPercentageOrFoo` types fatty (which is the problem we're trying to avoid - #15061) and so, I dropped that attempt. Along the way, I introduced an internal type for `Length`, for representing all its non-calc variants (which are `Copy`). We could still have this type for the `LengthOrPercentageOrFoo` types which don't really need  `Length` since they already have their own variants for calc.

r? @Manishearth @emilio @SimonSapin or anyone interested

---
<!-- 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

<!-- Either: -->
- [x] These changes do not require tests because it's a refactor

<!-- 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/15115)
<!-- Reviewable:end -->
  • Loading branch information
bors-servo committed Jan 28, 2017
2 parents 7a40f47 + e4243fa commit 9ca3542
Show file tree
Hide file tree
Showing 19 changed files with 306 additions and 230 deletions.
27 changes: 9 additions & 18 deletions components/script/dom/element.rs
Expand Up @@ -4,7 +4,6 @@

//! Element nodes.

use app_units::Au;
use cssparser::Color;
use devtools_traits::AttrInfo;
use dom::activation::Activatable;
Expand Down Expand Up @@ -455,18 +454,13 @@ impl LayoutElementHelpers for LayoutJS<Element> {
font_family)])))));
}

let font_size = if let Some(this) = self.downcast::<HTMLFontElement>() {
this.get_size()
} else {
None
};
let font_size = self.downcast::<HTMLFontElement>().and_then(|this| this.get_size());

if let Some(font_size) = font_size {
hints.push(from_declaration(
PropertyDeclaration::FontSize(
DeclaredValue::Value(
font_size::SpecifiedValue(
LengthOrPercentage::Length(font_size))))))
font_size::SpecifiedValue(font_size.into())))))
}

let cellspacing = if let Some(this) = self.downcast::<HTMLTableElement>() {
Expand All @@ -476,7 +470,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
};

if let Some(cellspacing) = cellspacing {
let width_value = specified::Length::Absolute(Au::from_px(cellspacing as i32));
let width_value = specified::Length::from_px(cellspacing as f32);
hints.push(from_declaration(
PropertyDeclaration::BorderSpacing(DeclaredValue::Value(
border_spacing::SpecifiedValue {
Expand Down Expand Up @@ -509,13 +503,12 @@ impl LayoutElementHelpers for LayoutJS<Element> {
};

if let Some(size) = size {
let value = specified::Length::ServoCharacterWidth(specified::CharacterWidth(size));
let value = specified::NoCalcLength::ServoCharacterWidth(specified::CharacterWidth(size));
hints.push(from_declaration(
PropertyDeclaration::Width(DeclaredValue::Value(
specified::LengthOrPercentageOrAuto::Length(value)))));
}


let width = if let Some(this) = self.downcast::<HTMLIFrameElement>() {
this.get_width()
} else if let Some(this) = self.downcast::<HTMLImageElement>() {
Expand All @@ -541,7 +534,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
}
LengthOrPercentageOrAuto::Length(length) => {
let width_value = specified::LengthOrPercentageOrAuto::Length(
specified::Length::Absolute(length));
specified::NoCalcLength::Absolute(length));
hints.push(from_declaration(
PropertyDeclaration::Width(DeclaredValue::Value(width_value))));
}
Expand All @@ -566,7 +559,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
}
LengthOrPercentageOrAuto::Length(length) => {
let height_value = specified::LengthOrPercentageOrAuto::Length(
specified::Length::Absolute(length));
specified::NoCalcLength::Absolute(length));
hints.push(from_declaration(
PropertyDeclaration::Height(DeclaredValue::Value(height_value))));
}
Expand All @@ -588,13 +581,12 @@ impl LayoutElementHelpers for LayoutJS<Element> {
// scrollbar size into consideration (but we don't have a scrollbar yet!)
//
// https://html.spec.whatwg.org/multipage/#textarea-effective-width
let value = specified::Length::ServoCharacterWidth(specified::CharacterWidth(cols));
let value = specified::NoCalcLength::ServoCharacterWidth(specified::CharacterWidth(cols));
hints.push(from_declaration(
PropertyDeclaration::Width(DeclaredValue::Value(
specified::LengthOrPercentageOrAuto::Length(value)))));
}


let rows = if let Some(this) = self.downcast::<HTMLTextAreaElement>() {
match this.get_rows() {
0 => None,
Expand All @@ -608,7 +600,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
// TODO(mttr) This should take scrollbar size into consideration.
//
// https://html.spec.whatwg.org/multipage/#textarea-effective-height
let value = specified::Length::FontRelative(specified::FontRelativeLength::Em(rows as CSSFloat));
let value = specified::NoCalcLength::FontRelative(specified::FontRelativeLength::Em(rows as CSSFloat));
hints.push(from_declaration(
PropertyDeclaration::Height(DeclaredValue::Value(
specified::LengthOrPercentageOrAuto::Length(value)))));
Expand All @@ -622,8 +614,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
};

if let Some(border) = border {
let width_value = specified::BorderWidth::from_length(
specified::Length::Absolute(Au::from_px(border as i32)));
let width_value = specified::BorderWidth::from_length(specified::Length::from_px(border as f32));
hints.push(from_declaration(
PropertyDeclaration::BorderTopWidth(DeclaredValue::Value(width_value.clone()))));
hints.push(from_declaration(
Expand Down
3 changes: 1 addition & 2 deletions components/style/gecko/media_queries.rs
Expand Up @@ -236,8 +236,7 @@ impl MediaExpressionValue {
nsMediaFeature_ValueType::eLength => {
debug_assert!(css_value.mUnit == nsCSSUnit::eCSSUnit_Pixel);
let pixels = css_value.float_unchecked();
Some(MediaExpressionValue::Length(
specified::Length::Absolute(Au::from_f32_px(pixels))))
Some(MediaExpressionValue::Length(specified::Length::from_px(pixels)))
}
nsMediaFeature_ValueType::eInteger => {
let i = css_value.integer_unchecked();
Expand Down
12 changes: 6 additions & 6 deletions components/style/properties/longhand/box.mako.rs
Expand Up @@ -1328,7 +1328,7 @@ ${helpers.single_keyword("animation-fill-mode",
result.push(SpecifiedOperation::Translate(TranslateKind::Translate,
tx,
ty,
specified::Length::Absolute(Au(0))));
specified::Length::zero()));
Ok(())
}))
},
Expand All @@ -1339,7 +1339,7 @@ ${helpers.single_keyword("animation-fill-mode",
TranslateKind::TranslateX,
tx,
specified::LengthOrPercentage::zero(),
specified::Length::Absolute(Au(0))));
specified::Length::zero()));
Ok(())
}))
},
Expand All @@ -1350,7 +1350,7 @@ ${helpers.single_keyword("animation-fill-mode",
TranslateKind::TranslateY,
specified::LengthOrPercentage::zero(),
ty,
specified::Length::Absolute(Au(0))));
specified::Length::zero()));
Ok(())
}))
},
Expand Down Expand Up @@ -1761,7 +1761,7 @@ ${helpers.single_keyword("transform-style",
use std::fmt;
use style_traits::ToCss;
use values::HasViewportPercentage;
use values::specified::{Length, LengthOrPercentage, Percentage};
use values::specified::{NoCalcLength, LengthOrPercentage, Percentage};

pub mod computed_value {
use properties::animated_properties::Interpolate;
Expand Down Expand Up @@ -1799,7 +1799,7 @@ ${helpers.single_keyword("transform-style",
pub struct SpecifiedValue {
horizontal: LengthOrPercentage,
vertical: LengthOrPercentage,
depth: Length,
depth: NoCalcLength,
}

impl ToCss for computed_value::T {
Expand Down Expand Up @@ -1836,7 +1836,7 @@ ${helpers.single_keyword("transform-style",
Ok(SpecifiedValue {
horizontal: result.horizontal.unwrap_or(LengthOrPercentage::Percentage(Percentage(0.5))),
vertical: result.vertical.unwrap_or(LengthOrPercentage::Percentage(Percentage(0.5))),
depth: result.depth.unwrap_or(Length::Absolute(Au(0))),
depth: result.depth.unwrap_or(NoCalcLength::zero()),
})
}

Expand Down
6 changes: 3 additions & 3 deletions components/style/properties/longhand/effects.mako.rs
Expand Up @@ -280,10 +280,10 @@ ${helpers.predefined_type("opacity",
left = try!(parse_argument(context, input));
}
Ok(SpecifiedValue(Some(SpecifiedClipRect {
top: top.unwrap_or(Length::Absolute(Au(0))),
top: top.unwrap_or(Length::zero()),
right: right,
bottom: bottom,
left: left.unwrap_or(Length::Absolute(Au(0))),
left: left.unwrap_or(Length::zero()),
})))
})
}
Expand Down Expand Up @@ -613,7 +613,7 @@ ${helpers.predefined_type("opacity",
pub struct OriginParseResult {
pub horizontal: Option<specified::LengthOrPercentage>,
pub vertical: Option<specified::LengthOrPercentage>,
pub depth: Option<specified::Length>
pub depth: Option<specified::NoCalcLength>
}

pub fn parse_origin(context: &ParserContext, input: &mut Parser) -> Result<OriginParseResult,()> {
Expand Down
15 changes: 7 additions & 8 deletions components/style/properties/longhand/font.mako.rs
Expand Up @@ -329,7 +329,7 @@ ${helpers.single_keyword("font-variant-caps",
use std::fmt;
use style_traits::ToCss;
use values::{FONT_MEDIUM_PX, HasViewportPercentage};
use values::specified::{LengthOrPercentage, Length, Percentage};
use values::specified::{LengthOrPercentage, Length, NoCalcLength, Percentage};

impl ToCss for SpecifiedValue {
fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
Expand Down Expand Up @@ -363,10 +363,10 @@ ${helpers.single_keyword("font-variant-caps",
#[inline]
fn to_computed_value(&self, context: &Context) -> computed_value::T {
match self.0 {
LengthOrPercentage::Length(Length::FontRelative(value)) => {
LengthOrPercentage::Length(NoCalcLength::FontRelative(value)) => {
value.to_computed_value(context, /* use inherited */ true)
}
LengthOrPercentage::Length(Length::ServoCharacterWidth(value)) => {
LengthOrPercentage::Length(NoCalcLength::ServoCharacterWidth(value)) => {
value.to_computed_value(context.inherited_style().get_font().clone_font_size())
}
LengthOrPercentage::Length(ref l) => {
Expand Down Expand Up @@ -397,11 +397,10 @@ ${helpers.single_keyword("font-variant-caps",
input.try(specified::LengthOrPercentage::parse_non_negative)
.or_else(|()| {
let ident = try!(input.expect_ident());
specified::Length::from_str(&ident as &str)
.ok_or(())
.map(specified::LengthOrPercentage::Length)
})
.map(SpecifiedValue)
NoCalcLength::from_str(&ident as &str)
.ok_or(())
.map(specified::LengthOrPercentage::Length)
}).map(SpecifiedValue)
}
</%helpers:longhand>

Expand Down
9 changes: 4 additions & 5 deletions components/style/properties/longhand/inherited_text.mako.rs
Expand Up @@ -113,13 +113,14 @@
specified::LengthOrPercentage::Length(ref value) =>
computed_value::T::Length(value.to_computed_value(context)),
specified::LengthOrPercentage::Percentage(specified::Percentage(value)) => {
let fr = specified::Length::FontRelative(specified::FontRelativeLength::Em(value));
let fr = specified::Length::NoCalc(specified::NoCalcLength::FontRelative(
specified::FontRelativeLength::Em(value)));
computed_value::T::Length(fr.to_computed_value(context))
},
specified::LengthOrPercentage::Calc(ref calc) => {
let calc = calc.to_computed_value(context);
let fr = specified::FontRelativeLength::Em(calc.percentage());
let fr = specified::Length::FontRelative(fr);
let fr = specified::Length::NoCalc(specified::NoCalcLength::FontRelative(fr));
computed_value::T::Length(calc.length() + fr.to_computed_value(context))
}
}
Expand Down Expand Up @@ -681,9 +682,7 @@ ${helpers.single_keyword("text-align-last",

fn parse_one_text_shadow(context: &ParserContext, input: &mut Parser) -> Result<SpecifiedTextShadow,()> {
use app_units::Au;
let mut lengths = [specified::Length::Absolute(Au(0)),
specified::Length::Absolute(Au(0)),
specified::Length::Absolute(Au(0))];
let mut lengths = [specified::Length::zero(), specified::Length::zero(), specified::Length::zero()];
let mut lengths_parsed = false;
let mut color = None;

Expand Down
3 changes: 1 addition & 2 deletions components/style/properties/shorthand/inherited_text.mako.rs
Expand Up @@ -74,7 +74,6 @@

pub fn parse_value(context: &ParserContext, input: &mut Parser) -> Result<Longhands, ()> {
use values::specified::{BorderWidth, Length};
use app_units::Au;

let mut color = None;
let mut width = None;
Expand All @@ -99,7 +98,7 @@
Ok(Longhands {
_webkit_text_stroke_color: color.or(Some(CSSColor { parsed: CSSParserColor::CurrentColor,
authored: None })),
_webkit_text_stroke_width: width.or(Some(BorderWidth::from_length(Length::Absolute(Au::from_px(0))))),
_webkit_text_stroke_width: width.or(Some(BorderWidth::from_length(Length::zero()))),
})
} else {
Err(())
Expand Down
5 changes: 2 additions & 3 deletions components/style/properties/shorthand/position.mako.rs
Expand Up @@ -57,8 +57,7 @@
<%helpers:shorthand name="flex" sub_properties="flex-grow flex-shrink flex-basis" extra_prefixes="webkit"
spec="https://drafts.csswg.org/css-flexbox/#flex-property">
use parser::Parse;
use app_units::Au;
use values::specified::{Number, Length, LengthOrPercentageOrAutoOrContent};
use values::specified::{Number, NoCalcLength, LengthOrPercentageOrAutoOrContent};

pub fn parse_flexibility(input: &mut Parser)
-> Result<(Number, Option<Number>),()> {
Expand Down Expand Up @@ -102,7 +101,7 @@
Ok(Longhands {
flex_grow: grow.or(Some(Number(1.0))),
flex_shrink: shrink.or(Some(Number(1.0))),
flex_basis: basis.or(Some(LengthOrPercentageOrAutoOrContent::Length(Length::Absolute(Au(0)))))
flex_basis: basis.or(Some(LengthOrPercentageOrAutoOrContent::Length(NoCalcLength::zero())))
})
}

Expand Down
39 changes: 39 additions & 0 deletions components/style/values/computed/length.rs
Expand Up @@ -17,6 +17,45 @@ pub use super::image::{EndingShape as GradientShape, Gradient, GradientKind, Ima
pub use super::image::{LengthOrKeyword, LengthOrPercentageOrKeyword};
pub use values::specified::{Angle, BorderStyle, Time, UrlOrNone};

impl ToComputedValue for specified::NoCalcLength {
type ComputedValue = Au;

#[inline]
fn to_computed_value(&self, context: &Context) -> Au {
match *self {
specified::NoCalcLength::Absolute(length) => length,
specified::NoCalcLength::FontRelative(length) =>
length.to_computed_value(context, /* use inherited */ false),
specified::NoCalcLength::ViewportPercentage(length) =>
length.to_computed_value(context.viewport_size()),
specified::NoCalcLength::ServoCharacterWidth(length) =>
length.to_computed_value(context.style().get_font().clone_font_size())
}
}

#[inline]
fn from_computed_value(computed: &Au) -> Self {
specified::NoCalcLength::Absolute(*computed)
}
}

impl ToComputedValue for specified::Length {
type ComputedValue = Au;

#[inline]
fn to_computed_value(&self, context: &Context) -> Au {
match *self {
specified::Length::NoCalc(l) => l.to_computed_value(context),
specified::Length::Calc(ref calc, range) => range.clamp(calc.to_computed_value(context).length()),
}
}

#[inline]
fn from_computed_value(computed: &Au) -> Self {
specified::Length::NoCalc(specified::NoCalcLength::from_computed_value(computed))
}
}

#[derive(Clone, PartialEq, Copy, Debug)]
#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
#[allow(missing_docs)]
Expand Down
24 changes: 0 additions & 24 deletions components/style/values/computed/mod.rs
Expand Up @@ -121,30 +121,6 @@ impl ToComputedValue for specified::CSSColor {

impl ComputedValueAsSpecified for specified::BorderStyle {}

impl ToComputedValue for specified::Length {
type ComputedValue = Au;

#[inline]
fn to_computed_value(&self, context: &Context) -> Au {
match *self {
specified::Length::Absolute(length) => length,
specified::Length::Calc(ref calc, range) => range.clamp(calc.to_computed_value(context).length()),
specified::Length::FontRelative(length) =>
length.to_computed_value(context, /* use inherited */ false),
specified::Length::ViewportPercentage(length) =>
length.to_computed_value(context.viewport_size()),
specified::Length::ServoCharacterWidth(length) =>
length.to_computed_value(context.style().get_font().clone_font_size())
}
}

#[inline]
fn from_computed_value(computed: &Au) -> Self {
specified::Length::Absolute(*computed)
}
}


#[derive(Debug, PartialEq, Clone, Copy)]
#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
#[allow(missing_docs)]
Expand Down
4 changes: 2 additions & 2 deletions components/style/values/specified/basic_shape.rs
Expand Up @@ -299,7 +299,7 @@ impl ToComputedValue for InsetRect {
/// the keywords are folded into the percentages
fn serialize_basicshape_position<W>(position: &Position, dest: &mut W)
-> fmt::Result where W: fmt::Write {
use values::specified::Length;
use values::specified::NoCalcLength;
use values::specified::position::Keyword;

// keyword-percentage pairs can be folded into a single percentage
Expand Down Expand Up @@ -327,7 +327,7 @@ fn serialize_basicshape_position<W>(position: &Position, dest: &mut W)
// 0 length should be replaced with 0%
fn replace_with_percent(input: LengthOrPercentage) -> LengthOrPercentage {
match input {
LengthOrPercentage::Length(Length::Absolute(au)) if au.0 == 0 => {
LengthOrPercentage::Length(ref l) if l.is_zero() => {
LengthOrPercentage::Percentage(Percentage(0.0))
}
_ => {
Expand Down

0 comments on commit 9ca3542

Please sign in to comment.