Don't use a whole in-order traversal for computing heights.

src/components/main/layout/block.rs

@@ -10,7 +10,7 @@ use layout::display_list_builder::{DisplayListBuilder, ExtraDisplayListData};
 use layout::flow::{BlockFlow, FlowContext, FlowData, InlineBlockFlow, FloatFlow};
 use layout::inline::InlineLayout;
 use layout::model::{MaybeAuto, Specified, Auto};
-use layout::float_context::FloatContext;
+use layout::float_context::{FloatContext, Invalid};
 
 use newcss::values::{CSSClearNone, CSSClearLeft, CSSClearRight, CSSClearBoth};
 use layout::float_context::{ClearLeft, ClearRight, ClearBoth};
@@ -187,6 +187,7 @@ impl BlockFlowData {
             self.common.position.origin = Au::zero_point();
             self.common.position.size.width = ctx.screen_size.size.width;
             self.common.floats_in = FloatContext::new(self.common.num_floats);
+            self.common.is_inorder = false;
         }
 
         //position was set to the containing block by the flow's parent
@@ -239,22 +240,45 @@ impl BlockFlowData {
             }
         }
 
+        let has_inorder_children = self.common.is_inorder || self.common.num_floats > 0;
         for BlockFlow(self).each_child |kid| {
             assert!(kid.starts_block_flow() || kid.starts_inline_flow());
 
             do kid.with_mut_base |child_node| {
                 child_node.position.origin.x = x_offset;
                 child_node.position.size.width = remaining_width;
+                child_node.is_inorder = has_inorder_children;
+
+                if !child_node.is_inorder {
+                    child_node.floats_in = FloatContext::new(0);
+                }
             }
         }
     }
 
