From a351710004758824111fea572bad6d78282ccaea Mon Sep 17 00:00:00 2001 From: Brendan Zabarauskas Date: Fri, 20 Jun 2014 09:07:11 -0700 Subject: [PATCH 1/6] Use iterators to make some logic clearer --- src/components/gfx/text/text_run.rs | 15 ++--- src/components/main/layout/inline.rs | 88 ++++++++++++---------------- 2 files changed, 45 insertions(+), 58 deletions(-) diff --git a/src/components/gfx/text/text_run.rs b/src/components/gfx/text/text_run.rs index 3575686079e..1f182572ac1 100644 --- a/src/components/gfx/text/text_run.rs +++ b/src/components/gfx/text/text_run.rs @@ -206,10 +206,9 @@ impl<'a> TextRun { } pub fn range_is_trimmable_whitespace(&self, range: &Range) -> bool { - for (slice_glyphs, _, _) in self.iter_slices_for_range(range) { - if !slice_glyphs.is_whitespace() { return false; } - } - true + self.iter_slices_for_range(range).all(|(slice_glyphs, _, _)| { + slice_glyphs.is_whitespace() + }) } pub fn ascent(&self) -> Au { @@ -242,13 +241,11 @@ impl<'a> TextRun { } pub fn min_width_for_range(&self, range: &Range) -> Au { - let mut max_piece_width = Au(0); debug!("iterating outer range {:?}", range); - for (_, offset, slice_range) in self.iter_slices_for_range(range) { + self.iter_slices_for_range(range).fold(Au(0), |max_piece_width, (_, offset, slice_range)| { debug!("iterated on {:?}[{:?}]", offset, slice_range); - max_piece_width = Au::max(max_piece_width, self.advance_for_range(&slice_range)); - } - max_piece_width + Au::max(max_piece_width, self.advance_for_range(&slice_range)) + }) } /// Returns the index of the first glyph run containing the given character index. diff --git a/src/components/main/layout/inline.rs b/src/components/main/layout/inline.rs index f1911b47226..5f8158a0bea 100644 --- a/src/components/main/layout/inline.rs +++ b/src/components/main/layout/inline.rs @@ -639,13 +639,9 @@ pub struct FragmentIterator<'a> { impl<'a> Iterator<(&'a Fragment, InlineFragmentContext<'a>)> for FragmentIterator<'a> { #[inline] fn next(&mut self) -> Option<(&'a Fragment, InlineFragmentContext<'a>)> { - match self.iter.next() { - None => None, - Some((i, fragment)) => Some(( - fragment, - InlineFragmentContext::new(self.ranges, FragmentIndex(i as int)), - )), - } + self.iter.next().map(|(i, fragment)| { + (fragment, InlineFragmentContext::new(self.ranges, FragmentIndex(i as int))) + }) } } @@ -658,13 +654,9 @@ pub struct MutFragmentIterator<'a> { impl<'a> Iterator<(&'a mut Fragment, InlineFragmentContext<'a>)> for MutFragmentIterator<'a> { #[inline] fn next(&mut self) -> Option<(&'a mut Fragment, InlineFragmentContext<'a>)> { - match self.iter.next() { - None => None, - Some((i, fragment)) => Some(( - fragment, - InlineFragmentContext::new(self.ranges, FragmentIndex(i as int)), - )), - } + self.iter.next().map(|(i, fragment)| { + (fragment, InlineFragmentContext::new(self.ranges, FragmentIndex(i as int))) + }) } } @@ -868,22 +860,15 @@ impl InlineFragments { /// Strips ignorable whitespace from the start of a list of fragments. pub fn strip_ignorable_whitespace_from_start(&mut self) { - if self.is_empty() { - return; - } + if self.is_empty() { return }; // Fast path - // FIXME(#2264, pcwalton): This is slow because vector shift is broken. :( - let mut found_nonwhitespace = false; - let mut new_fragments = Vec::new(); - for fragment in self.fragments.iter() { - if !found_nonwhitespace && fragment.is_whitespace_only() { - debug!("stripping ignorable whitespace from start"); - continue; - } - - found_nonwhitespace = true; - new_fragments.push(fragment.clone()) - } + let new_fragments = mem::replace(&mut self.fragments, vec![]) + .move_iter() + .skip_while(|fragment| { + if fragment.is_whitespace_only() { + debug!("stripping ignorable whitespace from start"); true + } else { false } + }).collect(); self.fixup(new_fragments); } @@ -1358,29 +1343,24 @@ struct InlineFragmentFixupWorkItem { pub struct RangeIterator<'a> { iter: Items<'a,InlineFragmentRange>, index: FragmentIndex, - seen_first: bool, + is_first: bool, } impl<'a> Iterator<&'a InlineFragmentRange> for RangeIterator<'a> { fn next(&mut self) -> Option<&'a InlineFragmentRange> { - if self.seen_first { - match self.iter.next() { - Some(fragment_range) if fragment_range.range.contains(self.index) => { - return Some(fragment_range) - } - Some(_) | None => return None - } - } - - loop { - match self.iter.next() { - None => return None, - Some(fragment_range) if fragment_range.range.contains(self.index) => { - self.seen_first = true; - return Some(fragment_range) - } - Some(_) => {} - } + if !self.is_first { + // Yield the next fragment range if it contains the index + self.iter.next().and_then(|frag_range| { + if frag_range.range.contains(self.index) { Some(frag_range) } else { None } + }) + } else { + // Find the first fragment range that contains the index if it exists + let index = self.index; + let first = self.iter.by_ref().find(|frag_range| { + frag_range.range.contains(index) + }); + self.is_first = false; // We have made our first iteration + first } } } @@ -1403,10 +1383,20 @@ impl<'a> InlineFragmentContext<'a> { /// Iterates over all ranges that contain the fragment at context's index, outermost first. #[inline(always)] pub fn ranges(&self) -> RangeIterator<'a> { + // TODO: It would be more straightforward to return an existing iterator + // rather defining our own `RangeIterator`, but this requires unboxed + // closures in order to satisfy the borrow checker: + // + // ~~~rust + // let index = self.index; + // self.ranges.iter() + // .skip_while(|fr| fr.range.contains(index)) + // .take_while(|fr| fr.range.contains(index)) + // ~~~ RangeIterator { iter: self.ranges.iter(), index: self.index, - seen_first: false, + is_first: true, } } } From 61216c301deadc95b4156f8ddf5348e6e410a5d4 Mon Sep 17 00:00:00 2001 From: Brendan Zabarauskas Date: Fri, 20 Jun 2014 09:07:42 -0700 Subject: [PATCH 2/6] Use Option functions to make logic clearer --- src/components/gfx/text/glyph.rs | 60 +++++++++++++------------------- 1 file changed, 24 insertions(+), 36 deletions(-) diff --git a/src/components/gfx/text/glyph.rs b/src/components/gfx/text/glyph.rs index 17ef3b5acd0..67091548fac 100644 --- a/src/components/gfx/text/glyph.rs +++ b/src/components/gfx/text/glyph.rs @@ -355,14 +355,12 @@ impl<'a> DetailedGlyphStore { detail_offset: 0, // unused }; - match self.detail_lookup.as_slice().binary_search_index(&key) { - None => fail!("Invalid index not found in detailed glyph lookup table!"), - Some(i) => { - assert!(i + (count as uint) <= self.detail_buffer.len()); - // return a slice into the buffer - self.detail_buffer.slice(i, i + count as uint) - } - } + let i = self.detail_lookup.as_slice().binary_search_index(&key) + .expect("Invalid index not found in detailed glyph lookup table!"); + + assert!(i + (count as uint) <= self.detail_buffer.len()); + // return a slice into the buffer + self.detail_buffer.slice(i, i + count as uint) } fn get_detailed_glyph_with_index(&'a self, @@ -377,13 +375,11 @@ impl<'a> DetailedGlyphStore { detail_offset: 0, // unused }; - match self.detail_lookup.as_slice().binary_search_index(&key) { - None => fail!("Invalid index not found in detailed glyph lookup table!"), - Some(i) => { - assert!(i + (detail_offset as uint) < self.detail_buffer.len()); - self.detail_buffer.get(i+(detail_offset as uint)) - } - } + let i = self.detail_lookup.as_slice().binary_search_index(&key) + .expect("Invalid index not found in detailed glyph lookup table!"); + + assert!(i + (detail_offset as uint) < self.detail_buffer.len()); + self.detail_buffer.get(i + (detail_offset as uint)) } fn ensure_sorted(&mut self) { @@ -432,15 +428,10 @@ impl GlyphData { cluster_start: bool, ligature_start: bool) -> GlyphData { - let offset = match offset { - None => Zero::zero(), - Some(o) => o, - }; - GlyphData { id: id, advance: advance, - offset: offset, + offset: offset.unwrap_or(Zero::zero()), is_missing: is_missing, cluster_start: cluster_start, ligature_start: ligature_start, @@ -743,21 +734,18 @@ impl<'a> Iterator<(CharIndex, GlyphInfo<'a>)> for GlyphIterator<'a> { if self.glyph_range.is_some() { self.next_glyph_range() } else { - // No glyph range. Look at next character. - match self.char_range.next() { - Some(i) => { - self.char_index = i; - assert!(i < self.store.char_len()); - let entry = self.store.entry_buffer.get(i.to_uint()); - if entry.is_simple() { - Some((self.char_index, SimpleGlyphInfo(self.store, i))) - } else { - // Fall back to the slow path. - self.next_complex_glyph(entry, i) - } - }, - None => None - } + // No glyph range. Look at next character. + self.char_range.next().and_then(|i| { + self.char_index = i; + assert!(i < self.store.char_len()); + let entry = self.store.entry_buffer.get(i.to_uint()); + if entry.is_simple() { + Some((self.char_index, SimpleGlyphInfo(self.store, i))) + } else { + // Fall back to the slow path. + self.next_complex_glyph(entry, i) + } + }) } } } From 520675b237eb40d23f83b028ec1521e8fb2214e3 Mon Sep 17 00:00:00 2001 From: Brendan Zabarauskas Date: Fri, 20 Jun 2014 10:02:37 -0700 Subject: [PATCH 3/6] Make move mutation out of compute_minimum_ascent_and_descent --- src/components/main/layout/construct.rs | 4 +++- src/components/main/layout/inline.rs | 7 +++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/components/main/layout/construct.rs b/src/components/main/layout/construct.rs index 4e9712caa8f..5f999eb2663 100644 --- a/src/components/main/layout/construct.rs +++ b/src/components/main/layout/construct.rs @@ -293,7 +293,9 @@ impl<'a> FlowConstructor<'a> { } let mut inline_flow = box InlineFlow::from_fragments((*node).clone(), fragments); - inline_flow.compute_minimum_ascent_and_descent(self.font_context(), &**node.style()); + let (ascent, descent) = inline_flow.compute_minimum_ascent_and_descent(self.font_context(), &**node.style()); + inline_flow.minimum_height_above_baseline = ascent; + inline_flow.minimum_depth_below_baseline = descent; let mut inline_flow = inline_flow as Box; TextRunScanner::new().scan_for_runs(self.font_context(), inline_flow); let mut inline_flow = FlowRef::new(inline_flow); diff --git a/src/components/main/layout/inline.rs b/src/components/main/layout/inline.rs index 5f8158a0bea..77cacb85cdc 100644 --- a/src/components/main/layout/inline.rs +++ b/src/components/main/layout/inline.rs @@ -1057,15 +1057,14 @@ impl InlineFlow { /// construction. /// /// `style` is the style of the block. - pub fn compute_minimum_ascent_and_descent(&mut self, + pub fn compute_minimum_ascent_and_descent(&self, font_context: &mut FontContext, - style: &ComputedValues) { + style: &ComputedValues) -> (Au, Au) { let font_style = text::computed_style_to_font_style(style); let font_metrics = text::font_metrics_for_style(font_context, &font_style); let line_height = text::line_height_from_style(style, style.get_font().font_size); let inline_metrics = InlineMetrics::from_font_metrics(&font_metrics, line_height); - self.minimum_height_above_baseline = inline_metrics.height_above_baseline; - self.minimum_depth_below_baseline = inline_metrics.depth_below_baseline; + (inline_metrics.height_above_baseline, inline_metrics.depth_below_baseline) } } From 344af248e7d81ac40f8549f0ab86cef23571c04d Mon Sep 17 00:00:00 2001 From: Brendan Zabarauskas Date: Fri, 20 Jun 2014 10:17:45 -0700 Subject: [PATCH 4/6] Remove some unnecessary mutable references --- src/components/main/layout/inline.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/main/layout/inline.rs b/src/components/main/layout/inline.rs index 77cacb85cdc..8001ad26e83 100644 --- a/src/components/main/layout/inline.rs +++ b/src/components/main/layout/inline.rs @@ -376,7 +376,7 @@ impl LineBreaker { /// Computes the position of a line that has only the provided fragment. 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 fragment after splitting. - fn initial_line_placement(&self, first_fragment: &Fragment, ceiling: Au, flow: &mut InlineFlow) + fn initial_line_placement(&self, first_fragment: &Fragment, ceiling: Au, flow: &InlineFlow) -> (Rect, Au) { debug!("LineBreaker: Trying to place first fragment of line {}", self.lines.len()); @@ -439,7 +439,7 @@ impl LineBreaker { /// Returns false if and only if we should break the line. fn avoid_floats(&mut self, in_fragment: Fragment, - flow: &mut InlineFlow, + flow: &InlineFlow, new_height: Au, line_is_empty: bool) -> bool { @@ -511,7 +511,7 @@ impl LineBreaker { /// Tries to append the given fragment to the line, splitting it if necessary. Returns false only if /// we should break the line. - fn try_append_to_line(&mut self, in_fragment: Fragment, flow: &mut InlineFlow) -> bool { + fn try_append_to_line(&mut self, in_fragment: Fragment, flow: &InlineFlow) -> bool { let line_is_empty = self.pending_line.range.length() == num::zero(); if line_is_empty { let (line_bounds, _) = self.initial_line_placement(&in_fragment, self.cur_y, flow); From 762669b5c6d63b95b319ca1246e10dab7012b1a1 Mon Sep 17 00:00:00 2001 From: Brendan Zabarauskas Date: Fri, 20 Jun 2014 10:50:40 -0700 Subject: [PATCH 5/6] Use Option::expect instead of pattern matching in try_append_to_line_by_new_line --- src/components/main/layout/inline.rs | 47 +++++++++++++--------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/src/components/main/layout/inline.rs b/src/components/main/layout/inline.rs index 8001ad26e83..c639aa61691 100644 --- a/src/components/main/layout/inline.rs +++ b/src/components/main/layout/inline.rs @@ -477,33 +477,30 @@ impl LineBreaker { true } else { debug!("LineBreaker: Found a new-line character, so splitting theline."); - match in_fragment.find_split_info_by_new_line() { - Some((left, right, run)) => { - // TODO(bjz): Remove fragment splitting - let split_fragment = |split: SplitInfo| { - let info = ScannedTextFragmentInfo::new(run.clone(), split.range); - let specific = ScannedTextFragment(info); - let size = Size2D(split.width, in_fragment.border_box.size.height); - in_fragment.transform(size, specific) - }; - debug!("LineBreaker: Pushing the fragment to the left of the new-line character \ - to the line."); - let mut left = split_fragment(left); - left.new_line_pos = vec!(); - self.push_fragment_to_line(left); + let (left, right, run) = in_fragment.find_split_info_by_new_line() + .expect("LineBreaker: This split case makes no sense!"); - for right in right.move_iter() { - debug!("LineBreaker: Deferring the fragment to the right of the new-line \ - character to the line."); - let mut right = split_fragment(right); - right.new_line_pos = in_fragment.new_line_pos.clone(); - self.work_list.push_front(right); - } - }, - None => { - error!("LineBreaker: This split case makes no sense!") - }, + // TODO(bjz): Remove fragment splitting + let split_fragment = |split: SplitInfo| { + let info = ScannedTextFragmentInfo::new(run.clone(), split.range); + let specific = ScannedTextFragment(info); + let size = Size2D(split.width, in_fragment.border_box.size.height); + in_fragment.transform(size, specific) + }; + + debug!("LineBreaker: Pushing the fragment to the left of the new-line character \ + to the line."); + let mut left = split_fragment(left); + left.new_line_pos = vec![]; + self.push_fragment_to_line(left); + + for right in right.move_iter() { + debug!("LineBreaker: Deferring the fragment to the right of the new-line \ + character to the line."); + let mut right = split_fragment(right); + right.new_line_pos = in_fragment.new_line_pos.clone(); + self.work_list.push_front(right); } false } From e5cf89d4f3eb718aa5abf9a2ad22813d0a1d2985 Mon Sep 17 00:00:00 2001 From: Brendan Zabarauskas Date: Fri, 20 Jun 2014 11:35:34 -0700 Subject: [PATCH 6/6] Clean up skip_while iteration --- src/components/main/layout/inline.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/components/main/layout/inline.rs b/src/components/main/layout/inline.rs index c639aa61691..92992e01446 100644 --- a/src/components/main/layout/inline.rs +++ b/src/components/main/layout/inline.rs @@ -862,9 +862,11 @@ impl InlineFragments { let new_fragments = mem::replace(&mut self.fragments, vec![]) .move_iter() .skip_while(|fragment| { - if fragment.is_whitespace_only() { - debug!("stripping ignorable whitespace from start"); true - } else { false } + let is_whitespace_only = fragment.is_whitespace_only(); + if is_whitespace_only { + debug!("stripping ignorable whitespace from start"); + } + is_whitespace_only }).collect(); self.fixup(new_fragments);