From 3995d967da34d9cb0197974f931b697f730bd1a3 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Mon, 6 May 2013 20:29:13 -0700 Subject: [PATCH] layout: Remove duplicate methods from flows --- src/servo/layout/block.rs | 56 ++++++------- src/servo/layout/box_builder.rs | 102 ++++++++++------------- src/servo/layout/display_list_builder.rs | 24 +++--- src/servo/layout/flow.rs | 98 ++++++++++------------ src/servo/layout/layout_task.rs | 13 +-- src/servo/layout/root.rs | 8 +- 6 files changed, 134 insertions(+), 167 deletions(-) diff --git a/src/servo/layout/block.rs b/src/servo/layout/block.rs index 43864617714..67886bd9960 100644 --- a/src/servo/layout/block.rs +++ b/src/servo/layout/block.rs @@ -16,7 +16,7 @@ use geom::point::Point2D; use geom::rect::Rect; use gfx::display_list::DisplayList; use gfx::geometry::Au; -use servo_util::tree::TreeUtils; +use servo_util::tree::{TreeNodeRef, TreeUtils}; pub struct BlockFlowData { /// Data common to all flows. @@ -99,9 +99,9 @@ impl BlockLayout for FlowContext { for self.each_child |child_ctx| { assert!(child_ctx.starts_block_flow() || child_ctx.starts_inline_flow()); - do child_ctx.with_common_info |child_info| { - min_width = au::max(min_width, child_info.min_width); - pref_width = au::max(pref_width, child_info.pref_width); + do child_ctx.with_immutable_node |child_node| { + min_width = au::max(min_width, child_node.min_width); + pref_width = au::max(pref_width, child_node.pref_width); } } @@ -112,39 +112,37 @@ impl BlockLayout for FlowContext { pref_width = pref_width.add(&box.get_pref_width(ctx)); } - do self.with_common_info |info| { - info.min_width = min_width; - info.pref_width = pref_width; + do self.with_mutable_node |this_node| { + this_node.min_width = min_width; + this_node.pref_width = pref_width; } } - /* 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. - - Dual boxes consume some width first, and the remainder is assigned to - all child (block) contexts. */ - - fn assign_widths_block(&self, _ctx: &LayoutContext) { + /// 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. + /// + /// Dual boxes consume some width first, and the remainder is assigned to all child (block) + /// contexts. + fn assign_widths_block(&self, _: &LayoutContext) { assert!(self.starts_block_flow()); - let mut remaining_width = self.with_common_info(|info| info.position.size.width); + let mut remaining_width = self.with_immutable_node(|this| this.position.size.width); let mut _right_used = Au(0); let mut left_used = Au(0); - /* Let the box consume some width. It will return the amount remaining - for its children. */ + // Let the box consume some width. It will return the amount remaining for its children. do self.with_block_box |box| { box.d().position.size.width = remaining_width; let (left_used, right_used) = box.get_used_width(); remaining_width -= left_used.add(&right_used); } - for self.each_child |child_ctx| { - assert!(child_ctx.starts_block_flow() || child_ctx.starts_inline_flow()); - do child_ctx.with_common_info |child_info| { - child_info.position.origin.x = left_used; - child_info.position.size.width = remaining_width; + for self.each_child |kid| { + assert!(kid.starts_block_flow() || kid.starts_inline_flow()); + + do kid.with_mutable_node |child_node| { + child_node.position.origin.x = left_used; + child_node.position.size.width = remaining_width; } } } @@ -154,15 +152,15 @@ impl BlockLayout for FlowContext { let mut cur_y = Au(0); - for self.each_child |child_ctx| { - do child_ctx.with_common_info |child_info| { - child_info.position.origin.y = cur_y; - cur_y += child_info.position.size.height; + for self.each_child |kid| { + do kid.with_mutable_node |child_node| { + child_node.position.origin.y = cur_y; + cur_y += child_node.position.size.height; } } - do self.with_common_info |info| { - info.position.size.height = cur_y; + do self.with_mutable_node |this_node| { + this_node.position.size.height = cur_y; } let _used_top = Au(0); diff --git a/src/servo/layout/box_builder.rs b/src/servo/layout/box_builder.rs index f7638e07c5d..10620b533af 100644 --- a/src/servo/layout/box_builder.rs +++ b/src/servo/layout/box_builder.rs @@ -19,7 +19,7 @@ use gfx::image::holder::ImageHolder; use newcss::values::{CSSDisplay, CSSDisplayBlock, CSSDisplayInline, CSSDisplayInlineBlock}; use newcss::values::{CSSDisplayNone}; use servo_util::range::Range; -use servo_util::tree::TreeUtils; +use servo_util::tree::{TreeNodeRef, TreeUtils}; pub struct LayoutTreeBuilder { root_flow: Option, @@ -122,7 +122,6 @@ impl BoxGenerator { // depending on flow, make a box for this node. match self.flow { InlineFlow(inline) => { - let mut inline = &mut *inline; let node_range_start = inline.boxes.len(); self.range_stack.push(node_range_start); @@ -140,42 +139,31 @@ impl BoxGenerator { } // TODO: cases for inline-block, etc. }, - BlockFlow(*) => { - do self.flow.with_common_info |flow_info| { - debug!("BoxGenerator[f%d]: point b", flow_info.id); - let new_box = builder.make_box(ctx, box_type, node, self.flow); - debug!("BoxGenerator[f%d]: attaching box[b%d] to block flow (node: %s)", - flow_info.id, - new_box.d().id, - node.debug_str()); + BlockFlow(block) => { + debug!("BoxGenerator[f%d]: point b", block.common.id); + let new_box = builder.make_box(ctx, box_type, node, self.flow); - assert!(self.flow.block().box.is_none()); - //XXXjdm We segfault when returning without this temporary. - let block = self.flow.block(); - block.box = Some(new_box); - } - }, - RootFlow(*) => { - do self.flow.with_common_info |info| { - debug!("BoxGenerator[f%d]: point c", info.id); - let new_box = builder.make_box(ctx, box_type, node, self.flow); - debug!("BoxGenerator[f%d]: (node is: %s)", info.id, node.debug_str()); - debug!("BoxGenerator[f%d]: attaching box[b%d] to root flow (node: %s)", - info.id, - new_box.d().id, - node.debug_str()); + debug!("BoxGenerator[f%d]: attaching box[b%d] to block flow (node: %s)", + block.common.id, + new_box.d().id, + node.debug_str()); - assert!(self.flow.root().box.is_none()); - //XXXjdm We segfault when returning without this temporary. - let root = self.flow.root(); - root.box = Some(new_box); - } + assert!(block.box.is_none()); + block.box = Some(new_box); }, - _ => { - do self.flow.with_common_info |flow_info| { - warn!("push_node() not implemented for flow f%d", flow_info.id) - } - } + RootFlow(root) => { + debug!("BoxGenerator[f%d]: point c", root.common.id); + let new_box = builder.make_box(ctx, box_type, node, self.flow); + debug!("BoxGenerator[f%d]: (node is: %s)", root.common.id, node.debug_str()); + debug!("BoxGenerator[f%d]: attaching box[b%d] to root flow (node: %s)", + root.common.id, + new_box.d().id, + node.debug_str()); + + assert!(root.box.is_none()); + root.box = Some(new_box); + }, + _ => warn!("push_node() not implemented for flow f%d", self.flow.id()), } } @@ -236,14 +224,10 @@ impl BuilderContext { priv fn attach_child_flow(&self, child: FlowContext) { let default_collector = &mut *self.default_collector; - do default_collector.flow.with_common_info |flow_info| { - do child.with_common_info |child_flow_info| { - debug!("BuilderContext: Adding child flow f%? of f%?", - flow_info.id, - child_flow_info.id); - default_collector.flow.add_child(child); - } - } + debug!("BuilderContext: Adding child flow f%? of f%?", + default_collector.flow.id(), + child.id()); + default_collector.flow.add_child(child); } priv fn create_child_flow_of_type(&self, @@ -352,23 +336,22 @@ pub impl LayoutTreeBuilder { let flow = &mut this_ctx.default_collector.flow; let flow: &FlowContext = flow; for flow.each_child |child_flow| { - do child_flow.with_common_info |child_flow_info| { - let node = child_flow_info.node; - assert!(node.has_layout_data()); - node.layout_data().flow = Some(child_flow); + do child_flow.with_immutable_node |child_node| { + let dom_node = child_node.node; + assert!(dom_node.has_layout_data()); + dom_node.layout_data().flow = Some(child_flow); } } } - // Fixup any irregularities such as: - // - // * split inlines (CSS 2.1 Section 9.2.1.1) - // * elide non-preformatted whitespace-only text boxes and their - // flows (CSS 2.1 Section 9.2.2.1). - // - // The latter can only be done immediately adjacent to, or at the - // beginning or end of a block flow. Otherwise, the whitespace - // might affect whitespace collapsing with adjacent text. + /// Fix up any irregularities such as: + /// + /// * split inlines (CSS 2.1 Section 9.2.1.1) + /// * elide non-preformatted whitespace-only text boxes and their flows (CSS 2.1 Section + /// 9.2.2.1). + /// + /// The latter can only be done immediately adjacent to, or at the beginning or end of a block + /// flow. Otherwise, the whitespace might affect whitespace collapsing with adjacent text. fn simplify_children_of_flow(&self, _: &LayoutContext, parent_ctx: &BuilderContext) { match parent_ctx.default_collector.flow { InlineFlow(*) => { @@ -394,10 +377,9 @@ pub impl LayoutTreeBuilder { // of its RenderBox or FlowContext children, and possibly keep alive other junk let parent_flow = parent_ctx.default_collector.flow; - let (first_child, last_child) = - do parent_flow.with_common_info |parent_flow_info| { - (parent_flow_info.first_child, parent_flow_info.last_child) - }; + let (first_child, last_child) = do parent_flow.with_immutable_node |parent_node| { + (parent_node.first_child, parent_node.last_child) + }; // check first/last child for whitespace-ness for first_child.each |first_flow| { diff --git a/src/servo/layout/display_list_builder.rs b/src/servo/layout/display_list_builder.rs index 85eed2db274..55b26180f5e 100644 --- a/src/servo/layout/display_list_builder.rs +++ b/src/servo/layout/display_list_builder.rs @@ -16,14 +16,14 @@ use geom::rect::Rect; use gfx::display_list::DisplayList; use gfx::geometry::Au; use gfx; +use servo_util::tree::TreeNodeRef; -/** A builder object that manages display list builder should mainly - hold information about the initial request and desired result---for - example, whether the DisplayList to be used for painting or hit - testing. This can affect which boxes are created. - - Right now, the builder isn't used for much, but it establishes the - pattern we'll need once we support DL-based hit testing &c. */ +/// A builder object that manages display list builder should mainly hold information about the +/// initial request and desired result--for example, whether the `DisplayList` is to be used for +/// painting or hit testing. This can affect which boxes are created. +/// +/// Right now, the builder isn't used for much, but it establishes the pattern we'll need once we +/// support display-list-based hit testing and so forth. pub struct DisplayListBuilder<'self> { ctx: &'self LayoutContext, } @@ -53,13 +53,13 @@ impl FlowDisplayListBuilderMethods for FlowContext { dirty: &Rect, offset: &Point2D, list: &Cell) { - // adjust the dirty rect to child flow context coordinates - do child_flow.with_common_info |child_flow_info| { - let abs_flow_bounds = child_flow_info.position.translate(offset); - let adj_offset = offset.add(&child_flow_info.position.origin); + // Adjust the dirty rect to child flow context coordinates. + do child_flow.with_immutable_node |child_node| { + let abs_flow_bounds = child_node.position.translate(offset); + let adj_offset = offset.add(&child_node.position.origin); debug!("build_display_list_for_child: rel=%?, abs=%?", - child_flow_info.position, + child_node.position, abs_flow_bounds); debug!("build_display_list_for_child: dirty=%?, offset=%?", dirty, offset); diff --git a/src/servo/layout/flow.rs b/src/servo/layout/flow.rs index 6086b974660..facbd1de3d6 100644 --- a/src/servo/layout/flow.rs +++ b/src/servo/layout/flow.rs @@ -73,10 +73,44 @@ impl Clone for FlowContext { impl TreeNodeRef for FlowContext { fn with_immutable_node(&self, callback: &fn(&FlowData) -> R) -> R { - self.with_common_imm_info(callback) + match *self { + AbsoluteFlow(info) => callback(info), + BlockFlow(info) => { + let info = &*info; // FIXME: Borrow check workaround. + callback(&info.common) + } + FloatFlow(info) => callback(info), + InlineBlockFlow(info) => callback(info), + InlineFlow(info) => { + let info = &*info; // FIXME: Borrow check workaround. + callback(&info.common) + } + RootFlow(info) => { + let info = &*info; // FIXME: Borrow check workaround. + callback(&info.common) + } + TableFlow(info) => callback(info), + } } fn with_mutable_node(&self, callback: &fn(&mut FlowData) -> R) -> R { - self.with_common_info(callback) + match *self { + AbsoluteFlow(info) => callback(info), + BlockFlow(info) => { + let info = &mut *info; // FIXME: Borrow check workaround. + callback(&mut info.common) + } + FloatFlow(info) => callback(info), + InlineBlockFlow(info) => callback(info), + InlineFlow(info) => { + let info = &mut *info; // FIXME: Borrow check workaround. + callback(&mut info.common) + } + RootFlow(info) => { + let info = &mut *info; // FIXME: Borrow check workaround. + callback(&mut info.common) + } + TableFlow(info) => callback(info), + } } } @@ -167,62 +201,20 @@ impl FlowData { } impl<'self> FlowContext { - // FIXME: This method is a duplicate of `with_immutable_node`; fix this. + /// A convenience method to return the position of this flow. Fails if the flow is currently + /// being borrowed mutably. #[inline(always)] - pub fn with_common_imm_info(&self, block: &fn(&FlowData) -> R) -> R { - match *self { - AbsoluteFlow(info) => block(info), - BlockFlow(info) => { - let info = &*info; // FIXME: Borrow check workaround. - block(&info.common) - } - FloatFlow(info) => block(info), - InlineBlockFlow(info) => block(info), - InlineFlow(info) => { - let info = &*info; // FIXME: Borrow check workaround. - block(&info.common) - } - RootFlow(info) => { - let info = &*info; // FIXME: Borrow check workaround. - block(&info.common) - } - TableFlow(info) => block(info), - } - } - - // FIXME: This method is a duplicate of `with_mutable_node`; fix this. - #[inline(always)] - pub fn with_common_info(&self, block: &fn(&mut FlowData) -> R) -> R { - match *self { - AbsoluteFlow(info) => block(info), - BlockFlow(info) => { - let info = &mut *info; // FIXME: Borrow check workaround. - block(&mut info.common) - } - FloatFlow(info) => block(info), - InlineBlockFlow(info) => block(info), - InlineFlow(info) => { - let info = &mut *info; // FIXME: Borrow check workaround. - block(&mut info.common) - } - RootFlow(info) => { - let info = &mut *info; // FIXME: Borrow check workaround. - block(&mut info.common) - } - TableFlow(info) => block(info), - } - } - pub fn position(&self) -> Rect { - do self.with_common_info |common_info| { + do self.with_immutable_node |common_info| { common_info.position } } - /// Returns the ID of this flow. + /// A convenience method to return the ID of this flow. Fails if the flow is currently being + /// borrowed mutably. #[inline(always)] pub fn id(&self) -> int { - do self.with_common_info |info| { + do self.with_immutable_node |info| { info.id } } @@ -280,7 +272,7 @@ impl<'self> FlowContext { dirty: &Rect, offset: &Point2D, list: &Cell) { - do self.with_common_info |info| { + do self.with_immutable_node |info| { debug!("FlowContext::build_display_list at %?: %s", info.position, self.debug_str()); } @@ -414,8 +406,8 @@ impl DebugMethods for FlowContext { _ => ~"(Unknown flow)" }; - do self.with_common_info |info| { - fmt!("f%? %?", info.id, repr) + do self.with_immutable_node |this_node| { + fmt!("f%? %?", this_node.id, repr) } } } diff --git a/src/servo/layout/layout_task.rs b/src/servo/layout/layout_task.rs index c445c7fdbb2..d03ac503e67 100644 --- a/src/servo/layout/layout_task.rs +++ b/src/servo/layout/layout_task.rs @@ -245,18 +245,13 @@ impl Layout { let display_list = @Cell(DisplayList::new()); - // TODO: set options on the builder before building - // TODO: be smarter about what needs painting - do layout_root.with_common_info |layout_root_info| { - layout_root.build_display_list(&builder, - © layout_root_info.position, - display_list); - } + // TODO: Set options on the builder before building. + // TODO: Be smarter about what needs painting. + layout_root.build_display_list(&builder, &layout_root.position(), display_list); let render_layer = RenderLayer { display_list: display_list.take(), - size: Size2D(screen_size.width.to_px() as uint, - screen_size.height.to_px() as uint) + size: Size2D(screen_size.width.to_px() as uint, screen_size.height.to_px() as uint) }; self.render_task.send(RenderMsg(render_layer)); diff --git a/src/servo/layout/root.rs b/src/servo/layout/root.rs index 46abe8489c6..4a0150dda3a 100644 --- a/src/servo/layout/root.rs +++ b/src/servo/layout/root.rs @@ -13,7 +13,7 @@ use layout::context::LayoutContext; use layout::flow::{FlowContext, FlowData, RootFlow}; use layout::display_list_builder::DisplayListBuilder; -use servo_util::tree::TreeUtils; +use servo_util::tree::{TreeNodeRef, TreeUtils}; pub struct RootFlowData { /// Data common to all flows. @@ -64,9 +64,9 @@ impl RootFlowData { let mut cur_y = Au(0); for RootFlow(self).each_child |child_flow| { - do child_flow.with_common_info |child_common_info| { - child_common_info.position.origin.y = cur_y; - cur_y += child_common_info.position.size.height; + do child_flow.with_mutable_node |child_node| { + child_node.position.origin.y = cur_y; + cur_y += child_node.position.size.height; } }