From af124f2d89f7f0c40a38ae7b9b1b1e335d3497d6 Mon Sep 17 00:00:00 2001 From: SimranGujral Date: Wed, 24 May 2017 14:15:22 -0700 Subject: [PATCH 1/2] Putting the font computation data in its own struct --- components/style/properties/gecko.mako.rs | 27 +++------- .../style/properties/longhand/font.mako.rs | 4 +- .../style/properties/properties.mako.rs | 52 ++++++++++++++++--- 3 files changed, 53 insertions(+), 30 deletions(-) diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index fd4975826d7..95f2338d3c5 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -57,6 +57,7 @@ use logical_geometry::WritingMode; use media_queries::Device; use properties::animated_properties::TransitionProperty; use properties::longhands; +use properties:: FontComputationData; use properties::{Importance, LonghandId}; use properties::{PropertyDeclaration, PropertyDeclarationBlock, PropertyDeclarationId}; use std::fmt::{self, Debug}; @@ -74,6 +75,7 @@ pub mod style_structs { % endfor } + #[derive(Clone, Debug)] pub struct ComputedValues { % for style_struct in data.style_structs: @@ -83,23 +85,7 @@ pub struct ComputedValues { custom_properties: Option>, pub writing_mode: WritingMode, pub root_font_size: Au, - /// font-size keyword values (and font-size-relative values applied - /// to keyword values) need to preserve their identity as originating - /// from keywords and relative font sizes. We store this information - /// out of band in the ComputedValues. When None, the font size on the - /// current struct was computed from a value that was not a keyword - /// or a chain of font-size-relative values applying to successive parents - /// terminated by a keyword. When Some, this means the font-size was derived - /// from a keyword value or a keyword value on some ancestor with only - /// font-size-relative keywords and regular inheritance in between. The - /// integer stores the final ratio of the chain of font size relative values. - /// and is 1 when there was just a keyword and no relative values. - /// - /// When this is Some, we compute font sizes by computing the keyword against - /// the generic font, and then multiplying it by the ratio. - pub font_size_keyword: Option<(longhands::font_size::KeywordSize, f32)>, - /// The cached system font. See longhand/font.mako.rs - pub cached_system_font: Option, + pub font_computation_data: FontComputationData, /// The element's computed values if visited, only computed if there's a /// relevant link for this element. A element's "relevant link" is the @@ -121,8 +107,7 @@ impl ComputedValues { custom_properties: custom_properties, writing_mode: writing_mode, root_font_size: root_font_size, - cached_system_font: None, - font_size_keyword: font_size_keyword, + font_computation_data: FontComputationData::new(font_size_keyword), visited_style: visited_style, % for style_struct in data.style_structs: ${style_struct.ident}: ${style_struct.ident}, @@ -135,8 +120,7 @@ impl ComputedValues { custom_properties: None, writing_mode: WritingMode::empty(), // FIXME(bz): This seems dubious root_font_size: longhands::font_size::get_initial_value(), // FIXME(bz): Also seems dubious? - font_size_keyword: Some((Default::default(), 1.)), - cached_system_font: None, + font_computation_data: FontComputationData::default_values(), visited_style: None, % for style_struct in data.style_structs: ${style_struct.ident}: style_structs::${style_struct.name}::default(pres_context), @@ -144,6 +128,7 @@ impl ComputedValues { }) } + #[inline] pub fn is_display_contents(&self) -> bool { self.get_box().clone_display() == longhands::display::computed_value::T::contents diff --git a/components/style/properties/longhand/font.mako.rs b/components/style/properties/longhand/font.mako.rs index bbeb892d1a9..110ae7d6800 100644 --- a/components/style/properties/longhand/font.mako.rs +++ b/components/style/properties/longhand/font.mako.rs @@ -900,7 +900,7 @@ ${helpers.single_keyword_system("font-variant-caps", // recomputed from the base size for the keyword and the relative size. // // See bug 1355707 - if let Some((kw, fraction)) = context.inherited_style().font_size_keyword { + if let Some((kw, fraction)) = context.inherited_style().font_computation_data.font_size_keyword { context.mutate_style().font_size_keyword = Some((kw, fraction * ratio)); } else { context.mutate_style().font_size_keyword = None; @@ -950,7 +950,7 @@ ${helpers.single_keyword_system("font-variant-caps", .inherit_font_size_from(parent, kw_inherited_size); if used_kw { context.mutate_style().font_size_keyword = - context.inherited_style.font_size_keyword; + context.inherited_style.font_computation_data.font_size_keyword; } else { context.mutate_style().font_size_keyword = None; } diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 92429d554dd..128fe91939f 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -65,6 +65,44 @@ pub trait MaybeBoxed { fn maybe_boxed(self) -> Out; } + +/// This is where we store extra font data while +/// while computing font sizes. +#[derive(Clone, Debug)] +pub struct FontComputationData { + /// font-size keyword values (and font-size-relative values applied + /// to keyword values) need to preserve their identity as originating + /// from keywords and relative font sizes. We store this information + /// out of band in the ComputedValues. When None, the font size on the + /// current struct was computed from a value that was not a keyword + /// or a chain of font-size-relative values applying to successive parents + /// terminated by a keyword. When Some, this means the font-size was derived + /// from a keyword value or a keyword value on some ancestor with only + /// font-size-relative keywords and regular inheritance in between. The + /// integer stores the final ratio of the chain of font size relative values. + /// and is 1 when there was just a keyword and no relative values. + /// + /// When this is Some, we compute font sizes by computing the keyword against + /// the generic font, and then multiplying it by the ratio. + pub font_size_keyword: Option<(longhands::font_size::KeywordSize, f32)> +} + + +impl FontComputationData{ + /// Assigns values for variables in struct FontComputationData + pub fn new(font_size_keyword: Option<(longhands::font_size::KeywordSize, f32)>) -> Self { + FontComputationData { + font_size_keyword: font_size_keyword + } + } + /// Assigns default values for variables in struct FontComputationData + pub fn default_values() -> Self { + FontComputationData{ + font_size_keyword: Some((Default::default(), 1.)) + } + } +} + impl MaybeBoxed for T { #[inline] fn maybe_boxed(self) -> T { self } @@ -1793,7 +1831,7 @@ pub struct ComputedValues { /// The root element's computed font size. pub root_font_size: Au, /// The keyword behind the current font-size property, if any - pub font_size_keyword: Option<(longhands::font_size::KeywordSize, f32)>, + pub font_computation_data: FontComputationData, /// The element's computed values if visited, only computed if there's a /// relevant link for this element. A element's "relevant link" is the @@ -1817,7 +1855,7 @@ impl ComputedValues { custom_properties: custom_properties, writing_mode: writing_mode, root_font_size: root_font_size, - font_size_keyword: font_size_keyword, + font_computation_data: FontComputationData::new(font_size_keyword), visited_style: visited_style, % for style_struct in data.active_style_structs(): ${style_struct.ident}: ${style_struct.ident}, @@ -2303,7 +2341,7 @@ impl<'a> StyleBuilder<'a> { Self::new(parent.custom_properties(), parent.writing_mode, parent.root_font_size, - parent.font_size_keyword, + parent.font_computation_data.font_size_keyword, parent.clone_visited_style(), % for style_struct in data.active_style_structs(): % if style_struct.inherited: @@ -2403,7 +2441,7 @@ pub use self::lazy_static_module::INITIAL_SERVO_VALUES; mod lazy_static_module { use logical_geometry::WritingMode; use stylearc::Arc; - use super::{ComputedValues, longhands, style_structs}; + use super::{ComputedValues, longhands, style_structs, FontComputationData}; /// The initial values for all style structs as defined by the specification. lazy_static! { @@ -2421,7 +2459,7 @@ mod lazy_static_module { custom_properties: None, writing_mode: WritingMode::empty(), root_font_size: longhands::font_size::get_initial_value(), - font_size_keyword: Some((Default::default(), 1.)), + font_computation_data: FontComputationData::default_values() visited_style: None, }; } @@ -2571,7 +2609,7 @@ pub fn apply_declarations<'a, F, I>(device: &Device, StyleBuilder::new(custom_properties, WritingMode::empty(), inherited_style.root_font_size, - inherited_style.font_size_keyword, + inherited_style.font_computation_data.font_size_keyword, visited_style, % for style_struct in data.active_style_structs(): % if style_struct.inherited: @@ -2585,7 +2623,7 @@ pub fn apply_declarations<'a, F, I>(device: &Device, StyleBuilder::new(custom_properties, WritingMode::empty(), inherited_style.root_font_size, - inherited_style.font_size_keyword, + inherited_style.font_computation_data.font_size_keyword, visited_style, % for style_struct in data.active_style_structs(): inherited_style.${style_struct.name_lower}_arc(), From ce2237e1232e8a2caed2da555fdec7acb2b54c75 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Thu, 25 May 2017 11:56:47 -0700 Subject: [PATCH 2/2] Move root_font_size to the device --- components/style/gecko/media_queries.rs | 28 +++++++++++++++++-- components/style/properties/gecko.mako.rs | 4 --- .../style/properties/properties.mako.rs | 17 ++--------- components/style/servo/media_queries.rs | 26 ++++++++++++++++- components/style/values/specified/length.rs | 2 +- 5 files changed, 53 insertions(+), 24 deletions(-) diff --git a/components/style/gecko/media_queries.rs b/components/style/gecko/media_queries.rs index cdf79c27079..7ba4959eeb5 100644 --- a/components/style/gecko/media_queries.rs +++ b/components/style/gecko/media_queries.rs @@ -17,7 +17,9 @@ use gecko_bindings::structs::RawGeckoPresContextOwned; use media_queries::MediaType; use parser::ParserContext; use properties::{ComputedValues, StyleBuilder}; +use properties::longhands::font_size; use std::fmt::{self, Write}; +use std::sync::atomic::{AtomicIsize, Ordering}; use str::starts_with_ignore_ascii_case; use string_cache::Atom; use style_traits::ToCss; @@ -35,8 +37,20 @@ pub struct Device { pub pres_context: RawGeckoPresContextOwned, default_values: Arc, viewport_override: Option, + /// 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. + /// + /// 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, } +unsafe impl Sync for Device {} +unsafe impl Send for Device {} + impl Device { /// Trivially constructs a new `Device`. pub fn new(pres_context: RawGeckoPresContextOwned) -> Self { @@ -45,6 +59,7 @@ impl Device { pres_context: pres_context, default_values: ComputedValues::default_values(unsafe { &*pres_context }), viewport_override: None, + root_font_size: AtomicIsize::new(font_size::get_initial_value().0 as isize), // FIXME(bz): Seems dubious? } } @@ -73,6 +88,16 @@ impl Device { &self.default_values } + /// Get the font size of the root element (for rem) + pub fn root_font_size(&self) -> Au { + Au::new(self.root_font_size.load(Ordering::Relaxed) as i32) + } + + /// Set the font size of the root element (for rem) + pub fn set_root_font_size(&self, size: Au) { + self.root_font_size.store(size.0 as isize, Ordering::Relaxed) + } + /// Recreates all the temporary state that the `Device` stores. /// /// This includes the viewport override from `@viewport` rules, and also the @@ -111,9 +136,6 @@ impl Device { } } -unsafe impl Sync for Device {} -unsafe impl Send for Device {} - /// A expression for gecko contains a reference to the media feature, the value /// the media query contained, and the range to evaluate. #[derive(Debug, Clone)] diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 95f2338d3c5..bf55c66f90e 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -84,7 +84,6 @@ pub struct ComputedValues { custom_properties: Option>, pub writing_mode: WritingMode, - pub root_font_size: Au, pub font_computation_data: FontComputationData, /// The element's computed values if visited, only computed if there's a @@ -96,7 +95,6 @@ pub struct ComputedValues { impl ComputedValues { pub fn new(custom_properties: Option>, writing_mode: WritingMode, - root_font_size: Au, font_size_keyword: Option<(longhands::font_size::KeywordSize, f32)>, visited_style: Option>, % for style_struct in data.style_structs: @@ -106,7 +104,6 @@ impl ComputedValues { ComputedValues { custom_properties: custom_properties, writing_mode: writing_mode, - root_font_size: root_font_size, font_computation_data: FontComputationData::new(font_size_keyword), visited_style: visited_style, % for style_struct in data.style_structs: @@ -119,7 +116,6 @@ impl ComputedValues { Arc::new(ComputedValues { custom_properties: None, writing_mode: WritingMode::empty(), // FIXME(bz): This seems dubious - root_font_size: longhands::font_size::get_initial_value(), // FIXME(bz): Also seems dubious? font_computation_data: FontComputationData::default_values(), visited_style: None, % for style_struct in data.style_structs: diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 128fe91939f..5036055d379 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1828,8 +1828,6 @@ pub struct ComputedValues { custom_properties: Option>, /// The writing mode of this computed values struct. pub writing_mode: WritingMode, - /// The root element's computed font size. - pub root_font_size: Au, /// The keyword behind the current font-size property, if any pub font_computation_data: FontComputationData, @@ -1844,7 +1842,6 @@ impl ComputedValues { /// Construct a `ComputedValues` instance. pub fn new(custom_properties: Option>, writing_mode: WritingMode, - root_font_size: Au, font_size_keyword: Option<(longhands::font_size::KeywordSize, f32)>, visited_style: Option>, % for style_struct in data.active_style_structs(): @@ -1854,7 +1851,6 @@ impl ComputedValues { ComputedValues { custom_properties: custom_properties, writing_mode: writing_mode, - root_font_size: root_font_size, font_computation_data: FontComputationData::new(font_size_keyword), visited_style: visited_style, % for style_struct in data.active_style_structs(): @@ -2292,8 +2288,6 @@ pub struct StyleBuilder<'a> { /// /// TODO(emilio): Make private. pub writing_mode: WritingMode, - /// The font size of the root element. - pub root_font_size: Au, /// The keyword behind the current font-size property, if any. pub font_size_keyword: Option<(longhands::font_size::KeywordSize, f32)>, /// The element's style if visited, only computed if there's a relevant link @@ -2310,7 +2304,6 @@ impl<'a> StyleBuilder<'a> { pub fn new( custom_properties: Option>, writing_mode: WritingMode, - root_font_size: Au, font_size_keyword: Option<(longhands::font_size::KeywordSize, f32)>, visited_style: Option>, % for style_struct in data.active_style_structs(): @@ -2320,7 +2313,6 @@ impl<'a> StyleBuilder<'a> { StyleBuilder { custom_properties: custom_properties, writing_mode: writing_mode, - root_font_size: root_font_size, font_size_keyword: font_size_keyword, visited_style: visited_style, % for style_struct in data.active_style_structs(): @@ -2340,7 +2332,6 @@ impl<'a> StyleBuilder<'a> { pub fn for_inheritance(parent: &'a ComputedValues, default: &'a ComputedValues) -> Self { Self::new(parent.custom_properties(), parent.writing_mode, - parent.root_font_size, parent.font_computation_data.font_size_keyword, parent.clone_visited_style(), % for style_struct in data.active_style_structs(): @@ -2414,7 +2405,6 @@ impl<'a> StyleBuilder<'a> { pub fn build(self) -> ComputedValues { ComputedValues::new(self.custom_properties, self.writing_mode, - self.root_font_size, self.font_size_keyword, self.visited_style, % for style_struct in data.active_style_structs(): @@ -2458,8 +2448,7 @@ mod lazy_static_module { % endfor custom_properties: None, writing_mode: WritingMode::empty(), - root_font_size: longhands::font_size::get_initial_value(), - font_computation_data: FontComputationData::default_values() + font_computation_data: FontComputationData::default_values(), visited_style: None, }; } @@ -2608,7 +2597,6 @@ pub fn apply_declarations<'a, F, I>(device: &Device, let builder = if !flags.contains(INHERIT_ALL) { StyleBuilder::new(custom_properties, WritingMode::empty(), - inherited_style.root_font_size, inherited_style.font_computation_data.font_size_keyword, visited_style, % for style_struct in data.active_style_structs(): @@ -2622,7 +2610,6 @@ pub fn apply_declarations<'a, F, I>(device: &Device, } else { StyleBuilder::new(custom_properties, WritingMode::empty(), - inherited_style.root_font_size, inherited_style.font_computation_data.font_size_keyword, visited_style, % for style_struct in data.active_style_structs(): @@ -2788,7 +2775,7 @@ pub fn apply_declarations<'a, F, I>(device: &Device, if is_root_element { let s = context.style.get_font().clone_font_size(); - context.style.root_font_size = s; + context.device.set_root_font_size(s); } % endif % endfor diff --git a/components/style/servo/media_queries.rs b/components/style/servo/media_queries.rs index 299970405db..2aaadfcd32b 100644 --- a/components/style/servo/media_queries.rs +++ b/components/style/servo/media_queries.rs @@ -12,7 +12,9 @@ use font_metrics::ServoMetricsProvider; use media_queries::MediaType; use parser::ParserContext; use properties::{ComputedValues, StyleBuilder}; +use properties::longhands::font_size; use std::fmt; +use std::sync::atomic::{AtomicIsize, Ordering}; use style_traits::{CSSPixel, ToCss}; use style_traits::viewport::ViewportConstraints; use values::computed::{self, ToComputedValue}; @@ -22,12 +24,23 @@ use values::specified; /// is displayed in. /// /// This is the struct against which media queries are evaluated. -#[derive(Debug, HeapSizeOf)] +#[derive(HeapSizeOf)] pub struct Device { /// The current media type used by de device. media_type: MediaType, /// The current viewport size, in CSS pixels. viewport_size: TypedSize2D, + + /// 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 + /// + /// 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. + #[ignore_heap_size_of = "Pure stack type"] + root_font_size: AtomicIsize, } impl Device { @@ -38,6 +51,7 @@ impl Device { Device { media_type: media_type, viewport_size: viewport_size, + root_font_size: AtomicIsize::new(font_size::get_initial_value().0 as isize), // FIXME(bz): Seems dubious? } } @@ -49,6 +63,16 @@ impl Device { ComputedValues::initial_values() } + /// Get the font size of the root element (for rem) + pub fn root_font_size(&self) -> Au { + Au::new(self.root_font_size.load(Ordering::Relaxed) as i32) + } + + /// Set the font size of the root element (for rem) + pub fn set_root_font_size(&self, size: Au) { + self.root_font_size.store(size.0 as isize, Ordering::Relaxed) + } + /// Returns the viewport size of the current device in app units, needed, /// among other things, to resolve viewport units. #[inline] diff --git a/components/style/values/specified/length.rs b/components/style/values/specified/length.rs index ef4083544ae..4c3adfc4d63 100644 --- a/components/style/values/specified/length.rs +++ b/components/style/values/specified/length.rs @@ -112,7 +112,7 @@ impl FontRelativeLength { let reference_font_size = base_size.resolve(context); - let root_font_size = context.style().root_font_size; + let root_font_size = context.device.root_font_size(); match *self { FontRelativeLength::Em(length) => reference_font_size.scale_by(length), FontRelativeLength::Ex(length) => {