nagai/fune
3
0
Fork 0
forked from mirrors/gecko-dev
Code Pull requests Activity

Bug 1858468 - Don't compute custom properties twice inside links. r=dshin

Browse source 
This matches the behavior of other browsers (in fact, I filed [1] about
it long time ago).

This avoids a bunch of overhead in some speedometer subtests. Makes me a
bit sad because I still think our approach is slightly more correct per
spec, but not worth the performance cost.

[1]: https://github.com/w3c/csswg-drafts/issues/2263

Differential Revision: https://phabricator.services.mozilla.com/D190705
This commit is contained in:
Emilio Cobos Álvarez 2023-10-16 19:55:36 +00:00
parent 6da195e1ff
commit 1ea6ab832c
5 changed files with 96 additions and 70 deletions
repo.diff.show_diff_stats Download patch file Download diff file Expand all files Collapse all files

7
layout/reftests/css-visited/variables-visited-ref.html Normal file
View file

@ -0,0 +1,7 @@
<!doctype html>
<style>
a {
color: green;
}
</style>
<a href="">Which color?</a>

12
layout/reftests/css-visited/variables-visited.html Normal file
View file

@ -0,0 +1,12 @@
<!doctype html>
<style>
a {
--foo: green;
}
:visited {
--foo: red;
color: var(--foo);
}
</style>
<a href="visited-page.html">Which color?</a>

2
layout/style/test/moz.build
View file

@ -112,6 +112,8 @@ TEST_HARNESS_FILES.testing.mochitest.tests.layout.style.test["css-visited"] += [
"/layout/reftests/css-visited/svg-paint-currentcolor-visited.svg", "/layout/reftests/css-visited/svg-paint-currentcolor-visited.svg",
"/layout/reftests/css-visited/transition-on-visited-ref.html", "/layout/reftests/css-visited/transition-on-visited-ref.html",
"/layout/reftests/css-visited/transition-on-visited.html", "/layout/reftests/css-visited/transition-on-visited.html",
"/layout/reftests/css-visited/variables-visited-ref.html",
"/layout/reftests/css-visited/variables-visited.html",
"/layout/reftests/css-visited/visited-inherit-1-ref.html", "/layout/reftests/css-visited/visited-inherit-1-ref.html",
"/layout/reftests/css-visited/visited-inherit-1.html", "/layout/reftests/css-visited/visited-inherit-1.html",
"/layout/reftests/css-visited/visited-page.html", "/layout/reftests/css-visited/visited-page.html",

1
layout/style/test/test_visited_reftests.html
View file

@ -95,6 +95,7 @@ var gTests = [
"== logical-box-border-color-visited-link-002.html logical-box-border-color-visited-link-ref.html", "== logical-box-border-color-visited-link-002.html logical-box-border-color-visited-link-ref.html",
"== logical-box-border-color-visited-link-003.html logical-box-border-color-visited-link-ref.html", "== logical-box-border-color-visited-link-003.html logical-box-border-color-visited-link-ref.html",
"== svg-paint-currentcolor-visited.svg svg-paint-currentcolor-visited-ref.svg", "== svg-paint-currentcolor-visited.svg svg-paint-currentcolor-visited-ref.svg",
"== variables-visited.html variables-visited-ref.html",
]; ];
// We record the maximum number of times we had to look at a test before // We record the maximum number of times we had to look at a test before

144
servo/components/style/properties/cascade.rs
View file

