From 9b78285fd3f19f46087fca6e0cd458bfb7fb7c01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 23 Jan 2020 00:36:34 +0000 Subject: [PATCH] Bug 1609737 - Simplify calc expressions earlier. r=boris This simplifies a bit the code, and guarantees that all calc()s have percentages and lengths. I also wanted to remove unclamped_length() / specified_percentage() (for the same reason as the above patch), but they're needed for animations for now. When I implement min() / max() for they'll be fixed. Differential Revision: https://phabricator.services.mozilla.com/D60194 --HG-- extra : moz-landing-system : lando --- layout/style/ServoStyleConstsInlines.h | 31 +-- .../style/values/animated/length.rs | 7 +- .../style/values/computed/length.rs | 2 +- .../values/computed/length_percentage.rs | 185 ++++++------------ .../style/values/generics/transform.rs | 2 +- .../components/style/values/specified/font.rs | 6 - .../style/values/specified/position.rs | 3 +- .../components/style/values/specified/text.rs | 2 +- servo/ports/geckolib/glue.rs | 9 +- 9 files changed, 88 insertions(+), 159 deletions(-) diff --git a/layout/style/ServoStyleConstsInlines.h b/layout/style/ServoStyleConstsInlines.h index ae2c0ed49d4d..87e6ff4d6aac 100644 --- a/layout/style/ServoStyleConstsInlines.h +++ b/layout/style/ServoStyleConstsInlines.h @@ -564,42 +564,42 @@ LengthPercentage LengthPercentage::FromPercentage(float aPercentage) { } bool LengthPercentage::HasPercent() const { - return IsPercentage() || (IsCalc() && AsCalc().has_percentage); + return IsPercentage() || IsCalc(); } -bool LengthPercentage::ConvertsToLength() const { return !HasPercent(); } +bool LengthPercentage::ConvertsToLength() const { return IsLength(); } nscoord LengthPercentage::ToLength() const { MOZ_ASSERT(ConvertsToLength()); - return IsLength() ? AsLength().ToAppUnits() : AsCalc().length.ToAppUnits(); + return AsLength().ToAppUnits(); } CSSCoord LengthPercentage::ToLengthInCSSPixels() const { MOZ_ASSERT(ConvertsToLength()); - return IsLength() ? AsLength().ToCSSPixels() : AsCalc().length.ToCSSPixels(); + return AsLength().ToCSSPixels(); } bool LengthPercentage::ConvertsToPercentage() const { if (IsPercentage()) { return true; } - if (IsCalc()) { - auto& calc = AsCalc(); - return calc.has_percentage && calc.length.IsZero(); - } + MOZ_ASSERT(IsLength() || !AsCalc().length.IsZero(), + "Should've been simplified to a percentage"); return false; } float LengthPercentage::ToPercentage() const { MOZ_ASSERT(ConvertsToPercentage()); - if (IsPercentage()) { - return AsPercentage()._0; - } - return AsCalc().percentage._0; + return AsPercentage()._0; } bool LengthPercentage::HasLengthAndPercentage() const { - return IsCalc() && !ConvertsToLength() && !ConvertsToPercentage(); + if (!IsCalc()) { + return false; + } + MOZ_ASSERT(!ConvertsToLength() && !ConvertsToPercentage(), + "Should've been simplified earlier"); + return true; } bool LengthPercentage::IsDefinitelyZero() const { @@ -609,8 +609,9 @@ bool LengthPercentage::IsDefinitelyZero() const { if (IsPercentage()) { return AsPercentage()._0 == 0.0f; } - auto& calc = AsCalc(); - return calc.length.IsZero() && calc.percentage._0 == 0.0f; + MOZ_ASSERT(!AsCalc().length.IsZero(), + "Should've been simplified to a percentage"); + return false; } CSSCoord LengthPercentage::ResolveToCSSPixels(CSSCoord aPercentageBasis) const { diff --git a/servo/components/style/values/animated/length.rs b/servo/components/style/values/animated/length.rs index 837b3ee86c3a..097243624414 100644 --- a/servo/components/style/values/animated/length.rs +++ b/servo/components/style/values/animated/length.rs @@ -7,6 +7,7 @@ use super::{Animate, Procedure}; use crate::values::computed::length::LengthPercentage; use crate::values::computed::Percentage; +use style_traits::values::specified::AllowedNumericType; /// impl Animate for LengthPercentage { @@ -27,8 +28,8 @@ impl Animate for LengthPercentage { let percentage = animate_percentage_half(self.specified_percentage(), other.specified_percentage())?; - // Gets clamped as needed after the animation, so no need to specify any - // particular AllowedNumericType. - Ok(LengthPercentage::new_calc(length, percentage)) + // Gets clamped as needed after the animation if needed, so no need to + // specify any particular AllowedNumericType. + Ok(LengthPercentage::new_calc(length, percentage, AllowedNumericType::All)) } } diff --git a/servo/components/style/values/computed/length.rs b/servo/components/style/values/computed/length.rs index e02462d12dd3..78c812996fbe 100644 --- a/servo/components/style/values/computed/length.rs +++ b/servo/components/style/values/computed/length.rs @@ -57,7 +57,7 @@ impl ToComputedValue for specified::Length { fn to_computed_value(&self, context: &Context) -> Self::ComputedValue { match *self { specified::Length::NoCalc(l) => l.to_computed_value(context), - specified::Length::Calc(ref calc) => calc.to_computed_value(context).length_component(), + specified::Length::Calc(ref calc) => calc.to_computed_value(context).to_length().unwrap(), } } diff --git a/servo/components/style/values/computed/length_percentage.rs b/servo/components/style/values/computed/length_percentage.rs index bf489197b5a4..f6537c3ecefc 100644 --- a/servo/components/style/values/computed/length_percentage.rs +++ b/servo/components/style/values/computed/length_percentage.rs @@ -205,8 +205,23 @@ impl LengthPercentage { /// Constructs a `calc()` value. #[inline] - pub fn new_calc(l: Length, percentage: Option) -> Self { - CalcLengthPercentage::new(l, percentage).to_length_percentge() + pub fn new_calc( + length: Length, + percentage: Option, + clamping_mode: AllowedNumericType, + ) -> Self { + let percentage = match percentage { + Some(p) => p, + None => return Self::new_length(Length::new(clamping_mode.clamp(length.px()))), + }; + if length.is_zero() { + return Self::new_percent(Percentage(clamping_mode.clamp(percentage.0))) + } + Self::new_calc_unchecked(Box::new(CalcLengthPercentage { + length, + percentage, + clamping_mode, + })) } /// Private version of new_calc() that constructs a calc() variant without @@ -269,7 +284,10 @@ impl LengthPercentage { match self.unpack() { Unpacked::Length(l) => l.px() == 0.0, Unpacked::Percentage(p) => p.0 == 0.0, - Unpacked::Calc(ref c) => c.is_definitely_zero(), + Unpacked::Calc(ref c) => { + debug_assert_ne!(c.length.px(), 0.0, "Should've been simplified to a percentage"); + false + }, } } @@ -316,16 +334,6 @@ impl LengthPercentage { } } - /// Returns the `` component of this `calc()`, clamped. - #[inline] - pub fn as_percentage(&self) -> Option { - match self.unpack() { - Unpacked::Length(..) => None, - Unpacked::Percentage(p) => Some(p), - Unpacked::Calc(ref c) => c.as_percentage(), - } - } - /// Resolves the percentage. #[inline] pub fn resolve(&self, basis: Length) -> Length { @@ -347,8 +355,18 @@ impl LengthPercentage { pub fn has_percentage(&self) -> bool { match self.unpack() { Unpacked::Length(..) => false, - Unpacked::Percentage(..) => true, - Unpacked::Calc(ref c) => c.has_percentage, + Unpacked::Percentage(..) | Unpacked::Calc(..) => true, + } + } + + /// Converts to a `` if possible. + pub fn to_length(&self) -> Option { + match self.unpack() { + Unpacked::Length(l) => Some(l), + Unpacked::Percentage(..) | Unpacked::Calc(..) => { + debug_assert!(self.has_percentage()); + return None; + } } } @@ -358,7 +376,10 @@ impl LengthPercentage { match self.unpack() { Unpacked::Length(..) => None, Unpacked::Percentage(p) => Some(p), - Unpacked::Calc(ref c) => c.specified_percentage(), + Unpacked::Calc(ref c) => { + debug_assert!(self.has_percentage()); + Some(c.percentage) + } } } @@ -396,7 +417,7 @@ impl LengthPercentage { match self.unpack() { Unpacked::Length(l) => Self::new_length(l.clamp_to_non_negative()), Unpacked::Percentage(p) => Self::new_percent(p.clamp_to_non_negative()), - Unpacked::Calc(c) => c.clamp_to_non_negative().to_length_percentge(), + Unpacked::Calc(c) => c.clamp_to_non_negative(), } } } @@ -445,7 +466,7 @@ impl ToComputedValue for specified::LengthPercentage { LengthPercentage::new_percent(value) }, specified::LengthPercentage::Calc(ref calc) => { - (**calc).to_computed_value(context).to_length_percentge() + (**calc).to_computed_value(context) }, } } @@ -459,12 +480,8 @@ impl ToComputedValue for specified::LengthPercentage { specified::LengthPercentage::Percentage(p) } Unpacked::Calc(c) => { - if let Some(p) = c.as_percentage() { - return specified::LengthPercentage::Percentage(p) - } - if !c.has_percentage { - return specified::LengthPercentage::Length(ToComputedValue::from_computed_value(&c.length_component())) - } + // We simplify before constructing the LengthPercentage if + // needed, so this is always fine. specified::LengthPercentage::Calc(Box::new(specified::CalcLengthPercentage::from_computed_value(c))) } } @@ -474,13 +491,12 @@ impl ToComputedValue for specified::LengthPercentage { impl ComputeSquaredDistance for LengthPercentage { #[inline] fn compute_squared_distance(&self, other: &Self) -> Result { - // FIXME(nox): This looks incorrect to me, to add a distance between lengths - // with a distance between percentages. - Ok(self - .unclamped_length() - .compute_squared_distance(&other.unclamped_length())? + - self.percentage() - .compute_squared_distance(&other.percentage())?) + // A somewhat arbitrary base, it doesn't really make sense to mix + // lengths with percentages, but we can't do much better here, and this + // ensures that the distance between length-only and percentage-only + // lengths makes sense. + let basis = Length::new(100.); + self.resolve(basis).compute_squared_distance(&other.resolve(basis)) } } @@ -522,7 +538,7 @@ impl<'de> Deserialize<'de> for LengthPercentage { } } -/// The representation of a calc() function. +/// The representation of a calc() function with mixed lengths and percentages. #[derive( Clone, Debug, Deserialize, MallocSizeOf, Serialize, ToAnimatedZero, ToResolvedValue, )] @@ -534,70 +550,15 @@ pub struct CalcLengthPercentage { #[animation(constant)] clamping_mode: AllowedNumericType, - - /// Whether we specified a percentage or not. - #[animation(constant)] - pub has_percentage: bool, } impl CalcLengthPercentage { - /// Returns a new `LengthPercentage`. - #[inline] - pub fn new(length: Length, percentage: Option) -> Self { - Self::with_clamping_mode(length, percentage, AllowedNumericType::All) - } - - /// Converts this to a `LengthPercentage`, simplifying if possible. - #[inline] - pub fn to_length_percentge(self) -> LengthPercentage { - if !self.has_percentage { - return LengthPercentage::new_length(self.length_component()) - } - if self.length.is_zero() { - return LengthPercentage::new_percent(Percentage(self.clamping_mode.clamp(self.percentage.0))); - } - LengthPercentage::new_calc_unchecked(Box::new(self)) - } - - fn specified_percentage(&self) -> Option { - if self.has_percentage { - Some(self.percentage) - } else { - None - } - } - - /// Returns a new `LengthPercentage` with a specific clamping mode. - #[inline] - fn with_clamping_mode( - length: Length, - percentage: Option, - clamping_mode: AllowedNumericType, - ) -> Self { - Self { - clamping_mode, - length, - percentage: percentage.unwrap_or_default(), - has_percentage: percentage.is_some(), - } - } - /// Returns the length component of this `calc()`, clamped. #[inline] - pub fn length_component(&self) -> Length { + fn length_component(&self) -> Length { Length::new(self.clamping_mode.clamp(self.length.px())) } - /// Returns the percentage component if this could be represented as a - /// non-calc percentage. - fn as_percentage(&self) -> Option { - if !self.has_percentage || self.length.px() != 0. { - return None; - } - - Some(Percentage(self.clamping_mode.clamp(self.percentage.0))) - } - /// Resolves the percentage. #[inline] pub fn resolve(&self, basis: Length) -> Length { @@ -605,44 +566,16 @@ impl CalcLengthPercentage { Length::new(self.clamping_mode.clamp(length)) } - /// Resolves the percentage. - #[inline] - pub fn percentage_relative_to(&self, basis: Length) -> Length { - self.resolve(basis) - } - /// Returns the length, without clamping. #[inline] - pub fn unclamped_length(&self) -> Length { + fn unclamped_length(&self) -> Length { self.length } - /// Returns true if the computed value is absolute 0 or 0%. - #[inline] - fn is_definitely_zero(&self) -> bool { - self.length.px() == 0.0 && self.percentage.0 == 0.0 - } - /// Returns the clamped non-negative values. #[inline] - fn clamp_to_non_negative(&self) -> Self { - if self.has_percentage { - // If we can eagerly clamp the percentage then just do that. - if self.length.is_zero() { - return Self::with_clamping_mode( - Length::zero(), - Some(self.percentage.clamp_to_non_negative()), - AllowedNumericType::NonNegative, - ); - } - return Self::with_clamping_mode(self.length, Some(self.percentage), AllowedNumericType::NonNegative); - } - - Self::with_clamping_mode( - self.length.clamp_to_non_negative(), - None, - AllowedNumericType::NonNegative, - ) + fn clamp_to_non_negative(&self) -> LengthPercentage { + LengthPercentage::new_calc(self.length, Some(self.percentage), AllowedNumericType::NonNegative) } } @@ -660,9 +593,7 @@ impl CalcLengthPercentage { // maybe. impl PartialEq for CalcLengthPercentage { fn eq(&self, other: &Self) -> bool { - self.length == other.length && - self.percentage == other.percentage && - self.has_percentage == other.has_percentage + self.length == other.length && self.percentage == other.percentage } } @@ -673,7 +604,7 @@ impl specified::CalcLengthPercentage { context: &Context, zoom_fn: F, base_size: FontBaseSize, - ) -> CalcLengthPercentage + ) -> LengthPercentage where F: Fn(Length) -> Length, { @@ -709,7 +640,7 @@ impl specified::CalcLengthPercentage { } } - CalcLengthPercentage::with_clamping_mode( + LengthPercentage::new_calc( Length::new(length.min(f32::MAX).max(f32::MIN)), self.percentage, self.clamping_mode, @@ -721,7 +652,7 @@ impl specified::CalcLengthPercentage { &self, context: &Context, base_size: FontBaseSize, - ) -> CalcLengthPercentage { + ) -> LengthPercentage { self.to_computed_value_with_zoom( context, |abs| context.maybe_zoom_text(abs.into()), @@ -755,7 +686,7 @@ impl specified::CalcLengthPercentage { } /// Compute the calc using the current font-size (and without text-zoom). - pub fn to_computed_value(&self, context: &Context) -> CalcLengthPercentage { + pub fn to_computed_value(&self, context: &Context) -> LengthPercentage { self.to_computed_value_with_zoom(context, |abs| abs, FontBaseSize::CurrentStyle) } @@ -766,7 +697,7 @@ impl specified::CalcLengthPercentage { specified::CalcLengthPercentage { clamping_mode: computed.clamping_mode, absolute: Some(AbsoluteLength::from_computed_value(&computed.length)), - percentage: computed.specified_percentage(), + percentage: Some(computed.percentage), ..Default::default() } } diff --git a/servo/components/style/values/generics/transform.rs b/servo/components/style/values/generics/transform.rs index 42a5197311f9..6583f9ebe101 100644 --- a/servo/components/style/values/generics/transform.rs +++ b/servo/components/style/values/generics/transform.rs @@ -377,7 +377,7 @@ impl ToAbsoluteLength for ComputedLengthPercentage { // distance without any layout info. // // FIXME(emilio): This looks wrong. - None => Ok(self.length_component().px()), + None => Ok(self.resolve(Zero::zero()).px()), } } } diff --git a/servo/components/style/values/specified/font.rs b/servo/components/style/values/specified/font.rs index 86a00ef97dfb..84c5141ed92d 100644 --- a/servo/components/style/values/specified/font.rs +++ b/servo/components/style/values/specified/font.rs @@ -614,12 +614,6 @@ pub enum FontSize { System(SystemFont), } -impl From for FontSize { - fn from(other: LengthPercentage) -> Self { - FontSize::Length(other) - } -} - /// Specifies a prioritized list of font family names or generic family names. #[derive(Clone, Debug, Eq, PartialEq, ToCss, ToShmem)] #[cfg_attr(feature = "servo", derive(Hash))] diff --git a/servo/components/style/values/specified/position.rs b/servo/components/style/values/specified/position.rs index 57a6715b0a62..d599d6a85d9f 100644 --- a/servo/components/style/values/specified/position.rs +++ b/servo/components/style/values/specified/position.rs @@ -23,6 +23,7 @@ use selectors::parser::SelectorParseErrorKind; use servo_arc::Arc; use std::fmt::{self, Write}; use style_traits::{CssWriter, ParseError, StyleParseErrorKind, ToCss}; +use style_traits::values::specified::AllowedNumericType; /// The specified value of a CSS `` pub type Position = GenericPosition; @@ -297,7 +298,7 @@ impl ToComputedValue for PositionComponent { let p = Percentage(1. - length.percentage()); let l = -length.unclamped_length(); // We represent ` ` as `calc(100% - )`. - ComputedLengthPercentage::new_calc(l, Some(p)) + ComputedLengthPercentage::new_calc(l, Some(p), AllowedNumericType::All) }, PositionComponent::Side(_, Some(ref length)) | PositionComponent::Length(ref length) => length.to_computed_value(context), diff --git a/servo/components/style/values/specified/text.rs b/servo/components/style/values/specified/text.rs index 8c50a61ef945..b1f5e1f5c3e8 100644 --- a/servo/components/style/values/specified/text.rs +++ b/servo/components/style/values/specified/text.rs @@ -97,7 +97,7 @@ impl ToComputedValue for LineHeight { let computed_calc = calc.to_computed_value_zoomed(context, FontBaseSize::CurrentStyle); let base = context.style().get_font().clone_font_size().size(); - computed_calc.percentage_relative_to(base) + computed_calc.resolve(base) }, }; GenericLineHeight::Length(result.into()) diff --git a/servo/ports/geckolib/glue.rs b/servo/ports/geckolib/glue.rs index ab6d403d91ae..03032d8b22f8 100644 --- a/servo/ports/geckolib/glue.rs +++ b/servo/ports/geckolib/glue.rs @@ -4966,9 +4966,9 @@ pub extern "C" fn Servo_DeclarationBlock_SetLengthValue( use style::properties::PropertyDeclaration; use style::values::generics::length::{LengthPercentageOrAuto, Size}; use style::values::generics::NonNegative; - use style::values::specified::length::LengthPercentage; - use style::values::specified::length::NoCalcLength; + use style::values::specified::length::{LengthPercentage, NoCalcLength}; use style::values::specified::length::{AbsoluteLength, FontRelativeLength}; + use style::values::specified::FontSize; let long = get_longhand_from_id!(property); let nocalc = match unit { @@ -5002,7 +5002,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetLengthValue( R => NonNegative(LengthPercentage::Length(nocalc)), Rx => LengthPercentageOrAuto::LengthPercentage(NonNegative(LengthPercentage::Length(nocalc))), Ry => LengthPercentageOrAuto::LengthPercentage(NonNegative(LengthPercentage::Length(nocalc))), - FontSize => LengthPercentage::from(nocalc).into(), + FontSize => FontSize::Length(LengthPercentage::Length(nocalc)), MozScriptMinSize => MozScriptMinSize(nocalc), }; write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| { @@ -5045,6 +5045,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetPercentValue( use style::values::generics::length::{LengthPercentageOrAuto, Size}; use style::values::generics::NonNegative; use style::values::specified::length::LengthPercentage; + use style::values::specified::FontSize; let long = get_longhand_from_id!(property); let pc = Percentage(value); @@ -5065,7 +5066,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetPercentValue( MarginRight => lp_or_auto, MarginBottom => lp_or_auto, MarginLeft => lp_or_auto, - FontSize => LengthPercentage::from(pc).into(), + FontSize => FontSize::Length(LengthPercentage::Percentage(pc)), }; write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| { decls.push(prop, Importance::Normal);