From 8040c8bfec07c8ff763ec84af0ba3fafd253a433 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 17 Sep 2018 14:08:29 +0000 Subject: [PATCH 01/13] style: Split apply_declarations into its own file, and without mako. All that font code thrown out in the middle was making me mad. There should be no change in behavior from this patch. I ran rustfmt on the code but I corrected manually the following: https://github.com/rust-lang-nursery/rustfmt/issues/3025 Differential Revision: https://phabricator.services.mozilla.com/D5970 --- components/style/properties/cascade.rs | 807 ++++++++++++++++++ .../style/properties/properties.mako.rs | 589 +------------ components/style/values/specified/color.rs | 10 +- 3 files changed, 830 insertions(+), 576 deletions(-) create mode 100644 components/style/properties/cascade.rs diff --git a/components/style/properties/cascade.rs b/components/style/properties/cascade.rs new file mode 100644 index 00000000000..41e308e3a83 --- /dev/null +++ b/components/style/properties/cascade.rs @@ -0,0 +1,807 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +//! The main cascading algorithm of the style system. + +use context::QuirksMode; +use custom_properties::CustomPropertiesBuilder; +use dom::TElement; +use font_metrics::FontMetricsProvider; +use logical_geometry::WritingMode; +use media_queries::Device; +use properties::{ComputedValues, StyleBuilder}; +use properties::{LonghandId, LonghandIdSet}; +use properties::{PropertyDeclaration, PropertyDeclarationId, DeclarationImportanceIterator}; +use properties::{CSSWideKeyword, WideKeywordDeclaration}; +use properties::CASCADE_PROPERTY; +use rule_cache::{RuleCache, RuleCacheConditions}; +use rule_tree::{CascadeLevel, StrongRuleNode}; +use selector_parser::PseudoElement; +use servo_arc::Arc; +use shared_lock::StylesheetGuards; +use smallbitvec::SmallBitVec; +use std::borrow::Cow; +use std::cell::RefCell; +use style_adjuster::StyleAdjuster; +use values::computed; + +/// We split the cascade in two phases: 'early' properties, and 'late' +/// properties. +/// +/// Early properties are the ones that don't have dependencies _and_ other +/// properties depend on, for example, writing-mode related properties, color +/// (for currentColor), or font-size (for em, etc). +/// +/// Late properties are all the others. +trait CascadePhase { + fn is_early() -> bool; +} + +struct EarlyProperties; +impl CascadePhase for EarlyProperties { + fn is_early() -> bool { + true + } +} + +struct LateProperties; +impl CascadePhase for LateProperties { + fn is_early() -> bool { + false + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum ApplyResetProperties { + No, + Yes, +} + +/// Performs the CSS cascade, computing new styles for an element from its parent style. +/// +/// The arguments are: +/// +/// * `device`: Used to get the initial viewport and other external state. +/// +/// * `rule_node`: The rule node in the tree that represent the CSS rules that +/// matched. +/// +/// * `parent_style`: The parent style, if applicable; if `None`, this is the root node. +/// +/// Returns the computed values. +/// * `flags`: Various flags. +/// +pub fn cascade( + device: &Device, + pseudo: Option<&PseudoElement>, + rule_node: &StrongRuleNode, + guards: &StylesheetGuards, + parent_style: Option<&ComputedValues>, + parent_style_ignoring_first_line: Option<&ComputedValues>, + layout_parent_style: Option<&ComputedValues>, + visited_rules: Option<&StrongRuleNode>, + font_metrics_provider: &FontMetricsProvider, + quirks_mode: QuirksMode, + rule_cache: Option<&RuleCache>, + rule_cache_conditions: &mut RuleCacheConditions, + element: Option, +) -> Arc +where + E: TElement, +{ + cascade_rules( + device, + pseudo, + rule_node, + guards, + parent_style, + parent_style_ignoring_first_line, + layout_parent_style, + font_metrics_provider, + CascadeMode::Unvisited { visited_rules }, + quirks_mode, + rule_cache, + rule_cache_conditions, + element, + ) +} + +fn cascade_rules( + device: &Device, + pseudo: Option<&PseudoElement>, + rule_node: &StrongRuleNode, + guards: &StylesheetGuards, + parent_style: Option<&ComputedValues>, + parent_style_ignoring_first_line: Option<&ComputedValues>, + layout_parent_style: Option<&ComputedValues>, + font_metrics_provider: &FontMetricsProvider, + cascade_mode: CascadeMode, + quirks_mode: QuirksMode, + rule_cache: Option<&RuleCache>, + rule_cache_conditions: &mut RuleCacheConditions, + element: Option, +) -> Arc +where + E: TElement, +{ + debug_assert_eq!( + parent_style.is_some(), + parent_style_ignoring_first_line.is_some() + ); + let empty = SmallBitVec::new(); + let restriction = pseudo.and_then(|p| p.property_restriction()); + let iter_declarations = || { + rule_node.self_and_ancestors().flat_map(|node| { + let cascade_level = node.cascade_level(); + let node_importance = node.importance(); + let declarations = match node.style_source() { + Some(source) => source + .read(cascade_level.guard(guards)) + .declaration_importance_iter(), + None => DeclarationImportanceIterator::new(&[], &empty), + }; + + declarations + // Yield declarations later in source order (with more precedence) first. + .rev() + .filter_map(move |(declaration, declaration_importance)| { + if let Some(restriction) = restriction { + // declaration.id() is either a longhand or a custom + // property. Custom properties are always allowed, but + // longhands are only allowed if they have our + // restriction flag set. + if let PropertyDeclarationId::Longhand(id) = declaration.id() { + if !id.flags().contains(restriction) { + return None; + } + } + } + + if declaration_importance == node_importance { + Some((declaration, cascade_level)) + } else { + None + } + }) + }) + }; + + apply_declarations( + device, + pseudo, + rule_node, + guards, + iter_declarations, + parent_style, + parent_style_ignoring_first_line, + layout_parent_style, + font_metrics_provider, + cascade_mode, + quirks_mode, + rule_cache, + rule_cache_conditions, + element, + ) +} + +/// Whether we're cascading for visited or unvisited styles. +#[derive(Clone, Copy)] +pub enum CascadeMode<'a> { + /// We're cascading for unvisited styles. + Unvisited { + /// The visited rules that should match the visited style. + visited_rules: Option<&'a StrongRuleNode>, + }, + /// We're cascading for visited styles. + Visited { + /// The writing mode of our unvisited style, needed to correctly resolve + /// logical properties.. + writing_mode: WritingMode, + }, +} + +/// NOTE: This function expects the declaration with more priority to appear +/// first. +pub fn apply_declarations<'a, E, F, I>( + device: &Device, + pseudo: Option<&PseudoElement>, + rules: &StrongRuleNode, + guards: &StylesheetGuards, + iter_declarations: F, + parent_style: Option<&ComputedValues>, + parent_style_ignoring_first_line: Option<&ComputedValues>, + layout_parent_style: Option<&ComputedValues>, + font_metrics_provider: &FontMetricsProvider, + cascade_mode: CascadeMode, + quirks_mode: QuirksMode, + rule_cache: Option<&RuleCache>, + rule_cache_conditions: &mut RuleCacheConditions, + element: Option, +) -> Arc +where + E: TElement, + F: Fn() -> I, + I: Iterator, +{ + debug_assert!(layout_parent_style.is_none() || parent_style.is_some()); + debug_assert_eq!( + parent_style.is_some(), + parent_style_ignoring_first_line.is_some() + ); + #[cfg(feature = "gecko")] + debug_assert!( + parent_style.is_none() || + ::std::ptr::eq( + parent_style.unwrap(), + parent_style_ignoring_first_line.unwrap() + ) || + parent_style.unwrap().pseudo() == Some(PseudoElement::FirstLine) + ); + + let inherited_style = parent_style.unwrap_or(device.default_computed_values()); + + let custom_properties = { + let mut builder = CustomPropertiesBuilder::new(inherited_style.custom_properties()); + + for (declaration, _cascade_level) in iter_declarations() { + if let PropertyDeclaration::Custom(ref declaration) = *declaration { + builder.cascade(&declaration.name, declaration.value.borrow()); + } + } + + builder.build() + }; + + let mut context = computed::Context { + is_root_element: pseudo.is_none() && element.map_or(false, |e| e.is_root()), + // We'd really like to own the rules here to avoid refcount traffic, but + // animation's usage of `apply_declarations` make this tricky. See bug + // 1375525. + builder: StyleBuilder::new( + device, + parent_style, + parent_style_ignoring_first_line, + pseudo, + Some(rules.clone()), + custom_properties, + ), + cached_system_font: None, + in_media_query: false, + for_smil_animation: false, + for_non_inherited_property: None, + font_metrics_provider, + quirks_mode, + rule_cache_conditions: RefCell::new(rule_cache_conditions), + }; + + let using_cached_reset_properties = { + let mut cascade = Cascade::new(&mut context, cascade_mode); + + cascade + .apply_properties::(ApplyResetProperties::Yes, iter_declarations()); + + cascade.compute_visited_style_if_needed( + element, + parent_style, + parent_style_ignoring_first_line, + layout_parent_style, + guards, + ); + + let using_cached_reset_properties = + cascade.try_to_use_cached_reset_properties(rule_cache, guards); + + let apply_reset = if using_cached_reset_properties { + ApplyResetProperties::No + } else { + ApplyResetProperties::Yes + }; + + cascade.apply_properties::(apply_reset, iter_declarations()); + + using_cached_reset_properties + }; + + context.builder.clear_modified_reset(); + + if matches!(cascade_mode, CascadeMode::Unvisited { .. }) { + StyleAdjuster::new(&mut context.builder) + .adjust(layout_parent_style.unwrap_or(inherited_style), element); + } + + if context.builder.modified_reset() || using_cached_reset_properties { + // If we adjusted any reset structs, we can't cache this ComputedValues. + // + // Also, if we re-used existing reset structs, don't bother caching it + // back again. (Aside from being wasted effort, it will be wrong, since + // context.rule_cache_conditions won't be set appropriately if we didn't + // compute those reset properties.) + context.rule_cache_conditions.borrow_mut().set_uncacheable(); + } + + context.builder.build() +} + +fn should_ignore_declaration_when_ignoring_document_colors( + device: &Device, + longhand_id: LonghandId, + cascade_level: CascadeLevel, + pseudo: Option<&PseudoElement>, + declaration: &mut Cow, +) -> bool { + if !longhand_id.ignored_when_document_colors_disabled() { + return false; + } + + let is_ua_or_user_rule = matches!( + cascade_level, + CascadeLevel::UANormal | + CascadeLevel::UserNormal | + CascadeLevel::UserImportant | + CascadeLevel::UAImportant + ); + + if is_ua_or_user_rule { + return false; + } + + let is_style_attribute = matches!( + cascade_level, + CascadeLevel::StyleAttributeNormal | CascadeLevel::StyleAttributeImportant + ); + + // Don't override colors on pseudo-element's style attributes. The + // background-color on ::-moz-color-swatch is an example. Those are set + // as an author style (via the style attribute), but it's pretty + // important for it to show up for obvious reasons :) + if pseudo.is_some() && is_style_attribute { + return false; + } + + // Treat background-color a bit differently. If the specified color is + // anything other than a fully transparent color, convert it into the + // Device's default background color. + { + let background_color = match **declaration { + PropertyDeclaration::BackgroundColor(ref color) => color, + _ => return true, + }; + + if background_color.is_transparent() { + return false; + } + } + + let color = device.default_background_color(); + *declaration.to_mut() = PropertyDeclaration::BackgroundColor(color.into()); + + false +} + +struct Cascade<'a, 'b: 'a> { + context: &'a mut computed::Context<'b>, + cascade_mode: CascadeMode<'a>, + seen: LonghandIdSet, + saved_font_size: Option, + saved_font_family: Option, +} + +impl<'a, 'b: 'a> Cascade<'a, 'b> { + fn new(context: &'a mut computed::Context<'b>, cascade_mode: CascadeMode<'a>) -> Self { + Self { + context, + cascade_mode, + seen: LonghandIdSet::default(), + saved_font_size: None, + saved_font_family: None, + } + } + + fn substitute_variables_if_needed<'decl>( + &mut self, + declaration: &'decl PropertyDeclaration, + ) -> Cow<'decl, PropertyDeclaration> { + let declaration = match *declaration { + PropertyDeclaration::WithVariables(ref declaration) => declaration, + ref d => return Cow::Borrowed(d), + }; + + if !declaration.id.inherited() { + self.context + .rule_cache_conditions + .borrow_mut() + .set_uncacheable(); + } + + Cow::Owned(declaration.value.substitute_variables( + declaration.id, + self.context.builder.custom_properties.as_ref(), + self.context.quirks_mode, + )) + } + + fn apply_declaration( + &mut self, + longhand_id: LonghandId, + declaration: &PropertyDeclaration, + ) { + // FIXME(emilio): Find a cleaner abstraction for this. + // + // font-size and font-family are special because in Gecko they're + // they're dependent on other early props, like lang and + // -moz-min-font-size-ratio. This sucks a bit, we should ideally + // move the font-size computation code somewhere else... + if Phase::is_early() { + if longhand_id == LonghandId::FontSize { + self.saved_font_size = Some(declaration.clone()); + return; + } + if longhand_id == LonghandId::FontFamily { + self.saved_font_family = Some(declaration.clone()); + return; + } + } + + self.apply_declaration_ignoring_phase(longhand_id, declaration); + } + + #[inline(always)] + fn apply_declaration_ignoring_phase( + &mut self, + longhand_id: LonghandId, + declaration: &PropertyDeclaration, + ) { + // We could (and used to) use a pattern match here, but that bloats this + // function to over 100K of compiled code! + // + // To improve i-cache behavior, we outline the individual functions and + // use virtual dispatch instead. + let discriminant = longhand_id as usize; + (CASCADE_PROPERTY[discriminant])(declaration, &mut self.context); + } + + fn apply_properties<'decls, Phase, I>( + &mut self, + apply_reset: ApplyResetProperties, + declarations: I, + ) where + Phase: CascadePhase, + I: Iterator, + { + let apply_reset = apply_reset == ApplyResetProperties::Yes; + + debug_assert!( + !Phase::is_early() || apply_reset, + "Should always apply reset properties in the early phase, since we \ + need to know font-size / writing-mode to decide whether to use the \ + cached reset properties" + ); + + let ignore_colors = !self.context.builder.device.use_document_colors(); + + for (declaration, cascade_level) in declarations { + let declaration_id = declaration.id(); + let longhand_id = match declaration_id { + PropertyDeclarationId::Longhand(id) => id, + PropertyDeclarationId::Custom(..) => continue, + }; + + let inherited = longhand_id.inherited(); + if !apply_reset && !inherited { + continue; + } + + if Phase::is_early() != longhand_id.is_early_property() { + continue; + } + + debug_assert!(!Phase::is_early() || !longhand_id.is_logical()); + let physical_longhand_id = if Phase::is_early() { + longhand_id + } else { + longhand_id.to_physical(self.context.builder.writing_mode) + }; + + if self.seen.contains(physical_longhand_id) { + continue; + } + + // Only a few properties are allowed to depend on the visited state + // of links. When cascading visited styles, we can save time by + // only processing these properties. + if matches!(self.cascade_mode, CascadeMode::Visited { .. }) && + !physical_longhand_id.is_visited_dependent() + { + continue; + } + + let mut declaration = self.substitute_variables_if_needed(declaration); + + // When document colors are disabled, skip properties that are + // marked as ignored in that mode, unless they come from a UA or + // user style sheet. + if ignore_colors { + let should_ignore = should_ignore_declaration_when_ignoring_document_colors( + self.context.builder.device, + longhand_id, + cascade_level, + self.context.builder.pseudo, + &mut declaration, + ); + if should_ignore { + continue; + } + } + + self.seen.insert(physical_longhand_id); + + // 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. + self.apply_declaration::(longhand_id, &*declaration); + } + + if Phase::is_early() { + self.fixup_font_and_apply_saved_font_properties(); + self.compute_writing_mode(); + } else { + self.finished_applying_properties(); + } + } + + fn compute_writing_mode(&mut self) { + let writing_mode = match self.cascade_mode { + CascadeMode::Unvisited { .. } => { + WritingMode::new(self.context.builder.get_inherited_box()) + }, + CascadeMode::Visited { writing_mode } => writing_mode, + }; + self.context.builder.writing_mode = writing_mode; + } + + fn compute_visited_style_if_needed( + &mut self, + element: Option, + parent_style: Option<&ComputedValues>, + parent_style_ignoring_first_line: Option<&ComputedValues>, + layout_parent_style: Option<&ComputedValues>, + guards: &StylesheetGuards, + ) where + E: TElement, + { + let visited_rules = match self.cascade_mode { + CascadeMode::Unvisited { visited_rules } => visited_rules, + CascadeMode::Visited { .. } => return, + }; + + let visited_rules = match visited_rules { + Some(rules) => rules, + None => return, + }; + + let is_link = self.context.builder.pseudo.is_none() && element.unwrap().is_link(); + + macro_rules! visited_parent { + ($parent:expr) => { + if is_link { + $parent + } else { + $parent.map(|p| p.visited_style().unwrap_or(p)) + } + }; + } + + let writing_mode = self.context.builder.writing_mode; + + // We could call apply_declarations directly, but that'd cause + // another instantiation of this function which is not great. + let style = cascade_rules( + self.context.builder.device, + self.context.builder.pseudo, + visited_rules, + guards, + visited_parent!(parent_style), + visited_parent!(parent_style_ignoring_first_line), + visited_parent!(layout_parent_style), + self.context.font_metrics_provider, + CascadeMode::Visited { writing_mode }, + self.context.quirks_mode, + // The rule cache doesn't care about caching :visited + // styles, we cache the unvisited style instead. We still do + // need to set the caching dependencies properly if present + // though, so the cache conditions need to match. + /* rule_cache = */ None, + &mut *self.context.rule_cache_conditions.borrow_mut(), + element, + ); + self.context.builder.visited_style = Some(style); + } + + fn finished_applying_properties(&mut self) { + let builder = &mut self.context.builder; + + #[cfg(feature = "gecko")] + { + if let Some(bg) = builder.get_background_if_mutated() { + bg.fill_arrays(); + } + + if let Some(svg) = builder.get_svg_if_mutated() { + svg.fill_arrays(); + } + } + + #[cfg(feature = "servo")] + { + // TODO(emilio): Use get_font_if_mutated instead. + if self.seen.contains(LonghandId::FontStyle) || + self.seen.contains(LonghandId::FontWeight) || + self.seen.contains(LonghandId::FontStretch) || + self.seen.contains(LonghandId::FontFamily) + { + builder.mutate_font().compute_font_hash(); + } + } + } + + fn try_to_use_cached_reset_properties( + &mut self, + cache: Option<&'b RuleCache>, + guards: &StylesheetGuards, + ) -> bool { + let cache = match cache { + Some(cache) => cache, + None => return false, + }; + + let cached_style = match cache.find(guards, &self.context.builder) { + Some(style) => style, + None => return false, + }; + + self.context.builder.copy_reset_from(cached_style); + true + } + + // FIXME(emilio): It'd be really nice to simplify all this, somehow. This is + // very annoying code in lots of ways, and there are various bits about it + // which I think are broken or could be improved, see the various FIXMEs + // below. + fn fixup_font_and_apply_saved_font_properties(&mut self) { + let font_family = self.saved_font_family.take(); + let font_size = self.saved_font_size.take(); + let mut _skip_font_family = false; + + #[cfg(feature = "gecko")] + { + // is not affected by text zoom, and it uses a preshint + // to disable it. We fix up the struct when this happens by + // unzooming its contained font values, which will have been zoomed + // in the parent. + // + // FIXME(emilio): Could be cleaner if we just removed this property + // and made a style adjustment o something like that. + if self.seen.contains(LonghandId::XTextZoom) { + let builder = &mut self.context.builder; + + let parent_zoom = builder.get_parent_font().gecko().mAllowZoom; + let zoom = builder.get_font().gecko().mAllowZoom; + if zoom != parent_zoom { + debug_assert!( + !zoom, + "We only ever disable text zoom (in svg:text), never enable it" + ); + let device = builder.device; + builder.mutate_font().unzoom_fonts(device); + } + } + + // Whenever a single generic value is specified, Gecko used to do a + // bunch of recalculation walking up the rule tree, including + // handling the font-size stuff. + // + // It basically repopulated the font struct with the default font + // for a given generic and language. We handle the font-size stuff + // separately, so this boils down to just copying over the + // font-family lists (no other aspect of the default font can be + // configured). + if self.seen.contains(LonghandId::XLang) || self.seen.contains(LonghandId::FontFamily) { + // If just the language changed, the inherited generic is all we + // need. + let mut generic = self.context.builder.get_parent_font().gecko().mGenericID; + + // FIXME(emilio): Isn't this bogus for CSS wide keywords like + // reset or such? + if let Some(ref declaration) = font_family { + if let PropertyDeclaration::FontFamily(ref fam) = *declaration { + if let Some(id) = fam.single_generic() { + generic = id; + // In case of a specified font family with a single + // generic, we will end up setting font family + // below, but its value would get overwritten later + // in the pipeline when cascading. + // + // We instead skip cascading font-family in that + // case. + // + // In case of the language changing, we wish for a + // specified font-family to override this, so we do + // not skip cascading then. + _skip_font_family = true; + } + } + } + + // FIXME(emilio): Why both setting the generic and passing it + // down? + let pres_context = self.context.builder.device.pres_context(); + let gecko_font = self.context.builder.mutate_font().gecko_mut(); + gecko_font.mGenericID = generic; + unsafe { + ::gecko_bindings::bindings::Gecko_nsStyleFont_PrefillDefaultForGeneric( + gecko_font, + pres_context, + generic, + ); + } + } + } + + // It is important that font-size is computed before the late + // properties (for em units), but after font-family (for the + // base-font-size dependence for default and keyword font-sizes). + // + // It's important that font-family comes after the other font properties + // to support system fonts. + // + // NOTE(emilio): I haven't verified that comment, but it was there. + // Verify, and if it's false make font-size the only weird property? + if !_skip_font_family { + if let Some(ref declaration) = font_family { + self.apply_declaration_ignoring_phase(LonghandId::FontFamily, declaration); + #[cfg(feature = "gecko")] + { + let context = &mut self.context; + let device = context.builder.device; + if let PropertyDeclaration::FontFamily(ref val) = *declaration { + if val.get_system().is_some() { + let default = context + .cached_system_font + .as_ref() + .unwrap() + .default_font_type; + context.builder.mutate_font().fixup_system(default); + } else { + context.builder.mutate_font().fixup_none_generic(device); + } + } + } + } + } + + if let Some(declaration) = font_size { + self.apply_declaration_ignoring_phase(LonghandId::FontSize, &declaration); + } else { + #[cfg(feature = "gecko")] + { + if self.seen.contains(LonghandId::XLang) || + self.seen.contains(LonghandId::MozScriptLevel) || + self.seen.contains(LonghandId::MozMinFontSizeRatio) || + self.seen.contains(LonghandId::FontFamily) + { + // 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); + } + } + } + } +} diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 681e9556653..39fc90b2f3d 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -13,13 +13,9 @@ #[cfg(feature = "servo")] use app_units::Au; use arrayvec::{ArrayVec, Drain as ArrayVecDrain}; -use dom::TElement; -use custom_properties::CustomPropertiesBuilder; use servo_arc::{Arc, UniqueArc}; -use smallbitvec::SmallBitVec; use std::borrow::Cow; use std::{ops, ptr}; -use std::cell::RefCell; use std::fmt::{self, Write}; use std::mem::{self, ManuallyDrop}; @@ -27,8 +23,6 @@ use cssparser::{Parser, RGBA, TokenSerializationType}; use cssparser::ParserInput; #[cfg(feature = "servo")] use euclid::SideOffsets2D; use context::QuirksMode; -use font_metrics::FontMetricsProvider; -#[cfg(feature = "gecko")] use gecko_bindings::bindings; #[cfg(feature = "gecko")] use gecko_bindings::structs::{self, nsCSSPropertyID}; #[cfg(feature = "servo")] use logical_geometry::LogicalMargin; #[cfg(feature = "servo")] use computed_values; @@ -37,11 +31,9 @@ use logical_geometry::WritingMode; use media_queries::Device; use parser::ParserContext; use properties::longhands::system_font::SystemFont; -use rule_cache::{RuleCache, RuleCacheConditions}; use selector_parser::PseudoElement; use selectors::parser::SelectorParseErrorKind; #[cfg(feature = "servo")] use servo_config::prefs::PREFS; -use shared_lock::StylesheetGuards; use style_traits::{CssWriter, KeywordsCollectFn, ParseError, ParsingMode}; use style_traits::{SpecifiedValueInfo, StyleParseErrorKind, ToCss}; use stylesheets::{CssRuleType, Origin, UrlExtraData}; @@ -49,12 +41,12 @@ use values::generics::text::LineHeight; use values::computed; use values::computed::NonNegativeLength; use values::serialize_atom_name; -use rule_tree::{CascadeLevel, StrongRuleNode}; +use rule_tree::StrongRuleNode; use self::computed_value_flags::*; use str::{CssString, CssStringBorrow, CssStringWriter}; -use style_adjuster::StyleAdjuster; pub use self::declaration_block::*; +pub use self::cascade::*; <%! from collections import defaultdict @@ -66,6 +58,8 @@ pub use self::declaration_block::*; pub mod computed_value_flags; #[path="${repr(os.path.join(os.path.dirname(__file__), 'declaration_block.rs'))[1:-1]}"] pub mod declaration_block; +#[path="${repr(os.path.join(os.path.dirname(__file__), 'cascade.rs'))[1:-1]}"] +pub mod cascade; /// Conversion with fewer impls than From/Into pub trait MaybeBoxed { @@ -731,7 +725,7 @@ static ${name}: LonghandIdSet = LonghandIdSet { /// A set of longhand properties -#[derive(Clone, Debug, MallocSizeOf, PartialEq)] +#[derive(Clone, Debug, Default, MallocSizeOf, PartialEq)] pub struct LonghandIdSet { storage: [u32; (${len(data.longhands)} - 1 + 32) / 32] } @@ -969,6 +963,7 @@ impl LonghandId { } /// Returns whether the longhand property is inherited by default. + #[inline] pub fn inherited(&self) -> bool { ${static_longhand_id_set("INHERITED", lambda p: p.style_struct.inherited)} INHERITED.contains(*self) @@ -1160,40 +1155,14 @@ impl LonghandId { /// Returns true if the property is one that is ignored when document /// colors are disabled. - fn is_ignored_when_document_colors_disabled( - &self, - cascade_level: CascadeLevel, - pseudo: Option<<&PseudoElement>, - ) -> bool { - let is_ua_or_user_rule = matches!( - cascade_level, - CascadeLevel::UANormal | - CascadeLevel::UserNormal | - CascadeLevel::UserImportant | - CascadeLevel::UAImportant - ); + #[inline] + fn ignored_when_document_colors_disabled(self) -> bool { + ${static_longhand_id_set( + "IGNORED_WHEN_COLORS_DISABLED", + lambda p: p.ignored_when_colors_disabled + )} - if is_ua_or_user_rule { - return false; - } - - let is_style_attribute = matches!( - cascade_level, - CascadeLevel::StyleAttributeNormal | - CascadeLevel::StyleAttributeImportant - ); - // Don't override colors on pseudo-element's style attributes. The - // background-color on ::-moz-color-swatch is an example. Those are set - // as an author style (via the style attribute), but it's pretty - // important for it to show up for obvious reasons :) - if pseudo.is_some() && is_style_attribute { - return false; - } - - matches!(*self, - ${" | ".join([("LonghandId::" + p.camel_case) - for p in data.longhands if p.ignored_when_colors_disabled])} - ) + IGNORED_WHEN_COLORS_DISABLED.contains(self) } /// The computed value of some properties depends on the (sometimes @@ -3469,6 +3438,10 @@ impl<'a> StyleBuilder<'a> { self.modified_reset = true; % endif + // TODO(emilio): There's a maybe-worth it optimization here: We should + // avoid allocating a new reset struct if `reset_struct` and our struct + // is the same pointer. Would remove a bunch of stupid allocations if + // you did something like `* { all: initial }` or what not. self.${property.style_struct.ident}.mutate() .reset_${property.ident}( reset_struct, @@ -3740,538 +3713,12 @@ pub type CascadePropertyFn = /// A per-longhand array of functions to perform the CSS cascade on each of /// them, effectively doing virtual dispatch. -static CASCADE_PROPERTY: [CascadePropertyFn; ${len(data.longhands)}] = [ +pub static CASCADE_PROPERTY: [CascadePropertyFn; ${len(data.longhands)}] = [ % for property in data.longhands: longhands::${property.ident}::cascade_property, % endfor ]; -/// Performs the CSS cascade, computing new styles for an element from its parent style. -/// -/// The arguments are: -/// -/// * `device`: Used to get the initial viewport and other external state. -/// -/// * `rule_node`: The rule node in the tree that represent the CSS rules that -/// matched. -/// -/// * `parent_style`: The parent style, if applicable; if `None`, this is the root node. -/// -/// Returns the computed values. -/// * `flags`: Various flags. -/// -pub fn cascade( - device: &Device, - pseudo: Option<<&PseudoElement>, - rule_node: &StrongRuleNode, - guards: &StylesheetGuards, - parent_style: Option<<&ComputedValues>, - parent_style_ignoring_first_line: Option<<&ComputedValues>, - layout_parent_style: Option<<&ComputedValues>, - visited_rules: Option<<&StrongRuleNode>, - font_metrics_provider: &FontMetricsProvider, - quirks_mode: QuirksMode, - rule_cache: Option<<&RuleCache>, - rule_cache_conditions: &mut RuleCacheConditions, - element: Option, -) -> Arc -where - E: TElement, -{ - cascade_rules( - device, - pseudo, - rule_node, - guards, - parent_style, - parent_style_ignoring_first_line, - layout_parent_style, - font_metrics_provider, - CascadeMode::Unvisited { visited_rules }, - quirks_mode, - rule_cache, - rule_cache_conditions, - element, - ) -} - -fn cascade_rules( - device: &Device, - pseudo: Option<<&PseudoElement>, - rule_node: &StrongRuleNode, - guards: &StylesheetGuards, - parent_style: Option<<&ComputedValues>, - parent_style_ignoring_first_line: Option<<&ComputedValues>, - layout_parent_style: Option<<&ComputedValues>, - font_metrics_provider: &FontMetricsProvider, - cascade_mode: CascadeMode, - quirks_mode: QuirksMode, - rule_cache: Option<<&RuleCache>, - rule_cache_conditions: &mut RuleCacheConditions, - element: Option, -) -> Arc -where - E: TElement, -{ - debug_assert_eq!(parent_style.is_some(), parent_style_ignoring_first_line.is_some()); - let empty = SmallBitVec::new(); - let restriction = pseudo.and_then(|p| p.property_restriction()); - let iter_declarations = || { - rule_node.self_and_ancestors().flat_map(|node| { - let cascade_level = node.cascade_level(); - let node_importance = node.importance(); - let declarations = match node.style_source() { - Some(source) => { - source.read(cascade_level.guard(guards)).declaration_importance_iter() - } - None => DeclarationImportanceIterator::new(&[], &empty), - }; - - declarations - // Yield declarations later in source order (with more precedence) first. - .rev() - .filter_map(move |(declaration, declaration_importance)| { - if let Some(restriction) = restriction { - // declaration.id() is either a longhand or a custom - // property. Custom properties are always allowed, but - // longhands are only allowed if they have our - // restriction flag set. - if let PropertyDeclarationId::Longhand(id) = declaration.id() { - if !id.flags().contains(restriction) { - return None - } - } - } - - if declaration_importance == node_importance { - Some((declaration, cascade_level)) - } else { - None - } - }) - }) - }; - - apply_declarations( - device, - pseudo, - rule_node, - guards, - iter_declarations, - parent_style, - parent_style_ignoring_first_line, - layout_parent_style, - font_metrics_provider, - cascade_mode, - quirks_mode, - rule_cache, - rule_cache_conditions, - element, - ) -} - -/// Whether we're cascading for visited or unvisited styles. -#[derive(Clone, Copy)] -pub enum CascadeMode<'a> { - /// We're cascading for unvisited styles. - Unvisited { - /// The visited rules that should match the visited style. - visited_rules: Option<<&'a StrongRuleNode>, - }, - /// We're cascading for visited styles. - Visited { - /// The writing mode of our unvisited style, needed to correctly resolve - /// logical properties.. - writing_mode: WritingMode, - }, -} - -/// NOTE: This function expects the declaration with more priority to appear -/// first. -pub fn apply_declarations<'a, E, F, I>( - device: &Device, - pseudo: Option<<&PseudoElement>, - rules: &StrongRuleNode, - guards: &StylesheetGuards, - iter_declarations: F, - parent_style: Option<<&ComputedValues>, - parent_style_ignoring_first_line: Option<<&ComputedValues>, - layout_parent_style: Option<<&ComputedValues>, - font_metrics_provider: &FontMetricsProvider, - cascade_mode: CascadeMode, - quirks_mode: QuirksMode, - rule_cache: Option<<&RuleCache>, - rule_cache_conditions: &mut RuleCacheConditions, - element: Option, -) -> Arc -where - E: TElement, - F: Fn() -> I, - I: Iterator, -{ - debug_assert!(layout_parent_style.is_none() || parent_style.is_some()); - debug_assert_eq!(parent_style.is_some(), parent_style_ignoring_first_line.is_some()); - #[cfg(feature = "gecko")] - debug_assert!(parent_style.is_none() || - ::std::ptr::eq(parent_style.unwrap(), - parent_style_ignoring_first_line.unwrap()) || - parent_style.unwrap().pseudo() == Some(PseudoElement::FirstLine)); - let inherited_style = - parent_style.unwrap_or(device.default_computed_values()); - - let custom_properties = { - let mut builder = - CustomPropertiesBuilder::new(inherited_style.custom_properties()); - - for (declaration, _cascade_level) in iter_declarations() { - if let PropertyDeclaration::Custom(ref declaration) = *declaration { - builder.cascade(&declaration.name, declaration.value.borrow()); - } - } - - builder.build() - }; - - let mut context = computed::Context { - is_root_element: pseudo.is_none() && element.map_or(false, |e| e.is_root()), - // We'd really like to own the rules here to avoid refcount traffic, but - // animation's usage of `apply_declarations` make this tricky. See bug - // 1375525. - builder: StyleBuilder::new( - device, - parent_style, - parent_style_ignoring_first_line, - pseudo, - Some(rules.clone()), - custom_properties, - ), - cached_system_font: None, - in_media_query: false, - for_smil_animation: false, - for_non_inherited_property: None, - font_metrics_provider, - quirks_mode, - rule_cache_conditions: RefCell::new(rule_cache_conditions), - }; - - let ignore_colors = !device.use_document_colors(); - - // Set computed values, overwriting earlier declarations for the same - // property. - let mut seen = LonghandIdSet::new(); - - // Declaration blocks are stored in increasing precedence order, we want - // them in decreasing order here. - // - // We could (and used to) use a pattern match here, but that bloats this - // function to over 100K of compiled code! - // - // To improve i-cache behavior, we outline the individual functions and use - // virtual dispatch instead. - let mut apply_reset = true; - % for category_to_cascade_now in ["early", "other"]: - % if category_to_cascade_now == "early": - // Pull these out so that we can compute them in a specific order - // without introducing more iterations. - let mut font_size = None; - let mut font_family = None; - % endif - for (declaration, cascade_level) in iter_declarations() { - let declaration_id = declaration.id(); - let longhand_id = match declaration_id { - PropertyDeclarationId::Longhand(id) => id, - PropertyDeclarationId::Custom(..) => continue, - }; - - if !apply_reset && !longhand_id.inherited() { - continue; - } - - if - % if category_to_cascade_now == "early": - ! - % endif - longhand_id.is_early_property() - { - continue - } - - <% maybe_to_physical = ".to_physical(writing_mode)" if category_to_cascade_now != "early" else "" %> - let physical_longhand_id = longhand_id ${maybe_to_physical}; - if seen.contains(physical_longhand_id) { - continue - } - - // Only a few properties are allowed to depend on the visited state - // of links. When cascading visited styles, we can save time by - // only processing these properties. - if matches!(cascade_mode, CascadeMode::Visited { .. }) && - !physical_longhand_id.is_visited_dependent() { - continue - } - - let mut declaration = match *declaration { - PropertyDeclaration::WithVariables(ref declaration) => { - if !declaration.id.inherited() { - context.rule_cache_conditions.borrow_mut() - .set_uncacheable(); - } - Cow::Owned(declaration.value.substitute_variables( - declaration.id, - context.builder.custom_properties.as_ref(), - context.quirks_mode - )) - } - ref d => Cow::Borrowed(d) - }; - - // When document colors are disabled, skip properties that are - // marked as ignored in that mode, unless they come from a UA or - // user style sheet. - if ignore_colors && - longhand_id.is_ignored_when_document_colors_disabled( - cascade_level, - context.builder.pseudo - ) - { - let non_transparent_background = match *declaration { - PropertyDeclaration::BackgroundColor(ref color) => { - // Treat background-color a bit differently. If the specified - // color is anything other than a fully transparent color, convert - // it into the Device's default background color. - color.is_non_transparent() - } - _ => continue - }; - - // FIXME: moving this out of `match` is a work around for - // borrows being lexical. - if non_transparent_background { - let color = device.default_background_color(); - declaration = - Cow::Owned(PropertyDeclaration::BackgroundColor(color.into())); - } - } - - seen.insert(physical_longhand_id); - - % if category_to_cascade_now == "early": - if LonghandId::FontSize == longhand_id { - font_size = Some(declaration.clone()); - continue; - } - if LonghandId::FontFamily == longhand_id { - font_family = Some(declaration.clone()); - continue; - } - % endif - - let discriminant = longhand_id as usize; - (CASCADE_PROPERTY[discriminant])(&*declaration, &mut context); - } - % if category_to_cascade_now == "early": - let writing_mode = match cascade_mode { - CascadeMode::Unvisited { .. } => { - WritingMode::new(context.builder.get_inherited_box()) - } - CascadeMode::Visited { writing_mode } => writing_mode, - }; - - context.builder.writing_mode = writing_mode; - if let CascadeMode::Unvisited { visited_rules: Some(visited_rules) } = cascade_mode { - let is_link = pseudo.is_none() && element.unwrap().is_link(); - macro_rules! visited_parent { - ($parent:expr) => { - if is_link { - $parent - } else { - $parent.map(|p| p.visited_style().unwrap_or(p)) - } - } - } - // We could call apply_declarations directly, but that'd cause - // another instantiation of this function which is not great. - context.builder.visited_style = Some(cascade_rules( - device, - pseudo, - visited_rules, - guards, - visited_parent!(parent_style), - visited_parent!(parent_style_ignoring_first_line), - visited_parent!(layout_parent_style), - font_metrics_provider, - CascadeMode::Visited { writing_mode }, - quirks_mode, - // The rule cache doesn't care about caching :visited - // styles, we cache the unvisited style instead. We still do - // need to set the caching dependencies properly if present - // though, so the cache conditions need to match. - /* rule_cache = */ None, - &mut *context.rule_cache_conditions.borrow_mut(), - element, - )); - } - - let mut _skip_font_family = false; - - % if product == "gecko": - - // is not affected by text zoom, and it uses a preshint to - // disable it. We fix up the struct when this happens by unzooming - // its contained font values, which will have been zoomed in the parent - if seen.contains(LonghandId::XTextZoom) { - let zoom = context.builder.get_font().gecko().mAllowZoom; - let parent_zoom = context.style().get_parent_font().gecko().mAllowZoom; - if zoom != parent_zoom { - debug_assert!(!zoom, - "We only ever disable text zoom (in svg:text), never enable it"); - // can't borrow both device and font, use the take/put machinery - let mut font = context.builder.take_font(); - font.unzoom_fonts(context.device()); - context.builder.put_font(font); - } - } - - // Whenever a single generic value is specified, gecko will do a bunch of - // recalculation walking up the rule tree, including handling the font-size stuff. - // It basically repopulates the font struct with the default font for a given - // generic and language. We handle the font-size stuff separately, so this boils - // down to just copying over the font-family lists (no other aspect of the default - // font can be configured). - - if seen.contains(LonghandId::XLang) || font_family.is_some() { - // if just the language changed, the inherited generic is all we need - let mut generic = inherited_style.get_font().gecko().mGenericID; - if let Some(ref declaration) = font_family { - if let PropertyDeclaration::FontFamily(ref fam) = **declaration { - if let Some(id) = fam.single_generic() { - generic = id; - // In case of a specified font family with a single generic, we will - // end up setting font family below, but its value would get - // overwritten later in the pipeline when cascading. - // - // We instead skip cascading font-family in that case. - // - // In case of the language changing, we wish for a specified font- - // family to override this, so we do not skip cascading then. - _skip_font_family = true; - } - } - } - - let pres_context = context.builder.device.pres_context(); - let gecko_font = context.builder.mutate_font().gecko_mut(); - gecko_font.mGenericID = generic; - unsafe { - bindings::Gecko_nsStyleFont_PrefillDefaultForGeneric( - gecko_font, - pres_context, - generic, - ); - } - } - % endif - - // It is important that font_size is computed before - // the late properties (for em units), but after font-family - // (for the base-font-size dependence for default and keyword font-sizes) - // Additionally, when we support system fonts they will have to be - // computed early, and *before* font_family, so I'm including - // font_family here preemptively instead of keeping it within - // the early properties. - // - // To avoid an extra iteration, we just pull out the property - // during the early iteration and cascade them in order - // after it. - if !_skip_font_family { - if let Some(ref declaration) = font_family { - - let discriminant = LonghandId::FontFamily as usize; - (CASCADE_PROPERTY[discriminant])(declaration, &mut context); - % if product == "gecko": - let device = context.builder.device; - if let PropertyDeclaration::FontFamily(ref val) = **declaration { - if val.get_system().is_some() { - let default = context.cached_system_font - .as_ref().unwrap().default_font_type; - context.builder.mutate_font().fixup_system(default); - } else { - context.builder.mutate_font().fixup_none_generic(device); - } - } - % endif - } - } - - if let Some(ref declaration) = font_size { - let discriminant = LonghandId::FontSize as usize; - (CASCADE_PROPERTY[discriminant])(declaration, &mut context); - % if product == "gecko": - // Font size must be explicitly inherited to handle lang changes and - // scriptlevel changes. - } else if seen.contains(LonghandId::XLang) || - seen.contains(LonghandId::MozScriptLevel) || - seen.contains(LonghandId::MozMinFontSizeRatio) || - font_family.is_some() { - let discriminant = LonghandId::FontSize as usize; - let size = PropertyDeclaration::CSSWideKeyword(WideKeywordDeclaration { - id: LonghandId::FontSize, - keyword: CSSWideKeyword::Inherit, - }); - - (CASCADE_PROPERTY[discriminant])(&size, &mut context); - % endif - } - - if let Some(style) = rule_cache.and_then(|c| c.find(guards, &context.builder)) { - context.builder.copy_reset_from(style); - apply_reset = false; - } - % endif // category == "early" - % endfor - - let mut builder = context.builder; - - % if product == "gecko": - if let Some(ref mut bg) = builder.get_background_if_mutated() { - bg.fill_arrays(); - } - - if let Some(ref mut svg) = builder.get_svg_if_mutated() { - svg.fill_arrays(); - } - % endif - - % if product == "servo": - if seen.contains(LonghandId::FontStyle) || - seen.contains(LonghandId::FontWeight) || - seen.contains(LonghandId::FontStretch) || - seen.contains(LonghandId::FontFamily) { - builder.mutate_font().compute_font_hash(); - } - % endif - - builder.clear_modified_reset(); - - if matches!(cascade_mode, CascadeMode::Unvisited { .. }) { - StyleAdjuster::new(&mut builder).adjust( - layout_parent_style.unwrap_or(inherited_style), - element, - ); - } - - if builder.modified_reset() || !apply_reset { - // If we adjusted any reset structs, we can't cache this ComputedValues. - // - // Also, if we re-used existing reset structs, don't bother caching it - // back again. (Aside from being wasted effort, it will be wrong, since - // context.rule_cache_conditions won't be set appropriately if we - // didn't compute those reset properties.) - context.rule_cache_conditions.borrow_mut().set_uncacheable(); - } - - builder.build() -} /// See StyleAdjuster::adjust_for_border_width. pub fn adjust_border_width(style: &mut StyleBuilder) { diff --git a/components/style/values/specified/color.rs b/components/style/values/specified/color.rs index cce8f381806..85d29f3e7e7 100644 --- a/components/style/values/specified/color.rs +++ b/components/style/values/specified/color.rs @@ -317,12 +317,12 @@ impl Color { .map_err(|()| location.new_custom_error(StyleParseErrorKind::UnspecifiedError)) } - /// Returns false if the color is completely transparent, and - /// true otherwise. - pub fn is_non_transparent(&self) -> bool { + /// Returns true if the color is completely transparent, and false + /// otherwise. + pub fn is_transparent(&self) -> bool { match *self { - Color::Numeric { ref parsed, .. } => parsed.alpha != 0, - _ => true, + Color::Numeric { ref parsed, .. } => parsed.alpha == 0, + _ => false, } } } From d5ba19696aa47f6b29473cd8fef7b38d441cbe3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 17 Sep 2018 00:54:43 +0000 Subject: [PATCH 02/13] style: Make ExtremumLength::valid_for static. Differential Revision: https://phabricator.services.mozilla.com/D5975 --- components/style/values/computed/length.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/components/style/values/computed/length.rs b/components/style/values/computed/length.rs index f2d43e2ada9..5acc2688224 100644 --- a/components/style/values/computed/length.rs +++ b/components/style/values/computed/length.rs @@ -953,7 +953,7 @@ impl ExtremumLength { /// TODO: After these values are supported for both axes (and maybe /// unprefixed, see bug 1322780) all this complexity can go away, and /// everything can be derived (no need for uncacheable stuff). - fn valid_for(&self, wm: WritingMode, longhand: LonghandId) -> bool { + fn valid_for(wm: WritingMode, longhand: LonghandId) -> bool { // We only make sense on the inline axis. match longhand { // FIXME(emilio): The flex-basis thing is not quite clear... @@ -1018,7 +1018,7 @@ impl ToComputedValue for specified::MozLength { .rule_cache_conditions .borrow_mut() .set_writing_mode_dependency(context.builder.writing_mode); - if !ext.valid_for( + if !ExtremumLength::valid_for( context.builder.writing_mode, context.for_non_inherited_property.unwrap(), ) { @@ -1080,7 +1080,7 @@ impl ToComputedValue for specified::MaxLength { .rule_cache_conditions .borrow_mut() .set_writing_mode_dependency(context.builder.writing_mode); - if !ext.valid_for( + if !ExtremumLength::valid_for( context.builder.writing_mode, context.for_non_inherited_property.unwrap(), ) { From c155efe7a517af254e02f1c79e3af57c761dcce0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 17 Sep 2018 01:33:15 +0000 Subject: [PATCH 03/13] style: Remove DeclaredValue::WithVariables. We never construct it. Differential Revision: https://phabricator.services.mozilla.com/D5976 --- components/style/custom_properties.rs | 1 - components/style/properties/helpers.mako.rs | 1 - components/style/properties/properties.mako.rs | 8 -------- 3 files changed, 10 deletions(-) diff --git a/components/style/custom_properties.rs b/components/style/custom_properties.rs index 985179db025..6ed6b6f3288 100644 --- a/components/style/custom_properties.rs +++ b/components/style/custom_properties.rs @@ -543,7 +543,6 @@ impl<'a> CustomPropertiesBuilder<'a> { self.may_have_cycles |= !specified_value.references.is_empty(); map.insert(name.clone(), (*specified_value).clone()); }, - DeclaredValue::WithVariables(_) => unreachable!(), DeclaredValue::CSSWideKeyword(keyword) => match keyword { CSSWideKeyword::Initial => { map.remove(name); diff --git a/components/style/properties/helpers.mako.rs b/components/style/properties/helpers.mako.rs index e07c9bd57cb..bf262f581d6 100644 --- a/components/style/properties/helpers.mako.rs +++ b/components/style/properties/helpers.mako.rs @@ -379,7 +379,6 @@ % endif % endif } - DeclaredValue::WithVariables(_) => unreachable!(), DeclaredValue::CSSWideKeyword(keyword) => match keyword { % if not data.current_style_struct.inherited: CSSWideKeyword::Unset | diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 39fc90b2f3d..40d2d52541e 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1444,8 +1444,6 @@ impl ShorthandId { pub enum DeclaredValue<'a, T: 'a> { /// A known specified value from the stylesheet. Value(&'a T), - /// An unparsed value that contains `var()` functions. - WithVariables(&'a Arc), /// An CSS-wide keyword. CSSWideKeyword(CSSWideKeyword), } @@ -1459,11 +1457,6 @@ pub enum DeclaredValue<'a, T: 'a> { pub enum DeclaredValueOwned { /// A known specified value from the stylesheet. Value(T), - /// An unparsed value that contains `var()` functions. - WithVariables( - #[cfg_attr(feature = "gecko", ignore_malloc_size_of = "XXX: how to handle this?")] - Arc - ), /// An CSS-wide keyword. CSSWideKeyword(CSSWideKeyword), } @@ -1473,7 +1466,6 @@ impl DeclaredValueOwned { fn borrow(&self) -> DeclaredValue { match *self { DeclaredValueOwned::Value(ref v) => DeclaredValue::Value(v), - DeclaredValueOwned::WithVariables(ref v) => DeclaredValue::WithVariables(v), DeclaredValueOwned::CSSWideKeyword(v) => DeclaredValue::CSSWideKeyword(v), } } From 4cd0f492f484788aab27285b1891b34f2220b575 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 17 Sep 2018 14:12:27 +0000 Subject: [PATCH 04/13] style: Deindent the non-css-wide-keyword-related code from cascade_property. There's no good reason we construct a DeclaredValue as an intermediate step. Differential Revision: https://phabricator.services.mozilla.com/D5977 --- components/style/properties/helpers.mako.rs | 160 ++++++++++---------- 1 file changed, 77 insertions(+), 83 deletions(-) diff --git a/components/style/properties/helpers.mako.rs b/components/style/properties/helpers.mako.rs index bf262f581d6..c1f4fc9d450 100644 --- a/components/style/properties/helpers.mako.rs +++ b/components/style/properties/helpers.mako.rs @@ -312,20 +312,6 @@ declaration: &PropertyDeclaration, context: &mut computed::Context, ) { - let value = match *declaration { - PropertyDeclaration::${property.camel_case}(ref value) => { - DeclaredValue::Value(value) - }, - PropertyDeclaration::CSSWideKeyword(ref declaration) => { - debug_assert_eq!(declaration.id, LonghandId::${property.camel_case}); - DeclaredValue::CSSWideKeyword(declaration.keyword) - }, - PropertyDeclaration::WithVariables(..) => { - panic!("variables should already have been substituted") - } - _ => panic!("entered the wrong cascade_property() implementation"), - }; - context.for_non_inherited_property = % if property.style_struct.inherited: None; @@ -333,78 +319,86 @@ Some(LonghandId::${property.camel_case}); % endif - match value { - DeclaredValue::Value(specified_value) => { - % if property.ident in SYSTEM_FONT_LONGHANDS and product == "gecko": - if let Some(sf) = specified_value.get_system() { - longhands::system_font::resolve_system_font(sf, context); + let specified_value = match *declaration { + PropertyDeclaration::${property.camel_case}(ref value) => value, + PropertyDeclaration::CSSWideKeyword(ref declaration) => { + debug_assert_eq!(declaration.id, LonghandId::${property.camel_case}); + match declaration.keyword { + % if not data.current_style_struct.inherited: + CSSWideKeyword::Unset | + % endif + CSSWideKeyword::Initial => { + % if property.ident == "font_size": + computed::FontSize::cascade_initial_font_size(context); + % else: + context.builder.reset_${property.ident}(); + % endif + }, + % if data.current_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); + % else: + context.builder.inherit_${property.ident}(); + % endif } - % endif - % if not property.style_struct.inherited and property.logical: - context.rule_cache_conditions.borrow_mut() - .set_writing_mode_dependency(context.builder.writing_mode); - % endif - % if property.is_vector: - // In the case of a vector property we want to pass - // down an iterator so that this can be computed - // without allocation - // - // However, computing requires a context, but the - // style struct being mutated is on the context. We - // temporarily remove it, mutate it, and then put it - // back. Vector longhands cannot touch their own - // style struct whilst computing, else this will - // panic. - let mut s = - context.builder.take_${data.current_style_struct.name_lower}(); - { - let iter = specified_value.compute_iter(context); - s.set_${property.ident}(iter); - } - context.builder.put_${data.current_style_struct.name_lower}(s); - % else: - % if property.boxed: - let computed = (**specified_value).to_computed_value(context); - % else: - let computed = specified_value.to_computed_value(context); - % endif - % if property.ident == "font_size": - specified::FontSize::cascade_specified_font_size( - context, - &specified_value, - computed, - ); - % else: - context.builder.set_${property.ident}(computed) - % endif - % endif - } - DeclaredValue::CSSWideKeyword(keyword) => match keyword { - % if not data.current_style_struct.inherited: - CSSWideKeyword::Unset | - % endif - CSSWideKeyword::Initial => { - % if property.ident == "font_size": - computed::FontSize::cascade_initial_font_size(context); - % else: - context.builder.reset_${property.ident}(); - % endif - }, - % if data.current_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); - % else: - context.builder.inherit_${property.ident}(); - % endif } + return; } - } + PropertyDeclaration::WithVariables(..) => { + panic!("variables should already have been substituted") + } + _ => panic!("entered the wrong cascade_property() implementation"), + }; + + % if property.ident in SYSTEM_FONT_LONGHANDS and product == "gecko": + if let Some(sf) = specified_value.get_system() { + longhands::system_font::resolve_system_font(sf, context); + } + % endif + + % if not property.style_struct.inherited and property.logical: + context.rule_cache_conditions.borrow_mut() + .set_writing_mode_dependency(context.builder.writing_mode); + % endif + + % if property.is_vector: + // In the case of a vector property we want to pass down an + // iterator so that this can be computed without allocation. + // + // However, computing requires a context, but the style struct + // being mutated is on the context. We temporarily remove it, + // mutate it, and then put it back. Vector longhands cannot + // touch their own style struct whilst computing, else this will + // panic. + let mut s = + context.builder.take_${data.current_style_struct.name_lower}(); + { + let iter = specified_value.compute_iter(context); + s.set_${property.ident}(iter); + } + context.builder.put_${data.current_style_struct.name_lower}(s); + % else: + % if property.boxed: + let computed = (**specified_value).to_computed_value(context); + % else: + let computed = specified_value.to_computed_value(context); + % endif + % if property.ident == "font_size": + specified::FontSize::cascade_specified_font_size( + context, + &specified_value, + computed, + ); + % else: + context.builder.set_${property.ident}(computed) + % endif + % endif } pub fn parse_declared<'i, 't>( From 5cafac5d10f51cb32aaba35661e3c4634a59ce5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 17 Sep 2018 04:47:02 +0000 Subject: [PATCH 05/13] style: Remove DeclaredValue. I think it used to be the case that all PropertyDeclaration variants had a DeclaredValueOwned inside. But that's no longer the case, so this abstraction seems less useful now. Differential Revision: https://phabricator.services.mozilla.com/D5978 --- components/style/custom_properties.rs | 26 ++++---- components/style/properties/cascade.rs | 2 +- .../style/properties/declaration_block.rs | 2 +- components/style/properties/helpers.mako.rs | 2 +- .../style/properties/properties.mako.rs | 63 ++++++------------- 5 files changed, 36 insertions(+), 59 deletions(-) diff --git a/components/style/custom_properties.rs b/components/style/custom_properties.rs index 6ed6b6f3288..331d2a74342 100644 --- a/components/style/custom_properties.rs +++ b/components/style/custom_properties.rs @@ -10,7 +10,7 @@ use Atom; use cssparser::{Delimiter, Parser, ParserInput, SourcePosition, Token, TokenSerializationType}; use hash::map::Entry; use precomputed_hash::PrecomputedHash; -use properties::{CSSWideKeyword, DeclaredValue}; +use properties::{CSSWideKeyword, CustomDeclarationValue}; use selector_map::{PrecomputedHashMap, PrecomputedHashSet}; use selectors::parser::SelectorParseErrorKind; use servo_arc::Arc; @@ -519,14 +519,14 @@ impl<'a> CustomPropertiesBuilder<'a> { pub fn cascade( &mut self, name: &'a Name, - specified_value: DeclaredValue<'a, Arc>, + specified_value: &CustomDeclarationValue, ) { let was_already_present = !self.seen.insert(name); if was_already_present { return; } - if !self.value_may_affect_style(name, &specified_value) { + if !self.value_may_affect_style(name, specified_value) { return; } @@ -538,12 +538,12 @@ impl<'a> CustomPropertiesBuilder<'a> { } let map = self.custom_properties.as_mut().unwrap(); - match specified_value { - DeclaredValue::Value(ref specified_value) => { - self.may_have_cycles |= !specified_value.references.is_empty(); - map.insert(name.clone(), (*specified_value).clone()); + match *specified_value { + CustomDeclarationValue::Value(ref unparsed_value) => { + self.may_have_cycles |= !unparsed_value.references.is_empty(); + map.insert(name.clone(), (*unparsed_value).clone()); }, - DeclaredValue::CSSWideKeyword(keyword) => match keyword { + CustomDeclarationValue::CSSWideKeyword(keyword) => match keyword { CSSWideKeyword::Initial => { map.remove(name); }, @@ -556,11 +556,11 @@ impl<'a> CustomPropertiesBuilder<'a> { fn value_may_affect_style( &self, name: &Name, - value: &DeclaredValue>, + value: &CustomDeclarationValue, ) -> bool { match *value { - DeclaredValue::CSSWideKeyword(CSSWideKeyword::Unset) | - DeclaredValue::CSSWideKeyword(CSSWideKeyword::Inherit) => { + CustomDeclarationValue::CSSWideKeyword(CSSWideKeyword::Unset) | + CustomDeclarationValue::CSSWideKeyword(CSSWideKeyword::Inherit) => { // Custom properties are inherited by default. So // explicit 'inherit' or 'unset' means we can just use // any existing value in the inherited CustomPropertiesMap. @@ -576,12 +576,12 @@ impl<'a> CustomPropertiesBuilder<'a> { .or_else(|| self.inherited.and_then(|m| m.get(name))); match (existing_value, value) { - (None, &DeclaredValue::CSSWideKeyword(CSSWideKeyword::Initial)) => { + (None, &CustomDeclarationValue::CSSWideKeyword(CSSWideKeyword::Initial)) => { // The initial value of a custom property is the same as it // not existing in the map. return false; }, - (Some(existing_value), &DeclaredValue::Value(specified_value)) => { + (Some(existing_value), &CustomDeclarationValue::Value(ref specified_value)) => { // Don't bother overwriting an existing inherited value with // the same specified value. if existing_value == specified_value { diff --git a/components/style/properties/cascade.rs b/components/style/properties/cascade.rs index 41e308e3a83..cb8a4ae7c39 100644 --- a/components/style/properties/cascade.rs +++ b/components/style/properties/cascade.rs @@ -246,7 +246,7 @@ where for (declaration, _cascade_level) in iter_declarations() { if let PropertyDeclaration::Custom(ref declaration) = *declaration { - builder.cascade(&declaration.name, declaration.value.borrow()); + builder.cascade(&declaration.name, &declaration.value); } } diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 56aa2fde4f4..aa8e93bd9c7 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -849,7 +849,7 @@ impl PropertyDeclarationBlock { for declaration in self.normal_declaration_iter() { if let PropertyDeclaration::Custom(ref declaration) = *declaration { - builder.cascade(&declaration.name, declaration.value.borrow()); + builder.cascade(&declaration.name, &declaration.value); } } diff --git a/components/style/properties/helpers.mako.rs b/components/style/properties/helpers.mako.rs index c1f4fc9d450..5b04e0dc02e 100644 --- a/components/style/properties/helpers.mako.rs +++ b/components/style/properties/helpers.mako.rs @@ -289,7 +289,7 @@ #[allow(unused_imports)] use properties::longhands; #[allow(unused_imports)] - use properties::{DeclaredValue, LonghandId, LonghandIdSet}; + use properties::{LonghandId, LonghandIdSet}; #[allow(unused_imports)] use properties::{CSSWideKeyword, ComputedValues, PropertyDeclaration}; #[allow(unused_imports)] diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 40d2d52541e..c78bd863438 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1438,39 +1438,6 @@ impl ShorthandId { } } -/// Servo's representation of a declared value for a given `T`, which is the -/// declared value for that property. -#[derive(Clone, Debug, Eq, PartialEq)] -pub enum DeclaredValue<'a, T: 'a> { - /// A known specified value from the stylesheet. - Value(&'a T), - /// An CSS-wide keyword. - CSSWideKeyword(CSSWideKeyword), -} - -/// A variant of DeclaredValue that owns its data. This separation exists so -/// that PropertyDeclaration can avoid embedding a DeclaredValue (and its -/// extra discriminant word) and synthesize dependent DeclaredValues for -/// PropertyDeclaration instances as needed. -#[cfg_attr(feature = "gecko", derive(MallocSizeOf))] -#[derive(Clone, Debug, Eq, PartialEq, ToCss)] -pub enum DeclaredValueOwned { - /// A known specified value from the stylesheet. - Value(T), - /// An CSS-wide keyword. - CSSWideKeyword(CSSWideKeyword), -} - -impl DeclaredValueOwned { - /// Creates a dependent DeclaredValue from this DeclaredValueOwned. - fn borrow(&self) -> DeclaredValue { - match *self { - DeclaredValueOwned::Value(ref v) => DeclaredValue::Value(v), - DeclaredValueOwned::CSSWideKeyword(v) => DeclaredValue::CSSWideKeyword(v), - } - } -} - /// An unparsed property value that contains `var()` functions. #[derive(Debug, Eq, PartialEq)] pub struct UnparsedValue { @@ -1929,6 +1896,16 @@ pub struct VariableDeclaration { value: Arc, } +/// A custom property declaration value is either an unparsed value or a CSS +/// wide-keyword. +#[derive(Clone, PartialEq, ToCss)] +pub enum CustomDeclarationValue { + /// A value. + Value(Arc<::custom_properties::SpecifiedValue>), + /// A wide keyword. + CSSWideKeyword(CSSWideKeyword), +} + /// A custom property declaration with the property name and the declared value. #[cfg_attr(feature = "gecko", derive(MallocSizeOf))] #[derive(Clone, PartialEq, ToCss)] @@ -1938,7 +1915,7 @@ pub struct CustomDeclaration { pub name: ::custom_properties::Name, /// The value of the custom property. #[cfg_attr(feature = "gecko", ignore_malloc_size_of = "XXX: how to handle this?")] - pub value: DeclaredValueOwned>, + pub value: CustomDeclarationValue, } impl fmt::Debug for PropertyDeclaration { @@ -2120,13 +2097,13 @@ impl PropertyDeclaration { /// This is the case of custom properties and values that contain /// unsubstituted variables. pub fn value_is_unparsed(&self) -> bool { - match *self { - PropertyDeclaration::WithVariables(..) => true, - PropertyDeclaration::Custom(ref declaration) => { - !matches!(declaration.value.borrow(), DeclaredValue::CSSWideKeyword(..)) - } - _ => false, - } + match *self { + PropertyDeclaration::WithVariables(..) => true, + PropertyDeclaration::Custom(ref declaration) => { + matches!(declaration.value, CustomDeclarationValue::Value(..)) + } + _ => false, + } } /// Returns true if this property declaration is for one of the animatable @@ -2171,9 +2148,9 @@ impl PropertyDeclaration { // before adding skip_whitespace here. // This probably affects some test results. let value = match input.try(CSSWideKeyword::parse) { - Ok(keyword) => DeclaredValueOwned::CSSWideKeyword(keyword), + Ok(keyword) => CustomDeclarationValue::CSSWideKeyword(keyword), Err(()) => match ::custom_properties::SpecifiedValue::parse(input) { - Ok(value) => DeclaredValueOwned::Value(value), + Ok(value) => CustomDeclarationValue::Value(value), Err(e) => return Err(StyleParseErrorKind::new_invalid( format!("--{}", property_name), e, From 72ce653daf838fa607e2f562faa86f17c79eeffb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 16 Sep 2018 21:35:16 +0200 Subject: [PATCH 06/13] style: Allow integer division inside calc() expressions. Differential Revision: https://phabricator.services.mozilla.com/D5980 --- components/style/values/specified/calc.rs | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/components/style/values/specified/calc.rs b/components/style/values/specified/calc.rs index 06c1dba2955..69631069990 100644 --- a/components/style/values/specified/calc.rs +++ b/components/style/values/specified/calc.rs @@ -47,8 +47,6 @@ pub enum CalcNode { pub enum CalcUnit { /// `` Number, - /// `` - Integer, /// `` Length, /// `` @@ -281,8 +279,7 @@ impl CalcNode { let new_root = CalcNode::Mul(Box::new(root), Box::new(rhs)); root = new_root; }, - // TODO(emilio): Figure out why the `Integer` check. - Ok(&Token::Delim('/')) if expected_unit != CalcUnit::Integer => { + Ok(&Token::Delim('/')) => { let rhs = Self::parse_one(context, input, expected_unit)?; let new_root = CalcNode::Div(Box::new(root), Box::new(rhs)); root = new_root; @@ -532,10 +529,8 @@ impl CalcNode { context: &ParserContext, input: &mut Parser<'i, 't>, ) -> Result> { - Self::parse(context, input, CalcUnit::Integer)? - .to_number() - .map(|n| n as CSSInteger) - .map_err(|()| input.new_custom_error(StyleParseErrorKind::UnspecifiedError)) + Self::parse_number(context, input) + .map(|n| n.round() as CSSInteger) } /// Convenience parsing function for ` | `. From bc39f16b4b94a7e93cf3fc73c764c3d0e60806b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 17 Sep 2018 00:53:37 +0000 Subject: [PATCH 07/13] style: Simplify CSSWideKeyword::parse. Differential Revision: https://phabricator.services.mozilla.com/D5972 --- .../style/properties/properties.mako.rs | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index c78bd863438..3ab358524ba 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -853,25 +853,22 @@ impl CSSWideKeyword { CSSWideKeyword::Unset => "unset", } } - - /// Takes the result of cssparser::Parser::expect_ident() and converts it - /// to a CSSWideKeyword. - pub fn from_ident<'i>(ident: &str) -> Option { - match_ignore_ascii_case! { ident, - // If modifying this set of keyword, also update values::CustomIdent::from_ident - "initial" => Some(CSSWideKeyword::Initial), - "inherit" => Some(CSSWideKeyword::Inherit), - "unset" => Some(CSSWideKeyword::Unset), - _ => None - } - } } impl CSSWideKeyword { fn parse(input: &mut Parser) -> Result { - let ident = input.expect_ident().map_err(|_| ())?.clone(); + let keyword = { + let ident = input.expect_ident().map_err(|_| ())?; + match_ignore_ascii_case! { ident, + // If modifying this set of keyword, also update values::CustomIdent::from_ident + "initial" => CSSWideKeyword::Initial, + "inherit" => CSSWideKeyword::Inherit, + "unset" => CSSWideKeyword::Unset, + _ => return Err(()), + } + }; input.expect_exhausted().map_err(|_| ())?; - CSSWideKeyword::from_ident(&ident).ok_or(()) + Ok(keyword) } } From 22da3c22a2751ac2de4a0dffcad171ab26a6a59e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 18 Sep 2018 09:15:12 +0000 Subject: [PATCH 08/13] style: Make LonghandId::flags an indexing operation. I always see a bunch of time in our profiles in the iterator over the declarations, this ensures it's not something dumb. I suspect it's just a bunch of cache misses from walking the rule tree but in any case this is consistent with the other getters we have and such. Differential Revision: https://phabricator.services.mozilla.com/D5971 --- .../style/properties/properties.mako.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 3ab358524ba..5968d84bd33 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -1113,16 +1113,19 @@ impl LonghandId { } /// Returns PropertyFlags for given longhand property. - pub fn flags(&self) -> PropertyFlags { - match *self { + #[inline(always)] + pub fn flags(self) -> PropertyFlags { + // TODO(emilio): This can be simplified further as Rust gains more + // constant expression support. + const FLAGS: [u8; ${len(data.longhands)}] = [ % for property in data.longhands: - LonghandId::${property.camel_case} => - % for flag in property.flags: - PropertyFlags::${flag} | - % endfor - PropertyFlags::empty(), + % for flag in property.flags: + PropertyFlags::${flag}.bits | + % endfor + 0, % endfor - } + ]; + PropertyFlags::from_bits_truncate(FLAGS[self as usize]) } /// Only a few properties are allowed to depend on the visited state of From 84ca681a788bf730ba142be408239c6f5c92b2ba Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Tue, 18 Sep 2018 09:19:18 +0000 Subject: [PATCH 09/13] servo_arc cleanups for publishing. Differential Revision: https://phabricator.services.mozilla.com/D6034 --- components/selectors/tree.rs | 12 +- components/servo_arc/lib.rs | 342 ++++++++++++++++++------------ components/style/rule_tree/mod.rs | 16 +- components/style/sharing/mod.rs | 13 +- 4 files changed, 229 insertions(+), 154 deletions(-) diff --git a/components/selectors/tree.rs b/components/selectors/tree.rs index 112b02dbbd9..bfa59898b50 100644 --- a/components/selectors/tree.rs +++ b/components/selectors/tree.rs @@ -8,18 +8,22 @@ use attr::{AttrSelectorOperation, CaseSensitivity, NamespaceConstraint}; use matching::{ElementSelectorFlags, MatchingContext}; use parser::SelectorImpl; -use servo_arc::NonZeroPtrMut; use std::fmt::Debug; +use std::ptr::NonNull; /// Opaque representation of an Element, for identity comparisons. We use /// NonZeroPtrMut to get the NonZero optimization. #[derive(Clone, Debug, Eq, Hash, PartialEq)] -pub struct OpaqueElement(NonZeroPtrMut<()>); +pub struct OpaqueElement(NonNull<()>); + +unsafe impl Send for OpaqueElement {} impl OpaqueElement { /// Creates a new OpaqueElement from an arbitrarily-typed pointer. - pub fn new(ptr: *const T) -> Self { - OpaqueElement(NonZeroPtrMut::new(ptr as *const () as *mut ())) + pub fn new(ptr: &T) -> Self { + unsafe { + OpaqueElement(NonNull::new_unchecked(ptr as *const T as *const () as *mut ())) + } } } diff --git a/components/servo_arc/lib.rs b/components/servo_arc/lib.rs index 3e6cd65e85d..99e9f67f0d0 100644 --- a/components/servo_arc/lib.rs +++ b/components/servo_arc/lib.rs @@ -19,7 +19,7 @@ //! //! [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1360883 -// The semantics of Arc are alread documented in the Rust docs, so we don't +// The semantics of `Arc` are alread documented in the Rust docs, so we don't // duplicate those here. #![allow(missing_docs)] @@ -74,72 +74,41 @@ macro_rules! offset_of { /// necessarily) at _exactly_ `MAX_REFCOUNT + 1` references. const MAX_REFCOUNT: usize = (isize::MAX) as usize; -/// Wrapper type for pointers to get the non-zero optimization. When -/// NonZero/Shared/Unique are stabilized, we should just use Shared -/// here to get the same effect. Gankro is working on this in [1]. +/// An atomically reference counted shared pointer /// -/// It's unfortunate that this needs to infect all the caller types -/// with 'static. It would be nice to just use a &() and a PhantomData -/// instead, but then the compiler can't determine whether the &() should -/// be thin or fat (which depends on whether or not T is sized). Given -/// that this is all a temporary hack, this restriction is fine for now. +/// See the documentation for [`Arc`] in the standard library. Unlike the +/// standard library `Arc`, this `Arc` does not support weak reference counting. /// -/// [1]: https://github.com/rust-lang/rust/issues/27730 -// FIXME: remove this and use std::ptr::NonNull when Firefox requires Rust 1.25+ -pub struct NonZeroPtrMut(&'static mut T); -impl NonZeroPtrMut { - pub fn new(ptr: *mut T) -> Self { - assert!(!(ptr as *mut u8).is_null()); - NonZeroPtrMut(unsafe { mem::transmute(ptr) }) - } - - pub fn ptr(&self) -> *mut T { - self.0 as *const T as *mut T - } -} - -impl Clone for NonZeroPtrMut { - fn clone(&self) -> Self { - NonZeroPtrMut::new(self.ptr()) - } -} - -impl fmt::Pointer for NonZeroPtrMut { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - fmt::Pointer::fmt(&self.ptr(), f) - } -} - -impl fmt::Debug for NonZeroPtrMut { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - ::fmt(self, f) - } -} - -impl PartialEq for NonZeroPtrMut { - fn eq(&self, other: &Self) -> bool { - self.ptr() == other.ptr() - } -} - -impl Eq for NonZeroPtrMut {} - -impl Hash for NonZeroPtrMut { - fn hash(&self, state: &mut H) { - self.ptr().hash(state) - } -} - +/// [`Arc`]: https://doc.rust-lang.org/stable/std/sync/struct.Arc.html #[repr(C)] -pub struct Arc { - p: NonZeroPtrMut>, +pub struct Arc { + p: ptr::NonNull>, } -/// An Arc that is known to be uniquely owned +/// An `Arc` that is known to be uniquely owned /// -/// This lets us build arcs that we can mutate before -/// freezing, without needing to change the allocation -pub struct UniqueArc(Arc); +/// When `Arc`s are constructed, they are known to be +/// uniquely owned. In such a case it is safe to mutate +/// the contents of the `Arc`. Normally, one would just handle +/// this by mutating the data on the stack before allocating the +/// `Arc`, however it's possible the data is large or unsized +/// and you need to heap-allocate it earlier in such a way +/// that it can be freely converted into a regular `Arc` once you're +/// done. +/// +/// `UniqueArc` exists for this purpose, when constructed it performs +/// the same allocations necessary for an `Arc`, however it allows mutable access. +/// Once the mutation is finished, you can call `.shareable()` and get a regular `Arc` +/// out of it. +/// +/// ```rust +/// # use servo_arc::UniqueArc; +/// let data = [1, 2, 3, 4, 5]; +/// let mut x = UniqueArc::new(data); +/// x[4] = 7; // mutate! +/// let y = x.shareable(); // y is an Arc +/// ``` +pub struct UniqueArc(Arc); impl UniqueArc { #[inline] @@ -149,7 +118,7 @@ impl UniqueArc { } #[inline] - /// Convert to a shareable Arc once we're done using it + /// Convert to a shareable Arc once we're done mutating it pub fn shareable(self) -> Arc { self.0 } @@ -172,6 +141,7 @@ impl DerefMut for UniqueArc { unsafe impl Send for Arc {} unsafe impl Sync for Arc {} +/// The object allocated by an Arc #[repr(C)] struct ArcInner { count: atomic::AtomicUsize, @@ -182,36 +152,52 @@ unsafe impl Send for ArcInner {} unsafe impl Sync for ArcInner {} impl Arc { + /// Construct an `Arc` #[inline] pub fn new(data: T) -> Self { let x = Box::new(ArcInner { count: atomic::AtomicUsize::new(1), data: data, }); - Arc { - p: NonZeroPtrMut::new(Box::into_raw(x)), + unsafe { + Arc { + p: ptr::NonNull::new_unchecked(Box::into_raw(x)), + } } } + /// Convert the Arc to a raw pointer, suitable for use across FFI + /// + /// Note: This returns a pointer to the data T, which is offset in the allocation. + /// + /// It is recommended to use RawOffsetArc for this. #[inline] - pub fn into_raw(this: Self) -> *const T { + fn into_raw(this: Self) -> *const T { let ptr = unsafe { &((*this.ptr()).data) as *const _ }; mem::forget(this); ptr } + /// Reconstruct the Arc from a raw pointer obtained from into_raw() + /// + /// Note: This raw pointer will be offset in the allocation and must be preceded + /// by the atomic count. + /// + /// It is recommended to use RawOffsetArc for this #[inline] unsafe fn from_raw(ptr: *const T) -> Self { // To find the corresponding pointer to the `ArcInner` we need // to subtract the offset of the `data` field from the pointer. let ptr = (ptr as *const u8).offset(-offset_of!(ArcInner, data)); Arc { - p: NonZeroPtrMut::new(ptr as *mut ArcInner), + p: ptr::NonNull::new_unchecked(ptr as *mut ArcInner), } } /// Produce a pointer to the data that can be converted back - /// to an arc + /// to an Arc. This is basically an `&Arc`, without the extra indirection. + /// It has the benefits of an `&T` but also knows about the underlying refcount + /// and can be converted into more `Arc`s if necessary. #[inline] pub fn borrow_arc<'a>(&'a self) -> ArcBorrow<'a, T> { ArcBorrow(&**self) @@ -240,7 +226,7 @@ impl Arc { /// Returns the address on the heap of the Arc itself -- not the T within it -- for memory /// reporting. pub fn heap_ptr(&self) -> *const c_void { - self.p.ptr() as *const ArcInner as *const c_void + self.p.as_ptr() as *const ArcInner as *const c_void } } @@ -261,13 +247,15 @@ impl Arc { let _ = Box::from_raw(self.ptr()); } + /// Test pointer equality between the two Arcs, i.e. they must be the _same_ + /// allocation #[inline] pub fn ptr_eq(this: &Self, other: &Self) -> bool { this.ptr() == other.ptr() } fn ptr(&self) -> *mut ArcInner { - self.p.ptr() + self.p.as_ptr() } } @@ -300,8 +288,10 @@ impl Clone for Arc { process::abort(); } - Arc { - p: NonZeroPtrMut::new(self.ptr()), + unsafe { + Arc { + p: ptr::NonNull::new_unchecked(self.ptr()), + } } } } @@ -316,6 +306,19 @@ impl Deref for Arc { } impl Arc { + /// Makes a mutable reference to the `Arc`, cloning if necessary + /// + /// This is functionally equivalent to [`Arc::make_mut`][mm] from the standard library. + /// + /// If this `Arc` is uniquely owned, `make_mut()` will provide a mutable + /// reference to the contents. If not, `make_mut()` will create a _new_ `Arc` + /// with a copy of the contents, update `this` to point to it, and provide + /// a mutable reference to its contents. + /// + /// This is useful for implementing copy-on-write schemes where you wish to + /// avoid copying things if your `Arc` is not shared. + /// + /// [mm]: https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.make_mut #[inline] pub fn make_mut(this: &mut Self) -> &mut T { if !this.is_unique() { @@ -335,6 +338,7 @@ impl Arc { } impl Arc { + /// Provides mutable access to the contents _if_ the `Arc` is uniquely owned. #[inline] pub fn get_mut(this: &mut Self) -> Option<&mut T> { if this.is_unique() { @@ -347,6 +351,7 @@ impl Arc { } } + /// Whether or not the `Arc` is uniquely owned (is the refcount 1?) #[inline] pub fn is_unique(&self) -> bool { // See the extensive discussion in [1] for why this needs to be Acquire. @@ -402,6 +407,7 @@ impl PartialEq for Arc { !Self::ptr_eq(self, other) && *(*self) != *(*other) } } + impl PartialOrd for Arc { fn partial_cmp(&self, other: &Arc) -> Option { (**self).partial_cmp(&**other) @@ -615,8 +621,10 @@ impl Arc> { size_of::() * 2, "The Arc will be fat" ); - Arc { - p: NonZeroPtrMut::new(ptr), + unsafe { + Arc { + p: ptr::NonNull::new_unchecked(ptr), + } } } @@ -651,7 +659,22 @@ impl HeaderWithLength { } type HeaderSliceWithLength = HeaderSlice, T>; -pub struct ThinArc { + +/// A "thin" `Arc` containing dynamically sized data +/// +/// This is functionally equivalent to Arc<(H, [T])> +/// +/// When you create an `Arc` containing a dynamically sized type +/// like `HeaderSlice`, the `Arc` is represented on the stack +/// as a "fat pointer", where the length of the slice is stored +/// alongside the `Arc`'s pointer. In some situations you may wish to +/// have a thin pointer instead, perhaps for FFI compatibility +/// or space efficiency. +/// +/// `ThinArc` solves this by storing the length in the allocation itself, +/// via `HeaderSliceWithLength`. +#[repr(C)] +pub struct ThinArc { ptr: *mut ArcInner>, } @@ -670,7 +693,7 @@ fn thin_to_thick( fake_slice as *mut ArcInner> } -impl ThinArc { +impl ThinArc { /// Temporarily converts |self| into a bonafide Arc and exposes it to the /// provided callback. The refcount is not modified. #[inline] @@ -679,9 +702,9 @@ impl ThinArc { F: FnOnce(&Arc>) -> U, { // Synthesize transient Arc, which never touches the refcount of the ArcInner. - let transient = NoDrop::new(Arc { - p: NonZeroPtrMut::new(thin_to_thick(self.ptr)), - }); + let transient = unsafe { NoDrop::new(Arc { + p: ptr::NonNull::new_unchecked(thin_to_thick(self.ptr)), + })}; // Expose the transient Arc to the callback, which may clone it if it wants. let result = f(&transient); @@ -695,6 +718,16 @@ impl ThinArc { result } + /// Creates a `ThinArc` for a HeaderSlice using the given header struct and + /// iterator to generate the slice. + pub fn from_header_and_iter(header: H, items: I) -> Self + where + I: Iterator + ExactSizeIterator + { + let header = HeaderWithLength::new(header, items.len()); + Arc::into_thin(Arc::from_header_and_iter(header, items)) + } + /// Returns the address on the heap of the ThinArc itself -- not the T /// within it -- for memory reporting. #[inline] @@ -712,22 +745,22 @@ impl Deref for ThinArc { } } -impl Clone for ThinArc { +impl Clone for ThinArc { #[inline] fn clone(&self) -> Self { ThinArc::with_arc(self, |a| Arc::into_thin(a.clone())) } } -impl Drop for ThinArc { +impl Drop for ThinArc { #[inline] fn drop(&mut self) { let _ = Arc::from_thin(ThinArc { ptr: self.ptr }); } } -impl Arc> { - /// Converts an Arc into a ThinArc. This consumes the Arc, so the refcount +impl Arc> { + /// Converts an `Arc` into a `ThinArc`. This consumes the `Arc`, so the refcount /// is not modified. #[inline] pub fn into_thin(a: Self) -> ThinArc { @@ -744,29 +777,31 @@ impl Arc> { } } - /// Converts a ThinArc into an Arc. This consumes the ThinArc, so the refcount + /// Converts a `ThinArc` into an `Arc`. This consumes the `ThinArc`, so the refcount /// is not modified. #[inline] pub fn from_thin(a: ThinArc) -> Self { let ptr = thin_to_thick(a.ptr); mem::forget(a); - Arc { - p: NonZeroPtrMut::new(ptr), + unsafe { + Arc { + p: ptr::NonNull::new_unchecked(ptr), + } } } } -impl PartialEq for ThinArc { +impl PartialEq for ThinArc { #[inline] fn eq(&self, other: &ThinArc) -> bool { ThinArc::with_arc(self, |a| ThinArc::with_arc(other, |b| *a == *b)) } } -impl Eq for ThinArc {} +impl Eq for ThinArc {} -/// An Arc, except it holds a pointer to the T instead of to the -/// entire ArcInner. +/// An `Arc`, except it holds a pointer to the T instead of to the +/// entire ArcInner. This struct is FFI-compatible. /// /// ```text /// Arc RawOffsetArc @@ -779,31 +814,35 @@ impl Eq for ThinArc {} /// /// This means that this is a direct pointer to /// its contained data (and can be read from by both C++ and Rust), -/// but we can also convert it to a "regular" Arc by removing the offset +/// but we can also convert it to a "regular" Arc by removing the offset. +/// +/// This is very useful if you have an Arc-containing struct shared between Rust and C++, +/// and wish for C++ to be able to read the data behind the `Arc` without incurring +/// an FFI call overhead. #[derive(Eq)] #[repr(C)] -pub struct RawOffsetArc { - ptr: NonZeroPtrMut, +pub struct RawOffsetArc { + ptr: ptr::NonNull, } -unsafe impl Send for RawOffsetArc {} -unsafe impl Sync for RawOffsetArc {} +unsafe impl Send for RawOffsetArc {} +unsafe impl Sync for RawOffsetArc {} -impl Deref for RawOffsetArc { +impl Deref for RawOffsetArc { type Target = T; fn deref(&self) -> &Self::Target { - unsafe { &*self.ptr.ptr() } + unsafe { &*self.ptr.as_ptr() } } } -impl Clone for RawOffsetArc { +impl Clone for RawOffsetArc { #[inline] fn clone(&self) -> Self { Arc::into_raw_offset(self.clone_arc()) } } -impl Drop for RawOffsetArc { +impl Drop for RawOffsetArc { fn drop(&mut self) { let _ = Arc::from_raw_offset(RawOffsetArc { ptr: self.ptr.clone(), @@ -811,7 +850,7 @@ impl Drop for RawOffsetArc { } } -impl fmt::Debug for RawOffsetArc { +impl fmt::Debug for RawOffsetArc { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Debug::fmt(&**self, f) } @@ -827,7 +866,7 @@ impl PartialEq for RawOffsetArc { } } -impl RawOffsetArc { +impl RawOffsetArc { /// Temporarily converts |self| into a bonafide Arc and exposes it to the /// provided callback. The refcount is not modified. #[inline] @@ -836,7 +875,7 @@ impl RawOffsetArc { F: FnOnce(&Arc) -> U, { // Synthesize transient Arc, which never touches the refcount of the ArcInner. - let transient = unsafe { NoDrop::new(Arc::from_raw(self.ptr.ptr())) }; + let transient = unsafe { NoDrop::new(Arc::from_raw(self.ptr.as_ptr())) }; // Expose the transient Arc to the callback, which may clone it if it wants. let result = f(&transient); @@ -852,6 +891,8 @@ impl RawOffsetArc { /// If uniquely owned, provide a mutable reference /// Else create a copy, and mutate that + /// + /// This is functionally the same thing as `Arc::make_mut` #[inline] pub fn make_mut(&mut self) -> &mut T where @@ -872,54 +913,57 @@ impl RawOffsetArc { } } - /// Clone it as an Arc + /// Clone it as an `Arc` #[inline] pub fn clone_arc(&self) -> Arc { RawOffsetArc::with_arc(self, |a| a.clone()) } /// Produce a pointer to the data that can be converted back - /// to an arc + /// to an `Arc` #[inline] pub fn borrow_arc<'a>(&'a self) -> ArcBorrow<'a, T> { ArcBorrow(&**self) } } -impl Arc { - /// Converts an Arc into a RawOffsetArc. This consumes the Arc, so the refcount +impl Arc { + /// Converts an `Arc` into a `RawOffsetArc`. This consumes the `Arc`, so the refcount /// is not modified. #[inline] pub fn into_raw_offset(a: Self) -> RawOffsetArc { - RawOffsetArc { - ptr: NonZeroPtrMut::new(Arc::into_raw(a) as *mut T), + unsafe { + RawOffsetArc { + ptr: ptr::NonNull::new_unchecked(Arc::into_raw(a) as *mut T), + } } } - /// Converts a RawOffsetArc into an Arc. This consumes the RawOffsetArc, so the refcount + /// Converts a `RawOffsetArc` into an `Arc`. This consumes the `RawOffsetArc`, so the refcount /// is not modified. #[inline] pub fn from_raw_offset(a: RawOffsetArc) -> Self { - let ptr = a.ptr.ptr(); + let ptr = a.ptr.as_ptr(); mem::forget(a); unsafe { Arc::from_raw(ptr) } } } -/// A "borrowed Arc". This is a pointer to +/// A "borrowed `Arc`". This is a pointer to /// a T that is known to have been allocated within an -/// Arc. +/// `Arc`. /// /// This is equivalent in guarantees to `&Arc`, however it is /// a bit more flexible. To obtain an `&Arc` you must have -/// an Arc instance somewhere pinned down until we're done with it. +/// an `Arc` instance somewhere pinned down until we're done with it. +/// It's also a direct pointer to `T`, so using this involves less pointer-chasing /// -/// However, Gecko hands us refcounted things as pointers to T directly, -/// so we have to conjure up a temporary Arc on the stack each time. The -/// same happens for when the object is managed by a RawOffsetArc. +/// However, C++ code may hand us refcounted things as pointers to T directly, +/// so we have to conjure up a temporary `Arc` on the stack each time. The +/// same happens for when the object is managed by a `RawOffsetArc`. /// -/// ArcBorrow lets us deal with borrows of known-refcounted objects -/// without needing to worry about how they're actually stored. +/// `ArcBorrow` lets us deal with borrows of known-refcounted objects +/// without needing to worry about where the `Arc` is. #[derive(Debug, Eq, PartialEq)] pub struct ArcBorrow<'a, T: 'a>(&'a T); @@ -932,6 +976,7 @@ impl<'a, T> Clone for ArcBorrow<'a, T> { } impl<'a, T> ArcBorrow<'a, T> { + /// Clone this as an `Arc`. This bumps the refcount. #[inline] pub fn clone_arc(&self) -> Arc { let arc = unsafe { Arc::from_raw(self.0) }; @@ -947,10 +992,14 @@ impl<'a, T> ArcBorrow<'a, T> { ArcBorrow(r) } + /// Compare two `ArcBorrow`s via pointer equality. Will only return + /// true if they come from the same allocation pub fn ptr_eq(this: &Self, other: &Self) -> bool { this.0 as *const T == other.0 as *const T } + /// Temporarily converts |self| into a bonafide Arc and exposes it to the + /// provided callback. The refcount is not modified. #[inline] pub fn with_arc(&self, f: F) -> U where @@ -989,18 +1038,26 @@ impl<'a, T> Deref for ArcBorrow<'a, T> { } } -/// A tagged union that can represent Arc or Arc while only consuming a -/// single word. The type is also NonZero, and thus can be stored in an Option +/// A tagged union that can represent `Arc` or `Arc` while only consuming a +/// single word. The type is also `NonNull`, and thus can be stored in an Option /// without increasing size. /// +/// This is functionally equivalent to +/// `enum ArcUnion { First(Arc), Second(Arc)` but only takes up +/// up a single word of stack space. +/// /// This could probably be extended to support four types if necessary. -pub struct ArcUnion { - p: NonZeroPtrMut<()>, - phantom_a: PhantomData<&'static A>, - phantom_b: PhantomData<&'static B>, +pub struct ArcUnion { + p: ptr::NonNull<()>, + phantom_a: PhantomData, + phantom_b: PhantomData, } -impl PartialEq for ArcUnion { +unsafe impl Send for ArcUnion {} +unsafe impl Sync for ArcUnion {} + + +impl PartialEq for ArcUnion { fn eq(&self, other: &Self) -> bool { use ArcUnionBorrow::*; match (self.borrow(), other.borrow()) { @@ -1011,16 +1068,17 @@ impl PartialEq for ArcUnion { +pub enum ArcUnionBorrow<'a, A: 'a, B: 'a> { First(ArcBorrow<'a, A>), Second(ArcBorrow<'a, B>), } -impl ArcUnion { - fn new(ptr: *mut ()) -> Self { +impl ArcUnion { + unsafe fn new(ptr: *mut ()) -> Self { ArcUnion { - p: NonZeroPtrMut::new(ptr), + p: ptr::NonNull::new_unchecked(ptr), phantom_a: PhantomData, phantom_b: PhantomData, } @@ -1034,37 +1092,41 @@ impl ArcUnion { /// Returns an enum representing a borrow of either A or B. pub fn borrow(&self) -> ArcUnionBorrow { if self.is_first() { - let ptr = self.p.ptr() as *const A; + let ptr = self.p.as_ptr() as *const A; let borrow = unsafe { ArcBorrow::from_ref(&*ptr) }; ArcUnionBorrow::First(borrow) } else { - let ptr = ((self.p.ptr() as usize) & !0x1) as *const B; + let ptr = ((self.p.as_ptr() as usize) & !0x1) as *const B; let borrow = unsafe { ArcBorrow::from_ref(&*ptr) }; ArcUnionBorrow::Second(borrow) } } - /// Creates an ArcUnion from an instance of the first type. + /// Creates an `ArcUnion` from an instance of the first type. pub fn from_first(other: Arc) -> Self { - Self::new(Arc::into_raw(other) as *mut _) + unsafe { + Self::new(Arc::into_raw(other) as *mut _) + } } - /// Creates an ArcUnion from an instance of the second type. + /// Creates an `ArcUnion` from an instance of the second type. pub fn from_second(other: Arc) -> Self { - Self::new(((Arc::into_raw(other) as usize) | 0x1) as *mut _) + unsafe { + Self::new(((Arc::into_raw(other) as usize) | 0x1) as *mut _) + } } - /// Returns true if this ArcUnion contains the first type. + /// Returns true if this `ArcUnion` contains the first type. pub fn is_first(&self) -> bool { - self.p.ptr() as usize & 0x1 == 0 + self.p.as_ptr() as usize & 0x1 == 0 } - /// Returns true if this ArcUnion contains the second type. + /// Returns true if this `ArcUnion` contains the second type. pub fn is_second(&self) -> bool { !self.is_first() } - /// Returns a borrow of the first type if applicable, otherwise None. + /// Returns a borrow of the first type if applicable, otherwise `None`. pub fn as_first(&self) -> Option> { match self.borrow() { ArcUnionBorrow::First(x) => Some(x), @@ -1081,7 +1143,7 @@ impl ArcUnion { } } -impl Clone for ArcUnion { +impl Clone for ArcUnion { fn clone(&self) -> Self { match self.borrow() { ArcUnionBorrow::First(x) => ArcUnion::from_first(x.clone_arc()), @@ -1090,7 +1152,7 @@ impl Clone for ArcUnion { } } -impl Drop for ArcUnion { +impl Drop for ArcUnion { fn drop(&mut self) { match self.borrow() { ArcUnionBorrow::First(x) => unsafe { diff --git a/components/style/rule_tree/mod.rs b/components/style/rule_tree/mod.rs index 26c089eb4ad..662c303be18 100644 --- a/components/style/rule_tree/mod.rs +++ b/components/style/rule_tree/mod.rs @@ -12,7 +12,7 @@ use gecko::selector_parser::PseudoElement; #[cfg(feature = "gecko")] use malloc_size_of::{MallocSizeOf, MallocSizeOfOps}; use properties::{Importance, LonghandIdSet, PropertyDeclarationBlock}; -use servo_arc::{Arc, ArcBorrow, ArcUnion, ArcUnionBorrow, NonZeroPtrMut}; +use servo_arc::{Arc, ArcBorrow, ArcUnion, ArcUnionBorrow}; use shared_lock::{Locked, SharedRwLockReadGuard, StylesheetGuards}; use smallvec::SmallVec; use std::io::{self, Write}; @@ -942,15 +942,17 @@ impl MallocSizeOf for RuleNode { #[derive(Clone)] struct WeakRuleNode { - p: NonZeroPtrMut, + p: ptr::NonNull, } /// A strong reference to a rule node. #[derive(Debug, Eq, Hash, PartialEq)] pub struct StrongRuleNode { - p: NonZeroPtrMut, + p: ptr::NonNull, } +unsafe impl Send for StrongRuleNode {} + #[cfg(feature = "servo")] malloc_size_of_is_0!(StrongRuleNode); @@ -968,7 +970,7 @@ impl StrongRuleNode { fn from_ptr(ptr: *mut RuleNode) -> Self { StrongRuleNode { - p: NonZeroPtrMut::new(ptr), + p: ptr::NonNull::new(ptr).expect("Pointer must not be null"), } } @@ -1052,7 +1054,7 @@ impl StrongRuleNode { /// Raw pointer to the RuleNode pub fn ptr(&self) -> *mut RuleNode { - self.p.ptr() + self.p.as_ptr() } fn get(&self) -> &RuleNode { @@ -1665,12 +1667,12 @@ impl WeakRuleNode { fn from_ptr(ptr: *mut RuleNode) -> Self { WeakRuleNode { - p: NonZeroPtrMut::new(ptr), + p: ptr::NonNull::new(ptr).expect("Pointer must not be null"), } } fn ptr(&self) -> *mut RuleNode { - self.p.ptr() + self.p.as_ptr() } } diff --git a/components/style/sharing/mod.rs b/components/style/sharing/mod.rs index b3177216f64..23916ce7ff4 100644 --- a/components/style/sharing/mod.rs +++ b/components/style/sharing/mod.rs @@ -76,12 +76,13 @@ use properties::ComputedValues; use rule_tree::StrongRuleNode; use selectors::NthIndexCache; use selectors::matching::{ElementSelectorFlags, VisitedHandlingMode}; -use servo_arc::{Arc, NonZeroPtrMut}; +use servo_arc::Arc; use smallbitvec::SmallBitVec; use smallvec::SmallVec; use std::marker::PhantomData; use std::mem; use std::ops::Deref; +use std::ptr::NonNull; use style_resolver::{PrimaryStyle, ResolvedElementStyles}; use stylist::Stylist; use uluru::{Entry, LRUCache}; @@ -112,10 +113,16 @@ pub enum StyleSharingBehavior { /// Opaque pointer type to compare ComputedValues identities. #[derive(Clone, Debug, Eq, PartialEq)] -pub struct OpaqueComputedValues(NonZeroPtrMut<()>); +pub struct OpaqueComputedValues(NonNull<()>); + +unsafe impl Send for OpaqueComputedValues {} +unsafe impl Sync for OpaqueComputedValues {} + impl OpaqueComputedValues { fn from(cv: &ComputedValues) -> Self { - let p = NonZeroPtrMut::new(cv as *const ComputedValues as *const () as *mut ()); + let p = unsafe { + NonNull::new_unchecked(cv as *const ComputedValues as *const () as *mut ()) + }; OpaqueComputedValues(p) } From 4c97f68f3ebbee121469d7cf123bf6d6ade94497 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 18 Sep 2018 11:44:47 +0200 Subject: [PATCH 10/13] Fix tidy issues. --- components/selectors/tree.rs | 2 +- components/servo_arc/lib.rs | 23 ++++++++++------------- components/style/properties/cascade.rs | 3 ++- 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/components/selectors/tree.rs b/components/selectors/tree.rs index bfa59898b50..9db190c8109 100644 --- a/components/selectors/tree.rs +++ b/components/selectors/tree.rs @@ -22,7 +22,7 @@ impl OpaqueElement { /// Creates a new OpaqueElement from an arbitrarily-typed pointer. pub fn new(ptr: &T) -> Self { unsafe { - OpaqueElement(NonNull::new_unchecked(ptr as *const T as *const () as *mut ())) + OpaqueElement(NonNull::new_unchecked(ptr as *const T as *const () as *mut ())) } } } diff --git a/components/servo_arc/lib.rs b/components/servo_arc/lib.rs index 99e9f67f0d0..05296165acc 100644 --- a/components/servo_arc/lib.rs +++ b/components/servo_arc/lib.rs @@ -169,7 +169,7 @@ impl Arc { /// Convert the Arc to a raw pointer, suitable for use across FFI /// /// Note: This returns a pointer to the data T, which is offset in the allocation. - /// + /// /// It is recommended to use RawOffsetArc for this. #[inline] fn into_raw(this: Self) -> *const T { @@ -312,7 +312,7 @@ impl Arc { /// /// If this `Arc` is uniquely owned, `make_mut()` will provide a mutable /// reference to the contents. If not, `make_mut()` will create a _new_ `Arc` - /// with a copy of the contents, update `this` to point to it, and provide + /// with a copy of the contents, update `this` to point to it, and provide /// a mutable reference to its contents. /// /// This is useful for implementing copy-on-write schemes where you wish to @@ -702,9 +702,11 @@ impl ThinArc { F: FnOnce(&Arc>) -> U, { // Synthesize transient Arc, which never touches the refcount of the ArcInner. - let transient = unsafe { NoDrop::new(Arc { - p: ptr::NonNull::new_unchecked(thin_to_thick(self.ptr)), - })}; + let transient = unsafe { + NoDrop::new(Arc { + p: ptr::NonNull::new_unchecked(thin_to_thick(self.ptr)), + }) + }; // Expose the transient Arc to the callback, which may clone it if it wants. let result = f(&transient); @@ -722,7 +724,7 @@ impl ThinArc { /// iterator to generate the slice. pub fn from_header_and_iter(header: H, items: I) -> Self where - I: Iterator + ExactSizeIterator + I: Iterator + ExactSizeIterator, { let header = HeaderWithLength::new(header, items.len()); Arc::into_thin(Arc::from_header_and_iter(header, items)) @@ -1056,7 +1058,6 @@ pub struct ArcUnion { unsafe impl Send for ArcUnion {} unsafe impl Sync for ArcUnion {} - impl PartialEq for ArcUnion { fn eq(&self, other: &Self) -> bool { use ArcUnionBorrow::*; @@ -1104,16 +1105,12 @@ impl ArcUnion { /// Creates an `ArcUnion` from an instance of the first type. pub fn from_first(other: Arc) -> Self { - unsafe { - Self::new(Arc::into_raw(other) as *mut _) - } + unsafe { Self::new(Arc::into_raw(other) as *mut _) } } /// Creates an `ArcUnion` from an instance of the second type. pub fn from_second(other: Arc) -> Self { - unsafe { - Self::new(((Arc::into_raw(other) as usize) | 0x1) as *mut _) - } + unsafe { Self::new(((Arc::into_raw(other) as usize) | 0x1) as *mut _) } } /// Returns true if this `ArcUnion` contains the first type. diff --git a/components/style/properties/cascade.rs b/components/style/properties/cascade.rs index cb8a4ae7c39..af5273e0a73 100644 --- a/components/style/properties/cascade.rs +++ b/components/style/properties/cascade.rs @@ -13,7 +13,6 @@ use media_queries::Device; use properties::{ComputedValues, StyleBuilder}; use properties::{LonghandId, LonghandIdSet}; use properties::{PropertyDeclaration, PropertyDeclarationId, DeclarationImportanceIterator}; -use properties::{CSSWideKeyword, WideKeywordDeclaration}; use properties::CASCADE_PROPERTY; use rule_cache::{RuleCache, RuleCacheConditions}; use rule_tree::{CascadeLevel, StrongRuleNode}; @@ -790,6 +789,8 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> { self.seen.contains(LonghandId::MozMinFontSizeRatio) || self.seen.contains(LonghandId::FontFamily) { + use properties::{CSSWideKeyword, WideKeywordDeclaration}; + // font-size must be explicitly inherited to handle lang // changes and scriptlevel changes. // From 5ab81c42542992be77c9be095d916c5f9b1fba75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 18 Sep 2018 11:46:23 +0200 Subject: [PATCH 11/13] style: Make Servo build. StrongRuleNode is really Sync. --- components/style/rule_tree/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/components/style/rule_tree/mod.rs b/components/style/rule_tree/mod.rs index 662c303be18..ff8c1cc86de 100644 --- a/components/style/rule_tree/mod.rs +++ b/components/style/rule_tree/mod.rs @@ -938,8 +938,6 @@ impl MallocSizeOf for RuleNode { } } -// FIXME: use std::ptr::NonNull when Firefox requires Rust 1.25+ - #[derive(Clone)] struct WeakRuleNode { p: ptr::NonNull, @@ -952,6 +950,7 @@ pub struct StrongRuleNode { } unsafe impl Send for StrongRuleNode {} +unsafe impl Sync for StrongRuleNode {} #[cfg(feature = "servo")] malloc_size_of_is_0!(StrongRuleNode); From 60e28c61e5c892d75db3a6ae90f32b8c0fe44ca4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 18 Sep 2018 11:47:25 +0200 Subject: [PATCH 12/13] Fix Servo build. --- components/layout_thread/dom_wrapper.rs | 8 ++++++-- components/script/dom/element.rs | 5 ++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/components/layout_thread/dom_wrapper.rs b/components/layout_thread/dom_wrapper.rs index 8fecbdbf53a..5900b912e73 100644 --- a/components/layout_thread/dom_wrapper.rs +++ b/components/layout_thread/dom_wrapper.rs @@ -685,7 +685,9 @@ impl<'le> ::selectors::Element for ServoLayoutElement<'le> { type Impl = SelectorImpl; fn opaque(&self) -> ::selectors::OpaqueElement { - ::selectors::OpaqueElement::new(self.as_node().opaque().0 as *const ()) + ::selectors::OpaqueElement::new(unsafe { + &*(self.as_node().opaque().0 as *const ()) + }) } fn parent_element(&self) -> Option> { @@ -1258,7 +1260,9 @@ impl<'le> ::selectors::Element for ServoThreadSafeLayoutElement<'le> { type Impl = SelectorImpl; fn opaque(&self) -> ::selectors::OpaqueElement { - ::selectors::OpaqueElement::new(self.as_node().opaque().0 as *const ()) + ::selectors::OpaqueElement::new(unsafe { + &*(self.as_node().opaque().0 as *const ()) + }) } fn parent_element(&self) -> Option { diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index af24036efd2..2101b1e6a7f 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -2616,8 +2616,11 @@ impl VirtualMethods for Element { impl<'a> SelectorsElement for DomRoot { type Impl = SelectorImpl; + #[allow(unsafe_code)] fn opaque(&self) -> ::selectors::OpaqueElement { - ::selectors::OpaqueElement::new(self.reflector().get_jsobject().get()) + ::selectors::OpaqueElement::new(unsafe { + &*self.reflector().get_jsobject().get() + }) } fn parent_element(&self) -> Option> { From c7fc80c5a37dac827cd660cad82ebb0d33215375 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 18 Sep 2018 12:01:31 +0200 Subject: [PATCH 13/13] Fix unit test build. --- tests/unit/style/custom_properties.rs | 4 ++-- tests/unit/style/stylesheets.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unit/style/custom_properties.rs b/tests/unit/style/custom_properties.rs index 7a2741633c8..dfb0c825627 100644 --- a/tests/unit/style/custom_properties.rs +++ b/tests/unit/style/custom_properties.rs @@ -5,7 +5,7 @@ use cssparser::{Parser, ParserInput}; use servo_arc::Arc; use style::custom_properties::{Name, SpecifiedValue, CustomPropertiesMap, CustomPropertiesBuilder}; -use style::properties::DeclaredValue; +use style::properties::CustomDeclarationValue; use test::{self, Bencher}; fn cascade( @@ -21,7 +21,7 @@ fn cascade( let mut builder = CustomPropertiesBuilder::new(inherited); for &(ref name, ref val) in &values { - builder.cascade(name, DeclaredValue::Value(val)); + builder.cascade(name, &CustomDeclarationValue::Value(val.clone())); } builder.build() diff --git a/tests/unit/style/stylesheets.rs b/tests/unit/style/stylesheets.rs index 824c7cab417..0726eeab1e7 100644 --- a/tests/unit/style/stylesheets.rs +++ b/tests/unit/style/stylesheets.rs @@ -18,7 +18,7 @@ use style::context::QuirksMode; use style::error_reporting::{ParseErrorReporter, ContextualParseError}; use style::media_queries::MediaList; use style::properties::{CSSWideKeyword, CustomDeclaration}; -use style::properties::{DeclaredValueOwned, Importance}; +use style::properties::{CustomDeclarationValue, Importance}; use style::properties::{PropertyDeclaration, PropertyDeclarationBlock}; use style::properties::longhands::{self, animation_timing_function}; use style::shared_lock::SharedRwLock; @@ -113,7 +113,7 @@ fn test_parse_stylesheet() { ( PropertyDeclaration::Custom(CustomDeclaration { name: Atom::from("a"), - value: DeclaredValueOwned::CSSWideKeyword(CSSWideKeyword::Inherit), + value: CustomDeclarationValue::CSSWideKeyword(CSSWideKeyword::Inherit), }), Importance::Important, ),