Update PropertyDeclarationBlock::to_css so that the iteration of

possible shorthands matches the linked spec.

Previously substep 5 attempted to serialize the complete shorthand
declaration and substep 6 skipped to the next shorthand only if the
current shorthand was not serialized, but this did not catch empty
serializations. The spec on the other hand specifically says that the
*value* should be evaluated first and if the value is empty substep 6
should skip to the next shorthand - which is what happens now.

To do this required some refactoring which mostly simplifies the code.
Specifically:
 - append_declaration_value was refactored so that importance is not
   required as a arg (by moving it to the end of append_serialization)
   and is_overflow_with_name was removed as an arg also (initially I
   refactored it elsewhere, but it turns out it's no longer required at
   all - more below). With these changes, append_declaration_value can
   be used within the algorithm for to_css to obtain just the value for
   substep 5.
 - Substeps 7 and 8 of the algorithm become explicit (they were implicit
   before) by passing the value, shorthand and importance to
   append_serialization.
 - serialize_shorthand_to_buffer is no longer required (as the algorithm
   serializes the value first instead, as per the spec.

A surprising result of this was that I could also remove a lot of code
handling the special case of the overflow properties serialization. This
is because the overflow's LonghandToCss implementation of
to_css_declared already does the right thing according to the spec - it
writes the single value if both overflow-x and -y are equal, and
writes nothing otherwise - so that the algorithm now skips that shorthand
instead rendering the longhands.

components/style/properties/declaration_block.rs

@@ -128,10 +128,9 @@ impl PropertyDeclarationBlock {
                 // Step 1.2.3
                 // We don't print !important when serializing individual properties,
                 // so we treat this as a normal-importance property
-                let importance = Importance::Normal;
                 match shorthand.get_shorthand_appendable_value(list) {
                     Some(appendable_value) =>
-                        append_declaration_value(dest, appendable_value, importance, false),
+                        append_declaration_value(dest, appendable_value),
                     None => return Ok(()),
                 }
             }
@@ -343,23 +342,28 @@ impl ToCss for PropertyDeclarationBlock {
                         Importance::Normal
                     };
 
-                    // Substep 5
+                    // Substep 5 - Let value be the result of invoking serialize a CSS
-                    let was_serialized =
+                    // value of current longhands.
-                        try!(
+                    let mut value = String::from("");
-                            shorthand.serialize_shorthand_to_buffer(
+                    match shorthand.get_shorthand_appendable_value(current_longhands.iter().cloned()) {
-                                dest,
+                        None => continue,
-                                current_longhands.iter().cloned(),
+                        Some(appendable_value) => {
-                                &mut is_first_serialization,
+                            // Also demo test of overflow with important - I think it'll not
-                                importance
+                            // be correct.
-                            )
+                            try!(append_declaration_value(&mut value, appendable_value));
-                        );
+                        }
-                    // If serialization occured, Substep 7 & 8 will have been completed
+                    }
 
                     // Substep 6
-                    if !was_serialized {
+                    if value == "" {
-                        continue;
+                        continue
                     }
 
+                    // Substeps 7 and 8
+                    try!(append_serialization::<W, Cloned<slice::Iter< &PropertyDeclaration>>, _>(
+                             dest, &shorthand, AppendableValue::Css(&value), importance,
+                             &mut is_first_serialization));
+
                     for current_longhand in current_longhands {
                         // Substep 9
                         already_serialized.push(current_longhand.id());
@@ -434,31 +438,22 @@ fn handle_first_serialization<W>(dest: &mut W,
 
 /// Append a given kind of appendable value to a serialization.
 pub fn append_declaration_value<'a, W, I>(dest: &mut W,
-                                          appendable_value: AppendableValue<'a, I>,
+                                          appendable_value: AppendableValue<'a, I>)
-                                          importance: Importance,
-                                          is_overflow_with_name: bool)
                                           -> fmt::Result
     where W: fmt::Write,
           I: Iterator<Item=&'a PropertyDeclaration>,
 {
     match appendable_value {
+        // Should be able to use this enum to pass the already serialized css?
         AppendableValue::Css(css) => {
             try!(write!(dest, "{}", css))
         },
         AppendableValue::Declaration(decl) => {
             try!(decl.to_css(dest));
-         },
+        },
-         AppendableValue::DeclarationsForShorthand(shorthand, decls) => {
+        AppendableValue::DeclarationsForShorthand(shorthand, decls) => {
-            if is_overflow_with_name {
+            try!(shorthand.longhands_to_css(decls, dest));
-              try!(shorthand.overflow_longhands_to_css(decls, dest));
+        }
-            } else {
-              try!(shorthand.longhands_to_css(decls, dest));
-            }
-         }
-    }
-
-    if importance.important() {
-        try!(write!(dest, " !important"));
     }
 
     Ok(())
@@ -477,12 +472,6 @@ pub fn append_serialization<'a, W, I, N>(dest: &mut W,
 {
     try!(handle_first_serialization(dest, is_first_serialization));
 
-    // Overflow does not behave like a normal shorthand. When overflow-x and overflow-y are not of equal
-    // values, they no longer use the shared property name "overflow" and must be handled differently
-    if shorthands::is_overflow_shorthand(&appendable_value) {
-        return append_declaration_value(dest, appendable_value, importance, true);
-    }
-
     try!(property_name.to_css(dest));
     try!(dest.write_char(':'));
 
@@ -500,7 +489,12 @@ pub fn append_serialization<'a, W, I, N>(dest: &mut W,
          &AppendableValue::DeclarationsForShorthand(..) => try!(write!(dest, " "))
     }
 
-    try!(append_declaration_value(dest, appendable_value, importance, false));
+    try!(append_declaration_value(dest, appendable_value));
+
+    if importance.important() {
+        try!(write!(dest, " !important"));
+    }
+
     write!(dest, ";")
 }
 

components/style/properties/properties.mako.rs

@@ -13,7 +13,7 @@
 use std::borrow::Cow;
 use std::boxed::Box as StdBox;
 use std::collections::HashSet;
-use std::fmt::{self, Write};
+use std::fmt;
 use std::sync::Arc;
 
 use app_units::Au;
@@ -549,59 +549,10 @@ impl ShorthandId {
         }
     }
 
-    /// Overflow does not behave like a normal shorthand. When overflow-x and
+    /// Find an return an appendable value for the given declarations.
-    /// overflow-y are not of equal values, they no longer use the shared
-    /// property name "overflow".
     ///
-    /// We use this function as a special-case for that.
+    /// Returns the optional appendable value.
-    pub fn overflow_longhands_to_css<'a, W, I>(&self,
+    pub fn get_shorthand_appendable_value<'a, I>(self,
-                                               declarations: I,
-                                               dest: &mut W)
-                                               -> fmt::Result
-        where W: fmt::Write,
-              I: Iterator<Item=&'a PropertyDeclaration>,
-    {
-        match *self {
-            ShorthandId::Overflow => {
-                match shorthands::overflow::LonghandsToSerialize::from_iter(declarations) {
-                    Ok(longhands) => longhands.to_css_declared_with_name(dest),
-                    Err(_) => Err(fmt::Error)
-                }
-            },
-            _ => Err(fmt::Error)
-        }
-    }
-
-    /// Serializes the possible shorthand name with value to input buffer given
-    /// a list of longhand declarations.
-    ///
-    /// On success, returns true if the shorthand value is written, or false if
-    /// no shorthand value is present.
-    pub fn serialize_shorthand_to_buffer<'a, W, I>(self,
-                                                   dest: &mut W,
-                                                   declarations: I,
-                                                   is_first_serialization: &mut bool,
-                                                   importance: Importance)
-                                                   -> Result<bool, fmt::Error>
-        where W: Write,
-              I: IntoIterator<Item=&'a PropertyDeclaration>,
-              I::IntoIter: Clone,
-    {
-        match self.get_shorthand_appendable_value(declarations) {
-            None => Ok(false),
-            Some(appendable_value) => {
-                append_serialization(
-                    dest,
-                    &self,
-                    appendable_value,
-                    importance,
-                    is_first_serialization
-                ).and_then(|_| Ok(true))
-            }
-        }
-    }
-
-    fn get_shorthand_appendable_value<'a, I>(self,
                                              declarations: I)
                                              -> Option<AppendableValue<'a, I::IntoIter>>
         where I: IntoIterator<Item=&'a PropertyDeclaration>,

