From 46df06b3e2823fe09e3fcc7a31605a90e5327753 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 22 Jun 2020 09:45:40 +0000 Subject: [PATCH] style: Use mozilla::Length rather than nscoord to store font sizes. This avoids arbitrary precision loss when computing REM units and so on, which is particularly important if we ever change the base of our app units (but useful regardless). Differential Revision: https://phabricator.services.mozilla.com/D79928 --- components/style/gecko/media_queries.rs | 35 ++++++------ components/style/gecko/wrapper.rs | 57 +++++++++++-------- components/style/properties/cascade.rs | 27 +++++---- components/style/properties/gecko.mako.rs | 42 +++++--------- .../style/properties/longhands/font.mako.rs | 3 +- components/style/servo/media_queries.rs | 14 ++--- components/style/style_adjuster.rs | 5 +- components/style/values/computed/font.rs | 2 +- components/style/values/computed/length.rs | 6 ++ components/style/values/computed/mod.rs | 2 +- components/style/values/specified/font.rs | 4 +- components/style/values/specified/length.rs | 2 +- 12 files changed, 99 insertions(+), 100 deletions(-) diff --git a/components/style/gecko/media_queries.rs b/components/style/gecko/media_queries.rs index 27cb666fc6f..663acae7b6e 100644 --- a/components/style/gecko/media_queries.rs +++ b/components/style/gecko/media_queries.rs @@ -11,6 +11,7 @@ use crate::gecko_bindings::structs; use crate::media_queries::MediaType; use crate::properties::ComputedValues; use crate::string_cache::Atom; +use crate::values::computed::Length; use crate::values::specified::font::FONT_MEDIUM_PX; use crate::values::{CustomIdent, KeyframesName}; use app_units::{Au, AU_PER_PX}; @@ -19,7 +20,7 @@ use euclid::default::Size2D; use euclid::{Scale, SideOffsets2D}; use servo_arc::Arc; use std::fmt; -use std::sync::atomic::{AtomicBool, AtomicIsize, AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU32, AtomicUsize, Ordering}; use style_traits::viewport::ViewportConstraints; use style_traits::{CSSPixel, DevicePixel}; @@ -30,15 +31,16 @@ pub struct Device { /// `Device`, so having a raw document pointer here is fine. document: *const structs::Document, default_values: Arc, - /// The font size of the root element - /// This is set when computing the style of the root - /// element, and used for rem units in other elements. + /// The font size of the root element. /// - /// When computing the style of the root element, there can't be any - /// other style being computed at the same time, given we need the style of - /// the parent to compute everything else. So it is correct to just use - /// a relaxed atomic here. - root_font_size: AtomicIsize, + /// This is set when computing the style of the root element, and used for + /// rem units in other elements. + /// + /// When computing the style of the root element, there can't be any other + /// style being computed at the same time, given we need the style of the + /// parent to compute everything else. So it is correct to just use a + /// relaxed atomic here. + root_font_size: AtomicU32, /// The body text color, stored as an `nscolor`, used for the "tables /// inherit from body" quirk. /// @@ -85,8 +87,7 @@ impl Device { Device { document, default_values: ComputedValues::default_values(doc), - // FIXME(bz): Seems dubious? - root_font_size: AtomicIsize::new(Au::from_px(FONT_MEDIUM_PX as i32).0 as isize), + root_font_size: AtomicU32::new(FONT_MEDIUM_PX.to_bits()), body_text_color: AtomicUsize::new(prefs.mDefaultColor as usize), used_root_font_size: AtomicBool::new(false), used_viewport_size: AtomicBool::new(false), @@ -131,15 +132,15 @@ impl Device { } /// Get the font size of the root element (for rem) - pub fn root_font_size(&self) -> Au { + pub fn root_font_size(&self) -> Length { self.used_root_font_size.store(true, Ordering::Relaxed); - Au::new(self.root_font_size.load(Ordering::Relaxed) as i32) + Length::new(f32::from_bits(self.root_font_size.load(Ordering::Relaxed))) } /// Set the font size of the root element (for rem) - pub fn set_root_font_size(&self, size: Au) { + pub fn set_root_font_size(&self, size: Length) { self.root_font_size - .store(size.0 as isize, Ordering::Relaxed) + .store(size.px().to_bits(), Ordering::Relaxed) } /// Sets the body text color for the "inherit color from body" quirk. @@ -298,13 +299,13 @@ impl Device { /// Applies text zoom to a font-size or line-height value (see nsStyleFont::ZoomText). #[inline] - pub fn zoom_text(&self, size: Au) -> Au { + pub fn zoom_text(&self, size: Length) -> Length { size.scale_by(self.effective_text_zoom()) } /// Un-apply text zoom. #[inline] - pub fn unzoom_text(&self, size: Au) -> Au { + pub fn unzoom_text(&self, size: Length) -> Length { size.scale_by(1. / self.effective_text_zoom()) } diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 8184ccaa3e4..d6c8e267a3c 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -71,7 +71,6 @@ use crate::values::computed::font::GenericFontFamily; use crate::values::computed::Length; use crate::values::specified::length::FontBaseSize; use crate::CaseSensitivityExt; -use app_units::Au; use atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut}; use selectors::attr::{AttrSelectorOperation, AttrSelectorOperator}; use selectors::attr::{CaseSensitivity, NamespaceConstraint}; @@ -922,14 +921,12 @@ fn get_animation_rule( #[derive(Debug)] /// Gecko font metrics provider pub struct GeckoFontMetricsProvider { - /// Cache of base font sizes for each language + /// Cache of base font sizes for each language. Usually will have 1 element. /// - /// Usually will have 1 element. - /// - // This may be slow on pages using more languages, might be worth optimizing - // by caching lang->group mapping separately and/or using a hashmap on larger - // loads. - pub font_size_cache: RefCell>, + /// This may be slow on pages using more languages, might be worth + /// optimizing by caching lang->group mapping separately and/or using a + /// hashmap on larger loads. + pub font_size_cache: RefCell>, } impl GeckoFontMetricsProvider { @@ -952,8 +949,9 @@ impl FontMetricsProvider for GeckoFontMetricsProvider { return sizes.1.size_for_generic(font_family); } let sizes = unsafe { bindings::Gecko_GetBaseSize(font_name.as_ptr()) }; + let size = sizes.size_for_generic(font_family); cache.push((font_name.clone(), sizes)); - sizes.size_for_generic(font_family) + size } fn query( @@ -967,7 +965,7 @@ impl FontMetricsProvider for GeckoFontMetricsProvider { None => return Default::default(), }; - let size = Au::from(base_size.resolve(context)); + let size = base_size.resolve(context); let style = context.style(); let (wm, font) = match base_size { @@ -987,15 +985,15 @@ impl FontMetricsProvider for GeckoFontMetricsProvider { pc, vertical_metrics, font.gecko(), - size.0, + size, // we don't use the user font set in a media query !context.in_media_query, ) }; FontMetrics { - x_height: Some(Au(gecko_metrics.mXSize).into()), - zero_advance_measure: if gecko_metrics.mChSize >= 0 { - Some(Au(gecko_metrics.mChSize).into()) + x_height: Some(gecko_metrics.mXSize), + zero_advance_measure: if gecko_metrics.mChSize.px() >= 0. { + Some(gecko_metrics.mChSize) } else { None }, @@ -1003,20 +1001,31 @@ impl FontMetricsProvider for GeckoFontMetricsProvider { } } -impl structs::FontSizePrefs { +/// The default font sizes for generic families for a given language group. +#[derive(Debug)] +#[repr(C)] +pub struct DefaultFontSizes { + variable: Length, + serif: Length, + sans_serif: Length, + monospace: Length, + cursive: Length, + fantasy: Length, +} + +impl DefaultFontSizes { fn size_for_generic(&self, font_family: GenericFontFamily) -> Length { - Au(match font_family { - GenericFontFamily::None => self.mDefaultVariableSize, - GenericFontFamily::Serif => self.mDefaultSerifSize, - GenericFontFamily::SansSerif => self.mDefaultSansSerifSize, - GenericFontFamily::Monospace => self.mDefaultMonospaceSize, - GenericFontFamily::Cursive => self.mDefaultCursiveSize, - GenericFontFamily::Fantasy => self.mDefaultFantasySize, + match font_family { + GenericFontFamily::None => self.variable, + GenericFontFamily::Serif => self.serif, + GenericFontFamily::SansSerif => self.sans_serif, + GenericFontFamily::Monospace => self.monospace, + GenericFontFamily::Cursive => self.cursive, + GenericFontFamily::Fantasy => self.fantasy, GenericFontFamily::MozEmoji => unreachable!( "Should never get here, since this doesn't (yet) appear on font family" ), - }) - .into() + } } } diff --git a/components/style/properties/cascade.rs b/components/style/properties/cascade.rs index 595534bd974..11f3beb2481 100644 --- a/components/style/properties/cascade.rs +++ b/components/style/properties/cascade.rs @@ -845,7 +845,6 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { fn recompute_keyword_font_size_if_needed(&mut self) { use crate::values::computed::ToComputedValue; use crate::values::specified; - use app_units::Au; if !self.seen.contains(LonghandId::XLang) && !self.seen.contains(LonghandId::FontFamily) { @@ -863,7 +862,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { } }; - if font.gecko().mScriptUnconstrainedSize == Au::from(new_size.size()).0 { + if font.gecko().mScriptUnconstrainedSize == new_size.size { return; } @@ -878,6 +877,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { #[cfg(feature = "gecko")] fn constrain_font_size_if_needed(&mut self) { use crate::gecko_bindings::bindings; + use crate::values::generics::NonNegative; if !self.seen.contains(LonghandId::XLang) && !self.seen.contains(LonghandId::FontFamily) && @@ -896,11 +896,11 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { ) }; - if font.mFont.size >= min_font_size { + if font.mFont.size.0 >= min_font_size { return; } - min_font_size + NonNegative(min_font_size) }; builder.mutate_font().gecko_mut().mFont.size = min_font_size; @@ -942,8 +942,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { /// not clear to me. For now just pretend those don't exist here. #[cfg(feature = "gecko")] fn handle_mathml_scriptlevel_if_needed(&mut self) { - use app_units::Au; - use std::cmp; + use crate::values::generics::NonNegative; if !self.seen.contains(LonghandId::MozScriptLevel) && !self.seen.contains(LonghandId::MozScriptMinSize) && @@ -968,14 +967,14 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { return; } - let mut min = Au(parent_font.mScriptMinSize); + let mut min = parent_font.mScriptMinSize; if font.mAllowZoomAndMinSize { min = builder.device.zoom_text(min); } let scale = (parent_font.mScriptSizeMultiplier as f32).powi(delta as i32); - let parent_size = Au(parent_font.mSize); - let parent_unconstrained_size = Au(parent_font.mScriptUnconstrainedSize); + let parent_size = parent_font.mSize.0; + let parent_unconstrained_size = parent_font.mScriptUnconstrainedSize.0; let new_size = parent_size.scale_by(scale); let new_unconstrained_size = parent_unconstrained_size.scale_by(scale); @@ -987,7 +986,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { if parent_size <= min { (parent_size, new_unconstrained_size) } else { - (cmp::max(min, new_size), new_unconstrained_size) + (min.max(new_size), new_unconstrained_size) } } else { // If the new unconstrained size is larger than the min size, @@ -996,15 +995,15 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { // However, if the new size is even larger (perhaps due to usage // of em units), use that instead. ( - cmp::min(new_size, cmp::max(new_unconstrained_size, min)), + new_size.min(new_unconstrained_size.max(min)), new_unconstrained_size ) } }; let font = builder.mutate_font().gecko_mut(); - font.mFont.size = new_size.0; - font.mSize = new_size.0; - font.mScriptUnconstrainedSize = new_unconstrained_size.0; + font.mFont.size = NonNegative(new_size); + font.mSize = NonNegative(new_size); + font.mScriptUnconstrainedSize = NonNegative(new_unconstrained_size); } /// Various properties affect how font-size and font-family are computed. diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index a8e246b4a0e..80199caa563 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -376,18 +376,6 @@ def set_gecko_property(ffi_name, expr): <%call expr="impl_simple_clone(ident, gecko_ffi_name)"> -<%def name="impl_absolute_length(ident, gecko_ffi_name)"> - #[allow(non_snake_case)] - pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) { - ${set_gecko_property(gecko_ffi_name, "v.to_i32_au()")} - } - <%call expr="impl_simple_copy(ident, gecko_ffi_name)"> - #[allow(non_snake_case)] - pub fn clone_${ident}(&self) -> longhands::${ident}::computed_value::T { - Au(self.gecko.${gecko_ffi_name}).into() - } - - <%def name="impl_non_negative_length(ident, gecko_ffi_name, inherit_from=None, round_to_pixels=False)"> #[allow(non_snake_case)] @@ -593,11 +581,6 @@ impl Clone for ${style_struct.gecko_struct_name} { longhands = [x for x in style_struct.longhands if not (skip_longhands == "*" or x.name in skip_longhands.split())] - # Types used with predefined_type()-defined properties that we can auto-generate. - predefined_types = { - "MozScriptMinSize": impl_absolute_length, - } - def longhand_method(longhand): args = dict(ident=longhand.ident, gecko_ffi_name=longhand.gecko_ffi_name) @@ -610,8 +593,6 @@ impl Clone for ${style_struct.gecko_struct_name} { args.update(keyword=longhand.keyword) if "font" in longhand.ident: args.update(cast_type=longhand.cast_type) - elif longhand.predefined_type in predefined_types: - method = predefined_types[longhand.predefined_type] else: method = impl_simple @@ -920,9 +901,10 @@ fn static_assert() { } pub fn unzoom_fonts(&mut self, device: &Device) { - self.gecko.mSize = device.unzoom_text(Au(self.gecko.mSize)).0; - self.gecko.mScriptUnconstrainedSize = device.unzoom_text(Au(self.gecko.mScriptUnconstrainedSize)).0; - self.gecko.mFont.size = device.unzoom_text(Au(self.gecko.mFont.size)).0; + use crate::values::generics::NonNegative; + self.gecko.mSize = NonNegative(device.unzoom_text(self.gecko.mSize.0)); + self.gecko.mScriptUnconstrainedSize = NonNegative(device.unzoom_text(self.gecko.mScriptUnconstrainedSize.0)); + self.gecko.mFont.size = NonNegative(device.unzoom_text(self.gecko.mFont.size.0)); } pub fn copy_font_size_from(&mut self, other: &Self) { @@ -942,25 +924,27 @@ fn static_assert() { } pub fn set_font_size(&mut self, v: FontSize) { - let size = Au::from(v.size()); - self.gecko.mScriptUnconstrainedSize = size.0; + let size = v.size; + self.gecko.mScriptUnconstrainedSize = size; // These two may be changed from Cascade::fixup_font_stuff. - self.gecko.mSize = size.0; - self.gecko.mFont.size = size.0; + self.gecko.mSize = size; + self.gecko.mFont.size = size; + self.gecko.mFontSizeKeyword = v.keyword_info.kw; self.gecko.mFontSizeFactor = v.keyword_info.factor; - self.gecko.mFontSizeOffset = v.keyword_info.offset.to_i32_au(); + self.gecko.mFontSizeOffset = v.keyword_info.offset; } pub fn clone_font_size(&self) -> FontSize { use crate::values::specified::font::KeywordInfo; + FontSize { - size: Au(self.gecko.mSize).into(), + size: self.gecko.mSize, keyword_info: KeywordInfo { kw: self.gecko.mFontSizeKeyword, factor: self.gecko.mFontSizeFactor, - offset: Au(self.gecko.mFontSizeOffset).into() + offset: self.gecko.mFontSizeOffset, } } } diff --git a/components/style/properties/longhands/font.mako.rs b/components/style/properties/longhands/font.mako.rs index 86575720eba..2ed226f781d 100644 --- a/components/style/properties/longhands/font.mako.rs +++ b/components/style/properties/longhands/font.mako.rs @@ -307,7 +307,6 @@ ${helpers.predefined_type( //! variable reference. We may want to improve this behavior at some //! point. See also https://github.com/w3c/csswg-drafts/issues/1586. - use app_units::Au; use cssparser::{Parser, ToCss}; use crate::values::computed::font::GenericFontFamily; use crate::properties::longhands; @@ -397,7 +396,7 @@ ${helpers.predefined_type( is_system_font: true, }, font_size: FontSize { - size: NonNegative(cx.maybe_zoom_text(Au(system.size).into())), + size: NonNegative(cx.maybe_zoom_text(system.size.0)), keyword_info: KeywordInfo::none() }, font_weight, diff --git a/components/style/servo/media_queries.rs b/components/style/servo/media_queries.rs index 8c0ac014437..039bcf95e29 100644 --- a/components/style/servo/media_queries.rs +++ b/components/style/servo/media_queries.rs @@ -17,7 +17,7 @@ use app_units::Au; use cssparser::RGBA; use euclid::default::Size2D as UntypedSize2D; use euclid::{Scale, SideOffsets2D, Size2D}; -use std::sync::atomic::{AtomicBool, AtomicIsize, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; use style_traits::viewport::ViewportConstraints; use style_traits::{CSSPixel, DevicePixel}; @@ -43,7 +43,7 @@ pub struct Device { /// the parent to compute everything else. So it is correct to just use /// a relaxed atomic here. #[ignore_malloc_size_of = "Pure stack type"] - root_font_size: AtomicIsize, + root_font_size: AtomicU32, /// Whether any styles computed in the document relied on the root font-size /// by using rem units. #[ignore_malloc_size_of = "Pure stack type"] @@ -68,7 +68,7 @@ impl Device { viewport_size, device_pixel_ratio, // FIXME(bz): Seems dubious? - root_font_size: AtomicIsize::new(Au::from_px(FONT_MEDIUM_PX).0 as isize), + root_font_size: AtomicU32::new(FONT_MEDIUM_PX.to_bits()), used_root_font_size: AtomicBool::new(false), used_viewport_units: AtomicBool::new(false), environment: CssEnvironment, @@ -90,15 +90,15 @@ impl Device { } /// Get the font size of the root element (for rem) - pub fn root_font_size(&self) -> Au { + pub fn root_font_size(&self) -> CSSPixelLength { self.used_root_font_size.store(true, Ordering::Relaxed); - Au::new(self.root_font_size.load(Ordering::Relaxed) as i32) + CSSPixelLength::new(f32::from_bits(self.root_font_size.load(Ordering::Relaxed))) } /// Set the font size of the root element (for rem) - pub fn set_root_font_size(&self, size: Au) { + pub fn set_root_font_size(&self, size: CSSPixelLength) { self.root_font_size - .store(size.0 as isize, Ordering::Relaxed) + .store(size.px().to_bits(), Ordering::Relaxed) } /// Sets the body text color for the "inherit color from body" quirk. diff --git a/components/style/style_adjuster.rs b/components/style/style_adjuster.rs index c2a15f7fa0e..712e0d68c4b 100644 --- a/components/style/style_adjuster.rs +++ b/components/style/style_adjuster.rs @@ -12,7 +12,6 @@ use crate::properties::longhands::float::computed_value::T as Float; use crate::properties::longhands::overflow_x::computed_value::T as Overflow; use crate::properties::longhands::position::computed_value::T as Position; use crate::properties::{self, ComputedValues, StyleBuilder}; -use app_units::Au; /// A struct that implements all the adjustment methods. /// @@ -414,7 +413,9 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> { .none_or_hidden() && self.style.get_outline().outline_has_nonzero_width() { - self.style.mutate_outline().set_outline_width(Au(0).into()); + self.style + .mutate_outline() + .set_outline_width(crate::Zero::zero()); } } diff --git a/components/style/values/computed/font.rs b/components/style/values/computed/font.rs index b8d2cb4c680..af56e59abd3 100644 --- a/components/style/values/computed/font.rs +++ b/components/style/values/computed/font.rs @@ -156,7 +156,7 @@ impl FontSize { /// Get default value of font size. pub fn medium() -> Self { Self { - size: NonNegative(Length::new(specified::FONT_MEDIUM_PX as CSSFloat)), + size: NonNegative(Length::new(specified::FONT_MEDIUM_PX)), keyword_info: KeywordInfo::medium(), } } diff --git a/components/style/values/computed/length.rs b/components/style/values/computed/length.rs index 9a7e2cbb1a4..2cca9f71b78 100644 --- a/components/style/values/computed/length.rs +++ b/components/style/values/computed/length.rs @@ -224,6 +224,12 @@ impl CSSPixelLength { CSSPixelLength(px) } + /// Scale the length by a given amount. + #[inline] + pub fn scale_by(self, scale: CSSFloat) -> Self { + CSSPixelLength(self.0 * scale) + } + /// Return the containing pixel value. #[inline] pub fn px(self) -> CSSFloat { diff --git a/components/style/values/computed/mod.rs b/components/style/values/computed/mod.rs index c05fb1ef0fe..aa13ff89d7c 100644 --- a/components/style/values/computed/mod.rs +++ b/components/style/values/computed/mod.rs @@ -232,7 +232,7 @@ impl<'a> Context<'a> { // -x-text-zoom property, which leads to a false value // in mAllowZoomAndMinSize if self.style().get_font().gecko.mAllowZoomAndMinSize { - self.device().zoom_text(Au::from(size)).into() + self.device().zoom_text(size) } else { size } diff --git a/components/style/values/specified/font.rs b/components/style/values/specified/font.rs index 49368548f98..05862f6aba7 100644 --- a/components/style/values/specified/font.rs +++ b/components/style/values/specified/font.rs @@ -787,13 +787,13 @@ impl ToComputedValue for FontSizeAdjust { const LARGER_FONT_SIZE_RATIO: f32 = 1.2; /// The default font size. -pub const FONT_MEDIUM_PX: i32 = 16; +pub const FONT_MEDIUM_PX: f32 = 16.0; impl FontSizeKeyword { #[inline] #[cfg(feature = "servo")] fn to_length(&self, _: &Context) -> NonNegativeLength { - let medium = Length::new(FONT_MEDIUM_PX as f32); + let medium = Length::new(FONT_MEDIUM_PX); // https://drafts.csswg.org/css-fonts-3/#font-size-prop NonNegative(match *self { FontSizeKeyword::XXSmall => medium * 3.0 / 5.0, diff --git a/components/style/values/specified/length.rs b/components/style/values/specified/length.rs index bf81a4d5e3d..64a2e8cf5db 100644 --- a/components/style/values/specified/length.rs +++ b/components/style/values/specified/length.rs @@ -241,7 +241,7 @@ impl FontRelativeLength { let reference_size = if context.builder.is_root_element || context.in_media_query { reference_font_size } else { - computed::Length::new(context.device().root_font_size().to_f32_px()) + context.device().root_font_size() }; (reference_size, length) },