Address review comments

2 files changed, 125 insertions, 117 deletions

diff --git a/components/style/values/computed/basic_shape.rs b/components/style/values/computed/basic_shape.rs
index 3316d829746..fe0a338db62 100644
--- a/components/style/values/computed/basic_shape.rs
+++ b/components/style/values/computed/basic_shape.rs
@@ -8,10 +8,13 @@
 //! [basic-shape]: https://drafts.csswg.org/css-shapes/#typedef-basic-shape
 
 use cssparser::ToCss;
+use properties::shorthands::serialize_four_sides;
 use std::fmt;
 use values::computed::position::Position;
 use values::computed::{BorderRadiusSize, LengthOrPercentage};
 
+pub use values::specified::basic_shape::FillRule;
+
 #[derive(Clone, PartialEq, Debug)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub enum BasicShape {
@@ -24,9 +27,9 @@ pub enum BasicShape {
 impl ToCss for BasicShape {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
         match *self {
-            BasicShape::Inset(rect) => rect.to_css(dest),
-            BasicShape::Circle(circle) => circle.to_css(dest),
-            BasicShape::Ellipse(e) => e.to_css(dest),
+            BasicShape::Inset(ref rect) => rect.to_css(dest),
+            BasicShape::Circle(ref circle) => circle.to_css(dest),
+            BasicShape::Ellipse(ref e) => e.to_css(dest),
             BasicShape::Polygon(ref poly) => poly.to_css(dest),
         }
     }
@@ -79,22 +82,23 @@ impl ToCss for Circle {
 #[derive(Clone, PartialEq, Copy, Debug)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub struct Ellipse {
-    pub semiaxis_a: ShapeRadius,
-    pub semiaxis_b: ShapeRadius,
+    pub semiaxis_x: ShapeRadius,
+    pub semiaxis_y: ShapeRadius,
     pub position: Position,
 }
 
 impl ToCss for Ellipse {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
-        if ShapeRadius::ClosestSide != self.semiaxis_a &&
-           ShapeRadius::ClosestSide != self.semiaxis_b {
-            try!(self.semiaxis_a.to_css(dest));
+        try!(dest.write_str("ellipse("));
+        if (self.semiaxis_x, self.semiaxis_y) != Default::default() {
+            try!(self.semiaxis_x.to_css(dest));
             try!(dest.write_str(" "));
-            try!(self.semiaxis_b.to_css(dest));
+            try!(self.semiaxis_y.to_css(dest));
             try!(dest.write_str(" "));
         }
         try!(dest.write_str("at "));
-        self.position.to_css(dest)
+        try!(self.position.to_css(dest));
+        dest.write_str(")")
     }
 }
 
@@ -108,6 +112,7 @@ pub struct Polygon {
 
 impl ToCss for Polygon {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+        try!(dest.write_str("polygon("));
         let mut need_space = false;
         if self.fill != Default::default() {
             try!(self.fill.to_css(dest));
@@ -115,14 +120,14 @@ impl ToCss for Polygon {
         }
         for coord in &self.coordinates {
             if need_space {
-                try!(dest.write_str(" "));
+                try!(dest.write_str(", "));
             }
             try!(coord.0.to_css(dest));
             try!(dest.write_str(" "));
             try!(coord.1.to_css(dest));
             need_space = true;
         }
-        Ok(())
+        dest.write_str(")")
     }
 }
 
@@ -157,36 +162,33 @@ impl ToCss for ShapeRadius {
 pub struct BorderRadius {
     pub top_left: BorderRadiusSize,
     pub top_right: BorderRadiusSize,
-    pub bottom_left: BorderRadiusSize,
     pub bottom_right: BorderRadiusSize,
+    pub bottom_left: BorderRadiusSize,
 }
 
 impl ToCss for BorderRadius {
-    // XXXManishearth: We should be producing minimal output:
-    // if height=width for all, we should not be printing the part after
-    // the slash. For any set of four values,
-    // we should try to reduce them to one or two. This probably should be
-    // a helper function somewhere, for all the parse_four_sides-like
-    // values
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
-        try!(self.top_left.0.width.to_css(dest));
-        try!(dest.write_str(" "));
-        try!(self.top_right.0.width.to_css(dest));
-        try!(dest.write_str(" "));
-        try!(self.bottom_left.0.width.to_css(dest));
-        try!(dest.write_str(" "));
-        try!(self.bottom_right.0.width.to_css(dest));
-        try!(dest.write_str(" / "));
-        try!(self.top_left.0.height.to_css(dest));
-        try!(dest.write_str(" "));
-        try!(self.top_right.0.height.to_css(dest));
-        try!(dest.write_str(" "));
-        try!(self.bottom_left.0.height.to_css(dest));
-        try!(dest.write_str(" "));
-        try!(self.bottom_right.0.height.to_css(dest));
-        dest.write_str(" ")
+        if self.top_left.0.width == self.top_left.0.height &&
+           self.top_right.0.width == self.top_right.0.height &&
+           self.bottom_right.0.width == self.bottom_right.0.height &&
+           self.bottom_left.0.width == self.bottom_left.0.height {
+            serialize_four_sides((&self.top_left.0.width,
+                                  &self.top_right.0.width,
+                                  &self.bottom_right.0.width,
+                                  &self.bottom_left.0.width),
+                                  dest)
+        } else {
+            try!(serialize_four_sides((&self.top_left.0.width,
+                                       &self.top_right.0.width,
+                                       &self.bottom_right.0.width,
+                                       &self.bottom_left.0.width),
+                                       dest));
+            try!(dest.write_str(" / "));
+            serialize_four_sides((&self.top_left.0.height,
+                                  &self.top_right.0.height,
+                                  &self.bottom_right.0.height,
+                                  &self.bottom_left.0.height),
+                                  dest)
+        }
     }
 }
-
-pub use values::specified::basic_shape::FillRule;
-
diff --git a/components/style/values/specified/basic_shape.rs b/components/style/values/specified/basic_shape.rs
index 82ae0729c25..a8193116b75 100644
--- a/components/style/values/specified/basic_shape.rs
+++ b/components/style/values/specified/basic_shape.rs
@@ -14,7 +14,7 @@ use std::fmt;
 use values::computed::basic_shape as computed_basic_shape;
 use values::computed::{Context, ToComputedValue, ComputedValueAsSpecified};
 use values::specified::position::{Position, PositionComponent};
-use values::specified::{BorderRadiusSize, Length, LengthOrPercentage};
+use values::specified::{BorderRadiusSize, Length, LengthOrPercentage, Percentage};
 
 #[derive(Clone, PartialEq, Debug)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
@@ -27,16 +27,24 @@ pub enum BasicShape {
 
 impl BasicShape {
     pub fn parse(input: &mut Parser) -> Result<BasicShape, ()> {
-        if let Ok(result) = input.try(InsetRect::parse) {
-            Ok(BasicShape::Inset(result))
-        } else if let Ok(result) = input.try(Circle::parse) {
-            Ok(BasicShape::Circle(result))
-        } else if let Ok(result) = input.try(Ellipse::parse) {
-            Ok(BasicShape::Ellipse(result))
-        } else if let Ok(result) = input.try(Polygon::parse) {
-            Ok(BasicShape::Polygon(result))
-        } else {
-            Err(())
+        match_ignore_ascii_case! { try!(input.expect_function()),
+            "inset" => {
+                Ok(BasicShape::Inset(
+                   try!(input.parse_nested_block(InsetRect::parse_function_arguments))))
+            },
+            "circle" => {
+                Ok(BasicShape::Circle(
+                   try!(input.parse_nested_block(Circle::parse_function_arguments))))
+            },
+            "ellipse" => {
+                Ok(BasicShape::Ellipse(
+                   try!(input.parse_nested_block(Ellipse::parse_function_arguments))))
+            },
+            "polygon" => {
+                Ok(BasicShape::Polygon(
+                   try!(input.parse_nested_block(Polygon::parse_function_arguments))))
+            },
+            _ => Err(())
         }
     }
 }
@@ -81,12 +89,12 @@ impl InsetRect {
     pub fn parse(input: &mut Parser) -> Result<InsetRect, ()> {
         match_ignore_ascii_case! { try!(input.expect_function()),
             "inset" => {
-                Ok(try!(input.parse_nested_block(InsetRect::parse_function)))
+                Ok(try!(input.parse_nested_block(InsetRect::parse_function_arguments)))
             },
             _ => Err(())
         }
     }
-    pub fn parse_function(input: &mut Parser) -> Result<InsetRect, ()> {
+    pub fn parse_function_arguments(input: &mut Parser) -> Result<InsetRect, ()> {
         let (t, r, b, l) = try!(parse_four_sides(input, LengthOrPercentage::parse));
         let mut rect = InsetRect {
             top: t,
@@ -148,18 +156,21 @@ impl Circle {
     pub fn parse(input: &mut Parser) -> Result<Circle, ()> {
         match_ignore_ascii_case! { try!(input.expect_function()),
             "circle" => {
-                Ok(try!(input.parse_nested_block(Circle::parse_function)))
+                Ok(try!(input.parse_nested_block(Circle::parse_function_arguments)))
             },
             _ => Err(())
         }
     }
-    pub fn parse_function(input: &mut Parser) -> Result<Circle, ()> {
+    pub fn parse_function_arguments(input: &mut Parser) -> Result<Circle, ()> {
         let radius = input.try(ShapeRadius::parse).ok().unwrap_or_else(Default::default);
         let position = if let Ok(_) = input.try(|input| input.expect_ident_matching("at")) {
             try!(Position::parse(input))
         } else {
             // Defaults to origin
-            try!(Position::new(PositionComponent::Center, PositionComponent::Center))
+            Position {
+                horizontal: LengthOrPercentage::Percentage(Percentage(0.5)),
+                vertical: LengthOrPercentage::Percentage(Percentage(0.5)),
+            }
         };
         Ok(Circle {
             radius: radius,
@@ -197,8 +208,8 @@ impl ToComputedValue for Circle {
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 /// https://drafts.csswg.org/css-shapes/#funcdef-ellipse
 pub struct Ellipse {
-    pub semiaxis_a: ShapeRadius,
-    pub semiaxis_b: ShapeRadius,
+    pub semiaxis_x: ShapeRadius,
+    pub semiaxis_y: ShapeRadius,
     pub position: Position,
 }
 
@@ -207,24 +218,27 @@ impl Ellipse {
     pub fn parse(input: &mut Parser) -> Result<Ellipse, ()> {
         match_ignore_ascii_case! { try!(input.expect_function()),
             "ellipse" => {
-                Ok(try!(input.parse_nested_block(Ellipse::parse_function)))
+                Ok(try!(input.parse_nested_block(Ellipse::parse_function_arguments)))
             },
             _ => Err(())
         }
     }
-    pub fn parse_function(input: &mut Parser) -> Result<Ellipse, ()> {
+    pub fn parse_function_arguments(input: &mut Parser) -> Result<Ellipse, ()> {
         let (a, b) = input.try(|input| -> Result<_, ()> {
             Ok((try!(ShapeRadius::parse(input)), try!(ShapeRadius::parse(input))))
-        }).unwrap_or((Default::default(), Default::default()));
+        }).ok().unwrap_or_default();
         let position = if let Ok(_) = input.try(|input| input.expect_ident_matching("at")) {
             try!(Position::parse(input))
         } else {
             // Defaults to origin
-            try!(Position::new(PositionComponent::Center, PositionComponent::Center))
+            Position {
+                horizontal: LengthOrPercentage::Percentage(Percentage(0.5)),
+                vertical: LengthOrPercentage::Percentage(Percentage(0.5)),
+            }
         };
         Ok(Ellipse {
-            semiaxis_a: a,
-            semiaxis_b: b,
+            semiaxis_x: a,
+            semiaxis_y: b,
             position: position,
         })
     }
@@ -233,11 +247,10 @@ impl Ellipse {
 impl ToCss for Ellipse {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
         try!(dest.write_str("ellipse("));
-        if ShapeRadius::ClosestSide != self.semiaxis_a ||
-           ShapeRadius::ClosestSide != self.semiaxis_b {
-            try!(self.semiaxis_a.to_css(dest));
+        if (self.semiaxis_x, self.semiaxis_y) != Default::default() {
+            try!(self.semiaxis_x.to_css(dest));
             try!(dest.write_str(" "));
-            try!(self.semiaxis_b.to_css(dest));
+            try!(self.semiaxis_y.to_css(dest));
             try!(dest.write_str(" "));
         }
         try!(dest.write_str("at "));
@@ -252,8 +265,8 @@ impl ToComputedValue for Ellipse {
     #[inline]
     fn to_computed_value(&self, cx: &Context) -> Self::ComputedValue {
         computed_basic_shape::Ellipse {
-            semiaxis_a: self.semiaxis_a.to_computed_value(cx),
-            semiaxis_b: self.semiaxis_b.to_computed_value(cx),
+            semiaxis_x: self.semiaxis_x.to_computed_value(cx),
+            semiaxis_y: self.semiaxis_y.to_computed_value(cx),
             position: self.position.to_computed_value(cx),
         }
     }
@@ -272,25 +285,22 @@ impl Polygon {
     pub fn parse(input: &mut Parser) -> Result<Polygon, ()> {
         match_ignore_ascii_case! { try!(input.expect_function()),
             "polygon" => {
-                Ok(try!(input.parse_nested_block(Polygon::parse_function)))
+                Ok(try!(input.parse_nested_block(Polygon::parse_function_arguments)))
             },
             _ => Err(())
         }
     }
-    pub fn parse_function(input: &mut Parser) -> Result<Polygon, ()> {
+    pub fn parse_function_arguments(input: &mut Parser) -> Result<Polygon, ()> {
         let fill = input.try(|input| {
             let fill = FillRule::parse(input);
             // only eat the comma if there is something before it
             try!(input.expect_comma());
             fill
         }).ok().unwrap_or_else(Default::default);
-        let first = (try!(LengthOrPercentage::parse(input)),
-                     try!(LengthOrPercentage::parse(input)));
-        let mut buf = vec![first];
-        while !input.is_exhausted() {
-            buf.push((try!(LengthOrPercentage::parse(input)),
-                      try!(LengthOrPercentage::parse(input))));
-        }
+        let buf = try!(input.parse_comma_separated(|input| {
+            Ok((try!(LengthOrPercentage::parse(input)),
+                try!(LengthOrPercentage::parse(input))))
+        }));
         Ok(Polygon {
             fill: fill,
             coordinates: buf,
@@ -308,7 +318,7 @@ impl ToCss for Polygon {
         }
         for coord in &self.coordinates {
             if need_space {
-                try!(dest.write_str(" "));
+                try!(dest.write_str(", "));
             }
             try!(coord.0.to_css(dest));
             try!(dest.write_str(" "));
@@ -396,38 +406,32 @@ impl ToComputedValue for ShapeRadius {
 pub struct BorderRadius {
     pub top_left: BorderRadiusSize,
     pub top_right: BorderRadiusSize,
-    pub bottom_left: BorderRadiusSize,
     pub bottom_right: BorderRadiusSize,
+    pub bottom_left: BorderRadiusSize,
 }
 
 impl ToCss for BorderRadius {
-    // XXXManishearth: We should be producing minimal output:
-    // if height=width for all, we should not be printing the part after
-    // the slash. For any set of four values,
-    // we should try to reduce them to one or two. This probably should be
-    // a helper function somewhere, for all the parse_four_sides-like
-    // values
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
         if self.top_left.0.width == self.top_left.0.height &&
            self.top_right.0.width == self.top_right.0.height &&
-           self.bottom_left.0.width == self.bottom_left.0.height &&
-           self.bottom_right.0.width == self.bottom_right.0.height {
+           self.bottom_right.0.width == self.bottom_right.0.height &&
+           self.bottom_left.0.width == self.bottom_left.0.height {
             serialize_four_sides((&self.top_left.0.width,
                                   &self.top_right.0.width,
-                                  &self.bottom_left.0.width,
-                                  &self.bottom_right.0.width),
+                                  &self.bottom_right.0.width,
+                                  &self.bottom_left.0.width),
                                   dest)
         } else {
             try!(serialize_four_sides((&self.top_left.0.width,
                                        &self.top_right.0.width,
-                                       &self.bottom_left.0.width,
-                                       &self.bottom_right.0.width),
+                                       &self.bottom_right.0.width,
+                                       &self.bottom_left.0.width),
                                        dest));
             try!(dest.write_str(" / "));
             serialize_four_sides((&self.top_left.0.height,
                                   &self.top_right.0.height,
-                                  &self.bottom_left.0.height,
-                                  &self.bottom_right.0.height),
+                                  &self.bottom_right.0.height,
+                                  &self.bottom_left.0.height),
                                   dest)
         }
     }
@@ -436,38 +440,40 @@ impl ToCss for BorderRadius {
 impl BorderRadius {
     pub fn parse(input: &mut Parser) -> Result<BorderRadius, ()> {
         let widths = try!(parse_one_set_of_border_values(input));
-        let mut heights = widths.clone();
-        if input.try(|input| input.expect_delim('/')).is_ok() {
-            heights = try!(parse_one_set_of_border_values(input));
-        }
+        let heights = if input.try(|input| input.expect_delim('/')).is_ok() {
+            try!(parse_one_set_of_border_values(input))
+        } else {
+            widths.clone()
+        };
         Ok(BorderRadius {
             top_left: BorderRadiusSize::new(widths[0], heights[0]),
             top_right: BorderRadiusSize::new(widths[1], heights[1]),
-            bottom_left: BorderRadiusSize::new(widths[2], heights[2]),
-            bottom_right: BorderRadiusSize::new(widths[3], heights[3]),
+            bottom_right: BorderRadiusSize::new(widths[2], heights[2]),
+            bottom_left: BorderRadiusSize::new(widths[3], heights[3]),
         })
     }
 }
 
 fn parse_one_set_of_border_values(mut input: &mut Parser)
                                  -> Result<[LengthOrPercentage; 4], ()> {
-    let mut count = 0;
-    let mut values = [LengthOrPercentage::Length(Length::Absolute(Au(0))); 4];
-    while count < 4 {
-        if let Ok(value) = input.try(LengthOrPercentage::parse) {
-            values[count] = value;
-            count += 1;
-        } else {
-            break
-        }
-    }
-
-    match count {
-        1 => Ok([values[0], values[0], values[0], values[0]]),
-        2 => Ok([values[0], values[1], values[0], values[1]]),
-        3 => Ok([values[0], values[1], values[2], values[1]]),
-        4 => Ok([values[0], values[1], values[2], values[3]]),
-        _ => Err(()),
+    let a = try!(LengthOrPercentage::parse(input));
+
+    let b = if let Ok(b) = input.try(LengthOrPercentage::parse) {
+        b
+    } else {
+        return Ok([a, a, a, a])
+    };
+
+    let c = if let Ok(c) = input.try(LengthOrPercentage::parse) {
+        c
+    } else {
+        return Ok([a, b, a, b])
+    };
+
+    if let Ok(d) = input.try(LengthOrPercentage::parse) {
+        Ok([a, b, c, d])
+    } else {
+        Ok([a, b, c, b])
     }
 }
 
@@ -480,8 +486,8 @@ impl ToComputedValue for BorderRadius {
         computed_basic_shape::BorderRadius {
             top_left: self.top_left.to_computed_value(cx),
             top_right: self.top_right.to_computed_value(cx),
-            bottom_left: self.bottom_left.to_computed_value(cx),
             bottom_right: self.bottom_right.to_computed_value(cx),
+            bottom_left: self.bottom_left.to_computed_value(cx),
         }
     }
 }