From 8d60353d14f6da1957bf57a9879d505612165ef0 Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Fri, 15 Aug 2025 09:26:00 -0700 Subject: [PATCH] script: Add presentation attributes as part of a single `PropertyDeclarationBlock` (#38684) Instead of making a block for each attribute, use a single block as described in a `FIXME` comment by @emilio. This also switch to using `map` and `and_then` in more places to make the code a bit more concise. Testing: This should not change behavior other than to incraese efficiency a bit and is thus covered by existing tests. Signed-off-by: Martin Robinson --- components/script/dom/element.rs | 351 +++++++++++-------------------- 1 file changed, 128 insertions(+), 223 deletions(-) diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 59335987d6d..a3ccfabef4b 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -51,7 +51,7 @@ use style::selector_parser::{ NonTSPseudoClass, PseudoElement, RestyleDamage, SelectorImpl, SelectorParser, extended_filtering, }; -use style::shared_lock::{Locked, SharedRwLock}; +use style::shared_lock::Locked; use style::stylesheets::layer_rule::LayerOrder; use style::stylesheets::{CssRuleType, Origin as CssOrigin, UrlExtraData}; use style::values::computed::Overflow; @@ -1368,32 +1368,19 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> { where V: Push, { - // FIXME(emilio): Just a single PDB should be enough. - #[inline] - fn from_declaration( - shared_lock: &SharedRwLock, - declaration: PropertyDeclaration, - ) -> ApplicableDeclarationBlock { - ApplicableDeclarationBlock::from_declarations( - Arc::new(shared_lock.wrap(PropertyDeclarationBlock::with_one( - declaration, - Importance::Normal, - ))), - CascadeLevel::PresHints, - LayerOrder::root(), - ) - } - - let document = self.upcast::().owner_doc_for_layout(); - let shared_lock = document.style_shared_lock(); + let mut property_declaration_block = None; + let mut push = |declaration| { + property_declaration_block + .get_or_insert_with(PropertyDeclarationBlock::default) + .push(declaration, Importance::Normal); + }; // TODO(xiaochengh): This is probably not enough. When the root element doesn't have a `lang`, // we should check the browser settings and system locale. if let Some(lang) = self.get_lang_attr_val_for_layout() { - hints.push(from_declaration( - shared_lock, - PropertyDeclaration::XLang(specified::XLang(Atom::from(lang.to_owned()))), - )); + push(PropertyDeclaration::XLang(specified::XLang(Atom::from( + lang.to_owned(), + )))); } let bgcolor = if let Some(this) = self.downcast::() { @@ -1411,24 +1398,19 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> { }; if let Some(color) = bgcolor { - hints.push(from_declaration( - shared_lock, - PropertyDeclaration::BackgroundColor(specified::Color::from_absolute_color(color)), + push(PropertyDeclaration::BackgroundColor( + specified::Color::from_absolute_color(color), )); } - let background = if let Some(this) = self.downcast::() { - this.get_background() - } else { - None - }; - + let background = self + .downcast::() + .and_then(HTMLBodyElementLayoutHelpers::get_background); if let Some(url) = background { - hints.push(from_declaration( - shared_lock, - PropertyDeclaration::BackgroundImage(background_image::SpecifiedValue( + push(PropertyDeclaration::BackgroundImage( + background_image::SpecifiedValue( vec![specified::Image::for_cascade(url.get_arc())].into(), - )), + ), )); } @@ -1445,61 +1427,41 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> { }; if let Some(color) = color { - hints.push(from_declaration( - shared_lock, - PropertyDeclaration::Color(longhands::color::SpecifiedValue( - specified::Color::from_absolute_color(color), - )), + push(PropertyDeclaration::Color( + longhands::color::SpecifiedValue(specified::Color::from_absolute_color(color)), )); } - let font_face = if let Some(this) = self.downcast::() { - this.get_face() - } else { - None - }; - + let font_face = self + .downcast::() + .and_then(HTMLFontElementLayoutHelpers::get_face); if let Some(font_face) = font_face { - hints.push(from_declaration( - shared_lock, - PropertyDeclaration::FontFamily(font_family::SpecifiedValue::Values( - computed::font::FontFamilyList { - list: ArcSlice::from_iter( - HTMLFontElement::parse_face_attribute(font_face).into_iter(), - ), - }, - )), + push(PropertyDeclaration::FontFamily( + font_family::SpecifiedValue::Values(computed::font::FontFamilyList { + list: ArcSlice::from_iter( + HTMLFontElement::parse_face_attribute(font_face).into_iter(), + ), + }), )); } let font_size = self .downcast::() - .and_then(|this| this.get_size()); - + .and_then(HTMLFontElementLayoutHelpers::get_size); if let Some(font_size) = font_size { - hints.push(from_declaration( - shared_lock, - PropertyDeclaration::FontSize(font_size::SpecifiedValue::from_html_size( - font_size as u8, - )), - )) + push(PropertyDeclaration::FontSize( + font_size::SpecifiedValue::from_html_size(font_size as u8), + )); } - let cellspacing = if let Some(this) = self.downcast::() { - this.get_cellspacing() - } else { - None - }; - + let cellspacing = self + .downcast::() + .and_then(HTMLTableElementLayoutHelpers::get_cellspacing); if let Some(cellspacing) = cellspacing { let width_value = specified::Length::from_px(cellspacing as f32); - hints.push(from_declaration( - shared_lock, - PropertyDeclaration::BorderSpacing(Box::new(border_spacing::SpecifiedValue::new( - width_value.clone().into(), - width_value.into(), - ))), - )); + push(PropertyDeclaration::BorderSpacing(Box::new( + border_spacing::SpecifiedValue::new(width_value.clone().into(), width_value.into()), + ))); } // Textual input, specifically text entry and domain specific input has @@ -1507,30 +1469,29 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> { // // // - let size = if let Some(this) = self.downcast::() { - // FIXME(pcwalton): More use of atoms, please! - match self.get_attr_val_for_layout(&ns!(), &local_name!("type")) { - Some("hidden") | Some("range") | Some("color") | Some("checkbox") | - Some("radio") | Some("file") | Some("submit") | Some("image") | Some("reset") | - Some("button") => None, - // Others - _ => match this.size_for_layout() { - 0 => None, - s => Some(s as i32), - }, - } - } else { - None - }; + let size = self + .downcast::() + .and_then(|input_element| { + // FIXME(pcwalton): More use of atoms, please! + match self.get_attr_val_for_layout(&ns!(), &local_name!("type")) { + Some("hidden") | Some("range") | Some("color") | Some("checkbox") | + Some("radio") | Some("file") | Some("submit") | Some("image") | + Some("reset") | Some("button") => None, + // Others + _ => match input_element.size_for_layout() { + 0 => None, + s => Some(s as i32), + }, + } + }); if let Some(size) = size { let value = specified::NoCalcLength::ServoCharacterWidth(specified::CharacterWidth(size)); - hints.push(from_declaration( - shared_lock, - PropertyDeclaration::Width(specified::Size::LengthPercentage(NonNegative( + push(PropertyDeclaration::Width( + specified::Size::LengthPercentage(NonNegative( specified::LengthPercentage::Length(value), - ))), + )), )); } @@ -1560,10 +1521,7 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> { let width_value = specified::Size::LengthPercentage(NonNegative( specified::LengthPercentage::Percentage(computed::Percentage(percentage)), )); - hints.push(from_declaration( - shared_lock, - PropertyDeclaration::Width(width_value), - )); + push(PropertyDeclaration::Width(width_value)); }, LengthOrPercentageOrAuto::Length(length) => { let width_value = specified::Size::LengthPercentage(NonNegative( @@ -1571,10 +1529,7 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> { specified::AbsoluteLength::Px(length.to_f32_px()), )), )); - hints.push(from_declaration( - shared_lock, - PropertyDeclaration::Width(width_value), - )); + push(PropertyDeclaration::Width(width_value)); }, } @@ -1602,10 +1557,7 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> { let height_value = specified::Size::LengthPercentage(NonNegative( specified::LengthPercentage::Percentage(computed::Percentage(percentage)), )); - hints.push(from_declaration( - shared_lock, - PropertyDeclaration::Height(height_value), - )); + push(PropertyDeclaration::Height(height_value)); }, LengthOrPercentageOrAuto::Length(length) => { let height_value = specified::Size::LengthPercentage(NonNegative( @@ -1613,10 +1565,7 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> { specified::AbsoluteLength::Px(length.to_f32_px()), )), )); - hints.push(from_declaration( - shared_lock, - PropertyDeclaration::Height(height_value), - )); + push(PropertyDeclaration::Height(height_value)); }, } @@ -1633,87 +1582,61 @@ impl<'dom> LayoutElementHelpers<'dom> for LayoutDom<'dom, Element> { auto: true, ratio: PreferredRatio::Ratio(Ratio(width_value, height_value)), }; - hints.push(from_declaration( - shared_lock, - PropertyDeclaration::AspectRatio(aspect_ratio), - )); + push(PropertyDeclaration::AspectRatio(aspect_ratio)); } } } - let cols = if let Some(this) = self.downcast::() { - match this.get_cols() { - 0 => None, - c => Some(c as i32), - } - } else { - None - }; - + let cols = self + .downcast::() + .map(LayoutHTMLTextAreaElementHelpers::get_cols); if let Some(cols) = cols { - // TODO(mttr) ServoCharacterWidth uses the size math for , but - // the math for