auto merge of #1365 : pcwalton/servo/minor-layout-refactoring, r=metajack

r? @metajack

src/components/main/layout/inline.rs

@@ -277,10 +277,56 @@ impl LineboxScanner {
         
     }
 
-    /// Returns false only if we should break the line.
+    /// Performs float collision avoidance. This is called when adding a box is going to increase
-    fn try_append_to_line(&mut self, in_box: @Box, flow: &mut InlineFlow) -> bool {
+    /// the height, and because of that we will collide with some floats.
-        let line_is_empty: bool = self.pending_line.range.length() == 0;
+    ///
+    /// We have two options here:
+    /// 1) Move the entire line so that it doesn't collide any more.
+    /// 2) Break the line and put the new box on the next line.
+    ///
+    /// The problem with option 1 is that we might move the line and then wind up breaking anyway,
+    /// which violates the standard.
+    /// But option 2 is going to look weird sometimes.
+    ///
+    /// So we'll try to move the line whenever we can, but break if we have to.
+    ///
+    /// Returns false if and only if we should break the line.
+    fn avoid_floats(&mut self,
+                    in_box: @Box,
+                    flow: &mut InlineFlow,
+                    new_height: Au,
+                    line_is_empty: bool)
+                    -> bool {
+        debug!("LineboxScanner: entering float collision avoider!");
 
+        // First predict where the next line is going to be.
+        let this_line_y = self.pending_line.bounds.origin.y;
+        let (next_line, first_box_width) = self.initial_line_placement(in_box, this_line_y, flow);
+        let next_green_zone = next_line.size;
+
+        let new_width = self.pending_line.bounds.size.width + first_box_width;
+
+        // Now, see if everything can fit at the new location.
+        if next_green_zone.width >= new_width && next_green_zone.height >= new_height {
+            debug!("LineboxScanner: case=adding box collides vertically with floats: moving line");
+
+            self.pending_line.bounds.origin = next_line.origin;
+            self.pending_line.green_zone = next_green_zone;
+
+            assert!(!line_is_empty, "Non-terminating line breaking");
+            self.work_list.push_front(in_box);
+            return true
+        }
+
+        debug!("LineboxScanner: case=adding box collides vertically with floats: breaking line");
+        self.work_list.push_front(in_box);
+        false
+    }
+
+    /// Tries to append the given box to the line, splitting it if necessary. Returns false only if
+    /// we should break the line.
+    fn try_append_to_line(&mut self, in_box: @Box, flow: &mut InlineFlow) -> bool {
+        let line_is_empty = self.pending_line.range.length() == 0;
         if line_is_empty {
             let (line_bounds, _) = self.initial_line_placement(in_box, self.cur_y, flow);
             self.pending_line.bounds.origin = line_bounds.origin;
@@ -294,132 +340,85 @@ impl LineboxScanner {
                self.pending_line.green_zone,
                in_box.debug_str());
 
-
         let green_zone = self.pending_line.green_zone;
 
-        //assert!(green_zone.width >= self.pending_line.bounds.size.width &&
+        // NB: At this point, if `green_zone.width < self.pending_line.bounds.size.width` or
-        //        green_zone.height >= self.pending_line.bounds.size.height,
+        // `green_zone.height < self.pending_line.bounds.size.height`, then we committed a line
-        //        "Committed a line that overlaps with floats");
+        // that overlaps with floats.
 
         let new_height = self.new_height_for_line(in_box);
         if new_height > green_zone.height {
-            debug!("LineboxScanner: entering float collision avoider!");
+            // Uh-oh. Float collision imminent. Enter the float collision avoider…
-
+            return self.avoid_floats(in_box, flow, new_height, line_is_empty)
-            // Uh-oh. Adding this box is going to increase the height,
-            // and because of that we will collide with some floats.
-
-            // We have two options here:
-            // 1) Move the entire line so that it doesn't collide any more.
-            // 2) Break the line and put the new box on the next line.
-
-            // The problem with option 1 is that we might move the line
-            // and then wind up breaking anyway, which violates the standard.
-            // But option 2 is going to look weird sometimes.
-
-            // So we'll try to move the line whenever we can, but break
-            // if we have to.
-
-            // First predict where the next line is going to be
-            let this_line_y = self.pending_line.bounds.origin.y;
-            let (next_line, first_box_width) = self.initial_line_placement(in_box, this_line_y, flow);
-            let next_green_zone = next_line.size;
-
-            let new_width = self.pending_line.bounds.size.width + first_box_width;
-            // Now, see if everything can fit at the new location.
-            if next_green_zone.width >= new_width && next_green_zone.height >= new_height{
-                debug!("LineboxScanner: case=adding box collides vertically with floats: moving line");
-
-                self.pending_line.bounds.origin = next_line.origin;
-                self.pending_line.green_zone = next_green_zone;
-
-                assert!(!line_is_empty, "Non-terminating line breaking");
-                self.work_list.push_front(in_box);
-                return true;
-            } else {
-                debug!("LineboxScanner: case=adding box collides vertically with floats: breaking line");
-                self.work_list.push_front(in_box);
-                return false;
-            }
         }
 
         // If we're not going to overflow the green zone vertically, we might still do so
-        // horizontally. We'll try to place the whole box on this line and break somewhere
+        // horizontally. We'll try to place the whole box on this line and break somewhere if it
-        // if it doesn't fit.
+        // doesn't fit.
 
         let new_width = self.pending_line.bounds.size.width + in_box.position.get().size.width;
-
         if new_width <= green_zone.width {
             debug!("LineboxScanner: case=box fits without splitting");
             self.push_box_to_line(in_box);
-            return true;
+            return true
         }
 
         if !in_box.can_split() {
-            // TODO(Issue #224): signal that horizontal overflow happened?
+            // TODO(eatkinson, issue #224): Signal that horizontal overflow happened?
             if line_is_empty {
                 debug!("LineboxScanner: case=box can't split and line {:u} is empty, so \
                         overflowing.",
                         self.lines.len());
-                self.push_box_to_line(in_box);
+                self.push_box_to_line(in_box)
-                return true;
             } else {
                 debug!("LineboxScanner: Case=box can't split, not appending.");
-                return false;
-            }
-        } else {
-            let available_width = green_zone.width - self.pending_line.bounds.size.width;
-
-            match in_box.split_to_width(available_width, line_is_empty) {
-                CannotSplit(_) => {
-                    error!("LineboxScanner: Tried to split unsplittable render box! {:s}",
-                            in_box.debug_str());
-                    return false;
-                }
-                SplitDidFit(left, right) => {
-                    debug!("LineboxScanner: case=split box did fit; deferring remainder box.");
-                    match (left, right) {
-                        (Some(left_box), Some(right_box)) => {
-                            self.push_box_to_line(left_box);
-                            self.work_list.push_front(right_box);
-                        }
-                        (Some(left_box), None) => self.push_box_to_line(left_box),
-                        (None, Some(right_box)) => self.push_box_to_line(right_box),
-                        (None, None) => error!("LineboxScanner: This split case makes no sense!"),
-                    }
-                    return true;
-                }
-                SplitDidNotFit(left, right) => {
-                    if line_is_empty {
-                        debug!("LineboxScanner: case=split box didn't fit and line {:u} is empty, so overflowing and deferring remainder box.",
-                                self.lines.len());
-                        // TODO(Issue #224): signal that horizontal overflow happened?
-                        match (left, right) {
-                            (Some(left_box), Some(right_box)) => {
-                                self.push_box_to_line(left_box);
-                                self.work_list.push_front(right_box);
-                            }
-                            (Some(left_box), None) => {
-                                self.push_box_to_line(left_box);
-                            }
-                            (None, Some(right_box)) => {
-                                self.push_box_to_line(right_box);
-                            }
-                            (None, None) => {
-                                error!("LineboxScanner: This split case makes no sense!");
-                            }
-                        }
-                        return true;
-                    } else {
-                        debug!("LineboxScanner: case=split box didn't fit, not appending and deferring original box.");
-                        self.work_list.push_front(in_box);
-                        return false;
-                    }
-                }
             }
+            return line_is_empty
         }
+
+        let available_width = green_zone.width - self.pending_line.bounds.size.width;
+        let split = in_box.split_to_width(available_width, line_is_empty);
+        let (left, right) = match (split, line_is_empty) {
+            (CannotSplit(_), _) => {
+                error!("LineboxScanner: Tried to split unsplittable render box! {:s}",
+                        in_box.debug_str());
+                return false
+            }
+            (SplitDidNotFit(_, _), false) => {
+                debug!("LineboxScanner: case=split box didn't fit, not appending and deferring \
+                        original box.");
+                self.work_list.push_front(in_box);
+                return false
+            }
+            (SplitDidFit(left, right), _) => {
+                debug!("LineboxScanner: case=split box did fit; deferring remainder box.");
+                (left, right)
+                // Fall through to push boxes to the line.
+            }
+            (SplitDidNotFit(left, right), true) => {
+                // TODO(eatkinson, issue #224): Signal that horizontal overflow happened?
+                debug!("LineboxScanner: case=split box didn't fit and line {:u} is empty, so \
+                        overflowing and deferring remainder box.",
+                        self.lines.len());
+                (left, right)
+                // Fall though to push boxes to the line.
+            }
+        };
+
+        match (left, right) {
+            (Some(left_box), Some(right_box)) => {
+                self.push_box_to_line(left_box);
+                self.work_list.push_front(right_box);
+            }
+            (Some(left_box), None) => self.push_box_to_line(left_box),
+            (None, Some(right_box)) => self.push_box_to_line(right_box),
+            (None, None) => error!("LineboxScanner: This split case makes no sense!"),
+        }
+
+        true
     }
 
-    // unconditional push
+    // An unconditional push.
     fn push_box_to_line(&mut self, box: @Box) {
         debug!("LineboxScanner: Pushing box {} to line {:u}", box.debug_id(), self.lines.len());
 
@@ -511,6 +510,114 @@ impl InlineFlow {
         // For now, don't traverse the subtree rooted here
         true
     }
+
+    /// Returns the relative offset from the baseline for this box, taking into account the value
+    /// of the CSS `vertical-align` property.
+    ///
+    /// The extra boolean is set if and only if `biggest_top` and/or `biggest_bottom` were updated.
+    /// That is, if the box has a `top` or `bottom` value, true is returned.
+    fn relative_offset_from_baseline(cur_box: @Box,
+                                     ascent: Au,
+                                     parent_text_top: Au,
+                                     parent_text_bottom: Au,
+                                     top_from_base: &mut Au,
+                                     bottom_from_base: &mut Au,
+                                     biggest_top: &mut Au,
+                                     biggest_bottom: &mut Au)
+                                     -> (Au, bool) {
+        match cur_box.vertical_align() {
+            vertical_align::baseline => (-ascent, false),
+            vertical_align::middle => {
+                // TODO: x-height value should be used from font info.
+                let xheight = Au::new(0);
+                (-(xheight + cur_box.box_height()).scale_by(0.5), false)
+            },
+            vertical_align::sub => {
+                // TODO: The proper position for subscripts should be used.
+                // Lower the baseline to the proper position for subscripts
+                let sub_offset = Au::new(0);
+                (sub_offset - ascent, false)
+            },
+            vertical_align::super_ => {
+                // TODO: The proper position for superscripts should be used.
+                // Raise the baseline to the proper position for superscripts
+                let super_offset = Au::new(0);
+                (-super_offset - ascent, false)
+            },
+            vertical_align::text_top => {
+                let box_height = *top_from_base + *bottom_from_base;
+                let prev_bottom_from_base = *bottom_from_base;
+                *top_from_base = parent_text_top;
+                *bottom_from_base = box_height - *top_from_base;
+                (*bottom_from_base - prev_bottom_from_base - ascent, false)
+            },
+            vertical_align::text_bottom => {
+                let box_height = *top_from_base + *bottom_from_base;
+                let prev_bottom_from_base = *bottom_from_base;
+                *bottom_from_base = parent_text_bottom;
+                *top_from_base = box_height - *bottom_from_base;
+                (*bottom_from_base - prev_bottom_from_base - ascent, false)
+            },
+            vertical_align::top => {
+                if *biggest_top < (*top_from_base + *bottom_from_base) {
+                    *biggest_top = *top_from_base + *bottom_from_base;
+                }
+                let offset_top = *top_from_base - ascent;
+                (offset_top, true)
+            },
+            vertical_align::bottom => {
+                if *biggest_bottom < (*top_from_base + *bottom_from_base) {
+                    *biggest_bottom = *top_from_base + *bottom_from_base;
+                }
+                let offset_bottom = -(*bottom_from_base + ascent);
+                (offset_bottom, true)
+            },
+            vertical_align::Length(length) => (-(length + ascent), false),
+            vertical_align::Percentage(p) => {
+                let pt_size = cur_box.font_style().pt_size; 
+                let line_height = cur_box.calculate_line_height(Au::from_pt(pt_size));
+                let percent_offset = line_height.scale_by(p);
+                (-(percent_offset + ascent), false)
+            }
+        }
+    }
+
+    /// Sets box X positions based on alignment for one line.
+    fn set_horizontal_box_positions(boxes: &[@Box], line: &LineBox) {
+        // Figure out how much width we have.
+        let slack_width = Au::max(Au(0), line.green_zone.width - line.bounds.size.width);
+
+        // Get the text alignment.
+        //
+        // TODO(burg, issue #222): use 'text-align' property from `InlineFlow`'s block container,
+        // not from the style of the first box child.
+        let linebox_align = if line.range.begin() < boxes.len() {
+            let first_box = boxes[line.range.begin()];
+            first_box.nearest_ancestor_element().style().Text.text_align
+        } else {
+            // Nothing to lay out, so assume left alignment.
+            text_align::left
+        };
+
+        // Set the box x positions based on that alignment.
+        let mut offset_x = line.bounds.origin.x;
+        offset_x = offset_x + match linebox_align {
+            // So sorry, but justified text is more complicated than shuffling linebox
+            // coordinates.
+            //
+            // TODO(burg, issue #213): Implement `text-align: justify`.
+            text_align::left | text_align::justify => Au(0),
+            text_align::center => slack_width.scale_by(0.5),
+            text_align::right => slack_width,
+        };
+
+        for i in line.range.eachi() {
+            let box = &boxes[i];
+            let size = box.position.get().size;
+            box.position.set(Rect(Point2D(offset_x, box.position.get().origin.y), size));
+            offset_x = offset_x + size.width;
+        }
+    }
 }
 
 impl Flow for InlineFlow {
@@ -590,7 +697,8 @@ impl Flow for InlineFlow {
     fn assign_height(&mut self, _: &mut LayoutContext) {
         debug!("assign_height_inline: assigning height for flow {}", self.base.id);
 
-        // Divide the boxes into lines
+        // Divide the boxes into lines.
+        //
         // TODO(#226): Get the CSS `line-height` property from the containing block's style to
         // determine minimum linebox height.
         //
@@ -608,68 +716,20 @@ impl Flow for InlineFlow {
 
         let mut line_height_offset = Au::new(0);
 
-        // Now, go through each line and lay out the boxes inside
+        // Now, go through each line and lay out the boxes inside.
         for line in self.lines.mut_iter() {
-            // We need to distribute extra width based on text-align.
+            // Lay out boxes horizontally.
-            let mut slack_width = line.green_zone.width - line.bounds.size.width;
+            InlineFlow::set_horizontal_box_positions(self.boxes, line);
-            if slack_width < Au::new(0) {
-                slack_width = Au::new(0);
-            }
-
-            // Get the text alignment.
-            // TODO(Issue #222): use 'text-align' property from InlineFlow's
-            // block container, not from the style of the first box child.
-            let linebox_align = if line.range.begin() < self.boxes.len() {
-                let first_box = self.boxes[line.range.begin()];
-                first_box.nearest_ancestor_element().style().Text.text_align
-            } else {
-                // Nothing to lay out, so assume left alignment.
-                text_align::left
-            };
-
-            // Set the box x positions
-            let mut offset_x = line.bounds.origin.x;
-            match linebox_align {
-                // So sorry, but justified text is more complicated than shuffling linebox coordinates.
-                // TODO(Issue #213): implement `text-align: justify`
-                text_align::left | text_align::justify => {
-                    for i in line.range.eachi() {
-                        let box = &self.boxes[i];
-                        box.position.mutate().ptr.origin.x = offset_x;
-                        offset_x = offset_x + box.position.get().size.width;
-                    }
-                }
-                text_align::center => {
-                    offset_x = offset_x + slack_width.scale_by(0.5);
-                    for i in line.range.eachi() {
-                        let box = &self.boxes[i];
-                        box.position.mutate().ptr.origin.x = offset_x;
-                        offset_x = offset_x + box.position.get().size.width;
-                    }
-                }
-                text_align::right => {
-                    offset_x = offset_x + slack_width;
-                    for i in line.range.eachi() {
-                        let box = &self.boxes[i];
-                        box.position.mutate().ptr.origin.x = offset_x;
-                        offset_x = offset_x + box.position.get().size.width;
-                    }
-                }
-            };
 
             // Set the top y position of the current linebox.
             // `line_height_offset` is updated at the end of the previous loop.
             line.bounds.origin.y = line.bounds.origin.y + line_height_offset;
 
-            // Calculate the distance from baseline to the top of the linebox.
+            // Calculate the distance from baseline to the top and bottom of the linebox.
-            let mut topmost = Au::new(0);
+            let (mut topmost, mut bottommost) = (Au(0), Au(0));
-            // Calculate the distance from baseline to the bottom of the linebox.
+            // Calculate the biggest height among boxes with 'top' and 'bottom' values
-            let mut bottommost = Au::new(0);
+            // respectively.
-
+            let (mut biggest_top, mut biggest_bottom) = (Au(0), Au(0));
-            // Calculate the biggest height among boxes with 'top' value.
-            let mut biggest_top = Au::new(0);
-            // Calculate the biggest height among boxes with 'bottom' value.
-            let mut biggest_bottom = Au::new(0);
 
             for box_i in line.range.eachi() {
                 let cur_box = self.boxes[box_i];
@@ -679,8 +739,8 @@ impl Flow for InlineFlow {
                     ImageBox(ref image_box) => {
                         let mut height = image_box.image_height(cur_box);
 
-                        // TODO: margin, border, padding's top and bottom should be calculated in advance,
+                        // TODO: margin, border, padding's top and bottom should be calculated in
-                        // since baseline of image is bottom margin edge.
+                        // advance, since baseline of image is bottom margin edge.
                         let mut top;
                         let mut bottom;
                         {
@@ -727,92 +787,45 @@ impl Flow for InlineFlow {
                         fail!("Unscanned text boxes should have been scanned by now.")
                     }
                 };
+
                 let mut top_from_base = top_from_base;
                 let mut bottom_from_base = bottom_from_base;
 
                 // To calculate text-top and text-bottom value of 'vertical-align',
                 //  we should find the top and bottom of the content area of parent box.
-                // The content area is defined in "http://www.w3.org/TR/CSS2/visudet.html#inline-non-replaced". 
+                // The content area is defined in:
-                // TODO: We should extract em-box info from font size of parent
+                //      http://www.w3.org/TR/CSS2/visudet.html#inline-non-replaced 
-                //  and calcuate the distances from baseline to the top and the bottom of parent's content area.
+                //
+                // TODO: We should extract em-box info from the font size of the parent and
+                // calculate the distances from the baseline to the top and the bottom of the
+                // parent's content area.
 
-                // It should calculate the distance from baseline to the top of parent's content area.
+                // We should calculate the distance from baseline to the top of parent's content
-                // But, it is assumed now as font size of parent.
+                // area. But for now we assume it's the parent's font size.
                 let mut parent_text_top;
-                // It should calculate the distance from baseline to the bottom of parent's content area.
-                // But, it is assumed now as 0.
-                let parent_text_bottom  = Au::new(0);
-                // Get parent node
-                let parent = cur_box.node.parent_node();
 
+                // We should calculate the distance from baseline to the bottom of the parent's
+                // content area. But for now we assume it's zero.
+                let parent_text_bottom = Au::new(0);
+
+                let parent = cur_box.node.parent_node();
                 let font_size = parent.unwrap().style().Font.font_size;
                 parent_text_top = font_size;
 
-                // This flag decides whether topmost and bottommost are updated or not.
+                // Calculate a relative offset from the baseline.
-                // That is, if the box has top or bottom value, no_update_flag becomes true.
+                //
-                let mut no_update_flag = false;
+                // The no-update flag decides whether `biggest_top` and `biggest_bottom` are
-                // Calculate a relative offset from baseline.
+                // updated or not. That is, if the box has a `top` or `bottom` value,
-                let offset = match cur_box.vertical_align() {
+                // `no_update_flag` becomes true.
-                    vertical_align::baseline => {
+                let (offset, no_update_flag) =
-                        -ascent
+                    InlineFlow::relative_offset_from_baseline(cur_box,
-                    },
+                                                              ascent,
-                    vertical_align::middle => {
+                                                              parent_text_top,
-                        // TODO: x-height value should be used from font info.
+                                                              parent_text_bottom,
-                        let xheight = Au::new(0);
+                                                              &mut top_from_base,
-                        -(xheight + cur_box.box_height()).scale_by(0.5)
+                                                              &mut bottom_from_base,
-                    },
+                                                              &mut biggest_top,
-                    vertical_align::sub => {
+                                                              &mut biggest_bottom);
-                        // TODO: The proper position for subscripts should be used.
-                        // Lower the baseline to the proper position for subscripts
-                        let sub_offset = Au::new(0);
-                        (sub_offset - ascent)
-                    },
-                    vertical_align::super_ => {
-                        // TODO: The proper position for superscripts should be used.
-                        // Raise the baseline to the proper position for superscripts
-                        let super_offset = Au::new(0);
-                        (-super_offset - ascent)
-                    },
-                    vertical_align::text_top => {
-                        let box_height = top_from_base + bottom_from_base;
-                        let prev_bottom_from_base = bottom_from_base;
-                        top_from_base = parent_text_top;
-                        bottom_from_base = box_height - top_from_base;
-                        (bottom_from_base - prev_bottom_from_base - ascent)
-                    },
-                    vertical_align::text_bottom => {
-                        let box_height = top_from_base + bottom_from_base;
-                        let prev_bottom_from_base = bottom_from_base;
-                        bottom_from_base = parent_text_bottom;
-                        top_from_base = box_height - bottom_from_base;
-                        (bottom_from_base - prev_bottom_from_base - ascent)
-                    },
-                    vertical_align::top => {
-                        if biggest_top < (top_from_base + bottom_from_base) {
-                            biggest_top = top_from_base + bottom_from_base;
-                        }
-                        let offset_top = top_from_base - ascent;
-                        no_update_flag = true;
-                        offset_top
-                    },
-                    vertical_align::bottom => {
-                        if biggest_bottom < (top_from_base + bottom_from_base) {
-                            biggest_bottom = top_from_base + bottom_from_base;
-                        }
-                        let offset_bottom = -(bottom_from_base + ascent);
-                        no_update_flag = true;
-                        offset_bottom
-                    },
-                    vertical_align::Length(length) => {
-                        -(length + ascent)
-                    },
-                    vertical_align::Percentage(p) => {
-                        let pt_size = cur_box.font_style().pt_size; 
-                        let line_height = cur_box.calculate_line_height(Au::from_pt(pt_size));
-                        let percent_offset = line_height.scale_by(p);
-                        -(percent_offset + ascent)
-                    }
-                };
 
                 // If the current box has 'top' or 'bottom' value, no_update_flag is true.
                 // Otherwise, topmost and bottomost are updated.
@@ -826,15 +839,15 @@ impl Flow for InlineFlow {
                 cur_box.position.mutate().ptr.origin.y = line.bounds.origin.y + offset;
             }
 
-            // Calculate the distance from baseline to the top of the biggest box with 'bottom' value.
+            // Calculate the distance from baseline to the top of the biggest box with 'bottom'
-            // Then, if necessary, update the topmost.
+            // value. Then, if necessary, update the topmost.
             let topmost_of_bottom = biggest_bottom - bottommost;
             if topmost_of_bottom > topmost {
                 topmost = topmost_of_bottom;
             }
 
-            // Calculate the distance from baseline to the bottom of the biggest box with 'top' value.
+            // Calculate the distance from baseline to the bottom of the biggest box with 'top'
-            // Then, if necessary, update the bottommost.
+            // value. Then, if necessary, update the bottommost.
             let bottommost_of_top = biggest_top - topmost;
             if bottommost_of_top > bottommost {
                 bottommost = bottommost_of_top;
@@ -857,7 +870,8 @@ impl Flow for InlineFlow {
             }
 
             // This is used to set the top y position of the next linebox in the next loop.
-            line_height_offset = line_height_offset + topmost + bottommost - line.bounds.size.height;
+            line_height_offset = line_height_offset + topmost + bottommost -
+                line.bounds.size.height;
             line.bounds.size.height = topmost + bottommost;
         } // End of `lines.each` loop.