Address review comments

4 files changed, 85 insertions, 59 deletions

diff --git a/src/components/main/layout/block.rs b/src/components/main/layout/block.rs
index 48626c7f087..07759121f4e 100644
--- a/src/components/main/layout/block.rs
+++ b/src/components/main/layout/block.rs
@@ -102,38 +102,39 @@ impl BlockFlowData {
             }
         }
 
-        /* if not an anonymous block context, add in block box's widths.
-           these widths will not include child elements, just padding etc. */
-        self.box.map(|&box| {
-            //Can compute border width here since it doesn't depend on anything
+        // If not an anonymous block context, add in the block box's widths. These widths will not
+        // include child elements, just padding etc.
+        for self.box.each |&box| {
+            // Can compute border width here since it doesn't depend on anything.
             let style = box.style();
             do box.with_model |model| {
                 model.compute_borders(style)
             }
             min_width = min_width.add(&box.get_min_width(ctx));
             pref_width = pref_width.add(&box.get_pref_width(ctx));
-        });
+        }
 
         self.common.min_width = min_width;
         self.common.pref_width = pref_width;
     }
  
     /// Computes left and right margins and width based on CSS 2.1 secion 10.3.3.
-    /// Requires borders and padding to already be computed
-    priv fn compute_horiz( &self, 
-                            width: MaybeAuto, 
-                            left_margin: MaybeAuto, 
-                            right_margin: MaybeAuto, 
-                            available_width: Au) -> (Au, Au, Au) {
-
-        //If width is not 'auto', and width + margins > available_width, all 'auto' margins are treated as '0'
-        let (left_margin, right_margin) = match width{
+    /// Requires borders and padding to already be computed.
+    fn compute_horiz(&self, 
+                     width: MaybeAuto, 
+                     left_margin: MaybeAuto, 
+                     right_margin: MaybeAuto, 
+                     available_width: Au)
+                     -> (Au, Au, Au) {
+        // If width is not 'auto', and width + margins > available_width, all 'auto' margins are
+        // treated as '0'.
+        let (left_margin, right_margin) = match width {
             Auto => (left_margin, right_margin),
             Specified(width) => {
                 let left = left_margin.spec_or_default(Au(0));
                 let right = right_margin.spec_or_default(Au(0));
                 
-                if((left + right + width) > available_width) {
+                if (left + right + width) > available_width {
                     (Specified(left), Specified(right))
                 } else {
                     (left_margin, right_margin)
@@ -141,31 +142,43 @@ impl BlockFlowData {
             }
         };
 
-        //Invariant: left_margin_Au + width_Au + right_margin_Au == available_width
+        // Invariant: left_margin_Au + width_Au + right_margin_Au == available_width
         let (left_margin_Au, width_Au, right_margin_Au) = match (left_margin, width, right_margin) {
-            //If all have a computed value other than 'auto', the system is over-constrained and we need to discard a margin.
-            //if direction is ltr, ignore the specified right margin and solve for it. If it is rtl, ignore the specified 
-            //left margin. FIXME(eatkinson): this assumes the direction is ltr
-            (Specified(margin_l), Specified(width), Specified(margin_r)) => (margin_l, width, available_width - (margin_l + width )),
+            // If all have a computed value other than 'auto', the system is over-constrained and
+            // we need to discard a margin. If direction is ltr, ignore the specified right margin
+            // and solve for it. If it is rtl, ignore the specified left margin.
+            //
+            // FIXME(eatkinson): this assumes the direction is ltr
+            (Specified(margin_l), Specified(width), Specified(_)) => {
+                (margin_l, width, available_width - (margin_l + width))
+            }
 
-            //If exactly one value is 'auto', solve for it
-            (Auto, Specified(width), Specified(margin_r)) => (available_width - (width + margin_r), width, margin_r),
-            (Specified(margin_l), Auto, Specified(margin_r)) => (margin_l, available_width - (margin_l + margin_r), margin_r),
-            (Specified(margin_l), Specified(width), Auto) => (margin_l, width, available_width - (margin_l + width)),
+            // If exactly one value is 'auto', solve for it
+            (Auto, Specified(width), Specified(margin_r)) => {
+                (available_width - (width + margin_r), width, margin_r)
+            }
+            (Specified(margin_l), Auto, Specified(margin_r)) => {
+                (margin_l, available_width - (margin_l + margin_r), margin_r)
+            }
+            (Specified(margin_l), Specified(width), Auto) => {
+                (margin_l, width, available_width - (margin_l + width))
+            }
 
-            //If width is set to 'auto', any other 'auto' value becomes '0', and width is solved for
+            // If width is set to 'auto', any other 'auto' value becomes '0', and width is solved
+            // for.
             (Auto, Auto, Specified(margin_r)) => (Au(0), available_width - margin_r, margin_r),
             (Specified(margin_l), Auto, Auto) => (margin_l, available_width - margin_l, Au(0)),
             (Auto, Auto, Auto) => (Au(0), available_width, Au(0)),
 
-            //If left and right margins are auto, they become equal
+            // If left and right margins are auto, they become equal.
             (Auto, Specified(width), Auto) => {
                 let margin = (available_width - width).scale_by(0.5);
                 (margin, width, margin)
             }
 
         };
-        //return values in same order as params
+
+        // Return values in same order as params.
         (width_Au, left_margin_Au, right_margin_Au)
     }
 
@@ -184,25 +197,31 @@ impl BlockFlowData {
         let mut remaining_width = self.common.position.size.width;
         let mut x_offset = Au(0);
 
-        self.box.map(|&box| {
+        for self.box.each |&box| {
             let style = box.style();
             do box.with_model |model| {
-                //Can compute padding here since we know containing block width
+                // Can compute padding here since we know containing block width.
                 model.compute_padding(style, remaining_width);
 
-                //Margins are 0 right now so model.noncontent_width() is just borders + padding.
+                // Margins are 0 right now so model.noncontent_width() is just borders + padding.
                 let available_width = remaining_width - model.noncontent_width();
 
-                //Top and bottom margins for blocks are 0 if auto
-                let margin_top = MaybeAuto::from_margin(style.margin_top()).spec_or_default(Au(0));
-                let margin_bottom = MaybeAuto::from_margin(style.margin_bottom()).spec_or_default(Au(0));
+                // Top and bottom margins for blocks are 0 if auto.
+                let margin_top = MaybeAuto::from_margin(style.margin_top());
+                let margin_top = margin_top.spec_or_default(Au(0));
+                let margin_bottom = MaybeAuto::from_margin(style.margin_bottom());
+                let margin_bottom = margin_bottom.spec_or_default(Au(0));
 
-                let (width, margin_left, margin_right) = (MaybeAuto::from_width(style.width()),
-                    MaybeAuto::from_margin(style.margin_left()),
-                    MaybeAuto::from_margin(style.margin_right()));
+                let (width, margin_left, margin_right) =
+                    (MaybeAuto::from_width(style.width()),
+                     MaybeAuto::from_margin(style.margin_left()),
+                     MaybeAuto::from_margin(style.margin_right()));
 
-                let (width, margin_left, margin_right) = 
-                    self.compute_horiz(width, margin_left, margin_right, available_width);
+                // FIXME(pcwalton): We discard the width here. Is that correct?
+                let (_, margin_left, margin_right) = self.compute_horiz(width,
+                                                                        margin_left,
+                                                                        margin_right,
+                                                                        available_width);
 
                 model.margin.top = margin_top;
                 model.margin.right = margin_right;
@@ -221,7 +240,7 @@ impl BlockFlowData {
                     base.model.border.left + base.model.border.right;
                 base.position.size.width = remaining_width + pb;
             }
-        });
+        }
 
         for BlockFlow(self).each_child |kid| {
             assert!(kid.starts_block_flow() || kid.starts_inline_flow());
diff --git a/src/components/main/layout/context.rs b/src/components/main/layout/context.rs
index 8646b9faabc..e0ad6bcad7e 100644
--- a/src/components/main/layout/context.rs
+++ b/src/components/main/layout/context.rs
@@ -8,12 +8,10 @@ use geom::rect::Rect;
 use gfx::font_context::FontContext;
 use gfx::geometry::Au;
 use servo_net::local_image_cache::LocalImageCache;
-use std::net::url::Url;
 
 /// Data needed by the layout task.
 pub struct LayoutContext {
     font_ctx: @mut FontContext,
     image_cache: @mut LocalImageCache,
-    doc_url: Url,
-    screen_size: Rect<Au>
+    screen_size: Rect<Au>,
 }
diff --git a/src/components/main/layout/layout_task.rs b/src/components/main/layout/layout_task.rs
index 69d1e6a031b..659272fa62f 100644
--- a/src/components/main/layout/layout_task.rs
+++ b/src/components/main/layout/layout_task.rs
@@ -115,12 +115,10 @@ impl Layout {
         let image_cache = self.local_image_cache;
         let font_ctx = self.font_ctx;
         let screen_size = self.screen_size.unwrap();
-        let doc_url = self.doc_url.clone();
 
         LayoutContext {
             image_cache: image_cache,
             font_ctx: font_ctx,
-            doc_url: doc_url.unwrap(),
             screen_size: Rect(Point2D(Au(0), Au(0)), screen_size),
         }
     }
@@ -348,10 +346,12 @@ impl Layout {
                         let display_list = &display_list.take().list;
                         for display_list.each_reverse |display_item| {
                             let bounds = display_item.bounds();
+
+                            // FIXME(pcwalton): Move this to be a method on Rect.
                             if x <= bounds.origin.x + bounds.size.width &&
-                               bounds.origin.x <= x &&
-                               y < bounds.origin.y + bounds.size.height &&
-                               bounds.origin.y <  y {
+                                    x >= bounds.origin.x &&
+                                    y < bounds.origin.y + bounds.size.height &&
+                                    y >= bounds.origin.y {
                                 resp = Ok(HitTestResponse(display_item.base().extra.node()));
                                 break;
                             }
diff --git a/src/components/main/layout/model.rs b/src/components/main/layout/model.rs
index 4871ddb5c85..8f222814dc1 100644
--- a/src/components/main/layout/model.rs
+++ b/src/components/main/layout/model.rs
@@ -22,12 +22,17 @@ use newcss::values::{CSSBorderWidthThick, CSSBorderWidthThin};
 use newcss::values::{CSSWidth, CSSWidthLength, CSSWidthPercentage, CSSWidthAuto};
 use newcss::values::{CSSMargin, CSSMarginLength, CSSMarginPercentage, CSSMarginAuto};
 use newcss::values::{CSSPadding, CSSPaddingLength, CSSPaddingPercentage};
+
 /// Encapsulates the borders, padding, and margins, which we collectively call the "box model".
 pub struct BoxModel {
+    /// The size of the borders.
     border: SideOffsets2D<Au>,
+    /// The size of the padding.
     padding: SideOffsets2D<Au>,
+    /// The size of the margins.
     margin: SideOffsets2D<Au>,
-    cb_width: Au,
+    /// The width of the content box.
+    content_box_width: Au,
 }
 
 /// Useful helper data type when computing values for blocks and positioned elements.
@@ -75,7 +80,7 @@ impl Zero for BoxModel {
             border: Zero::zero(),
             padding: Zero::zero(),
             margin: Zero::zero(),
-            cb_width: Zero::zero(),
+            content_box_width: Zero::zero(),
         }
     }
 
@@ -85,7 +90,7 @@ impl Zero for BoxModel {
 }
 
 impl BoxModel {
-    /// Populates the box model parameters from the given computed style.
+    /// Populates the box model border parameters from the given computed style.
     pub fn compute_borders(&mut self, style: CompleteStyle) {
         // Compute the borders.
         self.border.top = self.compute_border_width(style.border_top_width());
@@ -94,11 +99,16 @@ impl BoxModel {
         self.border.left = self.compute_border_width(style.border_left_width());
     }
 
-    pub fn compute_padding(&mut self, style: CompleteStyle, cb_width: Au){
-        self.padding.top = self.compute_padding_length(style.padding_top(), cb_width);
-        self.padding.right = self.compute_padding_length(style.padding_right(), cb_width);
-        self.padding.bottom = self.compute_padding_length(style.padding_bottom(), cb_width);
-        self.padding.left = self.compute_padding_length(style.padding_left(), cb_width);
+    /// Populates the box model padding parameters from the given computed style.
+    pub fn compute_padding(&mut self, style: CompleteStyle, content_box_width: Au) {
+        self.padding.top = self.compute_padding_length(style.padding_top(),
+                                                       content_box_width);
+        self.padding.right = self.compute_padding_length(style.padding_right(),
+                                                         content_box_width);
+        self.padding.bottom = self.compute_padding_length(style.padding_bottom(),
+                                                          content_box_width);
+        self.padding.left = self.compute_padding_length(style.padding_left(),
+                                                        content_box_width);
     }
 
     pub fn noncontent_width(&self) -> Au {
@@ -112,7 +122,7 @@ impl BoxModel {
     }
 
     /// Helper function to compute the border width in app units from the CSS border width.
-    priv fn compute_border_width(&self, width: CSSBorderWidth) -> Au {
+    fn compute_border_width(&self, width: CSSBorderWidth) -> Au {
         match width {
             CSSBorderWidthLength(Px(v)) |
             CSSBorderWidthLength(Em(v)) |
@@ -126,7 +136,7 @@ impl BoxModel {
         }
     }
 
-    priv fn compute_padding_length(&self, padding: CSSPadding, cb_width: Au) -> Au {
+    fn compute_padding_length(&self, padding: CSSPadding, content_box_width: Au) -> Au {
         match padding {
             CSSPaddingLength(Px(v)) |
             CSSPaddingLength(Pt(v)) |
@@ -134,10 +144,9 @@ impl BoxModel {
                 // FIXME(eatkinson): Handle 'em' and 'pt' correctly
                 Au::from_frac_px(v)
             }
-            CSSPaddingPercentage(p) => cb_width.scale_by(p)
+            CSSPaddingPercentage(p) => content_box_width.scale_by(p)
         }
     }
-
 }
 
 //