diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 03485db9bf0..6ef2c0e73ea 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -761,20 +761,46 @@ impl PropertyDeclarationBlock { /// /// https://drafts.csswg.org/cssom/#serialize-a-css-declaration-block pub fn to_css(&self, dest: &mut CssStringWriter) -> fmt::Result { + use std::iter::Cloned; + use std::slice; + let mut is_first_serialization = true; // trailing serializations should have a prepended space // Step 1 -> dest = result list // Step 2 - let mut already_serialized = PropertyDeclarationIdSet::new(); + // + // NOTE(emilio): We reuse this set for both longhands and shorthands + // with subtly different meaning. For longhands, only longhands that + // have actually been serialized (either by themselves, or as part of a + // shorthand) appear here. For shorthands, all the shorthands that we've + // attempted to serialize appear here. + let mut already_serialized = NonCustomPropertyIdSet::new(); // Step 3 for (declaration, importance) in self.declaration_importance_iter() { // Step 3.1 let property = declaration.id(); + let longhand_id = match property { + PropertyDeclarationId::Longhand(id) => id, + PropertyDeclarationId::Custom(..) => { + // Given the invariants that there are no duplicate + // properties in a declaration block, and that custom + // properties can't be part of a shorthand, we can just care + // about them here. + append_serialization::>, _>( + dest, + &property, + AppendableValue::Declaration(declaration), + importance, + &mut is_first_serialization, + )?; + continue; + } + }; // Step 3.2 - if already_serialized.contains(property) { + if already_serialized.contains(longhand_id.into()) { continue; } @@ -783,7 +809,13 @@ impl PropertyDeclarationBlock { // iterating below. // Step 3.3.2 - for &shorthand in declaration.shorthands() { + for &shorthand in longhand_id.shorthands() { + // We already attempted to serialize this shorthand before. + if already_serialized.contains(shorthand.into()) { + continue; + } + already_serialized.insert(shorthand.into()); + // Substep 2 & 3 let mut current_longhands = SmallVec::<[_; 10]>::new(); let mut important_count = 0; @@ -792,8 +824,19 @@ impl PropertyDeclarationBlock { let is_system_font = shorthand == ShorthandId::Font && self.declarations.iter().any(|l| { - !already_serialized.contains(l.id()) && - l.get_system().is_some() + match l.id() { + PropertyDeclarationId::Longhand(id) => { + if already_serialized.contains(id.into()) { + return false; + } + + l.get_system().is_some() + } + PropertyDeclarationId::Custom(..) => { + debug_assert!(l.get_system().is_none()); + false + } + } }); if is_system_font { @@ -885,7 +928,7 @@ impl PropertyDeclarationBlock { }; // Substeps 7 and 8 - append_serialization::>, _>( + append_serialization::>, _>( dest, &shorthand, value, @@ -894,8 +937,13 @@ impl PropertyDeclarationBlock { )?; for current_longhand in ¤t_longhands { + let longhand_id = match current_longhand.id() { + PropertyDeclarationId::Longhand(id) => id, + PropertyDeclarationId::Custom(..) => unreachable!(), + }; + // Substep 9 - already_serialized.insert(current_longhand.id()); + already_serialized.insert(longhand_id.into()); } // FIXME(https://github.com/w3c/csswg-drafts/issues/1774) @@ -907,13 +955,10 @@ impl PropertyDeclarationBlock { } // Step 3.3.4 - if already_serialized.contains(property) { + if already_serialized.contains(longhand_id.into()) { continue; } - use std::iter::Cloned; - use std::slice; - // Steps 3.3.5, 3.3.6 & 3.3.7 // Need to specify an iterator type here even though it’s unused to work around // "error: unable to infer enough type information about `_`; @@ -924,10 +969,11 @@ impl PropertyDeclarationBlock { &property, AppendableValue::Declaration(declaration), importance, - &mut is_first_serialization)?; + &mut is_first_serialization, + )?; // Step 3.3.8 - already_serialized.insert(property); + already_serialized.insert(longhand_id.into()); } // Step 4 diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 0b3112006e9..6ae1bead5ee 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -547,6 +547,20 @@ pub struct NonCustomPropertyIdSet { } impl NonCustomPropertyIdSet { + /// Creates an empty `NonCustomPropertyIdSet`. + pub fn new() -> Self { + Self { + storage: Default::default(), + } + } + + /// Insert a non-custom-property in the set. + #[inline] + pub fn insert(&mut self, id: NonCustomPropertyId) { + let bit = id.0; + self.storage[bit / 32] |= 1 << (bit % 32); + } + /// Return whether the given property is in the set #[inline] pub fn contains(&self, id: NonCustomPropertyId) -> bool { @@ -688,62 +702,6 @@ impl LonghandIdSet { } } -/// A specialized set of PropertyDeclarationId -pub struct PropertyDeclarationIdSet { - longhands: LonghandIdSet, - - // FIXME: Use a HashSet instead? This Vec is usually small, so linear scan might be ok. - custom: Vec<::custom_properties::Name>, -} - -impl PropertyDeclarationIdSet { - /// Empty set - pub fn new() -> Self { - PropertyDeclarationIdSet { - longhands: LonghandIdSet::new(), - custom: Vec::new(), - } - } - - /// Returns all the longhands that this set contains. - pub fn longhands(&self) -> &LonghandIdSet { - &self.longhands - } - - /// Returns whether the set is empty. - #[inline] - pub fn is_empty(&self) -> bool { - self.longhands.is_empty() && self.custom.is_empty() - } - - /// Clears the set. - #[inline] - pub fn clear(&mut self) { - self.longhands.clear(); - self.custom.clear(); - } - - /// Returns whether the given ID is in the set - pub fn contains(&mut self, id: PropertyDeclarationId) -> bool { - match id { - PropertyDeclarationId::Longhand(id) => self.longhands.contains(id), - PropertyDeclarationId::Custom(name) => self.custom.contains(name), - } - } - - /// Insert the given ID in the set - pub fn insert(&mut self, id: PropertyDeclarationId) { - match id { - PropertyDeclarationId::Longhand(id) => self.longhands.insert(id), - PropertyDeclarationId::Custom(name) => { - if !self.custom.contains(name) { - self.custom.push(name.clone()) - } - } - } - } -} - /// An enum to represent a CSS Wide keyword. #[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, ToCss)] pub enum CSSWideKeyword { diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 18bb3827031..332193c1817 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -3797,24 +3797,23 @@ struct PropertyAndIndex { } struct PrioritizedPropertyIter<'a> { - properties: &'a nsTArray, + properties: &'a [PropertyValuePair], sorted_property_indices: Vec, curr: usize, } impl<'a> PrioritizedPropertyIter<'a> { - pub fn new(properties: &'a nsTArray) -> PrioritizedPropertyIter { - // If we fail to convert a nsCSSPropertyID into a PropertyId we shouldn't fail outright - // but instead by treating that property as the 'all' property we make it sort last. - let all = PropertyId::Shorthand(ShorthandId::All); - + fn new(properties: &'a [PropertyValuePair]) -> PrioritizedPropertyIter { + // If we fail to convert a nsCSSPropertyID into a PropertyId we + // shouldn't fail outright but instead by treating that property as the + // 'all' property we make it sort last. let mut sorted_property_indices: Vec = properties.iter().enumerate().map(|(index, pair)| { - PropertyAndIndex { - property: PropertyId::from_nscsspropertyid(pair.mProperty) - .unwrap_or(all.clone()), - index, - } + let property = + PropertyId::from_nscsspropertyid(pair.mProperty) + .unwrap_or(PropertyId::Shorthand(ShorthandId::All)); + + PropertyAndIndex { property, index } }).collect(); sorted_property_indices.sort_by(|a, b| compare_property_priority(&a.property, &b.property)); diff --git a/tests/unit/style/properties/serialization.rs b/tests/unit/style/properties/serialization.rs index 89ab8c8ea6d..8f4813efab9 100644 --- a/tests/unit/style/properties/serialization.rs +++ b/tests/unit/style/properties/serialization.rs @@ -559,22 +559,6 @@ mod shorthand_serialization { } } - #[test] - fn columns_should_serialize_correctly() { - use style::values::{Auto, Either}; - - let mut properties = Vec::new(); - - let width = Either::Second(Auto); - let count = Either::Second(Auto); - - properties.push(PropertyDeclaration::ColumnWidth(width)); - properties.push(PropertyDeclaration::ColumnCount(count)); - - let serialization = shorthand_properties_to_string(properties); - assert_eq!(serialization, "columns: auto auto;"); - } - #[test] fn flex_should_serialize_all_available_properties() { use style::values::specified::{NonNegativeNumber, Percentage};