Skip to content

Commit

Permalink
style: Optimize cascading of other wide keywords if possible.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@emilio
emilio committed Mar 13, 2019
1 parent 6fd17cc commit 4a1f390
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 38 deletions.
31 changes: 22 additions & 9 deletions components/style/properties/cascade.rs
View file
Expand Up @@ -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};
Expand Down Expand Up @@ -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)
Expand All @@ -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.
Expand Down Expand Up @@ -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);
}
}
}
Expand Down
20 changes: 11 additions & 9 deletions components/style/properties/helpers.mako.rs
View file
Expand Up @@ -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
}
Expand Down
24 changes: 4 additions & 20 deletions components/style/properties/properties.mako.rs
View file
Expand Up @@ -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<SystemFont> {
match *self {
Expand Down Expand Up @@ -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);
Expand All @@ -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;
}
Expand All @@ -3507,6 +3491,7 @@ impl<'a> StyleBuilder<'a> {
% endif
);
}
% endif

% if not property.is_vector:
/// Set the `${property.ident}` to the computed value `value`.
Expand All @@ -3528,7 +3513,6 @@ impl<'a> StyleBuilder<'a> {
);
}
% endif
% endif
% endfor
<% del property %>

Expand Down

0 comments on commit 4a1f390

Please sign in to comment.