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 <length-percentage> they'll be fixed.

Differential Revision: https://phabricator.services.mozilla.com/D60194

--HG--
extra : moz-landing-system : lando

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()) {
+  MOZ_ASSERT(IsLength() || !AsCalc().length.IsZero(),
-    auto& calc = AsCalc();
+             "Should've been simplified to a percentage");
-    return calc.has_percentage && calc.length.IsZero();
-  }
   return false;
 }
 
 float LengthPercentage::ToPercentage() const {
   MOZ_ASSERT(ConvertsToPercentage());
-  if (IsPercentage()) {
+  return AsPercentage()._0;
-    return AsPercentage()._0;
-  }
-  return AsCalc().percentage._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();
+  MOZ_ASSERT(!AsCalc().length.IsZero(),
-  return calc.length.IsZero() && calc.percentage._0 == 0.0f;
+             "Should've been simplified to a percentage");
+  return false;
 }
 
 CSSCoord LengthPercentage::ResolveToCSSPixels(CSSCoord aPercentageBasis) const {

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;
 
 /// <https://drafts.csswg.org/css-transitions/#animtype-lpcalc>
 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
+        // Gets clamped as needed after the animation if needed, so no need to
-        // particular AllowedNumericType.
+        // specify any particular AllowedNumericType.
-        Ok(LengthPercentage::new_calc(length, percentage))
+        Ok(LengthPercentage::new_calc(length, percentage, AllowedNumericType::All))
     }
 }

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(),
         }
     }
 

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<Percentage>) -> Self {
+    pub fn new_calc(
-        CalcLengthPercentage::new(l, percentage).to_length_percentge()
+        length: Length,
+        percentage: Option<Percentage>,
+        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 `<length>` component of this `calc()`, clamped.
-    #[inline]
-    pub fn as_percentage(&self) -> Option<Percentage> {
-        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::Percentage(..) | Unpacked::Calc(..) => true,
-            Unpacked::Calc(ref c) => c.has_percentage,
+        }
+    }
+
+    /// Converts to a `<length>` if possible.
+    pub fn to_length(&self) -> Option<Length> {
+        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() {
+                // We simplify before constructing the LengthPercentage if
-                    return specified::LengthPercentage::Percentage(p)
+                // needed, so this is always fine.
-                }
-                if !c.has_percentage {
-                    return specified::LengthPercentage::Length(ToComputedValue::from_computed_value(&c.length_component()))
-                }
                 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<SquaredDistance, ()> {
-        // FIXME(nox): This looks incorrect to me, to add a distance between lengths
+        // A somewhat arbitrary base, it doesn't really make sense to mix
-        // with a distance between percentages.
+        // lengths with percentages, but we can't do much better here, and this
-        Ok(self
+        // ensures that the distance between length-only and percentage-only
-            .unclamped_length()
+        // lengths makes sense.
-            .compute_squared_distance(&other.unclamped_length())? +
+        let basis = Length::new(100.);
-            self.percentage()
+        self.resolve(basis).compute_squared_distance(&other.resolve(basis))
-                .compute_squared_distance(&other.percentage())?)
     }
 }
 
@@ -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<Percentage>) -> 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<Percentage> {
-        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<Percentage>,
-        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<Percentage> {
-        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 {
+    fn clamp_to_non_negative(&self) -> LengthPercentage {
-        if self.has_percentage {
+        LengthPercentage::new_calc(self.length, Some(self.percentage), AllowedNumericType::NonNegative)
-            // 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,
-        )
     }
 }
 
@@ -660,9 +593,7 @@ impl CalcLengthPercentage {
 // maybe.
 impl PartialEq for CalcLengthPercentage {
     fn eq(&self, other: &Self) -> bool {
-        self.length == other.length &&
+        self.length == other.length && self.percentage == other.percentage
-            self.percentage == other.percentage &&
-            self.has_percentage == other.has_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()
         }
     }

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()),
         }
     }
 }

servo/components/style/values/specified/font.rs

@@ -614,12 +614,6 @@ pub enum FontSize {
     System(SystemFont),
 }
 
-impl From<LengthPercentage> 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))]

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 `<position>`
 pub type Position = GenericPosition<HorizontalPosition, VerticalPosition>;
@@ -297,7 +298,7 @@ impl<S: Side> ToComputedValue for PositionComponent<S> {
                 let p = Percentage(1. - length.percentage());
                 let l = -length.unclamped_length();
                 // We represent `<end-side> <length>` as `calc(100% - <length>)`.
-                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),

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())

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::{LengthPercentage, NoCalcLength};
-    use style::values::specified::length::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);