From 349d6e71674fab9361f43d4b280abc9aa6f43abe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 2 Feb 2018 16:13:33 +0100 Subject: [PATCH] style: Cleanup StyleBuilder. This is in preparation of a cascade optimization for custom properties. This fixes various fishiness around our StyleBuilder stuff. In particular, StyleBuilder::for_derived_style (renamed to for_animation) is only used to compute specified values, and thus doesn't need to know about rules, visited style, or other things like that. The flag propagation that was done in StyleAdjuster is now done in StyleBuilder, since we know beforehand which ones are always inherited, and it simplified the callers and the StyleAdjuster code. It also fixed some fishiness wrt which flags were propagated to anon boxes and text. The text-decoration-lines bit is interesting, because the way it was implemented in #17722 meant that display: contents elements did get HAS_DECORATION_LINES flags only if its parent also had it, so in practice the Contents check preserves behavior, but it's only an optimization looking at Gecko's call-sites, so we can remove it too. MozReview-Commit-ID: 6BHCyEO2U8c --- components/style/matching.rs | 2 +- .../style/properties/computed_value_flags.rs | 32 ++++++++-- components/style/properties/gecko.mako.rs | 11 ---- .../style/properties/properties.mako.rs | 62 ++++++++----------- components/style/style_adjuster.rs | 58 ++++++----------- components/style/stylesheets/viewport_rule.rs | 9 +-- components/style/values/computed/mod.rs | 3 +- components/style/values/specified/box.rs | 10 +++ ports/geckolib/glue.rs | 49 +++++---------- 9 files changed, 104 insertions(+), 132 deletions(-) diff --git a/components/style/matching.rs b/components/style/matching.rs index 5145e26ec36..89b60c9905a 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -336,7 +336,7 @@ trait PrivateMatchMethods: TElement { // We need to cascade the children in order to ensure the correct // propagation of inherited computed value flags. - if old_values.flags.inherited() != new_values.flags.inherited() { + if old_values.flags.maybe_inherited() != new_values.flags.maybe_inherited() { debug!(" > flags changed: {:?} != {:?}", old_values.flags, new_values.flags); return ChildCascadeRequirement::MustCascadeChildren; } diff --git a/components/style/properties/computed_value_flags.rs b/components/style/properties/computed_value_flags.rs index 612f7c0470b..bddb451861e 100644 --- a/components/style/properties/computed_value_flags.rs +++ b/components/style/properties/computed_value_flags.rs @@ -71,11 +71,35 @@ bitflags! { } impl ComputedValueFlags { - /// Returns the flags that are inherited. + /// Flags that are unconditionally propagated to descendants. + #[inline] + fn inherited_flags() -> Self { + ComputedValueFlags::IS_STYLE_IF_VISITED | + ComputedValueFlags::IS_RELEVANT_LINK_VISITED | + ComputedValueFlags::CAN_BE_FRAGMENTED | + ComputedValueFlags::IS_IN_DISPLAY_NONE_SUBTREE | + ComputedValueFlags::IS_IN_PSEUDO_ELEMENT_SUBTREE | + ComputedValueFlags::HAS_TEXT_DECORATION_LINES + } + + /// Flags that may be propagated to descendants. + #[inline] + fn maybe_inherited_flags() -> Self { + Self::inherited_flags() | ComputedValueFlags::SHOULD_SUPPRESS_LINEBREAK + } + + /// Returns the flags that are always propagated to descendants. + /// + /// See StyleAdjuster::set_bits and StyleBuilder. #[inline] pub fn inherited(self) -> Self { - self & !(ComputedValueFlags::INHERITS_DISPLAY | - ComputedValueFlags::INHERITS_CONTENT | - ComputedValueFlags::INHERITS_RESET_STYLE) + self & Self::inherited_flags() + } + + /// Flags that are conditionally propagated to descendants, just to handle + /// properly style invalidation. + #[inline] + pub fn maybe_inherited(self) -> Self { + self & Self::maybe_inherited_flags() } } diff --git a/components/style/properties/gecko.mako.rs b/components/style/properties/gecko.mako.rs index 195425419e8..615df419d3a 100644 --- a/components/style/properties/gecko.mako.rs +++ b/components/style/properties/gecko.mako.rs @@ -252,17 +252,6 @@ impl ops::DerefMut for ComputedValues { } impl ComputedValuesInner { - /// Clone the visited style. Used for inheriting parent styles in - /// StyleBuilder::for_derived_style. - pub fn clone_visited_style(&self) -> Option> { - self.visited_style.as_ref().map(|x| x.clone_arc()) - } - - #[inline] - pub fn is_display_contents(&self) -> bool { - self.get_box().clone_display() == longhands::display::computed_value::T::Contents - } - /// Returns true if the value of the `content` property would make a /// pseudo-element not rendered. #[inline] diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 825a3c737cc..49bb8eba408 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -2216,6 +2216,11 @@ pub struct ComputedValues { } impl ComputedValues { + /// Returns whether this style's display value is equal to contents. + pub fn is_display_contents(&self) -> bool { + self.get_box().clone_display().is_contents() + } + /// Whether we're a visited style. pub fn is_style_if_visited(&self) -> bool { self.flags.contains(ComputedValueFlags::IS_STYLE_IF_VISITED) @@ -2335,17 +2340,6 @@ impl ComputedValuesInner { /// Servo for obvious reasons. pub fn has_moz_binding(&self) -> bool { false } - /// Clone the visited style. Used for inheriting parent styles in - /// StyleBuilder::for_derived_style. - pub fn clone_visited_style(&self) -> Option> { - self.visited_style.clone() - } - - /// Returns whether this style's display value is equal to contents. - /// - /// Since this isn't supported in Servo, this is always false for Servo. - pub fn is_display_contents(&self) -> bool { false } - #[inline] /// Returns whether the "content" property for the given style is completely /// ineffective, and would yield an empty `::before` or `::after` @@ -2728,8 +2722,6 @@ impl<'a> StyleBuilder<'a> { cascade_flags: CascadeFlags, rules: Option, custom_properties: Option>, - writing_mode: WritingMode, - mut flags: ComputedValueFlags, visited_style: Option>, ) -> Self { debug_assert_eq!(parent_style.is_some(), parent_style_ignoring_first_line.is_some()); @@ -2751,6 +2743,7 @@ impl<'a> StyleBuilder<'a> { reset_style }; + let mut flags = inherited_style.flags.inherited(); if cascade_flags.contains(CascadeFlags::VISITED_DEPENDENT_ONLY) { flags.insert(ComputedValueFlags::IS_STYLE_IF_VISITED); } @@ -2765,7 +2758,7 @@ impl<'a> StyleBuilder<'a> { rules, modified_reset: false, custom_properties, - writing_mode, + writing_mode: inherited_style.writing_mode, flags, visited_style, % for style_struct in data.active_style_structs(): @@ -2789,11 +2782,10 @@ impl<'a> StyleBuilder<'a> { /// /// Do _not_ actually call this to construct a style, this should mostly be /// used for animations. - pub fn for_derived_style( + pub fn for_animation( device: &'a Device, style_to_derive_from: &'a ComputedValues, parent_style: Option<<&'a ComputedValues>, - pseudo: Option<<&'a PseudoElement>, ) -> Self { let reset_style = device.default_computed_values(); let inherited_style = parent_style.unwrap_or(reset_style); @@ -2807,7 +2799,7 @@ impl<'a> StyleBuilder<'a> { // None of our callers pass in ::first-line parent styles. inherited_style_ignoring_first_line: inherited_style, reset_style, - pseudo, + pseudo: None, modified_reset: false, rules: None, custom_properties: style_to_derive_from.custom_properties().cloned(), @@ -2916,7 +2908,7 @@ impl<'a> StyleBuilder<'a> { /// computed values that need to be provided as well. pub fn for_inheritance( device: &'a Device, - parent: &'a ComputedValues, + parent: Option<<&'a ComputedValues>, pseudo: Option<<&'a PseudoElement>, ) -> Self { // Rebuild the visited style from the parent, ensuring that it will also @@ -2924,26 +2916,23 @@ impl<'a> StyleBuilder<'a> { // produced by this builder. This assumes that the caller doesn't need // to adjust or process visited style, so we can just build visited // style here for simplicity. - let visited_style = parent.visited_style().map(|style| { - Self::for_inheritance( - device, - style, - pseudo, - ).build() + let visited_style = parent.and_then(|parent| { + parent.visited_style().map(|style| { + Self::for_inheritance( + device, + Some(style), + pseudo, + ).build() + }) }); - // FIXME(emilio): This Some(parent) here is inconsistent with what we - // usually do if `parent` is the default computed values, but that's - // fine, and we want to eventually get rid of it. Self::new( device, - Some(parent), - Some(parent), + parent, + parent, pseudo, CascadeFlags::empty(), /* rules = */ None, - parent.custom_properties().cloned(), - parent.writing_mode, - parent.flags.inherited(), + parent.and_then(|p| p.custom_properties().cloned()), visited_style, ) } @@ -3076,9 +3065,10 @@ impl<'a> StyleBuilder<'a> { &self.inherited_style.writing_mode } - /// Inherited style flags. - pub fn inherited_flags(&self) -> &ComputedValueFlags { - &self.inherited_style.flags + /// The computed value flags of our parent. + #[inline] + pub fn get_parent_flags(&self) -> ComputedValueFlags { + self.inherited_style.flags } /// And access to inherited style structs. @@ -3330,8 +3320,6 @@ where flags, Some(rules.clone()), custom_properties, - WritingMode::empty(), - ComputedValueFlags::empty(), visited_style, ), cached_system_font: None, diff --git a/components/style/style_adjuster.rs b/components/style/style_adjuster.rs index 7f805bb3f5c..df80cb83723 100644 --- a/components/style/style_adjuster.rs +++ b/components/style/style_adjuster.rs @@ -8,6 +8,7 @@ use app_units::Au; use dom::TElement; use properties::{self, CascadeFlags, ComputedValues, StyleBuilder}; +use properties::computed_value_flags::ComputedValueFlags; use properties::longhands::display::computed_value::T as Display; use properties::longhands::float::computed_value::T as Float; use properties::longhands::overflow_x::computed_value::T as Overflow; @@ -111,23 +112,23 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> { /// Compute a few common flags for both text and element's style. pub fn set_bits(&mut self) { - use properties::computed_value_flags::ComputedValueFlags; + let display = self.style.get_box().clone_display(); - if self.style.inherited_flags().contains(ComputedValueFlags::IS_IN_DISPLAY_NONE_SUBTREE) || - self.style.get_box().clone_display() == Display::None { + if !display.is_contents() && !self.style.get_text().clone_text_decoration_line().is_empty() { + self.style.flags.insert(ComputedValueFlags::HAS_TEXT_DECORATION_LINES); + } + + if display == Display::None { self.style.flags.insert(ComputedValueFlags::IS_IN_DISPLAY_NONE_SUBTREE); } - if self.style.inherited_flags().contains(ComputedValueFlags::IS_IN_PSEUDO_ELEMENT_SUBTREE) || - self.style.is_pseudo_element() { + if self.style.is_pseudo_element() { self.style.flags.insert(ComputedValueFlags::IS_IN_PSEUDO_ELEMENT_SUBTREE); } #[cfg(feature = "servo")] { - if self.style.inherited_flags().contains(ComputedValueFlags::CAN_BE_FRAGMENTED) || - self.style.get_parent_column().is_multicol() - { + if self.style.get_parent_column().is_multicol() { self.style.flags.insert(ComputedValueFlags::CAN_BE_FRAGMENTED); } } @@ -160,7 +161,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> { fn adjust_for_text_combine_upright(&mut self) { use computed_values::text_combine_upright::T as TextCombineUpright; use computed_values::writing_mode::T as WritingMode; - use properties::computed_value_flags::ComputedValueFlags; let writing_mode = self.style.get_inheritedbox().clone_writing_mode(); @@ -174,15 +174,18 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> { } } - /// Applies the line break suppression flag to text if it is in any ruby - /// box. This is necessary because its parent may not itself have the flag - /// set (e.g. ruby or ruby containers), thus we may not inherit the flag - /// from them. + /// Unconditionally propagates the line break suppression flag to text, and + /// additionally it applies it if it is in any ruby box. + /// + /// This is necessary because its parent may not itself have the flag set + /// (e.g. ruby or ruby containers), thus we may not inherit the flag from + /// them. #[cfg(feature = "gecko")] fn adjust_for_text_in_ruby(&mut self) { - use properties::computed_value_flags::ComputedValueFlags; let parent_display = self.style.get_parent_box().clone_display(); - if parent_display.is_ruby_type() { + if parent_display.is_ruby_type() || + self.style.get_parent_flags().contains(ComputedValueFlags::SHOULD_SUPPRESS_LINEBREAK) + { self.style.flags.insert(ComputedValueFlags::SHOULD_SUPPRESS_LINEBREAK); } } @@ -426,18 +429,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> { self.style.mutate_inheritedtext().set_text_align(TextAlign::Start) } - /// Set the HAS_TEXT_DECORATION_LINES flag based on parent style. - fn adjust_for_text_decoration_lines( - &mut self, - layout_parent_style: &ComputedValues, - ) { - use properties::computed_value_flags::ComputedValueFlags; - if layout_parent_style.flags.contains(ComputedValueFlags::HAS_TEXT_DECORATION_LINES) || - !self.style.get_text().clone_text_decoration_line().is_empty() { - self.style.flags.insert(ComputedValueFlags::HAS_TEXT_DECORATION_LINES); - } - } - /// Computes the used text decoration for Servo. /// /// FIXME(emilio): This is a layout tree concept, should move away from @@ -458,7 +449,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> { &self, layout_parent_style: &ComputedValues, ) -> bool { - use properties::computed_value_flags::ComputedValueFlags; // Line break suppression should only be propagated to in-flow children. if self.style.floated() || self.style.out_of_flow_positioned() { return false; @@ -503,7 +493,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> { where E: TElement, { - use properties::computed_value_flags::ComputedValueFlags; use properties::longhands::unicode_bidi::computed_value::T as UnicodeBidi; let self_display = self.style.get_box().clone_display(); @@ -555,8 +544,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> { where E: TElement, { - use properties::computed_value_flags::ComputedValueFlags; - if !self.style.has_visited_style() { return; } @@ -565,13 +552,7 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> { self.style.pseudo.is_none() && element.map_or(false, |e| e.is_link()); - let relevant_link_visited = if is_link_element { - element.unwrap().is_visited_link() - } else { - self.style.inherited_flags().contains(ComputedValueFlags::IS_RELEVANT_LINK_VISITED) - }; - - if relevant_link_visited { + if is_link_element && element.unwrap().is_visited_link() { self.style.flags.insert(ComputedValueFlags::IS_RELEVANT_LINK_VISITED); } } @@ -674,7 +655,6 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> { self.adjust_for_border_width(); self.adjust_for_outline(); self.adjust_for_writing_mode(layout_parent_style); - self.adjust_for_text_decoration_lines(layout_parent_style); #[cfg(feature = "gecko")] { self.adjust_for_ruby(layout_parent_style, element); diff --git a/components/style/stylesheets/viewport_rule.rs b/components/style/stylesheets/viewport_rule.rs index f4bb2fb39df..3b2c32072dd 100644 --- a/components/style/stylesheets/viewport_rule.rs +++ b/components/style/stylesheets/viewport_rule.rs @@ -708,19 +708,16 @@ impl MaybeNew for ViewportConstraints { } // DEVICE-ADAPT § 6.2.3 Resolve non-auto lengths to pixel lengths - // - // Note: DEVICE-ADAPT § 5. states that relative length values are - // resolved against initial values let initial_viewport = device.au_viewport_size(); let provider = get_metrics_provider_for_product(); - let default_values = device.default_computed_values(); - let mut conditions = RuleCacheConditions::default(); let context = Context { is_root_element: false, - builder: StyleBuilder::for_inheritance(device, default_values, None), + // Note: DEVICE-ADAPT § 5. states that relative length values are + // resolved against initial values + builder: StyleBuilder::for_inheritance(device, None, None), font_metrics_provider: &provider, cached_system_font: None, in_media_query: false, diff --git a/components/style/values/computed/mod.rs b/components/style/values/computed/mod.rs index 5e1c519c7c4..e883cc79a89 100644 --- a/components/style/values/computed/mod.rs +++ b/components/style/values/computed/mod.rs @@ -174,12 +174,11 @@ impl<'a> Context<'a> { F: FnOnce(&Context) -> R { let mut conditions = RuleCacheConditions::default(); - let default_values = device.default_computed_values(); let provider = get_metrics_provider_for_product(); let context = Context { is_root_element: false, - builder: StyleBuilder::for_inheritance(device, default_values, None), + builder: StyleBuilder::for_inheritance(device, None, None), font_metrics_provider: &provider, cached_system_font: None, in_media_query: true, diff --git a/components/style/values/specified/box.rs b/components/style/values/specified/box.rs index 99858936702..3bf64452072 100644 --- a/components/style/values/specified/box.rs +++ b/components/style/values/specified/box.rs @@ -217,6 +217,16 @@ impl Display { } } + /// Returns true if the value is `Contents` + #[inline] + pub fn is_contents(&self) -> bool { + match *self { + #[cfg(feature = "gecko")] + Display::Contents => true, + _ => false, + } + } + /// Returns true if the value is `None` #[inline] pub fn is_none(&self) -> bool { diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index ba2614f7dcd..224f3a3798e 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -1997,10 +1997,11 @@ pub extern "C" fn Servo_FontFeatureValuesRule_GetValueText(rule: RawServoFontFea } #[no_mangle] -pub extern "C" fn Servo_ComputedValues_GetForAnonymousBox(parent_style_or_null: ServoStyleContextBorrowedOrNull, - pseudo_tag: *mut nsAtom, - raw_data: RawServoStyleSetBorrowed) - -> ServoStyleContextStrong { +pub extern "C" fn Servo_ComputedValues_GetForAnonymousBox( + parent_style_or_null: ServoStyleContextBorrowedOrNull, + pseudo_tag: *mut nsAtom, + raw_data: RawServoStyleSetBorrowed, +) -> ServoStyleContextStrong { let global_style_data = &*GLOBAL_STYLE_DATA; let guard = global_style_data.shared_lock.read(); let guards = StylesheetGuards::same(&guard); @@ -2299,7 +2300,7 @@ fn get_pseudo_style( Some(style.unwrap_or_else(|| { StyleBuilder::for_inheritance( doc_data.stylist.device(), - styles.primary(), + Some(styles.primary()), Some(pseudo), ).build() })) @@ -2318,11 +2319,10 @@ pub extern "C" fn Servo_ComputedValues_Inherit( let atom = Atom::from(pseudo_tag); let pseudo = PseudoElement::from_anon_box_atom(&atom) .expect("Not an anon-box? Gah!"); - let device = data.stylist.device(); let mut style = StyleBuilder::for_inheritance( data.stylist.device(), - parent_style_context.unwrap_or_else(|| device.default_computed_values()), + parent_style_context, Some(&pseudo) ); @@ -3647,22 +3647,20 @@ fn simulate_compute_values_failure(_: &PropertyValuePair) -> bool { false } -fn create_context<'a>( +fn create_context_for_animation<'a>( per_doc_data: &'a PerDocumentStyleDataImpl, font_metrics_provider: &'a FontMetricsProvider, style: &'a ComputedValues, parent_style: Option<&'a ComputedValues>, - pseudo: Option<&'a PseudoElement>, for_smil_animation: bool, rule_cache_conditions: &'a mut RuleCacheConditions, ) -> Context<'a> { Context { is_root_element: false, - builder: StyleBuilder::for_derived_style( + builder: StyleBuilder::for_animation( per_doc_data.stylist.device(), style, parent_style, - pseudo, ), font_metrics_provider: font_metrics_provider, cached_system_font: None, @@ -3740,14 +3738,12 @@ pub extern "C" fn Servo_GetComputedKeyframeValues( let parent_data = parent_element.as_ref().and_then(|e| e.borrow_data()); let parent_style = parent_data.as_ref().map(|d| d.styles.primary()).map(|x| &**x); - let pseudo = style.pseudo(); let mut conditions = Default::default(); - let mut context = create_context( + let mut context = create_context_for_animation( &data, &metrics, &style, parent_style, - pseudo.as_ref(), /* for_smil_animation = */ false, &mut conditions, ); @@ -3849,14 +3845,12 @@ pub extern "C" fn Servo_GetAnimationValues( let parent_data = parent_element.as_ref().and_then(|e| e.borrow_data()); let parent_style = parent_data.as_ref().map(|d| d.styles.primary()).map(|x| &**x); - let pseudo = style.pseudo(); let mut conditions = Default::default(); - let mut context = create_context( + let mut context = create_context_for_animation( &data, &metrics, &style, parent_style, - pseudo.as_ref(), /* for_smil_animation = */ true, &mut conditions, ); @@ -3893,14 +3887,12 @@ pub extern "C" fn Servo_AnimationValue_Compute( let parent_data = parent_element.as_ref().and_then(|e| e.borrow_data()); let parent_style = parent_data.as_ref().map(|d| d.styles.primary()).map(|x| &**x); - let pseudo = style.pseudo(); let mut conditions = Default::default(); - let mut context = create_context( + let mut context = create_context_for_animation( &data, &metrics, style, parent_style, - pseudo.as_ref(), /* for_smil_animation = */ false, &mut conditions, ); @@ -4598,18 +4590,11 @@ pub extern "C" fn Servo_ComputeColor( let computed_color = match raw_data { Some(raw_data) => { let data = PerDocumentStyleData::from_ffi(raw_data).borrow(); - let metrics = get_metrics_provider_for_product(); - let mut conditions = Default::default(); - let context = create_context( - &data, - &metrics, - data.stylist.device().default_computed_values(), - /* parent_style = */ None, - /* pseudo = */ None, - /* for_smil_animation = */ false, - &mut conditions, - ); - specified_color.to_computed_color(Some(&context)) + let device = data.stylist.device(); + let quirks_mode = data.stylist.quirks_mode(); + Context::for_media_query_evaluation(device, quirks_mode, |context| { + specified_color.to_computed_color(Some(&context)) + }) } None => { specified_color.to_computed_color(None)