servo: Merge #2682 - Assorted cleanups in gfx and layout (from bjz:assorted-cleanups)
authorBrendan Zabarauskas <bjzaba@yahoo.com.au>
Sat, 21 Jun 2014 08:07:14 -0500
changeset 334544 a0fb8814c269bb0804871fe898bc7ee013bcfe8a
parent 334543 617de8a552fa39b32c0514924cfaa5abc825ee4a
child 334545 aa0a536239ace5f292b5f5a3591b2745f3882ce8
push id31307
push usergszorc@mozilla.com
push dateSat, 04 Feb 2017 00:59:06 +0000
treeherdermozilla-central@94079d43835f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
servo: Merge #2682 - Assorted cleanups in gfx and layout (from bjz:assorted-cleanups) Source-Repo: https://github.com/servo/servo Source-Revision: 7df50ae089ff57b8f3160f5ad6ee3d5f2daf5d87
servo/src/components/gfx/text/glyph.rs
servo/src/components/gfx/text/text_run.rs
servo/src/components/main/layout/construct.rs
servo/src/components/main/layout/inline.rs
--- a/servo/src/components/gfx/text/glyph.rs
+++ b/servo/src/components/gfx/text/glyph.rs
@@ -350,45 +350,41 @@ impl<'a> DetailedGlyphStore {
         assert!((count as uint) <= self.detail_buffer.len());
         assert!(self.lookup_is_sorted);
 
         let key = DetailedGlyphRecord {
             entry_offset: entry_offset,
             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,
                                      entry_offset: CharIndex,
                                      detail_offset: u16)
             -> &'a DetailedGlyph {
         assert!((detail_offset as uint) <= self.detail_buffer.len());
         assert!(self.lookup_is_sorted);
 
         let key = DetailedGlyphRecord {
             entry_offset: entry_offset,
             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) {
         if self.lookup_is_sorted {
             return;
         }
 
         // Sorting a unique vector is surprisingly hard. The follwing
@@ -427,25 +423,20 @@ pub struct GlyphData {
 impl GlyphData {
     pub fn new(id: GlyphId,
                advance: Au,
                offset: Option<Point2D<Au>>,
                is_missing: bool,
                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,
         }
     }
 }
 
 // This enum is a proxy that's provided to GlyphStore clients when iterating
@@ -738,26 +729,23 @@ impl<'a> Iterator<(CharIndex, GlyphInfo<
     // `next_complex_glyph()`.
     #[inline(always)]
     fn next(&mut self) -> Option<(CharIndex, GlyphInfo<'a>)> {
         // Would use 'match' here but it borrows contents in a way that
         // interferes with mutation.
         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)
+                }
+            })
         }
     }
 }
