From 16e2b131e787d8989952e1933cf2f575317575be Mon Sep 17 00:00:00 2001 From: Brendan Zabarauskas Date: Thu, 15 May 2014 17:40:44 -0700 Subject: [PATCH 1/5] Use CharIndex instead of GlyphIndex for LineIndices All of the text run methods use character indices and the split_to_width function also uses character indices. Therefore it will be easier to use character indices to keep track of line breaks than glyph indices. --- src/components/main/layout/inline.rs | 77 +++++++++++++--------------- 1 file changed, 37 insertions(+), 40 deletions(-) diff --git a/src/components/main/layout/inline.rs b/src/components/main/layout/inline.rs index 112a10b5bc3..3c3fa9d8c5f 100644 --- a/src/components/main/layout/inline.rs +++ b/src/components/main/layout/inline.rs @@ -18,6 +18,7 @@ use geom::{Point2D, Rect, SideOffsets2D, Size2D}; use gfx::display_list::ContentLevel; use gfx::font::FontMetrics; use gfx::font_context::FontContext; +use gfx::text::glyph::CharIndex; use servo_util::geometry::Au; use servo_util::geometry; use servo_util::range; @@ -104,35 +105,31 @@ int_range_index! { struct FragmentIndex(int) } -int_range_index! { - #[doc = "The index of a glyph in a single DOM fragment. Ligatures and"] - #[doc = "continuous runs of whitespace are treated as single glyphs."] - #[doc = "Non-breakable DOM fragments such as images are treated as"] - #[doc = "having a range length of `1`."] - #[doc = ""] - #[doc = "For example, given the HTML below:"] - #[doc = ""] - #[doc = "~~~"] - #[doc = "like truffles,"] - #[doc = "~~~"] - #[doc = ""] - #[doc = "The glyphs would be indexed as follows:"] - #[doc = ""] - #[doc = "~~~"] - #[doc = "| 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 |"] - #[doc = "|---|---|---|---|---|---|---|---|-----|---|----|----|"] - #[doc = "| l | i | k | e | | t | r | u | ffl | e | s | , |"] - #[doc = "~~~"] - struct GlyphIndex(int) -} - /// A line index consists of two indices: a fragment index that refers to the /// index of a DOM fragment within a flattened inline element; and a glyph index /// where the 0th glyph refers to the first glyph of that fragment. #[deriving(Clone, Eq, Ord, TotalEq, TotalOrd, Zero)] pub struct LineIndices { pub fragment_index: FragmentIndex, - pub glyph_index: GlyphIndex, + /// The index of a character in a single DOM fragment. Ligatures and + /// continuous runs of whitespace are treated as single characters. + /// Non-breakable DOM fragments such as images are treated as + /// having a range length of `1`. + /// + /// For example, given the HTML below: + /// + /// ~~~ + /// like truffles, + /// ~~~ + /// + /// The characters would be indexed as follows: + /// + /// ~~~ + /// | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | + /// |---|---|---|---|---|---|---|---|---|---|----|----|----|----| + /// | l | i | k | e | | t | r | u | f | f | l | e | s | , | + /// ~~~ + pub char_index: CharIndex, } impl RangeIndex for LineIndices {} @@ -141,14 +138,14 @@ impl Add for LineIndices { fn add(&self, other: &LineIndices) -> LineIndices { // TODO: use debug_assert! after rustc upgrade if cfg!(not(ndebug)) { - assert!(other.fragment_index == num::zero() || other.glyph_index == num::zero(), + assert!(other.fragment_index == num::zero() || other.char_index == num::zero(), "Attempted to add {} to {}. Both the fragment_index and \ - glyph_index of the RHS are non-zero. This probably \ - was a mistake!", self, other); + char_index of the RHS are non-zero. This probably was a \ + mistake!", self, other); } LineIndices { fragment_index: self.fragment_index + other.fragment_index, - glyph_index: self.glyph_index + other.glyph_index, + char_index: self.char_index + other.char_index, } } } @@ -157,14 +154,14 @@ impl Sub for LineIndices { fn sub(&self, other: &LineIndices) -> LineIndices { // TODO: use debug_assert! after rustc upgrade if cfg!(not(ndebug)) { - assert!(other.fragment_index == num::zero() || other.glyph_index == num::zero(), - "Attempted to subtract {} from {}. Both the \ - fragment_index and glyph_index of the RHS are non-zero. \ - This probably was a mistake!", self, other); + assert!(other.fragment_index == num::zero() || other.char_index == num::zero(), + "Attempted to subtract {} from {}. Both the fragment_index \ + and char_index of the RHS are non-zero. This probably was \ + a mistake!", self, other); } LineIndices { fragment_index: self.fragment_index - other.fragment_index, - glyph_index: self.glyph_index - other.glyph_index, + char_index: self.char_index - other.char_index, } } } @@ -173,21 +170,21 @@ impl Neg for LineIndices { fn neg(&self) -> LineIndices { // TODO: use debug_assert! after rustc upgrade if cfg!(not(ndebug)) { - assert!(self.fragment_index == num::zero() || self.glyph_index == num::zero(), + assert!(self.fragment_index == num::zero() || self.char_index == num::zero(), "Attempted to negate {}. Both the fragment_index and \ - glyph_index are non-zero. This probably was a mistake!", + char_index are non-zero. This probably was a mistake!", self); } LineIndices { fragment_index: -self.fragment_index, - glyph_index: -self.glyph_index, + char_index: -self.char_index, } } } impl fmt::Show for LineIndices { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f.buf, "{}.{}", self.fragment_index, self.glyph_index) + write!(f.buf, "{}.{}", self.fragment_index, self.char_index) } } @@ -195,8 +192,8 @@ pub fn each_fragment_index(range: &Range) -> EachIndex) -> EachIndex { - range::each_index(range.begin().glyph_index, range.length().glyph_index) +pub fn each_char_index(range: &Range) -> EachIndex { + range::each_index(range.begin().char_index, range.length().char_index) } struct LineboxScanner { @@ -587,14 +584,14 @@ impl LineboxScanner { self.pending_line.range.reset( LineIndices { fragment_index: FragmentIndex(self.new_boxes.len() as int), - glyph_index: GlyphIndex(0) /* unused for now */, + char_index: CharIndex(0) /* unused for now */, }, num::zero() ); } self.pending_line.range.extend_by(LineIndices { fragment_index: FragmentIndex(1), - glyph_index: GlyphIndex(0) /* unused for now */ , + char_index: CharIndex(0) /* unused for now */ , }); self.pending_line.bounds.size.width = self.pending_line.bounds.size.width + box_.border_box.size.width; From d3dfb78683a47e4c01acbe16bf7424a07461acdd Mon Sep 17 00:00:00 2001 From: Brendan Zabarauskas Date: Thu, 15 May 2014 17:45:57 -0700 Subject: [PATCH 2/5] Reorganise comments --- src/components/main/layout/inline.rs | 85 ++++++++++++++-------------- 1 file changed, 44 insertions(+), 41 deletions(-) diff --git a/src/components/main/layout/inline.rs b/src/components/main/layout/inline.rs index 3c3fa9d8c5f..b4de9ba8e57 100644 --- a/src/components/main/layout/inline.rs +++ b/src/components/main/layout/inline.rs @@ -59,62 +59,65 @@ use sync::Arc; /// left corner of the green zone is the same as that of the line, but /// the green zone can be taller and wider than the line itself. pub struct LineBox { - /// Consider the following HTML and rendered element with linebreaks: - /// - /// ~~~html - /// I like truffles, yes I do. - /// ~~~ - /// - /// ~~~ - /// +-----------+ - /// | I like | - /// | truffles, | - /// | yes I do. | - /// +-----------+ - /// ~~~ - /// - /// The ranges that describe these lines would be: - /// - /// ~~~ - /// | [0.0, 1.4) | [1.5, 2.0) | [2.1, 3.0) | - /// |------------|-------------|-------------| - /// | 'I like' | 'truffles,' | 'yes I do.' | - /// ~~~ pub range: Range, pub bounds: Rect, pub green_zone: Size2D } int_range_index! { - #[doc = "The index of a box fragment into the flattened vector of DOM"] - #[doc = "elements."] - #[doc = ""] - #[doc = "For example, given the HTML below:"] - #[doc = ""] - #[doc = "~~~"] - #[doc = "I like truffles, yes I do."] - #[doc = "~~~"] - #[doc = ""] - #[doc = "The fragments would be indexed as follows:"] - #[doc = ""] - #[doc = "~~~"] - #[doc = "| 0 | 1 | 2 |"] - #[doc = "|------|------------------|--------------|"] - #[doc = "| 'I ' | 'like truffles,' | ' yes I do.' |"] - #[doc = "~~~"] + #[doc = "The index of a box fragment in a flattened vector of DOM elements."] struct FragmentIndex(int) } /// A line index consists of two indices: a fragment index that refers to the /// index of a DOM fragment within a flattened inline element; and a glyph index /// where the 0th glyph refers to the first glyph of that fragment. +/// +/// For example, consider the following HTML and rendered element with +/// linebreaks: +/// +/// ~~~html +/// I like truffles, yes I do. +/// ~~~ +/// +/// ~~~ +/// +-----------+ +/// | I like | +/// | truffles, | +/// | yes I do. | +/// +-----------+ +/// ~~~ +/// +/// The ranges that describe these lines would be: +/// +/// ~~~ +/// | [0.0, 1.4) | [1.5, 2.0) | [2.1, 3.0) | +/// |------------|-------------|-------------| +/// | 'I like' | 'truffles,' | 'yes I do.' | +/// ~~~ #[deriving(Clone, Eq, Ord, TotalEq, TotalOrd, Zero)] pub struct LineIndices { + /// The index of a box fragment into the flattened vector of DOM + /// elements. + /// + /// For example, given the HTML below: + /// + /// ~~~ + /// I like truffles, yes I do. + /// ~~~ + /// + /// The fragments would be indexed as follows: + /// + /// ~~~ + /// | 0 | 1 | 2 | + /// |------|------------------|--------------| + /// | 'I ' | 'like truffles,' | ' yes I do.' | + /// ~~~ pub fragment_index: FragmentIndex, - /// The index of a character in a single DOM fragment. Ligatures and - /// continuous runs of whitespace are treated as single characters. - /// Non-breakable DOM fragments such as images are treated as - /// having a range length of `1`. + + /// The index of a character in a DOM fragment. Ligatures and continuous + /// runs of whitespace are treated as single characters. Non-breakable DOM + /// fragments such as images are treated as having a range length of `1`. /// /// For example, given the HTML below: /// From 868f7501c982a88b925140d28445663c2278fe24 Mon Sep 17 00:00:00 2001 From: Brendan Zabarauskas Date: Thu, 15 May 2014 16:59:24 -0700 Subject: [PATCH 3/5] Remove extra split_to_width call This does not seem to do anything, and according to @pcwalton it might be unnecessary. Float layout is rather broken anyway, and needs to be reworked. --- src/components/main/layout/inline.rs | 47 ++-------------------------- 1 file changed, 3 insertions(+), 44 deletions(-) diff --git a/src/components/main/layout/inline.rs b/src/components/main/layout/inline.rs index b4de9ba8e57..d4039e28112 100644 --- a/src/components/main/layout/inline.rs +++ b/src/components/main/layout/inline.rs @@ -331,7 +331,6 @@ impl LineboxScanner { let first_box_size = first_box.border_box.size; let splittable = first_box.can_split(); debug!("LineboxScanner: box size: {}, splittable: {}", first_box_size, splittable); - let line_is_empty: bool = self.pending_line.range.length() == num::zero(); // Initally, pretend a splittable box has 0 width. // We will move it later if it has nonzero width @@ -342,7 +341,7 @@ impl LineboxScanner { first_box_size.width }; - let mut info = PlacementInfo { + let info = PlacementInfo { size: Size2D(placement_width, first_box_size.height), ceiling: ceiling, max_width: flow.base.position.size.width, @@ -368,48 +367,8 @@ impl LineboxScanner { return (line_bounds, first_box_size.width); } - // Otherwise, try and split the box - // 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(line_bounds.size.width, line_is_empty) { - CannotSplit => { - error!("LineboxScanner: Tried to split unsplittable render box! {}", - first_box); - return (line_bounds, first_box_size.width); - } - SplitDidFit(left, right) => { - - debug!("LineboxScanner: case=box split and fit"); - let actual_box_width = match (left, right) { - (Some(l_box), Some(_)) => l_box.border_box.size.width, - (Some(l_box), None) => l_box.border_box.size.width, - (None, Some(r_box)) => r_box.border_box.size.width, - (None, None) => fail!("This case makes no sense.") - }; - return (line_bounds, actual_box_width); - } - SplitDidNotFit(left, right) => { - // The split didn't fit, but we might be able to - // push it down past floats. - - - debug!("LineboxScanner: case=box split and fit didn't fit; trying to push it down"); - let actual_box_width = match (left, right) { - (Some(l_box), Some(_)) => l_box.border_box.size.width, - (Some(l_box), None) => l_box.border_box.size.width, - (None, Some(r_box)) => r_box.border_box.size.width, - (None, None) => fail!("This case makes no sense.") - }; - - info.size.width = actual_box_width; - let new_bounds = self.floats.place_between_floats(&info); - - debug!("LineboxScanner: case=new line position: {}", new_bounds); - return (new_bounds, actual_box_width); - } - } - + debug!("LineboxScanner: used to call split_to_width here"); + return (line_bounds, first_box_size.width); } /// Performs float collision avoidance. This is called when adding a box is going to increase From 1f7963e86bc851fba1ca4fa37d56e4d99a7aa19a Mon Sep 17 00:00:00 2001 From: Brendan Zabarauskas Date: Fri, 16 May 2014 13:34:53 -0700 Subject: [PATCH 4/5] Refactor font metric calculation This splits out the advance calculation and makes it more declarative --- src/components/gfx/text/glyph.rs | 6 ++++++ src/components/gfx/text/text_run.rs | 28 +++++++++++++++------------- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/components/gfx/text/glyph.rs b/src/components/gfx/text/glyph.rs index 09e4c773d00..1ae767f1d06 100644 --- a/src/components/gfx/text/glyph.rs +++ b/src/components/gfx/text/glyph.rs @@ -640,6 +640,12 @@ impl<'a> GlyphStore { } } + #[inline] + pub fn advance_for_char_range(&self, rang: &Range) -> Au { + self.iter_glyphs_for_char_range(rang) + .fold(Au(0), |advance, (_, glyph)| advance + glyph.advance()) + } + // getter methods pub fn char_is_space(&self, i: CharIndex) -> bool { assert!(i < CharIndex(self.entry_buffer.len() as int)); diff --git a/src/components/gfx/text/text_run.rs b/src/components/gfx/text/text_run.rs index a61769cdacf..cbea5fbd90e 100644 --- a/src/components/gfx/text/text_run.rs +++ b/src/components/gfx/text/text_run.rs @@ -183,25 +183,27 @@ impl<'a> TextRun { true } - pub fn metrics_for_range(&self, range: &Range) -> RunMetrics { + pub fn advance_for_range(&self, range: &Range) -> Au { // TODO(Issue #199): alter advance direction for RTL // TODO(Issue #98): using inter-char and inter-word spacing settings when measuring text - let mut advance = Au(0); - for (glyphs, _offset, slice_range) in self.iter_slices_for_range(range) { - for (_i, glyph) in glyphs.iter_glyphs_for_char_range(&slice_range) { - advance = advance + glyph.advance(); - } - } - RunMetrics::new(advance, self.font_metrics.ascent, self.font_metrics.descent) + self.iter_slices_for_range(range) + .fold(Au(0), |advance, (glyphs, _, slice_range)| { + advance + glyphs.advance_for_char_range(&slice_range) + }) + } + + pub fn metrics_for_range(&self, range: &Range) -> RunMetrics { + RunMetrics::new(self.advance_for_range(range), + self.font_metrics.ascent, + self.font_metrics.descent) } pub fn metrics_for_slice(&self, glyphs: &GlyphStore, slice_range: &Range) -> RunMetrics { - let mut advance = Au(0); - for (_i, glyph) in glyphs.iter_glyphs_for_char_range(slice_range) { - advance = advance + glyph.advance(); - } - RunMetrics::new(advance, self.font_metrics.ascent, self.font_metrics.descent) + RunMetrics::new(glyphs.advance_for_char_range(slice_range), + self.font_metrics.ascent, + self.font_metrics.descent) } + pub fn min_width_for_range(&self, range: &Range) -> Au { let mut max_piece_width = Au(0); debug!("iterating outer range {:?}", range); From 3cf82c7c925de23b56f469a85add528ac4a4b49b Mon Sep 17 00:00:00 2001 From: Brendan Zabarauskas Date: Fri, 16 May 2014 13:41:00 -0700 Subject: [PATCH 5/5] Avoid unnecessary calculations of the entire run metrics --- src/components/gfx/text/text_run.rs | 11 +++++++++-- src/components/main/layout/box_.rs | 23 +++++++++++------------ 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/src/components/gfx/text/text_run.rs b/src/components/gfx/text/text_run.rs index cbea5fbd90e..abcfbc7df54 100644 --- a/src/components/gfx/text/text_run.rs +++ b/src/components/gfx/text/text_run.rs @@ -183,6 +183,14 @@ impl<'a> TextRun { true } + pub fn ascent(&self) -> Au { + self.font_metrics.ascent + } + + pub fn descent(&self) -> Au { + self.font_metrics.descent + } + pub fn advance_for_range(&self, range: &Range) -> Au { // TODO(Issue #199): alter advance direction for RTL // TODO(Issue #98): using inter-char and inter-word spacing settings when measuring text @@ -209,8 +217,7 @@ impl<'a> TextRun { debug!("iterating outer range {:?}", range); for (_, offset, slice_range) in self.iter_slices_for_range(range) { debug!("iterated on {:?}[{:?}]", offset, slice_range); - let metrics = self.metrics_for_range(&slice_range); - max_piece_width = Au::max(max_piece_width, metrics.advance_width); + max_piece_width = Au::max(max_piece_width, self.advance_for_range(&slice_range)); } max_piece_width } diff --git a/src/components/main/layout/box_.rs b/src/components/main/layout/box_.rs index 5b867e06bfc..d5ba4037850 100644 --- a/src/components/main/layout/box_.rs +++ b/src/components/main/layout/box_.rs @@ -791,7 +791,7 @@ impl Box { display_list.push(BorderDisplayItemClass(border_display_item)); // Draw a rectangle representing the baselines. - let ascent = text_box.run.metrics_for_range(&text_box.range).ascent; + let ascent = text_box.run.ascent(); let baseline = Rect(absolute_box_bounds.origin + Point2D(Au(0), ascent), Size2D(absolute_box_bounds.size.width, Au(0))); @@ -1216,21 +1216,20 @@ impl Box { let left_box = if left_range.length() > CharIndex(0) { let new_text_box_info = ScannedTextBoxInfo::new(text_box_info.run.clone(), left_range); - let mut new_metrics = new_text_box_info.run.metrics_for_range(&left_range); - new_metrics.bounding_box.size.height = self.border_box.size.height; - Some(self.transform(new_metrics.bounding_box.size, - ScannedTextBox(new_text_box_info))) + let width = new_text_box_info.run.advance_for_range(&left_range); + let height = self.border_box.size.height; + let size = Size2D(width, height); + Some(self.transform(size, ScannedTextBox(new_text_box_info))) } else { None }; - let right_box = right_range.map_or(None, |range: Range| { - let new_text_box_info = ScannedTextBoxInfo::new(text_box_info.run.clone(), - range); - let mut new_metrics = new_text_box_info.run.metrics_for_range(&range); - new_metrics.bounding_box.size.height = self.border_box.size.height; - Some(self.transform(new_metrics.bounding_box.size, - ScannedTextBox(new_text_box_info))) + let right_box = right_range.map_or(None, |right_range: Range| { + let new_text_box_info = ScannedTextBoxInfo::new(text_box_info.run.clone(), right_range); + let width = new_text_box_info.run.advance_for_range(&right_range); + let height = self.border_box.size.height; + let size = Size2D(width, height); + Some(self.transform(size, ScannedTextBox(new_text_box_info))) }); if pieces_processed_count == 1 || left_box.is_none() {