Bug 1520989 - Represent the percentage in LengthPercentage with something other than an option. r=firefox-style-system-reviewers,boris
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 18 Jan 2019 19:32:27 +0000
changeset 514446 9b729145045dd68311801c749f3e6fe5e3c741ba
parent 514445 dd82c0dd27dffc6fd32e0cba59d74c70e3f96ecb
child 514447 e3a8a7245f627e6697056a18847f286c0a1d2bc9
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfirefox-style-system-reviewers, boris
bugs1520989
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1520989 - Represent the percentage in LengthPercentage with something other than an option. r=firefox-style-system-reviewers,boris Not the prettiest, but it will work, and LengthPercentage will be 12 bytes which is pretty good (we could do better if wanted I guess): * Au(i32) length; * f32 percentage; * AllowedNumericType(u8) clamping_mode; * bool has_percentage; * bool was_calc; This will allow me to start moving C++ stuff to use this representation. Differential Revision: https://phabricator.services.mozilla.com/D16929
layout/style/ServoBindings.toml
servo/components/style/cbindgen.toml
servo/components/style/gecko/conversions.rs
servo/components/style/gecko/values.rs
servo/components/style/gecko_bindings/sugar/ns_css_value.rs
servo/components/style/values/animated/length.rs
servo/components/style/values/animated/svg.rs
servo/components/style/values/computed/length.rs
--- a/layout/style/ServoBindings.toml
+++ b/layout/style/ServoBindings.toml
@@ -408,16 +408,17 @@ cbindgen-types = [
     { gecko = "StyleScrollSnapType", servo = "values::computed::ScrollSnapType" },
     { gecko = "StyleResize", servo = "values::computed::Resize" },
     { gecko = "StyleOverflowClipBox", servo = "values::computed::OverflowClipBox" },
     { gecko = "StyleFloat", servo = "values::computed::Float" },
     { gecko = "StyleOverscrollBehavior", servo = "values::computed::OverscrollBehavior" },
     { gecko = "StyleTextAlign", servo = "values::computed::TextAlign" },
     { gecko = "StyleOverflow", servo = "values::computed::Overflow" },
     { gecko = "StyleOverflowAnchor", servo = "values::computed::OverflowAnchor" },
+    { gecko = "StyleLengthPercentage", servo = "values::computed::LengthPercentage" },
 ]
 
 mapped-generic-types = [
     { generic = true, gecko = "mozilla::RustCell", servo = "::std::cell::Cell" },
     { generic = false, gecko = "ServoNodeData", servo = "AtomicRefCell<ElementData>" },
     { generic = false, gecko = "mozilla::ServoWritingMode", servo = "::logical_geometry::WritingMode" },
     { generic = false, gecko = "mozilla::ServoCustomPropertiesMap", servo = "Option<::servo_arc::Arc<::custom_properties::CustomPropertiesMap>>" },
     { generic = false, gecko = "mozilla::ServoRuleNode", servo = "Option<::rule_tree::StrongRuleNode>" },
--- a/servo/components/style/cbindgen.toml
+++ b/servo/components/style/cbindgen.toml
@@ -63,10 +63,11 @@ include = [
   "UserSelect",
   "Float",
   "OverscrollBehavior",
   "ScrollSnapType",
   "OverflowAnchor",
   "OverflowClipBox",
   "Resize",
   "Overflow",
+  "LengthPercentage",
 ]
 item_types = ["enums", "structs", "typedefs"]
--- a/servo/components/style/gecko/conversions.rs
+++ b/servo/components/style/gecko/conversions.rs
@@ -32,24 +32,23 @@ use crate::values::generics::NonNegative
 use app_units::Au;
 use std::f32::consts::PI;
 use style_traits::values::specified::AllowedNumericType;
 
 impl From<LengthPercentage> for nsStyleCoord_CalcValue {
     fn from(other: LengthPercentage) -> nsStyleCoord_CalcValue {
         debug_assert!(
             other.was_calc ||
-                other.percentage.is_none() ||
+                !other.has_percentage ||
                 other.unclamped_length() == Length::zero()
         );
-        let has_percentage = other.percentage.is_some();
         nsStyleCoord_CalcValue {
             mLength: other.unclamped_length().to_i32_au(),
-            mPercent: other.percentage.map_or(0., |p| p.0),
-            mHasPercent: has_percentage,
+            mPercent: other.percentage(),
+            mHasPercent: other.has_percentage,
         }
     }
 }
 
 impl From<nsStyleCoord_CalcValue> for LengthPercentage {
     fn from(other: nsStyleCoord_CalcValue) -> LengthPercentage {
         let percentage = if other.mHasPercent {
             Some(Percentage(other.mPercent))
--- a/servo/components/style/gecko/values.rs
+++ b/servo/components/style/gecko/values.rs
@@ -146,19 +146,19 @@ impl GeckoStyleCoordConvertible for Numb
     }
 }
 
 impl GeckoStyleCoordConvertible for LengthPercentage {
     fn to_gecko_style_coord<T: CoordDataMut>(&self, coord: &mut T) {
         if self.was_calc {
             return coord.set_value(CoordDataValue::Calc((*self).into()));
         }
-        debug_assert!(self.percentage.is_none() || self.unclamped_length() == Length::zero());
-        if let Some(p) = self.percentage {
-            return coord.set_value(CoordDataValue::Percent(p.0));
+        debug_assert!(!self.has_percentage || self.unclamped_length() == Length::zero());
+        if self.has_percentage {
+            return coord.set_value(CoordDataValue::Percent(self.percentage()));
         }
         coord.set_value(CoordDataValue::Coord(self.unclamped_length().to_i32_au()))
     }
 
     fn from_gecko_style_coord<T: CoordData>(coord: &T) -> Option<Self> {
         match coord.as_value() {
             CoordDataValue::Coord(coord) => Some(LengthPercentage::new(Au(coord).into(), None)),
             CoordDataValue::Percent(p) => {
--- a/servo/components/style/gecko_bindings/sugar/ns_css_value.rs
+++ b/servo/components/style/gecko_bindings/sugar/ns_css_value.rs
@@ -67,19 +67,19 @@ impl nsCSSValue {
         &*array
     }
 
     /// Sets LengthPercentage value to this nsCSSValue.
     pub unsafe fn set_length_percentage(&mut self, lp: LengthPercentage) {
         if lp.was_calc {
             return bindings::Gecko_CSSValue_SetCalc(self, lp.into());
         }
-        debug_assert!(lp.percentage.is_none() || lp.unclamped_length() == Length::zero());
-        if let Some(p) = lp.percentage {
-            return self.set_percentage(p.0);
+        debug_assert!(!lp.has_percentage || lp.unclamped_length() == Length::zero());
+        if lp.has_percentage {
+            return self.set_percentage(lp.percentage());
         }
         self.set_px(lp.unclamped_length().px());
     }
 
     /// Sets a px value to this nsCSSValue.
     pub unsafe fn set_px(&mut self, px: f32) {
         bindings::Gecko_CSSValue_SetPixelLength(self, px)
     }
--- a/servo/components/style/values/animated/length.rs
+++ b/servo/components/style/values/animated/length.rs
@@ -21,20 +21,23 @@ impl Animate for LengthPercentage {
             let this = this.unwrap_or_default();
             let other = other.unwrap_or_default();
             Ok(Some(this.animate(&other, procedure)?))
         };
 
         let length = self
             .unclamped_length()
             .animate(&other.unclamped_length(), procedure)?;
-        let percentage = animate_percentage_half(self.percentage, other.percentage)?;
+        let percentage = animate_percentage_half(
+            self.specified_percentage(),
+            other.specified_percentage(),
+        )?;
         let is_calc = self.was_calc ||
             other.was_calc ||
-            self.percentage.is_some() != other.percentage.is_some();
+            self.has_percentage != other.has_percentage;
         Ok(Self::with_clamping_mode(
             length,
             percentage,
             self.clamping_mode,
             is_calc,
         ))
     }
 }
--- a/servo/components/style/values/animated/svg.rs
+++ b/servo/components/style/values/animated/svg.rs
@@ -27,24 +27,26 @@ impl ToAnimatedZero for IntermediateSVGP
 }
 
 // FIXME: We need to handle calc here properly, see
 // https://bugzilla.mozilla.org/show_bug.cgi?id=1386967
 fn to_number_or_percentage(
     value: &SvgLengthPercentageOrNumber<LengthPercentage, Number>,
 ) -> Result<NumberOrPercentage, ()> {
     Ok(match *value {
-        SvgLengthPercentageOrNumber::LengthPercentage(ref l) => match l.percentage {
-            Some(p) => {
-                if l.unclamped_length().px() != 0. {
-                    return Err(());
-                }
-                NumberOrPercentage::Percentage(p)
-            },
-            None => NumberOrPercentage::Number(l.length().px()),
+        SvgLengthPercentageOrNumber::LengthPercentage(ref l) => {
+            match l.specified_percentage() {
+                Some(p) => {
+                    if l.unclamped_length().px() != 0. {
+                        return Err(());
+                    }
+                    NumberOrPercentage::Percentage(p)
+                },
+                None => NumberOrPercentage::Number(l.length().px()),
+            }
         },
         SvgLengthPercentageOrNumber::Number(ref n) => NumberOrPercentage::Number(*n),
     })
 }
 
 impl Animate for SvgLengthPercentageOrNumber<LengthPercentage, Number> {
     #[inline]
     fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
--- a/servo/components/style/values/computed/length.rs
+++ b/servo/components/style/values/computed/length.rs
@@ -72,17 +72,20 @@ impl ToComputedValue for specified::Leng
 ///
 /// https://drafts.csswg.org/css-values-4/#typedef-length-percentage
 #[allow(missing_docs)]
 #[derive(Clone, Copy, Debug, MallocSizeOf, ToAnimatedZero)]
 pub struct LengthPercentage {
     #[animation(constant)]
     pub clamping_mode: AllowedNumericType,
     length: Length,
-    pub percentage: Option<Percentage>,
+    percentage: Percentage,
+    /// Whether we specified a percentage or not.
+    #[animation(constant)]
+    pub has_percentage: bool,
     /// Whether this was from a calc() expression. This is needed because right
     /// now we don't treat calc() the same way as non-calc everywhere, but
     /// that's a bug in most cases.
     ///
     /// Please don't add new uses of this that aren't for converting to Gecko's
     /// representation, or to interpolate values.
     ///
     /// See https://github.com/w3c/csswg-drafts/issues/3482.
@@ -94,30 +97,30 @@ pub struct LengthPercentage {
 // representation of LengthPercentage with Gecko. The issue here is that Gecko
 // uses CalcValue to represent position components, so they always come back as
 // was_calc == true, and we mess up in the transitions code.
 //
 // This was a pre-existing bug, though arguably so only in pretty obscure cases
 // like calc(0px + 5%) and such.
 impl PartialEq for LengthPercentage {
     fn eq(&self, other: &Self) -> bool {
-        self.length == other.length && self.percentage == other.percentage
+        self.length == other.length && self.percentage == other.percentage &&
+            self.has_percentage == other.has_percentage
     }
 }
 
 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
         // with a distance between percentages.
         Ok(self
             .unclamped_length()
             .compute_squared_distance(&other.unclamped_length())? +
-            self.percentage()
-                .compute_squared_distance(&other.percentage())?)
+            self.percentage.compute_squared_distance(&other.percentage)?)
     }
 }
 
 impl LengthPercentage {
     /// Returns a new `LengthPercentage`.
     #[inline]
     pub fn new(length: Length, percentage: Option<Percentage>) -> Self {
         Self::with_clamping_mode(
@@ -139,27 +142,28 @@ impl LengthPercentage {
         length: Length,
         percentage: Option<Percentage>,
         clamping_mode: AllowedNumericType,
         was_calc: bool,
     ) -> Self {
         Self {
             clamping_mode,
             length,
-            percentage,
+            percentage: percentage.unwrap_or_default(),
+            has_percentage: percentage.is_some(),
             was_calc,
         }
     }
 
     /// Returns this `calc()` as a `<length>`.
     ///
     /// Panics in debug mode if a percentage is present in the expression.
     #[inline]
     pub fn length(&self) -> CSSPixelLength {
-        debug_assert!(self.percentage.is_none());
+        debug_assert!(!self.has_percentage);
         self.length_component()
     }
 
     /// Returns the length component of this `calc()`
     #[inline]
     pub fn length_component(&self) -> CSSPixelLength {
         CSSPixelLength::new(self.clamping_mode.clamp(self.length.px()))
     }
@@ -168,52 +172,59 @@ impl LengthPercentage {
     #[inline]
     pub fn unclamped_length(&self) -> CSSPixelLength {
         self.length
     }
 
     /// Return the percentage value as CSSFloat.
     #[inline]
     pub fn percentage(&self) -> CSSFloat {
-        self.percentage.map_or(0., |p| p.0)
+        self.percentage.0
+    }
+
+    /// Return the specified percentage if any.
+    #[inline]
+    pub fn specified_percentage(&self) -> Option<Percentage> {
+        if self.has_percentage {
+            Some(self.percentage)
+        } else {
+            None
+        }
     }
 
     /// Returns the percentage component if this could be represented as a
     /// non-calc percentage.
     pub fn as_percentage(&self) -> Option<Percentage> {
-        if self.length.px() != 0. {
+        if !self.has_percentage || self.length.px() != 0. {
             return None;
         }
 
-        let p = self.percentage?;
-        if self.clamping_mode.clamp(p.0) != p.0 {
+        if self.clamping_mode.clamp(self.percentage.0) != self.percentage.0 {
+            debug_assert!(self.was_calc);
             return None;
         }
 
-        Some(p)
+        Some(self.percentage)
     }
 
     /// Convert the computed value into used value.
     #[inline]
     pub fn maybe_to_used_value(&self, container_len: Option<Au>) -> Option<Au> {
         self.maybe_to_pixel_length(container_len).map(Au::from)
     }
 
     /// If there are special rules for computing percentages in a value (e.g.
     /// the height property), they apply whenever a calc() expression contains
     /// percentages.
     pub fn maybe_to_pixel_length(&self, container_len: Option<Au>) -> Option<Length> {
-        match (container_len, self.percentage) {
-            (Some(len), Some(percent)) => {
-                let pixel = self.length.px() + len.scale_by(percent.0).to_f32_px();
-                Some(Length::new(self.clamping_mode.clamp(pixel)))
-            },
-            (_, None) => Some(self.length()),
-            _ => None,
+        if self.has_percentage {
+            let length = self.unclamped_length().px() + container_len?.scale_by(self.percentage.0).to_f32_px();
+            return Some(Length::new(self.clamping_mode.clamp(length)));
         }
+        Some(self.length())
     }
 }
 
 impl ToCss for LengthPercentage {
     fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
     where
         W: Write,
     {
@@ -257,22 +268,22 @@ impl specified::CalcLengthPercentage {
             self.ex.map(FontRelativeLength::Ex),
             self.rem.map(FontRelativeLength::Rem),
         ] {
             if let Some(val) = *val {
                 length += val.to_computed_value(context, base_size).px();
             }
         }
 
-        LengthPercentage {
-            clamping_mode: self.clamping_mode,
-            length: Length::new(length.min(f32::MAX).max(f32::MIN)),
-            percentage: self.percentage,
-            was_calc: true,
-        }
+        LengthPercentage::with_clamping_mode(
+            Length::new(length.min(f32::MAX).max(f32::MIN)),
+            self.percentage,
+            self.clamping_mode,
+            /* was_calc = */ true,
+        )
     }
 
     /// Compute font-size or line-height taking into account text-zoom if necessary.
     pub fn to_computed_value_zoomed(
         &self,
         context: &Context,
         base_size: FontBaseSize,
     ) -> LengthPercentage {
@@ -317,17 +328,17 @@ impl ToComputedValue for specified::Calc
         self.to_computed_value_with_zoom(context, |abs| abs, FontBaseSize::CurrentStyle)
     }
 
     #[inline]
     fn from_computed_value(computed: &LengthPercentage) -> Self {
         specified::CalcLengthPercentage {
             clamping_mode: computed.clamping_mode,
             absolute: Some(AbsoluteLength::from_computed_value(&computed.length)),
-            percentage: computed.percentage,
+            percentage: computed.specified_percentage(),
             ..Default::default()
         }
     }
 }
 
 impl LengthPercentage {
     #[inline]
     #[allow(missing_docs)]
@@ -339,26 +350,26 @@ impl LengthPercentage {
     #[inline]
     pub fn one() -> LengthPercentage {
         LengthPercentage::new(Length::new(1.), None)
     }
 
     /// Returns true if the computed value is absolute 0 or 0%.
     #[inline]
     pub fn is_definitely_zero(&self) -> bool {
-        self.unclamped_length().px() == 0.0 && self.percentage.map_or(true, |p| p.0 == 0.0)
+        self.unclamped_length().px() == 0.0 && self.percentage.0 == 0.0
     }
 
     // CSSFloat doesn't implement Hash, so does CSSPixelLength. Therefore, we still use Au as the
     // hash key.
     #[allow(missing_docs)]
     pub fn to_hash_key(&self) -> (Au, NotNan<f32>) {
         (
             Au::from(self.unclamped_length()),
-            NotNan::new(self.percentage()).unwrap(),
+            NotNan::new(self.percentage.0).unwrap(),
         )
     }
 
     /// Returns the used value.
     pub fn to_used_value(&self, containing_length: Au) -> Au {
         Au::from(self.to_pixel_length(containing_length))
     }
 
@@ -371,24 +382,24 @@ impl LengthPercentage {
     ///
     /// TODO(emilio): It's a bit unfortunate that this depends on whether the
     /// value was a `calc()` value or not. Should it?
     #[inline]
     pub fn clamp_to_non_negative(self) -> Self {
         if self.was_calc {
             return Self::with_clamping_mode(
                 self.length,
-                self.percentage,
+                self.specified_percentage(),
                 AllowedNumericType::NonNegative,
                 self.was_calc,
             );
         }
 
-        debug_assert!(self.percentage.is_none() || self.unclamped_length() == Length::zero());
-        if let Some(p) = self.percentage {
+        debug_assert!(!self.has_percentage || self.unclamped_length() == Length::zero());
+        if let Some(p) = self.specified_percentage() {
             return Self::with_clamping_mode(
                 Length::zero(),
                 Some(p.clamp_to_non_negative()),
                 AllowedNumericType::NonNegative,
                 self.was_calc,
             );
         }
 
@@ -415,18 +426,18 @@ impl ToComputedValue for specified::Leng
     }
 
     fn from_computed_value(computed: &LengthPercentage) -> Self {
         let length = computed.unclamped_length();
         if let Some(p) = computed.as_percentage() {
             return specified::LengthPercentage::Percentage(p);
         }
 
-        let percentage = computed.percentage;
-        if percentage.is_none() && computed.clamping_mode.clamp(length.px()) == length.px() {
+        if !computed.has_percentage  &&
+            computed.clamping_mode.clamp(length.px()) == length.px() {
             return specified::LengthPercentage::Length(ToComputedValue::from_computed_value(
                 &length,
             ));
         }
 
         specified::LengthPercentage::Calc(Box::new(ToComputedValue::from_computed_value(computed)))
     }
 }