From 4a1f390788f647c4a96371a8a6608a677b600552 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 7 Mar 2019 12:48:07 +0000 Subject: [PATCH] style: Optimize cascading of other wide keywords if possible. The way the copy-on-write stuff works, and the way that we have to apply properties from most specific to less specific guarantees that always that we're going to inherit an inherited property, or reset a reset property, we have already the right value on the style. Revert relies on that, so there doesn't seem to be a reason to not use that fact more often and skip useless work earlier. Font-size is still special of course... I think I have a way to move the specialness outside of the style, but piece by piece. Differential Revision: https://phabricator.services.mozilla.com/D21882 --- components/style/properties/cascade.rs | 31 +++++++++++++------ components/style/properties/helpers.mako.rs | 20 ++++++------ .../style/properties/properties.mako.rs | 24 +++----------- 3 files changed, 37 insertions(+), 38 deletions(-) diff --git a/components/style/properties/cascade.rs b/components/style/properties/cascade.rs index 09b055b07cda..23970d648889 100644 --- a/components/style/properties/cascade.rs +++ b/components/style/properties/cascade.rs @@ -11,7 +11,7 @@ use crate::font_metrics::FontMetricsProvider; use crate::logical_geometry::WritingMode; use crate::media_queries::Device; use crate::properties::{ComputedValues, StyleBuilder}; -use crate::properties::{LonghandId, LonghandIdSet}; +use crate::properties::{LonghandId, LonghandIdSet, CSSWideKeyword}; use crate::properties::{PropertyDeclaration, PropertyDeclarationId, DeclarationImportanceIterator}; use crate::properties::CASCADE_PROPERTY; use crate::rule_cache::{RuleCache, RuleCacheConditions}; @@ -542,7 +542,11 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { } } - if declaration.is_revert() { + let css_wide_keyword = declaration.get_css_wide_keyword(); + if let Some(CSSWideKeyword::Revert) = css_wide_keyword { + // We intentionally don't want to insert it into `self.seen`, + // `reverted` takes care of rejecting other declarations as + // needed. for origin in origin.following_including() { self.reverted .borrow_mut_for_origin(&origin) @@ -553,6 +557,19 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { self.seen.insert(physical_longhand_id); + let unset = css_wide_keyword.map_or(false, |css_wide_keyword| { + match css_wide_keyword { + CSSWideKeyword::Unset => true, + CSSWideKeyword::Inherit => inherited, + CSSWideKeyword::Initial => !inherited, + CSSWideKeyword::Revert => unreachable!(), + } + }); + + if unset { + continue; + } + // FIXME(emilio): We should avoid generating code for logical // longhands and just use the physical ones, then rename // physical_longhand_id to just longhand_id. @@ -811,18 +828,14 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { self.seen.contains(LonghandId::MozMinFontSizeRatio) || self.seen.contains(LonghandId::FontFamily) { - use crate::properties::{CSSWideKeyword, WideKeywordDeclaration}; + use crate::values::computed::FontSize; // font-size must be explicitly inherited to handle lang // changes and scriptlevel changes. // // FIXME(emilio): That looks a bit bogus... - let inherit = PropertyDeclaration::CSSWideKeyword(WideKeywordDeclaration { - id: LonghandId::FontSize, - keyword: CSSWideKeyword::Inherit, - }); - - self.apply_declaration_ignoring_phase(LonghandId::FontSize, &inherit); + self.context.for_non_inherited_property = None; + FontSize::cascade_inherit_font_size(&mut self.context); } } } diff --git a/components/style/properties/helpers.mako.rs b/components/style/properties/helpers.mako.rs index 334ea5de09ef..81931bc713b0 100644 --- a/components/style/properties/helpers.mako.rs +++ b/components/style/properties/helpers.mako.rs @@ -326,26 +326,28 @@ PropertyDeclaration::CSSWideKeyword(ref declaration) => { debug_assert_eq!(declaration.id, LonghandId::${property.camel_case}); match declaration.keyword { - % if not data.current_style_struct.inherited: + % if not property.style_struct.inherited: CSSWideKeyword::Unset | % endif CSSWideKeyword::Initial => { - % if property.ident == "font_size": - computed::FontSize::cascade_initial_font_size(context); + % if not property.style_struct.inherited: + debug_assert!(false, "Should be handled in apply_properties"); % else: + % if property.name == "font-size": + computed::FontSize::cascade_initial_font_size(context); + % else: context.builder.reset_${property.ident}(); + % endif % endif }, - % if data.current_style_struct.inherited: + % if property.style_struct.inherited: CSSWideKeyword::Unset | % endif CSSWideKeyword::Inherit => { - % if not property.style_struct.inherited: - context.rule_cache_conditions.borrow_mut().set_uncacheable(); - % endif - % if property.ident == "font_size": - computed::FontSize::cascade_inherit_font_size(context); + % if property.style_struct.inherited: + debug_assert!(false, "Should be handled in apply_properties"); % else: + context.rule_cache_conditions.borrow_mut().set_uncacheable(); context.builder.inherit_${property.ident}(); % endif } diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 7bc10b785c6d..73965f90a3c4 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -2117,12 +2117,6 @@ impl PropertyDeclaration { } } - /// Whether this declaration is the `revert` keyword. - #[inline] - pub fn is_revert(&self) -> bool { - self.get_css_wide_keyword().map_or(false, |kw| kw == CSSWideKeyword::Revert) - } - /// Returns whether or not the property is set by a system font pub fn get_system(&self) -> Option { match *self { @@ -3447,22 +3441,16 @@ impl<'a> StyleBuilder<'a> { } % for property in data.longhands: - % if property.ident != "font_size": + % if not property.style_struct.inherited: /// Inherit `${property.ident}` from our parent style. #[allow(non_snake_case)] pub fn inherit_${property.ident}(&mut self) { let inherited_struct = - % if property.style_struct.inherited: - self.inherited_style.get_${property.style_struct.name_lower}(); - % else: self.inherited_style_ignoring_first_line .get_${property.style_struct.name_lower}(); - % endif - % if not property.style_struct.inherited: - self.flags.insert(ComputedValueFlags::INHERITS_RESET_STYLE); self.modified_reset = true; - % endif + self.flags.insert(ComputedValueFlags::INHERITS_RESET_STYLE); % if property.ident == "content": self.flags.insert(ComputedValueFlags::INHERITS_CONTENT); @@ -3484,17 +3472,13 @@ impl<'a> StyleBuilder<'a> { % endif ); } - + % elif property.name != "font-size": /// Reset `${property.ident}` to the initial value. #[allow(non_snake_case)] pub fn reset_${property.ident}(&mut self) { let reset_struct = self.reset_style.get_${property.style_struct.name_lower}(); - % if not property.style_struct.inherited: - self.modified_reset = true; - % endif - if self.${property.style_struct.ident}.ptr_eq(reset_struct) { return; } @@ -3507,6 +3491,7 @@ impl<'a> StyleBuilder<'a> { % endif ); } + % endif % if not property.is_vector: /// Set the `${property.ident}` to the computed value `value`. @@ -3528,7 +3513,6 @@ impl<'a> StyleBuilder<'a> { ); } % endif - % endif % endfor <% del property %>