From 4c979a8346600c98922f58892a312a9afdfd48a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 20 Feb 2018 13:54:41 +0100 Subject: [PATCH 1/5] style: Remove a unit test that relies on disabled-by-default properties. --- tests/unit/style/properties/serialization.rs | 16 ---------------- 1 file changed, 16 deletions(-) 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}; From c09257b54051f12f6406287a6e2b8d084737b0f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 20 Feb 2018 11:06:51 +0100 Subject: [PATCH 2/5] style: There's no need to check the serialized custom properties. Given they're not duplicated, and can't be part of a shorthand. --- .../style/properties/declaration_block.rs | 57 +++++++++++++++---- .../style/properties/properties.mako.rs | 56 ------------------ 2 files changed, 46 insertions(+), 67 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 03485db9bf0..278f616422c 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -761,20 +761,40 @@ 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(); + let mut already_serialized_longhands = LonghandIdSet::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_longhands.contains(longhand_id) { continue; } @@ -783,7 +803,7 @@ impl PropertyDeclarationBlock { // iterating below. // Step 3.3.2 - for &shorthand in declaration.shorthands() { + for &shorthand in longhand_id.shorthands() { // Substep 2 & 3 let mut current_longhands = SmallVec::<[_; 10]>::new(); let mut important_count = 0; @@ -792,8 +812,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_longhands.contains(id) { + return false; + } + + l.get_system().is_some() + } + PropertyDeclarationId::Custom(..) => { + debug_assert!(l.get_system().is_none()); + false + } + } }); if is_system_font { @@ -894,8 +925,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_longhands.insert(longhand_id); } // FIXME(https://github.com/w3c/csswg-drafts/issues/1774) @@ -907,12 +943,10 @@ impl PropertyDeclarationBlock { } // Step 3.3.4 - if already_serialized.contains(property) { + if already_serialized_longhands.contains(longhand_id) { 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 @@ -924,10 +958,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_longhands.insert(longhand_id); } // Step 4 diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs index 0b3112006e9..c7fdf29d858 100644 --- a/components/style/properties/properties.mako.rs +++ b/components/style/properties/properties.mako.rs @@ -688,62 +688,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 { From 1359c7c2184787b25e0300f2ebd8d472c4211ea3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 20 Feb 2018 11:16:12 +0100 Subject: [PATCH 3/5] style: Don't try to serialize the same shorthand over and over. I looked at what were we doing in that loop, and we're doing tons of dumb stuff. In particular, we try to serialize the "all" shorthand all the time. This patch prevents us from trying to serialize shorthands that we've already tried to serialize. --- .../style/properties/declaration_block.rs | 25 +++++++++++++------ .../style/properties/properties.mako.rs | 14 +++++++++++ 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index 278f616422c..baf68d071b0 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -769,7 +769,13 @@ impl PropertyDeclarationBlock { // Step 1 -> dest = result list // Step 2 - let mut already_serialized_longhands = LonghandIdSet::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() { @@ -794,7 +800,7 @@ impl PropertyDeclarationBlock { }; // Step 3.2 - if already_serialized_longhands.contains(longhand_id) { + if already_serialized.contains(longhand_id.into()) { continue; } @@ -804,6 +810,12 @@ impl PropertyDeclarationBlock { // Step 3.3.2 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; @@ -814,7 +826,7 @@ impl PropertyDeclarationBlock { self.declarations.iter().any(|l| { match l.id() { PropertyDeclarationId::Longhand(id) => { - if already_serialized_longhands.contains(id) { + if already_serialized.contains(id.into()) { return false; } @@ -931,7 +943,7 @@ impl PropertyDeclarationBlock { }; // Substep 9 - already_serialized_longhands.insert(longhand_id); + already_serialized.insert(longhand_id.into()); } // FIXME(https://github.com/w3c/csswg-drafts/issues/1774) @@ -943,11 +955,10 @@ impl PropertyDeclarationBlock { } // Step 3.3.4 - if already_serialized_longhands.contains(longhand_id) { + if already_serialized.contains(longhand_id.into()) { continue; } - // 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 `_`; @@ -962,7 +973,7 @@ impl PropertyDeclarationBlock { )?; // Step 3.3.8 - already_serialized_longhands.insert(longhand_id); + 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 c7fdf29d858..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 { From 0b1f78275cb1722d595bacef87e2b2a3f8329f86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 20 Feb 2018 14:08:28 +0100 Subject: [PATCH 4/5] style: Remove unneeded whitespace. I bet it was added when this code was part of a mako file. --- components/style/properties/declaration_block.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs index baf68d071b0..6ef2c0e73ea 100644 --- a/components/style/properties/declaration_block.rs +++ b/components/style/properties/declaration_block.rs @@ -788,7 +788,7 @@ impl PropertyDeclarationBlock { // 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::>, _>( + append_serialization::>, _>( dest, &property, AppendableValue::Declaration(declaration), @@ -928,7 +928,7 @@ impl PropertyDeclarationBlock { }; // Substeps 7 and 8 - append_serialization::>, _>( + append_serialization::>, _>( dest, &shorthand, value, From a4c7728062972a804bdb2e3c1f730a7ac624540a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 20 Feb 2018 15:36:43 +0100 Subject: [PATCH 5/5] style: Cleanup PrioritizedPropertyIter. --- ports/geckolib/glue.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) 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));