Auto merge of #20081 - emilio:more-longhand-stuff, r=nox

style: More serialization tweaks.

This still doesn't fix everything. In particular, we need to check whether the
subproperty will be enabled in Longhands and LonghandsToSerialize too.

I haven't decided yet on what's the best way to do that.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20081)
<!-- Reviewable:end -->

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::<Cloned<slice::Iter<_>>, _>(
+                        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()) &&
+                        match l.id() {
-                        l.get_system().is_some()
+                            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::<Cloned<slice::Iter< _>>, _>(
+                append_serialization::<Cloned<slice::Iter<_>>, _>(
                     dest,
                     &shorthand,
                     value,
@@ -894,8 +937,13 @@ impl PropertyDeclarationBlock {
                 )?;
 
                 for current_longhand in &current_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

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 {

ports/geckolib/glue.rs

@@ -3797,24 +3797,23 @@ struct PropertyAndIndex {
 }
 
 struct PrioritizedPropertyIter<'a> {
-    properties: &'a nsTArray<PropertyValuePair>,
+    properties: &'a [PropertyValuePair],
     sorted_property_indices: Vec<PropertyAndIndex>,
     curr: usize,
 }
 
 impl<'a> PrioritizedPropertyIter<'a> {
-    pub fn new(properties: &'a nsTArray<PropertyValuePair>) -> PrioritizedPropertyIter {
+    fn new(properties: &'a [PropertyValuePair]) -> PrioritizedPropertyIter {
-        // If we fail to convert a nsCSSPropertyID into a PropertyId we shouldn't fail outright
+        // If we fail to convert a nsCSSPropertyID into a PropertyId we
-        // but instead by treating that property as the 'all' property we make it sort last.
+        // shouldn't fail outright but instead by treating that property as the
-        let all = PropertyId::Shorthand(ShorthandId::All);
+        // 'all' property we make it sort last.
-
         let mut sorted_property_indices: Vec<PropertyAndIndex> =
             properties.iter().enumerate().map(|(index, pair)| {
-                PropertyAndIndex {
+                let property =
-                    property: PropertyId::from_nscsspropertyid(pair.mProperty)
+                    PropertyId::from_nscsspropertyid(pair.mProperty)
-                              .unwrap_or(all.clone()),
+                        .unwrap_or(PropertyId::Shorthand(ShorthandId::All));
-                    index,
+
-                }
+                PropertyAndIndex { property, index }
             }).collect();
         sorted_property_indices.sort_by(|a, b| compare_property_priority(&a.property, &b.property));
 

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};