servo: Merge #20081 - style: More serialization tweaks (from emilio:more-longhand-stuff); r=nox
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 20 Feb 2018 23:49:19 -0500
changeset 404653 01328de0be21dfeeef394b2b0f048fc8d6994e6c
parent 404652 f0e351b326220c91e8da3dc33398a9f895b43944
child 404654 76ab16daf148be1bc301874082940673be4e44b8
push id100058
push userrgurzau@mozilla.com
push dateWed, 21 Feb 2018 17:32:49 +0000
treeherdermozilla-inbound@fa4bede5ed1f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnox
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #20081 - style: More serialization tweaks (from emilio:more-longhand-stuff); r=nox 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. Source-Repo: https://github.com/servo/servo Source-Revision: 2c060eb81a8eab0fdcbf13231bf7703ea96bc657
servo/components/style/properties/declaration_block.rs
servo/components/style/properties/properties.mako.rs
servo/ports/geckolib/glue.rs
servo/tests/unit/style/properties/serialization.rs
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -756,49 +756,92 @@ impl PropertyDeclarationBlock {
 
 impl PropertyDeclarationBlock {
     /// Like the method on ToCss, but without the type parameter to avoid
     /// accidentally monomorphizing this large function multiple times for
     /// different writers.
     ///
     /// 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;
             }
 
             // Step 3.3
             // Step 3.3.1 is done by checking already_serialized while
             // 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;
                 let mut found_system = None;
 
                 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 {
                     for (longhand, importance) in self.declaration_importance_iter() {
                         if longhand.get_system().is_some() || longhand.is_default_line_height() {
                             current_longhands.push(longhand);
                             if found_system.is_none() {
                                found_system = longhand.get_system();
@@ -880,59 +923,62 @@ impl PropertyDeclarationBlock {
                         AppendableValue::Css {
                             css: CssStringBorrow::from(&v),
                             with_variables: false,
                         }
                     }
                 };
 
                 // Substeps 7 and 8
-                append_serialization::<Cloned<slice::Iter< _>>, _>(
+                append_serialization::<Cloned<slice::Iter<_>>, _>(
                     dest,
                     &shorthand,
                     value,
                     importance,
                     &mut is_first_serialization,
                 )?;
 
                 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)
                 // The specification does not include an instruction to abort
                 // the shorthand loop at this point, but doing so both matches
                 // Gecko and makes sense since shorthands are checked in
                 // preferred order.
                 break;
             }
 
             // 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 `_`;
             //  type annotations or generic parameter binding required [E0282]"
             // Use the same type as earlier call to reuse generated code.
             append_serialization::<Cloned<slice::Iter<_>>, _>(
                 dest,
                 &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
         Ok(())
     }
 }
 
 /// A convenient enum to represent different kinds of stuff that can represent a
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -542,16 +542,30 @@ impl From<AliasId> for NonCustomProperty
 
 /// A set of all properties
 #[derive(Clone, PartialEq)]
 pub struct NonCustomPropertyIdSet {
     storage: [u32; (${len(data.longhands) + len(data.shorthands) + len(data.all_aliases())} - 1 + 32) / 32]
 }
 
 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 {
         let bit = id.0;
         (self.storage[bit / 32] & (1 << (bit % 32))) != 0
     }
 }
 
@@ -683,72 +697,16 @@ impl LonghandIdSet {
 
     /// Returns whether the set is empty.
     #[inline]
     pub fn is_empty(&self) -> bool {
         self.storage.iter().all(|c| *c == 0)
     }
 }
 
-/// 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 {
     /// The `initial` keyword.
     Initial,
     /// The `inherit` keyword.
     Inherit,
     /// The `unset` keyword.
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -3792,34 +3792,33 @@ fn create_context_for_animation<'a>(
 }
 
 struct PropertyAndIndex {
     property: PropertyId,
     index: usize,
 }
 
 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 {
-        // 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<PropertyAndIndex> =
             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));
 
         PrioritizedPropertyIter {
             properties,
             sorted_property_indices,
             curr: 0,
         }
--- a/servo/tests/unit/style/properties/serialization.rs
+++ b/servo/tests/unit/style/properties/serialization.rs
@@ -555,32 +555,16 @@ mod shorthand_serialization {
             properties.push(PropertyDeclaration::OutlineColor(color));
 
             let serialization = shorthand_properties_to_string(properties);
             assert_eq!(serialization, "outline: 4px auto rgb(255, 0, 0);");
         }
     }
 
     #[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};
 
         let mut properties = Vec::new();
 
         let grow = NonNegativeNumber::new(2f32);
         let shrink = NonNegativeNumber::new(3f32);
         let basis =