@ -217,7 +217,7 @@ where
/// Whether we're cascading for visited or unvisited styles. /// Whether we're cascading for visited or unvisited styles.
#[derive(Clone, Copy)] #[derive(Clone, Copy)]
pub enum CascadeMode<'a> { pub enum CascadeMode<'a, 'b> {
/// We're cascading for unvisited styles. /// We're cascading for unvisited styles.
Unvisited { Unvisited {
/// The visited rules that should match the visited style. /// The visited rules that should match the visited style.
@ -225,12 +225,28 @@ pub enum CascadeMode<'a> {
}, },
/// We're cascading for visited styles. /// We're cascading for visited styles.
Visited { Visited {
/// The writing mode of our unvisited style, needed to correctly resolve /// The cascade for our unvisited style.
/// logical properties.. unvisited_context: &'a computed::Context<'b>,
writing_mode: WritingMode,
}, },
} }
fn iter_declarations<'builder, 'decls: 'builder>(
iter: impl Iterator<Item = (&'decls PropertyDeclaration, CascadePriority)>,
declarations: &mut Declarations<'decls>,
mut custom_builder: Option<&mut CustomPropertiesBuilder<'builder>>,
) {
for (declaration, priority) in iter {
if let PropertyDeclaration::Custom(ref declaration) = *declaration {
if let Some(ref mut builder) = custom_builder {
builder.cascade(declaration, priority);
}
} else {
let id = declaration.id().as_longhand().unwrap();
declarations.note_declaration(declaration, priority, id);
}
}
}
/// NOTE: This function expects the declaration with more priority to appear /// NOTE: This function expects the declaration with more priority to appear
/// first. /// first.
pub fn apply_declarations<'a, E, I>( pub fn apply_declarations<'a, E, I>(
@ -285,34 +301,32 @@ where
context.style().add_flags(cascade_input_flags); context.style().add_flags(cascade_input_flags);
let using_cached_reset_properties; let using_cached_reset_properties;
let mut cascade = Cascade::new(&mut context, cascade_mode, first_line_reparenting); let mut cascade = Cascade::new(&mut context, first_line_reparenting);
let mut data = CascadeData::default(); let mut declarations = Default::default();
let mut shorthand_cache = ShorthandsWithPropertyReferencesCache::default();
{ let properties_to_apply = match cascade_mode {
let mut builder = CascadeMode::Visited { unvisited_context } => {
CustomPropertiesBuilder::new(inherited_style.custom_properties(), stylist, is_root_element); cascade.context.builder.custom_properties = unvisited_context.builder.custom_properties.clone();
for (declaration, priority) in iter { cascade.context.builder.writing_mode = unvisited_context.builder.writing_mode;
if let PropertyDeclaration::Custom(ref declaration) = *declaration {
builder.cascade(declaration, priority);
} else {
let id = declaration.id().as_longhand().unwrap();
data.note_declaration(declaration, priority, id);
}
}
cascade.context.builder.custom_properties = builder.build();
};
let properties_to_apply = match cascade.cascade_mode {
CascadeMode::Visited { writing_mode } => {
cascade.context.builder.writing_mode = writing_mode;
// We never insert visited styles into the cache so we don't need to try looking it up. // We never insert visited styles into the cache so we don't need to try looking it up.
// It also wouldn't be super-profitable, only a handful :visited properties are // It also wouldn't be super-profitable, only a handful :visited properties are
// non-inherited. // non-inherited.
using_cached_reset_properties = false; using_cached_reset_properties = false;
// TODO(bug 1859385): If we match the same rules when visited and unvisited, we could
// try to avoid gathering the declarations. That'd be:
// unvisited_context.builder.rules.as_ref() == Some(rules)
iter_declarations(iter, &mut declarations, None);
LonghandIdSet::visited_dependent() LonghandIdSet::visited_dependent()
}, },
CascadeMode::Unvisited { visited_rules } => { CascadeMode::Unvisited { visited_rules } => {
cascade.apply_prioritary_properties(&mut data); cascade.context.builder.custom_properties = {
let mut builder =
CustomPropertiesBuilder::new(inherited_style.custom_properties(), stylist, is_root_element);
iter_declarations(iter, &mut declarations, Some(&mut builder));
builder.build()
};
cascade.apply_prioritary_properties(&declarations, &mut shorthand_cache);
if let Some(visited_rules) = visited_rules { if let Some(visited_rules) = visited_rules {
cascade.compute_visited_style_if_needed( cascade.compute_visited_style_if_needed(
@ -336,7 +350,7 @@ where
}, },
}; };
cascade.apply_non_prioritary_properties(&mut data, &properties_to_apply); cascade.apply_non_prioritary_properties(&declarations.longhand_declarations, &mut shorthand_cache, &properties_to_apply);
cascade.finished_applying_properties(); cascade.finished_applying_properties();
@ -536,22 +550,18 @@ struct Declaration<'a> {
next_index: DeclarationIndex, next_index: DeclarationIndex,
} }
/// A bit of a kitchen-sink struct for things that need to mutate in ways that otherwise rustc /// The set of property declarations from our rules.
/// can't reason about if we put these in Cascade.
#[derive(Default)] #[derive(Default)]
struct CascadeData<'a> { struct Declarations<'a> {
/// Whether we have any prioritary property. This is just a minor optimization. /// Whether we have any prioritary property. This is just a minor optimization.
has_prioritary_properties: bool, has_prioritary_properties: bool,
/// A cache for shorthands with property references, to avoid substituting the same value over
/// and over for each of the longhands.
shorthand_cache: ShorthandsWithPropertyReferencesCache,
/// A list of all the applicable longhand declarations. /// A list of all the applicable longhand declarations.
longhand_declarations: SmallVec<[Declaration<'a>; 32]>, longhand_declarations: SmallVec<[Declaration<'a>; 32]>,
/// The prioritary property position data. /// The prioritary property position data.
prioritary_positions: [PrioritaryDeclarationPosition; PRIORITARY_PROPERTY_COUNT], prioritary_positions: [PrioritaryDeclarationPosition; PRIORITARY_PROPERTY_COUNT],
} }
impl<'a> CascadeData<'a> { impl<'a> Declarations<'a> {
fn note_prioritary_property(&mut self, id: PrioritaryPropertyId) { fn note_prioritary_property(&mut self, id: PrioritaryPropertyId) {
let new_index = self.longhand_declarations.len(); let new_index = self.longhand_declarations.len();
if new_index >= DeclarationIndex::MAX as usize { if new_index >= DeclarationIndex::MAX as usize {
@ -593,7 +603,6 @@ impl<'a> CascadeData<'a> {
struct Cascade<'a, 'b: 'a> { struct Cascade<'a, 'b: 'a> {
context: &'a mut computed::Context<'b>, context: &'a mut computed::Context<'b>,
cascade_mode: CascadeMode<'a>,
first_line_reparenting: FirstLineReparenting<'b>, first_line_reparenting: FirstLineReparenting<'b>,
ignore_colors: bool, ignore_colors: bool,
seen: LonghandIdSet, seen: LonghandIdSet,
@ -606,13 +615,11 @@ struct Cascade<'a, 'b: 'a> {
impl<'a, 'b: 'a> Cascade<'a, 'b> { impl<'a, 'b: 'a> Cascade<'a, 'b> {
fn new( fn new(
context: &'a mut computed::Context<'b>, context: &'a mut computed::Context<'b>,
cascade_mode: CascadeMode<'a>,
first_line_reparenting: FirstLineReparenting<'b>, first_line_reparenting: FirstLineReparenting<'b>,
) -> Self { ) -> Self {
let ignore_colors = !context.builder.device.use_document_colors(); let ignore_colors = !context.builder.device.use_document_colors();
Self { Self {
context, context,
cascade_mode,
first_line_reparenting, first_line_reparenting,
ignore_colors, ignore_colors,
seen: LonghandIdSet::default(), seen: LonghandIdSet::default(),
@ -678,10 +685,11 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
fn apply_one_prioritary_property( fn apply_one_prioritary_property(
&mut self, &mut self,
data: &mut CascadeData, decls: &Declarations,
cache: &mut ShorthandsWithPropertyReferencesCache,
id: PrioritaryPropertyId, id: PrioritaryPropertyId,
) -> bool { ) -> bool {
let mut index = data.prioritary_positions[id as usize].most_important; let mut index = decls.prioritary_positions[id as usize].most_important;
if index == DeclarationIndex::MAX { if index == DeclarationIndex::MAX {
return false; return false;
} }
@ -692,13 +700,13 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
"That could require more book-keeping" "That could require more book-keeping"
); );
loop { loop {
let decl = data.longhand_declarations[index as usize]; let decl = decls.longhand_declarations[index as usize];
self.apply_one_longhand( self.apply_one_longhand(
longhand_id, longhand_id,
longhand_id, longhand_id,
decl.decl, decl.decl,
decl.priority, decl.priority,
&mut data.shorthand_cache, cache,
); );
if self.seen.contains(longhand_id) { if self.seen.contains(longhand_id) {
return true; // Common case, we're done. return true; // Common case, we're done.
@ -721,27 +729,27 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
false false
} }
fn apply_prioritary_properties(&mut self, data: &mut CascadeData) { fn apply_prioritary_properties(&mut self, decls: &Declarations, cache: &mut ShorthandsWithPropertyReferencesCache) {
if !data.has_prioritary_properties { if !decls.has_prioritary_properties {
return; return;
} }
let has_writing_mode = self let has_writing_mode = self
.apply_one_prioritary_property(data, PrioritaryPropertyId::WritingMode) | .apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::WritingMode) |
self.apply_one_prioritary_property(data, PrioritaryPropertyId::Direction) | self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::Direction) |
self.apply_one_prioritary_property(data, PrioritaryPropertyId::TextOrientation); self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::TextOrientation);
if has_writing_mode { if has_writing_mode {
self.compute_writing_mode(); self.compute_writing_mode();
} }
if self.apply_one_prioritary_property(data, PrioritaryPropertyId::Zoom) { if self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::Zoom) {
self.compute_zoom(); self.compute_zoom();
} }
// Compute font-family. // Compute font-family.
let has_font_family = let has_font_family =
self.apply_one_prioritary_property(data, PrioritaryPropertyId::FontFamily); self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::FontFamily);
let has_lang = self.apply_one_prioritary_property(data, PrioritaryPropertyId::XLang); let has_lang = self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::XLang);
if has_lang { if has_lang {
self.recompute_initial_font_family_if_needed(); self.recompute_initial_font_family_if_needed();
} }
@ -750,15 +758,15 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
} }
// Compute font-size. // Compute font-size.
if self.apply_one_prioritary_property(data, PrioritaryPropertyId::XTextScale) { if self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::XTextScale) {
self.unzoom_fonts_if_needed(); self.unzoom_fonts_if_needed();
} }
let has_font_size = let has_font_size =
self.apply_one_prioritary_property(data, PrioritaryPropertyId::FontSize); self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::FontSize);
let has_math_depth = let has_math_depth =
self.apply_one_prioritary_property(data, PrioritaryPropertyId::MathDepth); self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::MathDepth);
let has_min_font_size_ratio = let has_min_font_size_ratio =
self.apply_one_prioritary_property(data, PrioritaryPropertyId::MozMinFontSizeRatio); self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::MozMinFontSizeRatio);
if has_math_depth && has_font_size { if has_math_depth && has_font_size {
self.recompute_math_font_size_if_needed(); self.recompute_math_font_size_if_needed();
@ -771,26 +779,27 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
} }
// Compute the rest of the first-available-font-affecting properties. // Compute the rest of the first-available-font-affecting properties.
self.apply_one_prioritary_property(data, PrioritaryPropertyId::FontWeight); self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::FontWeight);
self.apply_one_prioritary_property(data, PrioritaryPropertyId::FontStretch); self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::FontStretch);
self.apply_one_prioritary_property(data, PrioritaryPropertyId::FontStyle); self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::FontStyle);
self.apply_one_prioritary_property(data, PrioritaryPropertyId::FontSizeAdjust); self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::FontSizeAdjust);
self.apply_one_prioritary_property(data, PrioritaryPropertyId::ColorScheme); self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::ColorScheme);
self.apply_one_prioritary_property(data, PrioritaryPropertyId::ForcedColorAdjust); self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::ForcedColorAdjust);
// Compute the line height. // Compute the line height.
self.apply_one_prioritary_property(data, PrioritaryPropertyId::LineHeight); self.apply_one_prioritary_property(decls, cache, PrioritaryPropertyId::LineHeight);
} }
fn apply_non_prioritary_properties( fn apply_non_prioritary_properties(
&mut self, &mut self,
data: &mut CascadeData, longhand_declarations: &[Declaration],
shorthand_cache: &mut ShorthandsWithPropertyReferencesCache,
properties_to_apply: &LonghandIdSet, properties_to_apply: &LonghandIdSet,
) { ) {
debug_assert!(!properties_to_apply.contains_any(LonghandIdSet::prioritary_properties())); debug_assert!(!properties_to_apply.contains_any(LonghandIdSet::prioritary_properties()));
debug_assert!(self.declarations_to_apply_unless_overridden.is_empty()); debug_assert!(self.declarations_to_apply_unless_overridden.is_empty());
for declaration in &data.longhand_declarations { for declaration in &*longhand_declarations {
let longhand_id = declaration.decl.id().as_longhand().unwrap(); let longhand_id = declaration.decl.id().as_longhand().unwrap();
if !properties_to_apply.contains(longhand_id) { if !properties_to_apply.contains(longhand_id) {
continue; continue;
@ -802,7 +811,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
physical_longhand_id, physical_longhand_id,
declaration.decl, declaration.decl,
declaration.priority, declaration.priority,
&mut data.shorthand_cache, shorthand_cache,
); );
} }
if !self.declarations_to_apply_unless_overridden.is_empty() { if !self.declarations_to_apply_unless_overridden.is_empty() {
@ -823,7 +832,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
physical_longhand_id: LonghandId, physical_longhand_id: LonghandId,
declaration: &PropertyDeclaration, declaration: &PropertyDeclaration,
priority: CascadePriority, priority: CascadePriority,
shorthand_cache: &mut ShorthandsWithPropertyReferencesCache, cache: &mut ShorthandsWithPropertyReferencesCache,
) { ) {
debug_assert!(!physical_longhand_id.is_logical()); debug_assert!(!physical_longhand_id.is_logical());
let origin = priority.cascade_level().origin(); let origin = priority.cascade_level().origin();
@ -841,7 +850,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
} }
} }
let mut declaration = self.substitute_variables_if_needed(shorthand_cache, declaration); let mut declaration = self.substitute_variables_if_needed(cache, declaration);
// When document colors are disabled, do special handling of // When document colors are disabled, do special handling of
// properties that are marked as ignored in that mode. // properties that are marked as ignored in that mode.
@ -896,7 +905,6 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
} }
fn compute_zoom(&mut self) { fn compute_zoom(&mut self) {
debug_assert!(matches!(self.cascade_mode, CascadeMode::Unvisited { .. }));
self.context.builder.effective_zoom = self self.context.builder.effective_zoom = self
.context .context
.builder .builder
@ -905,7 +913,6 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
} }
fn compute_writing_mode(&mut self) { fn compute_writing_mode(&mut self) {
debug_assert!(matches!(self.cascade_mode, CascadeMode::Unvisited { .. }));
self.context.builder.writing_mode = self.context.builder.writing_mode =
WritingMode::new(self.context.builder.get_inherited_box()) WritingMode::new(self.context.builder.get_inherited_box())
} }
@ -921,7 +928,6 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
) where ) where
E: TElement, E: TElement,
{ {
debug_assert!(matches!(self.cascade_mode, CascadeMode::Unvisited { .. }));
let is_link = self.context.builder.pseudo.is_none() && element.unwrap().is_link(); let is_link = self.context.builder.pseudo.is_none() && element.unwrap().is_link();
macro_rules! visited_parent { macro_rules! visited_parent {
@ -934,8 +940,6 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
}; };
} }
let writing_mode = self.context.builder.writing_mode;
// We could call apply_declarations directly, but that'd cause // We could call apply_declarations directly, but that'd cause
// another instantiation of this function which is not great. // another instantiation of this function which is not great.
let style = cascade_rules( let style = cascade_rules(
@ -947,7 +951,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
visited_parent!(parent_style), visited_parent!(parent_style),
visited_parent!(layout_parent_style), visited_parent!(layout_parent_style),
self.first_line_reparenting, self.first_line_reparenting,
CascadeMode::Visited { writing_mode }, CascadeMode::Visited { unvisited_context: &*self.context },
// Cascade input flags don't matter for the visited style, they are // Cascade input flags don't matter for the visited style, they are
// in the main (unvisited) style. // in the main (unvisited) style.
Default::default(), Default::default(),