components/style/properties/shorthand/box.mako.rs

@@ -34,42 +34,6 @@
             }
             Ok(())
         }
-
-        // Overflow does not behave like a normal shorthand. When overflow-x and overflow-y are not of equal
-        // values, they no longer use the shared property name "overflow".
-        // Other shorthands do not include their name in the to_css method
-        pub fn to_css_declared_with_name<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
-            let x_and_y_equal = match (self.overflow_x, self.overflow_y) {
-                (&DeclaredValue::Value(ref x_value), &DeclaredValue::Value(ref y_container)) => {
-                    *x_value == y_container.0
-                },
-                (_, &DeclaredValue::WithVariables(_)) |
-                (&DeclaredValue::WithVariables(_), _) => {
-                    // We don't serialize shorthands with variables
-                    return dest.write_str("");
-                },
-                (&DeclaredValue::Initial, &DeclaredValue::Initial) => true,
-                (&DeclaredValue::Inherit, &DeclaredValue::Inherit) => true,
-                (&DeclaredValue::Unset, &DeclaredValue::Unset) => true,
-                _ => false
-            };
-
-            if x_and_y_equal {
-                try!(write!(dest, "overflow"));
-                try!(write!(dest, ": "));
-                try!(self.overflow_x.to_css(dest));
-            } else {
-                try!(write!(dest, "overflow-x"));
-                try!(write!(dest, ": "));
-                try!(self.overflow_x.to_css(dest));
-                try!(write!(dest, "; "));
-
-                try!(write!(dest, "overflow-y: "));
-                try!(self.overflow_y.to_css(dest));
-            }
-
-            write!(dest, ";")
-        }
     }
 </%helpers:shorthand>
 

components/style/properties/shorthand/serialize.mako.rs

@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use properties::{AppendableValue, DeclaredValue, PropertyDeclaration, ShorthandId};
+use properties::DeclaredValue;
 use style_traits::ToCss;
 use values::specified::{BorderStyle, CSSColor};
 use std::fmt;
@@ -91,17 +91,3 @@ fn serialize_directional_border<W, I,>(dest: &mut W,
         _ => Ok(())
     }
 }
-
-
-#[allow(missing_docs)]
-pub fn is_overflow_shorthand<'a, I>(appendable_value: &AppendableValue<'a, I>) -> bool
-    where I: Iterator<Item=&'a PropertyDeclaration>
-{
-    if let AppendableValue::DeclarationsForShorthand(shorthand, _) = *appendable_value {
-        if let ShorthandId::Overflow = shorthand {
-            return true;
-        }
-    }
-
-    false
-}