--- a/servo/src/components/gfx/text/text_run.rs
+++ b/servo/src/components/gfx/text/text_run.rs
@@ -201,20 +201,19 @@ impl<'a> TextRun {
         }
     }
 
     pub fn glyphs(&'a self) -> &'a Vec<GlyphRun> {
         &*self.glyphs
     }
 
     pub fn range_is_trimmable_whitespace(&self, range: &Range<CharIndex>) -> 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 {
         self.font_metrics.ascent
     }
 
     pub fn descent(&self) -> Au {
         self.font_metrics.descent
@@ -237,23 +236,21 @@ impl<'a> TextRun {
 
     pub fn metrics_for_slice(&self, glyphs: &GlyphStore, slice_range: &Range<CharIndex>) -> RunMetrics {
         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<CharIndex>) -> 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.
     fn index_of_first_glyph_run_containing(&self, index: CharIndex) -> Option<uint> {
         self.glyphs.as_slice().binary_search_index_by(&index, CharIndexComparator)
     }
 
     pub fn iter_slices_for_range(&'a self, range: &Range<CharIndex>) -> SliceIterator<'a> {
--- a/servo/src/components/main/layout/construct.rs
+++ b/servo/src/components/main/layout/construct.rs
@@ -288,17 +288,19 @@ impl<'a> FlowConstructor<'a> {
             }
             StripWhitespaceFromEnd => {
                 fragments.strip_ignorable_whitespace_from_end();
                 if fragments.is_empty() { return };
             }
         }
 
         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<Flow>;
         TextRunScanner::new().scan_for_runs(self.font_context(), inline_flow);
         let mut inline_flow = FlowRef::new(inline_flow);
         inline_flow.finish(self.layout_context);
 
         if flow.get().need_anonymous_flow(inline_flow.get()) {
             flow_list.push(inline_flow)
         } else {
--- a/servo/src/components/main/layout/inline.rs
+++ b/servo/src/components/main/layout/inline.rs
@@ -371,17 +371,17 @@ impl LineBreaker {
         } else {
             self.pending_line.bounds.size.height
         }
     }
 
     /// 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>, Au) {
         debug!("LineBreaker: Trying to place first fragment of line {}", self.lines.len());
 
         let first_fragment_size = first_fragment.border_box.size;
         let splittable = first_fragment.can_split();
         debug!("LineBreaker: fragment size: {}, splittable: {}", first_fragment_size, splittable);
 
         // Initally, pretend a splittable fragment has 0 width.
@@ -434,17 +434,17 @@ impl LineBreaker {
     /// which violates the standard.
     /// But option 2 is going to look weird sometimes.
     ///
     /// So we'll try to move the line whenever we can, but break if we have to.
     ///
     /// 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 {
         debug!("LineBreaker: entering float collision avoider!");
 
         // First predict where the next line is going to be.
         let this_line_y = self.pending_line.bounds.origin.y;
         let (next_line, first_fragment_width) = self.initial_line_placement(&in_fragment, this_line_y, flow);
@@ -472,51 +472,48 @@ impl LineBreaker {
     fn try_append_to_line_by_new_line(&mut self, in_fragment: Fragment) -> bool {
         if in_fragment.new_line_pos.len() == 0 {
                 debug!("LineBreaker: Did not find a new-line character, so pushing the fragment to \
                        the line without splitting.");
             self.push_fragment_to_line(in_fragment);
             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)
-                    };
+
+            let (left, right, run) = in_fragment.find_split_info_by_new_line()
+                .expect("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);
+            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);
-                    }
-                },
-                None => {
-                    error!("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);
             }
             false
         }
     }
 
     /// 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);
             self.pending_line.bounds.origin = line_bounds.origin;
             self.pending_line.green_zone = line_bounds.size;
         }
 
         debug!("LineBreaker: Trying to append fragment to line {:u} (fragment size: {}, green zone: \
@@ -634,42 +631,34 @@ impl LineBreaker {
 pub struct FragmentIterator<'a> {
     iter: Enumerate<Items<'a,Fragment>>,
     ranges: &'a Vec<InlineFragmentRange>,
 }
 
 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)))
+        })
     }
 }
 
 /// Mutable iterator over fragments.
 pub struct MutFragmentIterator<'a> {
     iter: Enumerate<MutItems<'a,Fragment>>,
     ranges: &'a Vec<InlineFragmentRange>,
 }
 
 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)))
+        })
     }
 }
 
 /// Represents a list of inline fragments, including element ranges.
 pub struct InlineFragments {
     /// The fragments themselves.
     pub fragments: Vec<Fragment>,
     /// Tracks the elements that made up the fragments above. This is used to
@@ -863,32 +852,27 @@ impl InlineFragments {
                 }
             }
         }
         self.fragments = new_fragments;
     }
 
     /// 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| {
+                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);
     }
 
     /// Strips ignorable whitespace from the end of a list of fragments.
     pub fn strip_ignorable_whitespace_from_end(&mut self) {
         if self.is_empty() {
             return;
@@ -1067,25 +1051,24 @@ impl InlineFlow {
             offset_x = offset_x + size.width;
         }
     }
 
     /// Computes the minimum ascent and descent for each line. This is done during flow
     /// 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)
     }
 }
 
 impl Flow for InlineFlow {
     fn class(&self) -> FlowClass {
         InlineFlowClass
     }
 
@@ -1353,39 +1336,34 @@ struct InlineFragmentFixupWorkItem {
     new_start_index: FragmentIndex,
     old_end_index: FragmentIndex,
 }
 
 /// The type of an iterator over fragment ranges in the fragment map.
 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
         }
     }
 }
 
 /// The context that an inline fragment appears in. This allows the fragment map to be passed in
 /// conveniently to various fragment functions.
 pub struct InlineFragmentContext<'a> {
     ranges: &'a Vec<InlineFragmentRange>,
@@ -1398,20 +1376,30 @@ impl<'a> InlineFragmentContext<'a> {
             ranges: ranges,
             index: index,
         }
     }
 
     /// 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,
         }
     }
 }
 
 /// Height above the baseline, depth below the baseline, and ascent for a fragment. See CSS 2.1 ยง
 /// 10.8.1.
 pub struct InlineMetrics {
     pub height_above_baseline: Au,