Making fixes based on suggestions from nox and SimonSapin

components/script/dom/element.rs

@@ -697,11 +697,12 @@ impl Element {
     }
 
     fn sync_property_with_attrs_style(&self) {
-        let mut style_str = String::new();
-
-        if let &Some(ref declarations) = &*self.style_attribute().borrow() {
-            style_str.push_str(&declarations.serialize());
+        let style_str = if let &Some(ref declarations) = &*self.style_attribute().borrow() {
+            declarations.serialize()
         }
+        else {
+            String::new()
+        };
 
         let new_style = AttrValue::String(DOMString::from_string(style_str));
 

components/style/properties/properties.mako.rs

@@ -248,10 +248,9 @@ mod property_bit_field {
 
 /// Declarations are stored in reverse order.
 /// Overridden declarations are skipped.
-// TODO: Because normal & important are stored in seperate lists, it is impossible to know their
-// exact declaration order. They should be changed into one list, adding an important/normal
-// flag to PropertyDeclaration
 
+
+// FIXME (https://github.com/servo/servo/issues/3426)
 #[derive(Debug, PartialEq, HeapSizeOf)]
 pub struct PropertyDeclarationBlock {
     #[ignore_heap_size_of = "#7038"]
@@ -267,30 +266,26 @@ impl PropertyDeclarationBlock {
         let mut result_list = String::new();
 
         // Step 2
-        let mut already_serialized = HashSet::new();
+        let mut already_serialized = Vec::new();
 
         // Step 3
         // restore order of declarations since PropertyDeclarationBlock is stored in reverse order
-        let declarations = self.normal.iter().rev().chain(self.important.iter().rev()).collect::<Vec<_>>();
-
-
-        for declaration in &declarations {
+        for declaration in self.important.iter().chain(self.normal.iter()).rev() {
             // Step 3.1
-            let property = declaration.name().to_string();
+            let property = declaration.name();
 
             // Step 3.2
             if already_serialized.contains(&property) {
                 continue;
             }
 
             // Step 3.3
-            let mut shorthands = Vec::from(declaration.shorthands());
-            if shorthands.len() > 0 {
-                shorthands.sort_by(|a,b| a.longhands().len().cmp(&b.longhands().len()));
+            let shorthands = declaration.shorthands();
+            if !shorthands.is_empty() {
 
                 // Step 3.3.1
-                let mut longhands = declarations.iter().cloned()
-                .filter(|d| !already_serialized.contains(&d.name().to_string()))
+                let mut longhands = self.important.iter().chain(self.normal.iter()).rev()
+                    .filter(|d| !already_serialized.contains(&d.name()))
                     .collect::<Vec<_>>();
 
                 // Step 3.3.2
@@ -299,22 +294,16 @@ impl PropertyDeclarationBlock {
 
                     // Substep 2 & 3
                     let mut current_longhands = Vec::new();
-                    let mut missing_properties: HashSet<_> = HashSet::from_iter(
-                        properties.iter().map(|&s| s.to_owned())
-                    );
 
-                    for longhand in longhands.iter().cloned() {
-                        let longhand_string = longhand.name().to_string();
-                        if !properties.contains(&&*longhand_string) {
-                            continue;
+                    for longhand in longhands.iter() {
+                        let longhand_name = longhand.name();
+                        if properties.iter().any(|p| &longhand_name == *p) {
+                            current_longhands.push(*longhand);
                         }
-
-                        missing_properties.remove(&longhand_string);
-                        current_longhands.push(longhand);
                     }
 
                     // Substep 1
-                    if current_longhands.len() == 0 || missing_properties.len() > 0 {
+                    if current_longhands.is_empty() || current_longhands.len() != properties.len() {
                         continue;
                     }
 
@@ -338,11 +327,11 @@ impl PropertyDeclarationBlock {
                     }
 
                     // Substep 7 & 8
-                    result_list.push_str(&format!("{}: {}; ", &shorthand.to_name(), value));
+                    result_list.push_str(&format!("{}: {}; ", &shorthand.name(), value));
 
                     for current_longhand in current_longhands {
                         // Substep 9
-                        already_serialized.insert(current_longhand.name().to_string());
+                        already_serialized.push(current_longhand.name());
                         let index_to_remove = longhands.iter().position(|l| l == &current_longhand);
                         if let Some(index) = index_to_remove {
                             // Substep 10
@@ -360,13 +349,13 @@ impl PropertyDeclarationBlock {
             // Step 3.3.5
             let mut value = declaration.value();
             if self.important.contains(declaration) {
-                value.push_str(" ! important");
+                value.push_str(" !important");
             }
             // Steps 3.3.6 & 3.3.7
             result_list.push_str(&format!("{}: {}; ", &property, value));
 
             // Step 3.3.8
-            already_serialized.insert(property);
+            already_serialized.push(property);
         }
 
         result_list.pop(); // remove trailling whitespace
@@ -512,8 +501,6 @@ pub enum Shorthand {
     % endfor
 }
 
-use std::borrow::ToOwned;
-use std::iter::FromIterator;
 use util::str::str_join;
 
 impl Shorthand {
@@ -526,7 +513,7 @@ impl Shorthand {
         }
     }
 
-    pub fn to_name(&self) -> &str {
+    pub fn name(&self) -> &'static str {
         match *self {
             % for property in SHORTHANDS:
                 Shorthand::${property.camel_case} => "${property.name}",
@@ -827,7 +814,7 @@ impl PropertyDeclaration {
         }
     }
 
-    pub fn shorthands(&self) -> &[Shorthand] {
+    pub fn shorthands(&self) -> &'static [Shorthand] {
         // first generate longhand to shorthands lookup map
         <%
             longhand_to_shorthand_map = {}
@@ -855,7 +842,7 @@ impl PropertyDeclaration {
             % for property in LONGHANDS:
                 PropertyDeclaration::${property.camel_case}(_) => ${property.ident.upper()},
             % endfor
-            _ => &[] // include outlet for Custom enum value
+            PropertyDeclaration::Custom(_, _) => &[]
         }
     }
 }