Fix the disturbing state of unpurity in the glyph storage and

iteration code. Move from DVec to ~[] inside GlyphStore.

Add public API trait for the shaper, clean up other shaping API stuff.

Remove legacy exports from servo_util::vec.

src/servo-gfx/font.rs

@@ -376,7 +376,7 @@ pub trait FontMethods {
                               baseline_origin: Point2D<Au>,
                               color: Color);
     fn measure_text(&TextRun, &const Range) -> RunMetrics;
-    fn shape_text(@self, &str) -> GlyphStore;
+    fn shape_text(@self, &str, &mut GlyphStore);
     fn get_descriptor() -> FontDescriptor;
 
     // these are used to get glyphs and advances in the case that the
@@ -471,11 +471,11 @@ pub impl Font : FontMethods {
         }
     }
 
-    fn shape_text(@self, text: &str) -> GlyphStore {
+    fn shape_text(@self, text: &str, store: &mut GlyphStore) {
-        let store = GlyphStore(str::char_len(text));
+        // TODO(Issue #229): use a more efficient strategy for repetitive shaping.
+        // For example, Gecko uses a per-"word" hashtable of shaper results.
         let shaper = self.get_shaper();
-        shaper.shape_text(text, &store);
+        shaper.shape_text(text, store);
-        return move store;
     }
 
     fn get_descriptor() -> FontDescriptor {

src/servo-gfx/servo_gfx.rc

@@ -94,4 +94,4 @@ pub mod util {
 use servo_util = util;
 use gfx_font = font;
 use gfx_font_context = font_context;
-use gfx_font_list = font_list;
+use gfx_font_list = font_list;

src/servo-gfx/text/glyph.rs

@@ -259,7 +259,7 @@ impl GlyphEntry {
     }
 
     #[inline(always)]
-    pure fn adapt_character_flags_of_entry(other: &GlyphEntry) -> GlyphEntry {
+    pure fn adapt_character_flags_of_entry(other: GlyphEntry) -> GlyphEntry {
         GlyphEntry { value: self.value | other.value }
     }
 }
@@ -308,21 +308,21 @@ impl DetailedGlyphRecord : Eq {
 // usage pattern of setting/appending all the detailed glyphs, and
 // then querying without setting.
 struct DetailedGlyphStore {
-    detail_buffer: DVec<DetailedGlyph>,
+    detail_buffer: ~[DetailedGlyph],
-    detail_lookup: DVec<DetailedGlyphRecord>,
+    detail_lookup: ~[DetailedGlyphRecord],
-    mut lookup_is_sorted: bool,
+    lookup_is_sorted: bool,
 }
 
 fn DetailedGlyphStore() -> DetailedGlyphStore {
     DetailedGlyphStore {
-        detail_buffer: DVec(),
+        detail_buffer: ~[], // TODO: default size?
-        detail_lookup: DVec(),
+        detail_lookup: ~[],
         lookup_is_sorted: false
     }
 }
 
 impl DetailedGlyphStore {
-    fn add_detailed_glyphs_for_entry(entry_offset: uint, glyphs: &[DetailedGlyph]) {
+    fn add_detailed_glyphs_for_entry(&mut self, entry_offset: uint, glyphs: &[DetailedGlyph]) {
         let entry = DetailedGlyphRecord {
             entry_offset: entry_offset,
             detail_offset: self.detail_buffer.len()
@@ -346,71 +346,72 @@ impl DetailedGlyphStore {
         self.lookup_is_sorted = false;
     }
 
-    // not pure; may perform a deferred sort.
+    pure fn get_detailed_glyphs_for_entry(&self, entry_offset: uint, count: u16) -> &self/[DetailedGlyph] {
-    fn get_detailed_glyphs_for_entry(&self, entry_offset: uint, count: u16) -> &[DetailedGlyph] {
         debug!("Requesting detailed glyphs[n=%u] for entry[off=%u]", count as uint, entry_offset);
 
         // FIXME: Is this right? --pcwalton
         // TODO: should fix this somewhere else
         if count == 0 {
-            let result =  do self.detail_buffer.borrow |glyphs: &[DetailedGlyph]| {
+            return vec::view(self.detail_buffer, 0, 0);
-                vec::view(glyphs, 0, 0)
-            };
-            return result;
         }
 
         assert (count as uint) <= self.detail_buffer.len();
-        self.ensure_sorted();
+        assert self.lookup_is_sorted;
 
         let key = DetailedGlyphRecord {
             entry_offset: entry_offset,
             detail_offset: 0 // unused
         };
 
-        do self.detail_lookup.borrow |records : &[DetailedGlyphRecord]| {
+        // FIXME: This is a workaround for borrow of self.detail_lookup not getting inferred.
-            match records.binary_search_index(&key) {
+        let records : &[DetailedGlyphRecord] = self.detail_lookup;
-                None => fail ~"Invalid index not found in detailed glyph lookup table!",
+        match records.binary_search_index(&key) {
-                Some(i) => {
+            None => fail ~"Invalid index not found in detailed glyph lookup table!",
-                    do self.detail_buffer.borrow |glyphs : &[DetailedGlyph]| {
+            Some(i) => {
-                        assert i + (count as uint)  <= glyphs.len();
+                assert i + (count as uint) <= self.detail_buffer.len();
-                        // return a view into the buffer
+                // return a view into the buffer
-                        vec::view(glyphs, i, i + count as uint)
+                vec::view(self.detail_buffer, i, i + count as uint)
-                    }
-                }
             }
         }
     }
 
-    fn get_detailed_glyph_with_index(&self, entry_offset: uint, detail_offset: u16) -> &DetailedGlyph {
+    pure fn get_detailed_glyph_with_index(&self, entry_offset: uint, detail_offset: u16) -> &self/DetailedGlyph {
         assert (detail_offset as uint) <= self.detail_buffer.len();
-        self.ensure_sorted();
+        assert self.lookup_is_sorted;
 
         let key = DetailedGlyphRecord {
             entry_offset: entry_offset,
             detail_offset: 0 // unused
         };
 
-        do self.detail_lookup.borrow |records : &[DetailedGlyphRecord]| {
+        // FIXME: This is a workaround for borrow of self.detail_lookup not getting inferred.
-            match records.binary_search_index(&key) {
+        let records : &[DetailedGlyphRecord] = self.detail_lookup;
-                None => fail ~"Invalid index not found in detailed glyph lookup table!",
+        match records.binary_search_index(&key) {
-                Some(i) => {
+            None => fail ~"Invalid index not found in detailed glyph lookup table!",
-                    do self.detail_buffer.borrow |glyphs : &[DetailedGlyph]| {
+            Some(i) => {
-                        assert i + (detail_offset as uint)  < glyphs.len();
+                assert i + (detail_offset as uint)  < self.detail_buffer.len();
-                        &glyphs[i+(detail_offset as uint)]
+                &self.detail_buffer[i+(detail_offset as uint)]
-                    }
-                }
             }
         }
     }
 
-    /*priv*/ fn ensure_sorted() {
+    fn ensure_sorted(&mut self) {
         if self.lookup_is_sorted {
             return;
         }
 
-        do self.detail_lookup.borrow_mut |arr| {
+        // Sorting a unique vector is surprisingly hard. The follwing
-            sort::quick_sort3(arr);
+        // code is a good argument for using DVecs, but they require
-        };
+        // immutable locations thus don't play well with freezing.
+
+        // Thar be dragons here. You have been warned. (Tips accepted.)
+        let mut unsorted_records : ~[DetailedGlyphRecord] = ~[];
+        core::util::swap(&mut self.detail_lookup, &mut unsorted_records);
+        let mut_records : ~[mut DetailedGlyphRecord] = vec::to_mut(move unsorted_records);
+        sort::quick_sort3(mut_records);
+        let mut sorted_records = vec::from_mut(move mut_records);
+        core::util::swap(&mut self.detail_lookup, &mut sorted_records);
+
         self.lookup_is_sorted = true;
     }
 }
@@ -498,25 +499,27 @@ impl GlyphInfo {
 // Public data structure and API for storing and retrieving glyph data
 struct GlyphStore {
     // we use a DVec here instead of a mut vec, since this is much safer.
-    entry_buffer: DVec<GlyphEntry>,
+    entry_buffer: ~[GlyphEntry],
     detail_store: DetailedGlyphStore,
 }
 
-// Initializes the glyph store, but doesn't actually shape anything.
-// Use the set_glyph, set_glyphs() methods to store glyph data.
-fn GlyphStore(length: uint) -> GlyphStore {
-    assert length > 0;
-
-    let buffer = vec::from_elem(length, InitialGlyphEntry());
-
-    GlyphStore {
-        entry_buffer: dvec::from_vec(move buffer),
-        detail_store: DetailedGlyphStore(),
-    }
-}
-
 impl GlyphStore {
-    fn add_glyph_for_char_index(i: uint, data: &GlyphData) {
+    // Initializes the glyph store, but doesn't actually shape anything.
+    // Use the set_glyph, set_glyphs() methods to store glyph data.
+    static fn new(length: uint) -> GlyphStore {
+        assert length > 0;
+
+        GlyphStore {
+            entry_buffer: vec::from_elem(length, InitialGlyphEntry()),
+            detail_store: DetailedGlyphStore(),
+        }
+    }
+
+    fn finalize_changes(&mut self) {
+        self.detail_store.ensure_sorted();
+    }
+
+    fn add_glyph_for_char_index(&mut self, i: uint, data: &GlyphData) {
 
         pure fn glyph_is_compressible(data: &GlyphData) -> bool {
             is_simple_glyph_id(data.index)
@@ -536,14 +539,12 @@ impl GlyphStore {
                 self.detail_store.add_detailed_glyphs_for_entry(i, glyph);
                 ComplexGlyphEntry(data.cluster_start, data.ligature_start, 1)
             }
-        }.adapt_character_flags_of_entry(&self.entry_buffer[i]);
+        }.adapt_character_flags_of_entry(self.entry_buffer[i]);
 
-        //debug!("Adding single glyph[idx=%u]: %?", i, entry);
+        self.entry_buffer[i] = entry;
-
-        self.entry_buffer.set_elt(i, entry);
     }
 
-    fn add_glyphs_for_char_index(i: uint, data_for_glyphs: &[GlyphData]) {
+    fn add_glyphs_for_char_index(&mut self, i: uint, data_for_glyphs: &[GlyphData]) {
         assert i < self.entry_buffer.len();
         assert data_for_glyphs.len() > 0;
 
@@ -564,36 +565,36 @@ impl GlyphStore {
                                   first_glyph_data.ligature_start,
                                   glyph_count)
             }
-        }.adapt_character_flags_of_entry(&self.entry_buffer[i]);
+        }.adapt_character_flags_of_entry(self.entry_buffer[i]);
 
         debug!("Adding multiple glyphs[idx=%u, count=%u]: %?", i, glyph_count, entry);
 
-        self.entry_buffer.set_elt(i, entry);
+        self.entry_buffer[i] = entry;
     }
 
     // used when a character index has no associated glyph---for example, a ligature continuation.
-    fn add_nonglyph_for_char_index(&self, i: uint, cluster_start: bool, ligature_start: bool) {
+    fn add_nonglyph_for_char_index(&mut self, i: uint, cluster_start: bool, ligature_start: bool) {
         assert i < self.entry_buffer.len();
 
         let entry = ComplexGlyphEntry(cluster_start, ligature_start, 0);
         debug!("adding spacer for chracter without associated glyph[idx=%u]", i);
 
-        self.entry_buffer.set_elt(i, entry);
+        self.entry_buffer[i] = entry;
     }
 
-    fn iter_glyphs_for_char_index(&self, i: uint, cb: fn&(uint, GlyphInfo/&) -> bool) -> bool {
+    pure fn iter_glyphs_for_char_index(i: uint, cb: fn&(uint, GlyphInfo/&) -> bool) -> bool {
         assert i < self.entry_buffer.len();
 
         let entry = &self.entry_buffer[i];
         match entry.is_simple() {
             true => { 
-                let proxy = SimpleGlyphInfo(self, i);
+                let proxy = SimpleGlyphInfo(&self, i);
                 cb(i, move proxy);
             },
             false => {
                 let glyphs = self.detail_store.get_detailed_glyphs_for_entry(i, entry.glyph_count());
                 for uint::range(0, glyphs.len()) |j| {
-                    let proxy = DetailGlyphInfo(self, i, j as u16);
+                    let proxy = DetailGlyphInfo(&self, i, j as u16);
                     cb(i, move proxy);
                 }
             }
@@ -602,7 +603,7 @@ impl GlyphStore {
 		return true;
     }
 
-    fn iter_glyphs_for_byte_range(&self, range: &const Range, cb: fn&(uint, GlyphInfo/&) -> bool) {
+    pure fn iter_glyphs_for_byte_range(range: &const Range, cb: fn&(uint, GlyphInfo/&) -> bool) {
         if range.begin() >= self.entry_buffer.len() {
             error!("iter_glyphs_for_range: range.begin beyond length!");
             return;
@@ -619,24 +620,24 @@ impl GlyphStore {
 	}
     }
 
-    fn iter_all_glyphs(cb: fn&(uint, GlyphInfo/&) -> bool) {
+    pure fn iter_all_glyphs(cb: fn&(uint, GlyphInfo/&) -> bool) {
         for uint::range(0, self.entry_buffer.len()) |i| {
             if !self.iter_glyphs_for_char_index(i, cb) { break; }
         }
     }
 
     // getter methods
-    fn char_is_space(i: uint) -> bool {
+    pure fn char_is_space(i: uint) -> bool {
         assert i < self.entry_buffer.len();
         self.entry_buffer[i].char_is_space()
     }
 
-    fn char_is_tab(i: uint) -> bool {
+    pure fn char_is_tab(i: uint) -> bool {
         assert i < self.entry_buffer.len();
         self.entry_buffer[i].char_is_tab()
     }
 
-    fn char_is_newline(i: uint) -> bool {
+    pure fn char_is_newline(i: uint) -> bool {
         if i >= self.entry_buffer.len() {
             error!("char_is_newline: beyond entry buffer!");
             false
@@ -645,45 +646,45 @@ impl GlyphStore {
         }
     }
 
-    fn is_ligature_start(i: uint) -> bool {
+    pure fn is_ligature_start(i: uint) -> bool {
         assert i < self.entry_buffer.len();
         self.entry_buffer[i].is_ligature_start()
     }
 
-    fn is_cluster_start(i: uint) -> bool {
+    pure fn is_cluster_start(i: uint) -> bool {
         assert i < self.entry_buffer.len();
         self.entry_buffer[i].is_cluster_start()
     }
 
-    fn can_break_before(i: uint) -> BreakType {
+    pure fn can_break_before(i: uint) -> BreakType {
         assert i < self.entry_buffer.len();
         self.entry_buffer[i].can_break_before()
     }
 
     // setter methods
-    fn set_char_is_space(i: uint) {
+    fn set_char_is_space(&mut self, i: uint) {
         assert i < self.entry_buffer.len();
         let entry = self.entry_buffer[i];
-        self.entry_buffer.set_elt(i, entry.set_char_is_space())
+        self.entry_buffer[i] = entry.set_char_is_space();
     }
 
-    fn set_char_is_tab(i: uint) {
+    fn set_char_is_tab(&mut self, i: uint) {
         assert i < self.entry_buffer.len();
         let entry = self.entry_buffer[i];
-        self.entry_buffer.set_elt(i, entry.set_char_is_tab())
+        self.entry_buffer[i] = entry.set_char_is_tab();
     }
 
-    fn set_char_is_newline(i: uint) {
+    fn set_char_is_newline(&mut self, i: uint) {
         assert i < self.entry_buffer.len();
         let entry = self.entry_buffer[i];
-        self.entry_buffer.set_elt(i, entry.set_char_is_newline())
+        self.entry_buffer[i] = entry.set_char_is_newline();
     }
 
-    fn set_can_break_before(i: uint, t: BreakType) {
+    fn set_can_break_before(&mut self, i: uint, t: BreakType) {
         assert i < self.entry_buffer.len();
         let entry = self.entry_buffer[i];
         match entry.set_can_break_before(t) {
-            Some(e) => self.entry_buffer.set_elt(i, e),
+            Some(e) => self.entry_buffer[i] = e,
             None => {}
         };
     }

src/servo-gfx/text/harfbuzz/shaper.rs

@@ -12,6 +12,8 @@ use font::{
 };
 
 use glyph::{GlyphStore, GlyphIndex, GlyphData};
+use text::shaper::ShaperMethods;
+
 use servo_util::range;
 use range::Range;
 
@@ -84,7 +86,7 @@ pub impl ShapedGlyphData {
     }
 
     #[inline(always)]
-    pure fn byte_offset_of_glyph(i: uint) -> uint unsafe {
+    priv pure fn byte_offset_of_glyph(&const self, i: uint) -> uint unsafe {
         assert i < self.count;
 
         let glyph_info_i = ptr::offset(self.glyph_infos, i);
@@ -171,12 +173,26 @@ pub impl HarfbuzzShaper {
             hb_funcs: hb_funcs,
         }
     }
-    
+
+    static priv fn float_to_fixed(f: float) -> i32 {
+        util::float_to_fixed(16, f)
+    }
+
+    static priv fn fixed_to_float(i: hb_position_t) -> float {
+        util::fixed_to_float(16, i)
+    }
+
+    static priv fn fixed_to_rounded_int(f: hb_position_t) -> int {
+        util::fixed_to_rounded_int(16, f)
+    }
+}
+
+pub impl HarfbuzzShaper : ShaperMethods {    
     /**
     Calculate the layout metrics associated with a some given text
     when rendered in a specific font.
     */
-    pub fn shape_text(text: &str, glyphs: &GlyphStore) {
+    fn shape_text(text: &str, glyphs: &mut GlyphStore) {
         let hb_buffer: *hb_buffer_t = hb_buffer_create();
         hb_buffer_set_direction(hb_buffer, HB_DIRECTION_LTR);
 
@@ -194,7 +210,7 @@ pub impl HarfbuzzShaper {
         hb_buffer_destroy(hb_buffer);
     }
 
-    priv fn save_glyph_results(text: &str, glyphs: &GlyphStore, buffer: *hb_buffer_t) {
+    priv fn save_glyph_results(text: &str, glyphs: &mut GlyphStore, buffer: *hb_buffer_t) {
         let glyph_data = ShapedGlyphData::new(buffer);
         let glyph_count = glyph_data.len();
         let byte_max = text.len();
@@ -397,18 +413,10 @@ pub impl HarfbuzzShaper {
             char_byte_span.reset(char_byte_span.end(), 0);
             char_idx += 1;
         }
-    }
 
-    static priv fn float_to_fixed(f: float) -> i32 {
+        // this must be called after adding all glyph data; it sorts the
-        util::float_to_fixed(16, f)
+        // lookup table for finding detailed glyphs by associated char index.
-    }
+        glyphs.finalize_changes();
-
-    static priv fn fixed_to_float(i: hb_position_t) -> float {
-        util::fixed_to_float(16, i)
-    }
-
-    static priv fn fixed_to_rounded_int(f: hb_position_t) -> int {
-        util::fixed_to_rounded_int(16, f)
     }
 }
 

src/servo-gfx/text/shaper.rs

@@ -4,10 +4,16 @@ Uniscribe, Pango, or Coretext.
 
 Currently, only harfbuzz bindings are implemented.
 */
-use servo_gfx_font::Font;
+use gfx_font::Font;
+use text::glyph::GlyphStore;
 
 pub type Shaper/& = harfbuzz::shaper::HarfbuzzShaper;
 
+trait ShaperMethods {
+    fn shape_text(text: &str, glyphs: &mut GlyphStore);
+
+}
+
 // TODO(Issue #163): this is a workaround for static methods and
 // typedefs not working well together. It should be removed.
 impl Shaper {

src/servo-gfx/text/text_run.rs

@@ -41,7 +41,9 @@ impl SendableTextRun {
 
 impl TextRun {
     static fn new(font: @Font, text: ~str) -> TextRun {
-        let glyph_store = font.shape_text(text);
+        let mut glyph_store = GlyphStore::new(str::char_len(text));
+        font.shape_text(text, &mut glyph_store);
+
         let run = TextRun {
             text: move text,
             font: font,

src/servo-gfx/util/vec.rs

@@ -1,13 +1,11 @@
 use core::cmp::{Ord, Eq};
 
-export BinarySearchMethods, binary_search, binary_search_index;
+pub trait BinarySearchMethods<T: Ord Eq> {
-
-trait BinarySearchMethods<T: Ord Eq> {
     pure fn binary_search(&self, key: &T) -> Option<&self/T>;
     pure fn binary_search_index(&self, key: &T) -> Option<uint>;
 }
 
-impl<T: Ord Eq> &[T]: BinarySearchMethods<T> {
+pub impl<T: Ord Eq> &[T]: BinarySearchMethods<T> {
     pure fn binary_search(&self, key: &T) -> Option<&self/T> {
         match self.binary_search_index(key) {
             None => None,