Changing update_inline_style to process multiple declarations at once to handle shorthand serialization better

components/script/dom/cssstyledeclaration.rs

@@ -157,7 +157,9 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
                     self.0.next().map(|r| &**r)
                 }
             }
-            let serialized_value = shorthand.serialize_shorthand_to_string(Map(list.iter()));
+
+            // TODO: important is hardcoded to false because method does not implement it yet
+            let serialized_value = shorthand.serialize_shorthand_value_to_string(Map(list.iter()), false);
             return DOMString::from(serialized_value);
         }
 
@@ -241,10 +243,8 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
         let element = self.owner.upcast::<Element>();
 
         // Step 8
-        for decl in declarations {
-            // Step 9
-            element.update_inline_style(decl, priority);
-        }
+        // Step 9
+        element.update_inline_style(declarations, priority);
 
         Ok(())
     }

components/script/dom/element.rs

@@ -748,37 +748,45 @@ impl Element {
     }
 
     pub fn update_inline_style(&self,
-                               property_decl: PropertyDeclaration,
+                               declarations: Vec<PropertyDeclaration>,
                                style_priority: StylePriority) {
 
-        fn update(element: &Element, property_decl: PropertyDeclaration, style_priority: StylePriority) {
+        fn update(element: &Element, mut declarations: Vec<PropertyDeclaration>, style_priority: StylePriority) {
             let mut inline_declarations = element.style_attribute().borrow_mut();
-            if let &mut Some(ref mut declarations) = &mut *inline_declarations {
+            if let &mut Some(ref mut existing_declarations) = &mut *inline_declarations {
                 let existing_declarations = if style_priority == StylePriority::Important {
-                    &mut declarations.important
+                    &mut existing_declarations.important
                 } else {
-                    &mut declarations.normal
+                    &mut existing_declarations.normal
                 };
 
                 // Usually, the reference count will be 1 here. But transitions could make it greater
                 // than that.
                 let existing_declarations = Arc::make_mut(existing_declarations);
-                for declaration in &mut *existing_declarations {
-                    if declaration.name() == property_decl.name() {
-                        *declaration = property_decl;
-                        return;
+
+                while let Some(mut incoming_declaration) = declarations.pop() {
+                    let mut replaced = false;
+                    for existing_declaration in &mut *existing_declarations {
+                        if existing_declaration.name() == incoming_declaration.name() {
+                            mem::swap(existing_declaration, &mut incoming_declaration);
+                            replaced = true;
+                            break;
+                        }
+                    }
+
+                    if !replaced {
+                        // inserting instead of pushing since the declarations are in reverse order
+                        existing_declarations.insert(0, incoming_declaration);
                     }
                 }
 
-                // inserting instead of pushing since the declarations are in reverse order
-                existing_declarations.insert(0, property_decl);
                 return;
             }
 
             let (important, normal) = if style_priority == StylePriority::Important {
-                (vec![property_decl], vec![])
+                (declarations, vec![])
             } else {
-                (vec![], vec![property_decl])
+                (vec![], declarations)
             };
 
             *inline_declarations = Some(PropertyDeclarationBlock {
@@ -787,7 +795,7 @@ impl Element {
             });
         }
 
-        update(self, property_decl, style_priority);
+        update(self, declarations, style_priority);
         self.sync_property_with_attrs_style();
     }
 

components/style/properties/properties.mako.rs

@@ -404,7 +404,55 @@ enum AppendableValue<'a, I>
 where I: Iterator<Item=&'a PropertyDeclaration> {
     Declaration(&'a PropertyDeclaration),
     DeclarationsForShorthand(I),
-    Css(&'a str)
+    Css(&'a str, bool)
+}
+
+fn append_property_name<W>(dest: &mut W,
+                           property_name: &str,
+                           is_first_serialization: &mut bool)
+                           -> fmt::Result where W: fmt::Write {
+
+    // after first serialization(key: value;) add whitespace between the pairs
+    if !*is_first_serialization {
+        try!(write!(dest, " "));
+    }
+    else {
+        *is_first_serialization = false;
+    }
+
+    write!(dest, "{}", property_name)
+}
+
+fn append_declaration_value<'a, W, I>
+                           (dest: &mut W,
+                            appendable_value: AppendableValue<'a, I>,
+                            is_important: bool)
+                            -> fmt::Result
+                            where W: fmt::Write, I: Iterator<Item=&'a PropertyDeclaration> {
+  match appendable_value {
+      AppendableValue::Css(css, _) => {
+          try!(write!(dest, "{}", css))
+      },
+      AppendableValue::Declaration(decl) => {
+          try!(decl.to_css(dest));
+       },
+       AppendableValue::DeclarationsForShorthand(decls) => {
+           let mut decls = decls.peekable();
+           while let Some(decl) = decls.next() {
+               try!(decl.to_css(dest));
+
+               if decls.peek().is_some() {
+                   try!(write!(dest, " "));
+               }
+           }
+       }
+  }
+
+  if is_important {
+      try!(write!(dest, " !important"));
+  }
+
+  Ok(())
 }
 
 fn append_serialization<'a, W, I>(dest: &mut W,
@@ -415,38 +463,26 @@ fn append_serialization<'a, W, I>(dest: &mut W,
                                   -> fmt::Result
                                   where W: fmt::Write, I: Iterator<Item=&'a PropertyDeclaration> {
 
-    // after first serialization(key: value;) add whitespace between the pairs
-    if !*is_first_serialization {
-        try!(write!(dest, " "));
-    }
-    else {
-        *is_first_serialization = false;
-    }
+    try!(append_property_name(dest, property_name, is_first_serialization));
+    try!(write!(dest, ":"));
 
-    try!(write!(dest, "{}:", property_name));
-
-    match appendable_value {
-        AppendableValue::Css(css) => try!(write!(dest, " {}", css)),
-        AppendableValue::Declaration(decl) => {
+    // for normal parsed values, add a space between key: and value
+    match &appendable_value {
+        &AppendableValue::Css(_, is_unparsed) => {
+            if !is_unparsed {
+                try!(write!(dest, " "))
+            }
+        },
+        &AppendableValue::Declaration(decl) => {
             if !decl.value_is_unparsed() {
                 // for normal parsed values, add a space between key: and value
                 try!(write!(dest, " "));
             }
-
-            try!(decl.to_css(dest));
          },
-         AppendableValue::DeclarationsForShorthand(decls) => {
-             for decl in decls {
-                 try!(write!(dest, " "));
-                 try!(decl.to_css(dest));
-             }
-         }
-    }
-
-    if is_important {
-        try!(write!(dest, " !important"));
+         &AppendableValue::DeclarationsForShorthand(_) => try!(write!(dest, " "))
     }
 
+    try!(append_declaration_value(dest, appendable_value, is_important));
     write!(dest, ";")
 }
 
@@ -622,15 +658,15 @@ impl Shorthand {
     }
 
     /// Serializes possible shorthand value to String.
-    pub fn serialize_shorthand_to_string<'a, I>(self, declarations: I) -> String
+    pub fn serialize_shorthand_value_to_string<'a, I>(self, declarations: I, is_important: bool) -> String
     where I: Iterator<Item=&'a PropertyDeclaration> + Clone {
+        let appendable_value = self.get_shorthand_appendable_value(declarations).unwrap();
         let mut result = String::new();
-        self.serialize_shorthand_to_buffer(&mut result, declarations, &mut true).unwrap();
+        append_declaration_value(&mut result, appendable_value, is_important).unwrap();
         result
     }
 
-
-    /// Serializes possible shorthand value to input buffer given a list of longhand declarations.
+    /// Serializes possible shorthand name with value to input buffer given a list of longhand declarations.
     /// On success, returns true if shorthand value is written and false if no shorthand value is present.
     pub fn serialize_shorthand_to_buffer<'a, W, I>(self,
                                                    dest: &mut W,
@@ -638,47 +674,53 @@ impl Shorthand {
                                                    is_first_serialization: &mut bool)
                                                    -> Result<bool, fmt::Error>
     where W: Write, I: Iterator<Item=&'a PropertyDeclaration> + Clone {
+        match self.get_shorthand_appendable_value(declarations) {
+            None => Ok(false),
+            Some(appendable_value) => {
+                let property_name = self.name();
 
-        // Only cloning iterators (a few pointers each) not declarations.
-        let mut declarations2 = declarations.clone();
-        let mut declarations3 = declarations.clone();
-
-        let first_declaration = match declarations2.next() {
-            Some(declaration) => declaration,
-            None => return Ok(false)
-        };
-
-        let property_name = self.name();
-
-        // https://drafts.csswg.org/css-variables/#variables-in-shorthands
-        if let Some(css) = first_declaration.with_variables_from_shorthand(self) {
-            if declarations2.all(|d| d.with_variables_from_shorthand(self) == Some(css)) {
-               return append_serialization::<W, I>(
-                   dest, property_name, AppendableValue::Css(css), false, is_first_serialization
-               ).and_then(|_| Ok(true));
-           }
-           else {
-               return Ok(false);
-           }
-        }
-
-        if !declarations3.any(|d| d.with_variables()) {
-            try!(
                 append_serialization(
                     dest,
                     property_name,
-                    AppendableValue::DeclarationsForShorthand(declarations),
+                    appendable_value,
                     false,
                     is_first_serialization
-                )
-            );
-            // FIXME: this needs property-specific code, which probably should be in style/
-            // "as appropriate according to the grammar of shorthand "
-            // https://drafts.csswg.org/cssom/#serialize-a-css-value
-            return Ok(true);
+                ).and_then(|_| Ok(true))
+            }
         }
+    }
 
-        Ok(false)
+    fn get_shorthand_appendable_value<'a, I>(self, declarations: I) -> Option<AppendableValue<'a, I>>
+        where I: Iterator<Item=&'a PropertyDeclaration> + Clone {
+
+            // Only cloning iterators (a few pointers each) not declarations.
+            let mut declarations2 = declarations.clone();
+            let mut declarations3 = declarations.clone();
+
+            let first_declaration = match declarations2.next() {
+                Some(declaration) => declaration,
+                None => return None
+            };
+
+            // https://drafts.csswg.org/css-variables/#variables-in-shorthands
+            if let Some(css) = first_declaration.with_variables_from_shorthand(self) {
+                if declarations2.all(|d| d.with_variables_from_shorthand(self) == Some(css)) {
+                   let is_unparsed = first_declaration.value_is_unparsed();
+                   return Some(AppendableValue::Css(css, is_unparsed));
+               }
+               else {
+                   return None;
+               }
+            }
+
+            if !declarations3.any(|d| d.with_variables()) {
+                return Some(AppendableValue::DeclarationsForShorthand(declarations));
+                // FIXME: this needs property-specific code, which probably should be in style/
+                // "as appropriate according to the grammar of shorthand "
+                // https://drafts.csswg.org/cssom/#serialize-a-css-value
+            }
+
+            None
     }
 }