From b7ac68813612f40223b8b0c0ffe85c56f642e0ed Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Tue, 7 May 2013 10:15:52 -0700 Subject: [PATCH] layout: Refactor inline layout a bit --- src/servo/layout/inline.rs | 198 ++++++++++++++++++++----------------- 1 file changed, 107 insertions(+), 91 deletions(-) diff --git a/src/servo/layout/inline.rs b/src/servo/layout/inline.rs index 57cbe17de56..706f2b1f8cc 100644 --- a/src/servo/layout/inline.rs +++ b/src/servo/layout/inline.rs @@ -61,30 +61,30 @@ struct ElementMapping { priv entries: ~[NodeRange], } -pub impl ElementMapping { - fn new() -> ElementMapping { +impl ElementMapping { + pub fn new() -> ElementMapping { ElementMapping { entries: ~[] } } - fn add_mapping(&mut self, node: AbstractNode, range: &Range) { + pub fn add_mapping(&mut self, node: AbstractNode, range: &Range) { self.entries.push(NodeRange::new(node, range)) } - fn each(&self, cb: &fn(nr: &NodeRange) -> bool) { + pub fn each(&self, cb: &fn(nr: &NodeRange) -> bool) { do self.entries.each |nr| { cb(nr) } } - fn eachi(&self, cb: &fn(i: uint, nr: &NodeRange) -> bool) { + pub fn eachi(&self, cb: &fn(i: uint, nr: &NodeRange) -> bool) { do self.entries.eachi |i, nr| { cb(i, nr) } } - fn eachi_mut(&self, cb: &fn(i: uint, nr: &NodeRange) -> bool) { + pub fn eachi_mut(&self, cb: &fn(i: uint, nr: &NodeRange) -> bool) { do self.entries.eachi |i, nr| { cb(i, nr) } } - fn repair_for_box_changes(&mut self, - old_boxes: &[@mut RenderBox], - new_boxes: &[@mut RenderBox]) { + pub fn repair_for_box_changes(&mut self, + old_boxes: &[@mut RenderBox], + new_boxes: &[@mut RenderBox]) { let entries = &mut self.entries; debug!("--- Old boxes: ---"); @@ -154,13 +154,12 @@ pub impl ElementMapping { } } -// stack-allocated object for scanning an inline flow into -// TextRun-containing TextBoxes. -priv struct TextRunScanner { +/// A stack-allocated object for scanning an inline flow into `TextRun`-containing `TextBox`es. +struct TextRunScanner { clump: Range, } -priv impl TextRunScanner { +impl TextRunScanner { fn new() -> TextRunScanner { TextRunScanner { clump: Range::empty(), @@ -168,7 +167,7 @@ priv impl TextRunScanner { } } -priv impl TextRunScanner { +impl TextRunScanner { fn scan_for_runs(&mut self, ctx: &mut LayoutContext, flow: FlowContext) { let inline = &mut *flow.inline(); assert!(inline.boxes.len() > 0); @@ -206,18 +205,16 @@ priv impl TextRunScanner { } } - // a 'clump' is a range of inline flow leaves that can be merged - // together into a single RenderBox. Adjacent text with the same - // style can be merged, and nothing else can. - // - // the flow keeps track of the RenderBoxes contained by all - // non-leaf DOM nodes. This is necessary for correct painting - // order. Since we compress several leaf RenderBoxes here, the - // mapping must be adjusted. - // - // N.B. in_boxes is passed by reference, since we cannot - // recursively borrow or swap the flow's dvec of boxes. When all - // boxes are appended, the caller swaps the flow's box list. + /// A "clump" is a range of inline flow leaves that can be merged together into a single + /// `RenderBox`. Adjacent text with the same style can be merged, and nothing else can. + /// + /// The flow keeps track of the `RenderBox`es contained by all non-leaf DOM nodes. This is + /// necessary for correct painting order. Since we compress several leaf `RenderBox`es here, + /// the mapping must be adjusted. + /// + /// N.B. `in_boxes` is passed by reference, since the old code used a `DVec`. The caller is + /// responsible for swapping out the list. It is not clear to me (pcwalton) that this is still + /// necessary. fn flush_clump_to_list(&mut self, ctx: &mut LayoutContext, flow: FlowContext, @@ -227,8 +224,8 @@ priv impl TextRunScanner { debug!("TextRunScanner: flushing boxes in range=%?", self.clump); let is_singleton = self.clump.length() == 1; - let is_text_clump = match in_boxes[self.clump.begin()] { - @UnscannedTextBox(*) => true, + let is_text_clump = match *in_boxes[self.clump.begin()] { + UnscannedTextBox(*) => true, _ => false }; @@ -244,14 +241,18 @@ priv impl TextRunScanner { let old_box = in_boxes[self.clump.begin()]; let text = old_box.raw_text(); let font_style = old_box.font_style(); - // TODO(Issue #115): use actual CSS 'white-space' property of relevant style. + + // TODO(#115): Use the actual CSS `white-space` property of the relevant style. let compression = CompressWhitespaceNewline; + let transformed_text = transform_text(text, compression); - // TODO(Issue #177): text run creation must account for text-renderability by fontgroup fonts. - // this is probably achieved by creating fontgroup above, and then letting FontGroup decide - // which Font to stick into the TextRun. + + // TODO(#177): Text run creation must account for the renderability of text by + // font group fonts. This is probably achieved by creating the font group above + // and then letting `FontGroup` decide which `Font` to stick into the text run. let fontgroup = ctx.font_ctx.get_resolved_font_for_style(&font_style); let run = @fontgroup.create_textrun(transformed_text); + debug!("TextRunScanner: pushing single text box in range: %?", self.clump); let new_box = adapt_textbox_with_range(old_box.d(), run, @@ -259,23 +260,23 @@ priv impl TextRunScanner { out_boxes.push(new_box); }, (false, true) => { - // TODO(Issue #115): use actual CSS 'white-space' property of relevant style. + // TODO(#115): Use the actual CSS `white-space` property of the relevant style. let compression = CompressWhitespaceNewline; - // first, transform/compress text of all the nodes - let transformed_strs : ~[~str] = vec::from_fn(self.clump.length(), |i| { - // TODO(Issue #113): we shoud be passing compression context - // between calls to transform_text, so that boxes - // starting/ending with whitespace &c can be - // compressed correctly w.r.t. the TextRun. + // First, transform/compress text of all the nodes. + let transformed_strs: ~[~str] = do vec::from_fn(self.clump.length()) |i| { + // TODO(#113): We should be passing the compression context between calls to + // `transform_text`, so that boxes starting and/or ending with whitespace can + // be compressed correctly with respect to the text run. let idx = i + self.clump.begin(); transform_text(in_boxes[idx].raw_text(), compression) - }); + }; - // next, concatenate all of the transformed strings together, saving the new char indices - let mut run_str : ~str = ~""; - let mut new_ranges : ~[Range] = ~[]; - let mut char_total = 0u; + // Next, concatenate all of the transformed strings together, saving the new + // character indices. + let mut run_str: ~str = ~""; + let mut new_ranges: ~[Range] = ~[]; + let mut char_total = 0; for uint::range(0, transformed_strs.len()) |i| { let added_chars = str::char_len(transformed_strs[i]); new_ranges.push(Range::new(char_total, added_chars)); @@ -283,28 +284,32 @@ priv impl TextRunScanner { char_total += added_chars; } - // create the run, then make new boxes with the run and adjusted text indices - - // TODO(Issue #177): text run creation must account for text-renderability by fontgroup fonts. - // this is probably achieved by creating fontgroup above, and then letting FontGroup decide - // which Font to stick into the TextRun. + // Now create the run. + // + // TODO(#177): Text run creation must account for the renderability of text by + // font group fonts. This is probably achieved by creating the font group above + // and then letting `FontGroup` decide which `Font` to stick into the text run. let font_style = in_boxes[self.clump.begin()].font_style(); let fontgroup = ctx.font_ctx.get_resolved_font_for_style(&font_style); let run = @TextRun::new(fontgroup.fonts[0], run_str); + + // Make new boxes with the run and adjusted text indices. debug!("TextRunScanner: pushing box(es) in range: %?", self.clump); let clump = self.clump; for clump.eachi |i| { let range = &new_ranges[i - self.clump.begin()]; if range.length() == 0 { - error!("Elided an UnscannedTextbox because it was zero-length after compression; %s", - in_boxes[i].debug_str()); + error!("Elided an `UnscannedTextbox` because it was zero-length after \ + compression; %s", + in_boxes[i].debug_str()); loop } + let new_box = adapt_textbox_with_range(in_boxes[i].d(), run, range); out_boxes.push(new_box); } } - } /* /match */ + } // End of match. debug!("--- In boxes: ---"); for in_boxes.eachi |i, box| { @@ -327,7 +332,7 @@ priv impl TextRunScanner { let end = self.clump.end(); // FIXME: borrow checker workaround self.clump.reset(end, 0); - } /* /fn flush_clump_to_list */ + } // End of `flush_clump_to_list`. } struct PendingLine { @@ -343,19 +348,19 @@ struct LineboxScanner { line_spans: ~[Range], } -fn LineboxScanner(inline: FlowContext) -> LineboxScanner { - assert!(inline.starts_inline_flow()); - - LineboxScanner { - flow: inline, - new_boxes: ~[], - work_list: @mut Deque::new(), - pending_line: PendingLine {mut range: Range::empty(), mut width: Au(0)}, - line_spans: ~[] - } -} - impl LineboxScanner { + fn new(inline: FlowContext) -> LineboxScanner { + assert!(inline.starts_inline_flow()); + + LineboxScanner { + flow: inline, + new_boxes: ~[], + work_list: @mut Deque::new(), + pending_line: PendingLine {mut range: Range::empty(), mut width: Au(0)}, + line_spans: ~[] + } + } + priv fn reset_scanner(&mut self) { debug!("Resetting line box scanner's state for flow f%d.", self.flow.id()); self.line_spans = ~[]; @@ -636,10 +641,11 @@ 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, ctx: &mut 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 + // `RenderBox`. { - // initialize (content) box widths, if they haven't been - // already. This could be combined with LineboxScanner's walk - // over the box list, and/or put into RenderBox. let this = &mut *self; for this.boxes.each |&box| { let box2 = &mut *box; @@ -652,17 +658,18 @@ impl InlineFlowData { // Text boxes are initialized with dimensions. box.d().position.size.width }, - // TODO(Issue #225): different cases for 'inline-block', other replaced content + // TODO(#225): There will be different cases here for `inline-block` and other + // replaced content. GenericBox(*) => Au::from_px(45), _ => fail!(fmt!("Tried to assign width to unknown Box variant: %?", box)) }; - } // for boxes.each |box| + } // End of for loop. } - let mut scanner = LineboxScanner(InlineFlow(self)); + let mut scanner = LineboxScanner::new(InlineFlow(self)); scanner.scan_for_lines(ctx); - /* There are no child contexts, so stop here. */ + // There are no child contexts, so stop here. // TODO(Issue #225): once there are 'inline-block' elements, this won't be // true. In that case, set the InlineBlockBox's width to the @@ -671,7 +678,7 @@ 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_inline(&mut self, _: &mut LayoutContext) { // TODO(#226): Get the CSS `line-height` property from the containing block's style to // determine minimum linebox height. // @@ -683,16 +690,16 @@ impl InlineFlowData { for self.lines.eachi |i, line_span| { debug!("assign_height_inline: processing line %u with box span: %?", i, line_span); - // coords relative to left baseline + + // These coordinates are relative to the left baseline. let mut linebox_bounding_box = Au::zero_rect(); let boxes = &mut self.boxes; for line_span.eachi |box_i| { - let cur_box: &mut RenderBox = boxes[box_i]; // FIXME: borrow checker workaround + let cur_box = boxes[box_i]; // FIXME: borrow checker workaround // Compute the height of each box. let d = cur_box.d(); // FIXME: borrow checker workaround - let cur_box: &mut RenderBox = boxes[box_i]; // FIXME: borrow checker workaround - + let cur_box = &mut *cur_box; // FIXME: borrow checker workaround d.position.size.height = match *cur_box { ImageBox(_, ref img) => { Au::from_px(img.size().height) @@ -710,8 +717,9 @@ impl InlineFlowData { } }; - // Compute the bounding rect with the left baseline as origin. Linebox height is a - // matter of lining up ideal baselines and then using the union of all these rects. + // Compute the bounding rect with the left baseline as origin. Determining line box + // height is a matter of lining up ideal baselines and then taking the union of all + // these rects. let bounding_box = match *cur_box { // Adjust to baseline coordinates. // @@ -726,6 +734,7 @@ impl InlineFlowData { }, // Adjust the bounding box metric to the box's horizontal offset. + // // TODO: We can use font metrics directly instead of re-measuring for the // bounding box. TextBox(_, data) => { @@ -739,32 +748,39 @@ impl InlineFlowData { cur_box.debug_str())) } }; + debug!("assign_height_inline: bounding box for box b%d = %?", cur_box.d().id, bounding_box); + linebox_bounding_box = linebox_bounding_box.union(&bounding_box); + debug!("assign_height_inline: linebox bounding box = %?", linebox_bounding_box); } + let linebox_height = linebox_bounding_box.size.height; let baseline_offset = -linebox_bounding_box.origin.y; - // now go back and adjust y coordinates to match determined baseline + + // Now go back and adjust the Y coordinates to match the baseline we determined. for line_span.eachi |box_i| { let cur_box = boxes[box_i]; - // TODO(Issue #226): this is completely wrong. Need to use element's - // 'line-height' when calculating linebox height. Then, go back over - // and set y offsets according to 'vertical-align' property of containing block. - let halfleading = match cur_box { - @TextBox(_, data) => { + + // TODO(#226): This is completely wrong. We need to use the element's `line-height` + // when calculating line box height. Then we should go back over and set Y offsets + // according to the `vertical-align` property of the containing block. + let halfleading = match *cur_box { + TextBox(_, data) => { (data.run.font.metrics.em_size - line_height).scale_by(0.5f) }, _ => Au(0), }; + cur_box.d().position.origin.y = cur_y + halfleading + (baseline_offset - cur_box.d().position.size.height); } cur_y += Au::max(line_height, linebox_height); - } // /lines.each |line_span| + } // End of `lines.each` loop. self.common.position.size.height = cur_y; } @@ -774,8 +790,8 @@ impl InlineFlowData { dirty: &Rect, offset: &Point2D, list: &Cell) { - // TODO(Issue #228): once we form line boxes and have their cached bounds, we can be - // smarter and not recurse on a line if nothing in it can intersect dirty + // TODO(#228): Once we form line boxes and have their cached bounds, we can be smarter and + // not recurse on a line if nothing in it can intersect the dirty region. debug!("FlowContext[%d]: building display list for %u inline boxes", self.common.id, self.boxes.len()); @@ -784,8 +800,8 @@ impl InlineFlowData { box.build_display_list(builder, dirty, offset, list) } - // TODO(Issue #225): should inline-block elements have flows as children - // of the inline flow, or should the flow be nested inside the box somehow? + // TODO(#225): Should `inline-block` elements have flows as children of the inline flow or + // should the flow be nested inside the box somehow? } +} -} // @FlowContext : InlineLayout