+    pub fn assign_height_inorder_block(@mut self, ctx: &mut LayoutContext) {
+        debug!("assign_height_inorder_block: assigning height for block %?", self.common.id);
+        self.assign_height_block_base(ctx, true);
+    }
+
     pub fn assign_height_block(@mut self, ctx: &mut LayoutContext) {
+        debug!("assign_height_block: assigning height for block %?", self.common.id);
+        // This is the only case in which a block flow can start an inorder
+        // subtraversal.
+        if self.is_root && self.common.num_floats > 0 {
+            self.assign_height_inorder_block(ctx);
+            return;
+        }
+        self.assign_height_block_base(ctx, false);
+    }
+
+    fn assign_height_block_base(@mut self, ctx: &mut LayoutContext, inorder: bool) {
         let mut cur_y = Au(0);
         let mut clearance = Au(0);
         let mut top_offset = Au(0);
         let mut bottom_offset = Au(0);
         let mut left_offset = Au(0);
+        let mut float_ctx = Invalid;
 
         for self.box.iter().advance |&box| {
             let style = box.style();
@@ -279,27 +303,25 @@ impl BlockFlowData {
             };
         }
 
-        // TODO(eatkinson): the translation here is probably
+        if inorder {
-        // totally wrong. We need to do it right or pages
+            // Floats for blocks work like this:
-        // with floats will look very strange.
+            // self.floats_in -> child[0].floats_in
+            // visit child[0]
+            // child[i-1].floats_out -> child[i].floats_in
+            // visit child[i]
+            // repeat until all children are visited.
+            // last_child.floats_out -> self.floats_out (done at the end of this method)
+            float_ctx = self.common.floats_in.translate(Point2D(-left_offset, -top_offset));
+            for BlockFlow(self).each_child |kid| {
+                do kid.with_mut_base |child_node| {
+                    child_node.floats_in = float_ctx.clone();
+                }
+                kid.assign_height_inorder(ctx);
+                do kid.with_mut_base |child_node| {
+                    float_ctx = child_node.floats_out.clone();
+                }
 
-        // Floats for blocks work like this:
-        // self.floats_in -> child[0].floats_in
-        // visit child[0]
-        // child[i-1].floats_out -> child[i].floats_in
-        // visit child[i]
-        // repeat until all children are visited.
-        // last_child.floats_out -> self.floats_out (done at the end of this method)
-        let mut float_ctx = self.common.floats_in.translate(Point2D(-left_offset, -top_offset));
-        for BlockFlow(self).each_child |kid| {
-            do kid.with_mut_base |child_node| {
-                child_node.floats_in = float_ctx.clone();
             }
-            kid.assign_height(ctx);
-            do kid.with_mut_base |child_node| {
-                float_ctx = child_node.floats_out.clone();
-            }
-
         }
         for BlockFlow(self).each_child |kid| {
             do kid.with_mut_base |child_node| {
@@ -311,7 +333,7 @@ impl BlockFlowData {
         let mut height = if self.is_root {
             Au::max(ctx.screen_size.size.height, cur_y)
         } else {
-                cur_y - top_offset
+            cur_y - top_offset
         };
 
         for self.box.iter().advance |&box| {
@@ -338,8 +360,12 @@ impl BlockFlowData {
         //TODO(eatkinson): compute heights using the 'height' property.
         self.common.position.size.height = height + noncontent_height;
 
-        let extra_height = height - (cur_y - top_offset) + bottom_offset; 
+        if inorder {
-        self.common.floats_out = float_ctx.translate(Point2D(left_offset, -extra_height));
+            let extra_height = height - (cur_y - top_offset) + bottom_offset; 
+            self.common.floats_out = float_ctx.translate(Point2D(left_offset, -extra_height));
+        } else {
+            self.common.floats_out = self.common.floats_in.clone();
+        }
     }
 
     pub fn build_display_list_block<E:ExtraDisplayListData>(@mut self,

src/components/main/layout/box.rs

@@ -271,7 +271,7 @@ impl RenderBox {
 
     /// Attempts to split this box so that its width is no more than `max_width`. Fails if this box
     /// is an unscanned text box.
-    pub fn split_to_width(&self, _: &LayoutContext, max_width: Au, starts_line: bool)
+    pub fn split_to_width(&self, max_width: Au, starts_line: bool)
                       -> SplitBoxResult {
         match *self {
             GenericRenderBoxClass(*) | ImageRenderBoxClass(*) => CannotSplit(*self),

src/components/main/layout/float.rs

@@ -35,6 +35,9 @@ pub struct FloatFlowData {
     /// Index into the box list for inline floats
     index: Option<uint>,
 
+    /// Number of floated children
+    floated_children: uint,
+
 }
 
 impl FloatFlowData {
@@ -46,6 +49,7 @@ impl FloatFlowData {
             index: None,
             float_type: float_type,
             rel_pos: Point2D(Au(0), Au(0)),
+            floated_children: 0,
         }
     }
 
@@ -63,7 +67,7 @@ impl FloatFlowData {
     pub fn bubble_widths_float(@mut self, ctx: &LayoutContext) {
         let mut min_width = Au(0);
         let mut pref_width = Au(0);
-        let mut num_floats = 1;
+        let mut num_floats = 0;
 
         for FloatFlow(self).each_child |child_ctx| {
             //assert!(child_ctx.starts_block_flow() || child_ctx.starts_inline_flow());
@@ -71,12 +75,12 @@ impl FloatFlowData {
             do child_ctx.with_mut_base |child_node| {
                 min_width = geometry::max(min_width, child_node.min_width);
                 pref_width = geometry::max(pref_width, child_node.pref_width);
-
                 num_floats = num_floats + child_node.num_floats;
             }
         }
 
-        self.common.num_floats = num_floats;
+        self.common.num_floats = 1;
+        self.floated_children = num_floats;
 
 
         self.box.map(|&box| {
@@ -93,13 +97,16 @@ impl FloatFlowData {
         self.common.pref_width = pref_width;
     }
 
-    pub fn assign_widths_float(@mut self, _: &LayoutContext) { 
+    pub fn assign_widths_float(@mut self) { 
         debug!("assign_widths_float: assigning width for flow %?",  self.common.id);
         // position.size.width is set by parent even though we don't know
         // position.origin yet.
         let mut remaining_width = self.common.position.size.width;
         self.containing_width = remaining_width;
         let mut x_offset = Au(0);
+        
+        // Parent usually sets this, but floats are never inorder
+        self.common.is_inorder = false;
 
         for self.box.iter().advance |&box| {
             let style = box.style();
@@ -154,25 +161,77 @@ impl FloatFlowData {
 
         self.common.position.size.width = remaining_width;
 
+        let has_inorder_children = self.common.num_floats > 0;
         for FloatFlow(self).each_child |kid| {
             //assert!(kid.starts_block_flow() || kid.starts_inline_flow());
 
             do kid.with_mut_base |child_node| {
                 child_node.position.origin.x = x_offset;
                 child_node.position.size.width = remaining_width;
+                child_node.is_inorder = has_inorder_children;
+
+                if !child_node.is_inorder {
+                    child_node.floats_in = FloatContext::new(0);
+                }
             }
         }
     }
 
-    pub fn assign_height_float(@mut self, ctx: &mut LayoutContext) {
+    pub fn assign_height_inorder_float(@mut self) {
-        let mut float_ctx = FloatContext::new(self.common.num_floats);
+        debug!("assign_height_inorder_float: assigning height for float %?", self.common.id);
-        for FloatFlow(self).each_child |kid| {
+        // assign_height_float was already called by the traversal function
-            do kid.with_mut_base |child_node| {
+        // so this is well-defined
-                child_node.floats_in = float_ctx.clone();
+
+        let mut height = Au(0);
+        let mut full_noncontent_width = Au(0);
+        let mut full_noncontent_height = Au(0);
+
+        self.box.map(|&box| {
+            height = do box.with_base |base| {
+                base.position.size.height
+            };
+
+            do box.with_base |base| {
+
+            let noncontent_width = base.model.padding.left + base.model.padding.right +
+                base.model.border.left + base.model.border.right;
+            let noncontent_height = base.model.padding.top + base.model.padding.bottom +
+                base.model.border.top + base.model.border.bottom;
+
+            full_noncontent_width = noncontent_width + base.model.margin.left + base.model.margin.right;
+            full_noncontent_height = noncontent_height + base.model.margin.top + base.model.margin.bottom;
+
             }
-            kid.assign_height(ctx);
+
-            do kid.with_mut_base |child_node| {
+        });
-                float_ctx = child_node.floats_out.clone();
+
+        let info = PlacementInfo {
+            width: self.common.position.size.width + full_noncontent_width,
+            height: height + full_noncontent_height,
+            ceiling: Au(0),
+            max_width: self.containing_width,
+            f_type: self.float_type,
+        };
+
+        // Place the float and return the FloatContext back to the parent flow.
+        // After, grab the position and use that to set our position.
+        self.common.floats_out = self.common.floats_in.add_float(&info);
+        self.rel_pos = self.common.floats_out.last_float_pos();
+    }
+
+    pub fn assign_height_float(@mut self, ctx: &mut LayoutContext) {
+        debug!("assign_height_float: assigning height for float %?", self.common.id);
+        let has_inorder_children = self.common.num_floats > 0;
+        if has_inorder_children {
+            let mut float_ctx = FloatContext::new(self.floated_children);
+            for FloatFlow(self).each_child |kid| {
+                do kid.with_mut_base |child_node| {
+                    child_node.floats_in = float_ctx.clone();
+                }
+                kid.assign_height_inorder(ctx);
+                do kid.with_mut_base |child_node| {
+                    float_ctx = child_node.floats_out.clone();
+                }
             }
         }
 
@@ -197,8 +256,6 @@ impl FloatFlowData {
 
         let mut noncontent_width = Au(0);
         let mut noncontent_height = Au(0);
-        let mut full_noncontent_width = Au(0);
-        let mut full_noncontent_height = Au(0);
         self.box.map(|&box| {
             do box.with_mut_base |base| {
                 //The associated box is the border box of this flow
@@ -210,8 +267,6 @@ impl FloatFlowData {
                     base.model.border.top + base.model.border.bottom;
                 base.position.size.height = height + noncontent_height;
 
-                full_noncontent_width = noncontent_width + base.model.margin.left + base.model.margin.right;
-                full_noncontent_height = noncontent_height + base.model.margin.top + base.model.margin.bottom;
             }
         });
 
@@ -229,19 +284,6 @@ impl FloatFlowData {
                 base.position.size.height = height;
             }
         }
-
-        let info = PlacementInfo {
-            width: self.common.position.size.width + full_noncontent_width,
-            height: height + full_noncontent_height,
-            ceiling: Au(0),
-            max_width: self.containing_width,
-            f_type: self.float_type,
-        };
-
-        // Place the float and return the FloatContext back to the parent flow.
-        // After, grab the position and use that to set our position.
-        self.common.floats_out = self.common.floats_in.add_float(&info);
-        self.rel_pos = self.common.floats_out.last_float_pos();
     }
 
     pub fn build_display_list_float<E:ExtraDisplayListData>(@mut self,

src/components/main/layout/flow.rs

@@ -94,6 +94,21 @@ impl FlowContext {
             kid.partially_traverse_preorder(|a| callback(a));
         }
     }
+
+    fn traverse_bu_sub_inorder (&self, callback: &fn(FlowContext) -> bool) -> bool {
+        for self.each_child |kid| {
+            // FIXME: Work around rust#2202. We should be able to pass the callback directly.
+            if !kid.traverse_bu_sub_inorder(|a| callback(a)) {
+                return false;
+            }
+        }
+
+        if !self.is_inorder() {
+            callback((*self).clone())
+        } else {
+            true
+        }
+    }
 }
 
 impl FlowData {
@@ -177,7 +192,8 @@ pub struct FlowData {
     floats_in: FloatContext,
     floats_out: FloatContext,
     num_floats: uint,
-    abs_position: Point2D<Au>
+    abs_position: Point2D<Au>,
+    is_inorder: bool,
 }
 
 impl TreeNode<FlowContext> for FlowData {
@@ -242,7 +258,8 @@ impl FlowData {
             floats_in: Invalid,
             floats_out: Invalid,
             num_floats: 0,
-            abs_position: Point2D(Au(0), Au(0))
+            abs_position: Point2D(Au(0), Au(0)),
+            is_inorder: false
         }
     }
 }
@@ -257,6 +274,13 @@ impl<'self> FlowContext {
         }
     }
 
+    #[inline(always)]
+    pub fn is_inorder(&self) -> bool {
+        do self.with_base |common_info| {
+            common_info.is_inorder
+        }
+    }
+
     /// A convenience method to return the ID of this flow. Fails if the flow is currently being
     /// borrowed mutably.
     #[inline(always)]
@@ -266,14 +290,6 @@ impl<'self> FlowContext {
         }
     }
 
-    /// A convenience method to return the restyle damage of this flow. Fails if the flow is
-    /// currently being borrowed mutably.
-    #[inline(always)]
-    pub fn restyle_damage(&self) -> RestyleDamage {
-        do self.with_base |info| {
-            info.restyle_damage
-        }
-    }
 
     pub fn inline(&self) -> @mut InlineFlowData {
         match *self {
@@ -309,7 +325,7 @@ impl<'self> FlowContext {
         match *self {
             BlockFlow(info)  => info.assign_widths_block(ctx),
             InlineFlow(info) => info.assign_widths_inline(ctx),
-            FloatFlow(info)  => info.assign_widths_float(ctx),
+            FloatFlow(info)  => info.assign_widths_float(),
             _ => fail!(fmt!("Tried to assign_widths of flow: f%d", self.id()))
         }
     }
@@ -323,6 +339,15 @@ impl<'self> FlowContext {
         }
     }
 
+    pub fn assign_height_inorder(&self, ctx: &mut LayoutContext) {
+        match *self {
+            BlockFlow(info)  => info.assign_height_inorder_block(ctx),
+            InlineFlow(info) => info.assign_height_inorder_inline(ctx),
+            FloatFlow(info)  => info.assign_height_inorder_float(),
+            _ => fail!(fmt!("Tried to assign_height of flow: f%d", self.id()))
+        }
+    }
+
     pub fn build_display_list<E:ExtraDisplayListData>(&self,
                                                      builder: &DisplayListBuilder,
                                                      dirty: &Rect<Au>,
@@ -340,6 +365,15 @@ impl<'self> FlowContext {
         }
 
     }
+    /// A convenience method to return the restyle damage of this flow. Fails if the flow is
+    /// currently being borrowed mutably.
+    #[inline(always)]
+    pub fn restyle_damage(&self) -> RestyleDamage {
+        do self.with_base |info| {
+            info.restyle_damage
+        }
+    }
+
 
     // Actual methods that do not require much flow-specific logic
     pub fn foldl_all_boxes<B:Clone>(&self, seed: B, cb: &fn(a: B, b: RenderBox) -> B) -> B {

src/components/main/layout/inline.rs

@@ -105,7 +105,7 @@ impl LineboxScanner {
         self.pending_line.green_zone = Size2D(Au(0), Au(0))     
     }
 
-    pub fn scan_for_lines(&mut self, ctx: &LayoutContext) {
+    pub fn scan_for_lines(&mut self) {
         self.reset_scanner();
 
         { // FIXME: manually control borrow length
@@ -127,7 +127,7 @@ impl LineboxScanner {
                     box
                 };
 
-                let box_was_appended = self.try_append_to_line(ctx, cur_box);
+                let box_was_appended = self.try_append_to_line(cur_box);
                 if !box_was_appended {
                     debug!("LineboxScanner: Box wasn't appended, because line %u was full.",
                            self.lines.len());
@@ -222,7 +222,7 @@ impl LineboxScanner {
     /// Computes the position of a line that has only the provided RenderBox.
     /// Returns: the bounding rect of the line's green zone (whose origin coincides
     /// with the line's origin) and the actual width of the first box after splitting.
-    fn initial_line_placement (&self, ctx: &LayoutContext, first_box: RenderBox, ceiling: Au) -> (Rect<Au>, Au) {
+    fn initial_line_placement (&self, first_box: RenderBox, ceiling: Au) -> (Rect<Au>, Au) {
         debug!("LineboxScanner: Trying to place first box of line %?", self.lines.len());
         debug!("LineboxScanner: box size: %?", first_box.position().size);
         let splitable = first_box.can_split();
@@ -266,7 +266,7 @@ impl LineboxScanner {
         // FIXME(eatkinson): calling split_to_width here seems excessive and expensive.
         // We should find a better abstraction or merge it with the call in
         // try_append_to_line.
-        match first_box.split_to_width(ctx, line_bounds.size.width, line_is_empty) {
+        match first_box.split_to_width(line_bounds.size.width, line_is_empty) {
             CannotSplit(_) => {
                 error!("LineboxScanner: Tried to split unsplittable render box! %s",
                         first_box.debug_str());
@@ -279,7 +279,7 @@ impl LineboxScanner {
                     (Some(l_box), Some(_))  => l_box.position().size.width,
                     (Some(l_box), None)     => l_box.position().size.width,
                     (None, Some(r_box))     => r_box.position().size.width,
-                    (None, None)            => fail!("This cas makes no sense.")
+                    (None, None)            => fail!("This case makes no sense.")
                 };
                 return (line_bounds, actual_box_width);
             }
@@ -293,7 +293,7 @@ impl LineboxScanner {
                     (Some(l_box), Some(_))  => l_box.position().size.width,
                     (Some(l_box), None)     => l_box.position().size.width,
                     (None, Some(r_box))     => r_box.position().size.width,
-                    (None, None)            => fail!("This cas makes no sense.")
+                    (None, None)            => fail!("This case makes no sense.")
                 };
 
                 info.width = actual_box_width;
@@ -307,11 +307,11 @@ impl LineboxScanner {
     }
 
     /// Returns false only if we should break the line.
-    fn try_append_to_line(&mut self, ctx: &LayoutContext, in_box: RenderBox) -> bool {
+    fn try_append_to_line(&mut self, in_box: RenderBox) -> bool {
         let line_is_empty: bool = self.pending_line.range.length() == 0;
 
         if line_is_empty {
-            let (line_bounds, _) = self.initial_line_placement(ctx, in_box, self.cur_y);
+            let (line_bounds, _) = self.initial_line_placement(in_box, self.cur_y);
             self.pending_line.bounds.origin = line_bounds.origin;
             self.pending_line.green_zone = line_bounds.size;
         }
@@ -348,7 +348,7 @@ impl LineboxScanner {
 
             // 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(ctx, in_box, this_line_y);
+            let (next_line, first_box_width) = self.initial_line_placement(in_box, this_line_y);
             let next_green_zone = next_line.size;
 
             let new_width = self.pending_line.bounds.size.width + first_box_width;
@@ -396,7 +396,7 @@ impl LineboxScanner {
         } else {
             let available_width = green_zone.width - self.pending_line.bounds.size.width;
 
-            match in_box.split_to_width(ctx, available_width, line_is_empty) {
+            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());
@@ -542,7 +542,7 @@ impl InlineFlowData {
 
     /// Recursively (top-down) determines the actual width of child contexts and boxes. When called
     /// on this context, the context has had its width set by the parent context.
-    pub fn assign_widths_inline(@mut self, _: &mut LayoutContext) {
+    pub fn assign_widths_inline(@mut self, _: &LayoutContext) {
         // Initialize content box widths if they haven't been initialized already.
         //
         // TODO: Combine this with `LineboxScanner`'s walk in the box list, or put this into
@@ -574,6 +574,7 @@ impl InlineFlowData {
         for InlineFlow(self).each_child |kid| {
             do kid.with_mut_base |base| {
                 base.position.size.width = self.common.position.size.width;
+                base.is_inorder = self.common.is_inorder;
             }
         }
         // There are no child contexts, so stop here.
@@ -585,11 +586,16 @@ impl InlineFlowData {
         // 'inline-block' box that created this flow before recursing.
     }
 
-    pub fn assign_height_inline(@mut self, ctx: &mut LayoutContext) {
+    pub fn assign_height_inorder_inline(@mut self, ctx: &mut LayoutContext) {
-
         for InlineFlow(self).each_child |kid| {
-            kid.assign_height(ctx);
+            kid.assign_height_inorder(ctx);
         }
+        self.assign_height_inline(ctx);
+    }
+
+    pub fn assign_height_inline(@mut self, _: &LayoutContext) {
+
+        debug!("assign_height_inline: assigning height for flow %?", self.common.id);
 
         // Divide the boxes into lines
         // TODO(#226): Get the CSS `line-height` property from the containing block's style to
@@ -598,7 +604,7 @@ impl InlineFlowData {
         // TODO(#226): Get the CSS `line-height` property from each non-replaced inline element to
         // determine its height for computing linebox height.
         let mut scanner = LineboxScanner::new(InlineFlow(self), self.common.floats_in.clone());
-        scanner.scan_for_lines(ctx);
+        scanner.scan_for_lines();
 
         // Now, go through each line and lay out the boxes inside
         for self.lines.iter().advance |line| {
@@ -765,7 +771,9 @@ impl InlineFlowData {
 
         // TODO(#225): Should `inline-block` elements have flows as children of the inline flow or
         // should the flow be nested inside the box somehow?
-        true
+        
+        // For now, don't traverse the subtree rooted here
+        false
     }
 }
 

src/components/main/layout/layout_task.rs

@@ -251,14 +251,10 @@ impl LayoutTask {
         }
 
         for layout_root.traverse_postorder |flow| {
-            do flow.with_base |base| {
+            for flow.each_child |child| {
-                match base.parent {
+                do child.with_base |child_base| {
-                    None => {},
+                    do flow.with_mut_base |base| {
-                    Some(parent_ctx) => {
+                        base.restyle_damage.union_in_place(child_base.restyle_damage);
-                        let prop = base.restyle_damage.propagate_up();
-                        do parent_ctx.with_mut_base |parent| {
-                            parent.restyle_damage.union_in_place(prop);
-                        }
                     }
                 }
             }
@@ -283,7 +279,9 @@ impl LayoutTask {
 
             // For now, this is an inorder traversal
             // FIXME: prune this traversal as well
-            layout_root.assign_height(&mut layout_ctx);
+            for layout_root.traverse_bu_sub_inorder |flow| {
+                flow.assign_height(&mut layout_ctx);
+            }
         }
 
         // Build the display list if necessary, and send it to the renderer.