From e663005ce21e1d5a9237bdfc1c1392e0eef56c2f Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Wed, 30 Sep 2015 09:36:01 -0700 Subject: [PATCH 1/5] Simplify add_glyph_for_char_index --- components/gfx/text/glyph.rs | 14 ++++++-------- components/gfx/text/shaping/harfbuzz.rs | 4 ++-- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/components/gfx/text/glyph.rs b/components/gfx/text/glyph.rs index 71b42fe2c9e..c602a3b73e5 100644 --- a/components/gfx/text/glyph.rs +++ b/components/gfx/text/glyph.rs @@ -465,19 +465,17 @@ impl<'a> GlyphStore { /// otherwise, this glyph represents multiple characters. pub fn add_glyph_for_char_index(&mut self, i: CharIndex, - character: Option, + character: char, data: &GlyphData) { - fn glyph_is_compressible(data: &GlyphData) -> bool { - is_simple_glyph_id(data.id) - && is_simple_advance(data.advance) + let glyph_is_compressible = is_simple_glyph_id(data.id) && + is_simple_advance(data.advance) && data.offset == Point2D::zero() - && data.cluster_start // others are stored in detail buffer - } + && data.cluster_start; // others are stored in detail buffer debug_assert!(data.ligature_start); // can't compress ligature continuation glyphs. debug_assert!(i < self.char_len()); - let mut entry = match (data.is_missing, glyph_is_compressible(data)) { + let mut entry = match (data.is_missing, glyph_is_compressible) { (true, _) => GlyphEntry::missing(1), (false, true) => GlyphEntry::simple(data.id, data.advance), (false, false) => { @@ -488,7 +486,7 @@ impl<'a> GlyphStore { } }; - if character == Some(' ') { + if character == ' ' { entry = entry.set_char_is_space() } diff --git a/components/gfx/text/shaping/harfbuzz.rs b/components/gfx/text/shaping/harfbuzz.rs index 5a539358779..87397b46c28 100644 --- a/components/gfx/text/shaping/harfbuzz.rs +++ b/components/gfx/text/shaping/harfbuzz.rs @@ -456,7 +456,7 @@ impl Shaper { false, true, true); - glyphs.add_glyph_for_char_index(char_idx, Some(character), &data); + glyphs.add_glyph_for_char_index(char_idx, character, &data); } else { let shape = glyph_data.entry_for_glyph(glyph_span.begin(), &mut y_pos); let advance = self.advance_for_shaped_glyph(shape.advance, character, options); @@ -466,7 +466,7 @@ impl Shaper { false, true, true); - glyphs.add_glyph_for_char_index(char_idx, Some(character), &data); + glyphs.add_glyph_for_char_index(char_idx, character, &data); } } else { // collect all glyphs to be assigned to the first character. From 4174c918ad73ed9aba67059e58e1102eb27e3227 Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Wed, 30 Sep 2015 09:38:12 -0700 Subject: [PATCH 2/5] Make set_char_is_space mutate self in place --- components/gfx/text/glyph.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/components/gfx/text/glyph.rs b/components/gfx/text/glyph.rs index c602a3b73e5..713a1caa708 100644 --- a/components/gfx/text/glyph.rs +++ b/components/gfx/text/glyph.rs @@ -129,8 +129,8 @@ impl GlyphEntry { } #[inline(always)] - fn set_char_is_space(&self) -> GlyphEntry { - GlyphEntry::new(self.value | FLAG_CHAR_IS_SPACE) + fn set_char_is_space(&mut self) { + self.value |= FLAG_CHAR_IS_SPACE; } fn glyph_count(&self) -> u16 { @@ -487,7 +487,7 @@ impl<'a> GlyphStore { }; if character == ' ' { - entry = entry.set_char_is_space() + entry.set_char_is_space() } self.entry_buffer[i.to_usize()] = entry; From fa85d5f312c16da752112e34baa9785679ffe74c Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Wed, 30 Sep 2015 09:41:53 -0700 Subject: [PATCH 3/5] Remove unneeded adapt_character_flags_of_entry --- components/gfx/text/glyph.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/components/gfx/text/glyph.rs b/components/gfx/text/glyph.rs index 713a1caa708..bd8d79cab83 100644 --- a/components/gfx/text/glyph.rs +++ b/components/gfx/text/glyph.rs @@ -147,11 +147,6 @@ impl GlyphEntry { fn has_flag(&self, flag: u32) -> bool { (self.value & flag) != 0 } - - #[inline(always)] - fn adapt_character_flags_of_entry(&self, other: GlyphEntry) -> GlyphEntry { - GlyphEntry { value: self.value | other.value } - } } // Stores data for a detailed glyph, in the case that several glyphs @@ -515,7 +510,7 @@ impl<'a> GlyphStore { first_glyph_data.ligature_start, glyph_count) } - }.adapt_character_flags_of_entry(self.entry_buffer[i.to_usize()]); + }; debug!("Adding multiple glyphs[idx={:?}, count={}]: {:?}", i, glyph_count, entry); From d3d1d156153f9689faa7afcbfc92f263e8fbf353 Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Wed, 30 Sep 2015 09:45:24 -0700 Subject: [PATCH 4/5] Remove unused is_missing flag --- components/gfx/text/glyph.rs | 53 +++++++++---------------- components/gfx/text/shaping/harfbuzz.rs | 3 -- 2 files changed, 18 insertions(+), 38 deletions(-) diff --git a/components/gfx/text/glyph.rs b/components/gfx/text/glyph.rs index bd8d79cab83..d3eb0c2fc7a 100644 --- a/components/gfx/text/glyph.rs +++ b/components/gfx/text/glyph.rs @@ -62,14 +62,6 @@ impl GlyphEntry { GlyphEntry::new(glyph_count as u32) } - /// Create a GlyphEntry for the case where glyphs couldn't be found for the specified - /// character. - fn missing(glyph_count: usize) -> GlyphEntry { - assert!(glyph_count <= u16::MAX as usize); - - GlyphEntry::new(glyph_count as u32) - } - fn is_initial(&self) -> bool { *self == GlyphEntry::initial() } @@ -317,7 +309,6 @@ pub struct GlyphData { id: GlyphId, advance: Au, offset: Point2D, - is_missing: bool, cluster_start: bool, ligature_start: bool, } @@ -327,7 +318,6 @@ impl GlyphData { pub fn new(id: GlyphId, advance: Au, offset: Option>, - is_missing: bool, cluster_start: bool, ligature_start: bool) -> GlyphData { @@ -335,7 +325,6 @@ impl GlyphData { id: id, advance: advance, offset: offset.unwrap_or(Point2D::zero()), - is_missing: is_missing, cluster_start: cluster_start, ligature_start: ligature_start, } @@ -470,15 +459,13 @@ impl<'a> GlyphStore { debug_assert!(data.ligature_start); // can't compress ligature continuation glyphs. debug_assert!(i < self.char_len()); - let mut entry = match (data.is_missing, glyph_is_compressible) { - (true, _) => GlyphEntry::missing(1), - (false, true) => GlyphEntry::simple(data.id, data.advance), - (false, false) => { - let glyph = &[DetailedGlyph::new(data.id, data.advance, data.offset)]; - self.has_detailed_glyphs = true; - self.detail_store.add_detailed_glyphs_for_entry(i, glyph); - GlyphEntry::complex(data.cluster_start, data.ligature_start, 1) - } + let mut entry = if glyph_is_compressible { + GlyphEntry::simple(data.id, data.advance) + } else { + let glyph = &[DetailedGlyph::new(data.id, data.advance, data.offset)]; + self.has_detailed_glyphs = true; + self.detail_store.add_detailed_glyphs_for_entry(i, glyph); + GlyphEntry::complex(data.cluster_start, data.ligature_start, 1) }; if character == ' ' { @@ -495,22 +482,18 @@ impl<'a> GlyphStore { let glyph_count = data_for_glyphs.len(); let first_glyph_data = data_for_glyphs[0]; - let entry = match first_glyph_data.is_missing { - true => GlyphEntry::missing(glyph_count), - false => { - let glyphs_vec: Vec = (0..glyph_count).map(|i| { - DetailedGlyph::new(data_for_glyphs[i].id, - data_for_glyphs[i].advance, - data_for_glyphs[i].offset) - }).collect(); + let glyphs_vec: Vec = (0..glyph_count).map(|i| { + DetailedGlyph::new(data_for_glyphs[i].id, + data_for_glyphs[i].advance, + data_for_glyphs[i].offset) + }).collect(); - self.has_detailed_glyphs = true; - self.detail_store.add_detailed_glyphs_for_entry(i, &glyphs_vec); - GlyphEntry::complex(first_glyph_data.cluster_start, - first_glyph_data.ligature_start, - glyph_count) - } - }; + self.has_detailed_glyphs = true; + self.detail_store.add_detailed_glyphs_for_entry(i, &glyphs_vec); + + let entry = GlyphEntry::complex(first_glyph_data.cluster_start, + first_glyph_data.ligature_start, + glyph_count); debug!("Adding multiple glyphs[idx={:?}, count={}]: {:?}", i, glyph_count, entry); diff --git a/components/gfx/text/shaping/harfbuzz.rs b/components/gfx/text/shaping/harfbuzz.rs index 87397b46c28..15b92c528b3 100644 --- a/components/gfx/text/shaping/harfbuzz.rs +++ b/components/gfx/text/shaping/harfbuzz.rs @@ -453,7 +453,6 @@ impl Shaper { let data = GlyphData::new(space_glyph_id, advance, Default::default(), - false, true, true); glyphs.add_glyph_for_char_index(char_idx, character, &data); @@ -463,7 +462,6 @@ impl Shaper { let data = GlyphData::new(shape.codepoint, advance, shape.offset, - false, true, true); glyphs.add_glyph_for_char_index(char_idx, character, &data); @@ -477,7 +475,6 @@ impl Shaper { datas.push(GlyphData::new(shape.codepoint, shape.advance, shape.offset, - false, // not missing true, // treat as cluster start glyph_i > glyph_span.begin())); // all but first are ligature continuations From 5bacfe9db9f0474b89b9c86678b209cd74b7394b Mon Sep 17 00:00:00 2001 From: Matt Brubeck Date: Wed, 30 Sep 2015 09:51:06 -0700 Subject: [PATCH 5/5] Remove unused CharIndex field from GlyphIterator::Item --- components/gfx/paint_context.rs | 2 +- components/gfx/text/glyph.rs | 16 +++++++--------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/components/gfx/paint_context.rs b/components/gfx/paint_context.rs index 87463981aa8..7711ea2a0be 100644 --- a/components/gfx/paint_context.rs +++ b/components/gfx/paint_context.rs @@ -1791,7 +1791,7 @@ impl ScaledFontExtensionMethods for ScaledFont { azglyphs.reserve(range.length().to_usize()); for slice in run.natural_word_slices_in_visual_order(range) { - for (_i, glyph) in slice.glyphs.iter_glyphs_for_char_range(&slice.range) { + for glyph in slice.glyphs.iter_glyphs_for_char_range(&slice.range) { let glyph_advance = glyph.advance(); let glyph_offset = glyph.offset().unwrap_or(Point2D::zero()); let azglyph = struct__AzGlyph { diff --git a/components/gfx/text/glyph.rs b/components/gfx/text/glyph.rs index d3eb0c2fc7a..8bc1a6e5bf0 100644 --- a/components/gfx/text/glyph.rs +++ b/components/gfx/text/glyph.rs @@ -542,7 +542,7 @@ impl<'a> GlyphStore { #[inline] pub fn advance_for_char_range_slow_path(&self, rang: &Range) -> Au { self.iter_glyphs_for_char_range(rang) - .fold(Au(0), |advance, (_, glyph)| advance + glyph.advance()) + .fold(Au(0), |advance, glyph| advance + glyph.advance()) } #[inline] @@ -664,11 +664,10 @@ pub struct GlyphIterator<'a> { impl<'a> GlyphIterator<'a> { // Slow path when there is a glyph range. #[inline(never)] - fn next_glyph_range(&mut self) -> Option<(CharIndex, GlyphInfo<'a>)> { + fn next_glyph_range(&mut self) -> Option> { match self.glyph_range.as_mut().unwrap().next() { Some(j) => { - Some((self.char_index, - GlyphInfo::Detail(self.store, self.char_index, j.get() as u16 /* ??? */))) + Some(GlyphInfo::Detail(self.store, self.char_index, j.get() as u16 /* ??? */)) } None => { // No more glyphs for current character. Try to get another. @@ -680,8 +679,7 @@ impl<'a> GlyphIterator<'a> { // Slow path when there is a complex glyph. #[inline(never)] - fn next_complex_glyph(&mut self, entry: &GlyphEntry, i: CharIndex) - -> Option<(CharIndex, GlyphInfo<'a>)> { + fn next_complex_glyph(&mut self, entry: &GlyphEntry, i: CharIndex) -> Option> { let glyphs = self.store.detail_store.detailed_glyphs_for_entry(i, entry.glyph_count()); self.glyph_range = Some(range::each_index(CharIndex(0), CharIndex(glyphs.len() as isize))); self.next() @@ -689,7 +687,7 @@ impl<'a> GlyphIterator<'a> { } impl<'a> Iterator for GlyphIterator<'a> { - type Item = (CharIndex, GlyphInfo<'a>); + type Item = GlyphInfo<'a>; // I tried to start with something simpler and apply FlatMap, but the // inability to store free variables in the FlatMap struct was problematic. @@ -698,7 +696,7 @@ impl<'a> Iterator for GlyphIterator<'a> { // slow paths, which should not be inlined, are `next_glyph_range()` and // `next_complex_glyph()`. #[inline(always)] - fn next(&mut self) -> Option<(CharIndex, GlyphInfo<'a>)> { + fn next(&mut self) -> Option> { // Would use 'match' here but it borrows contents in a way that interferes with mutation. if self.glyph_range.is_some() { return self.next_glyph_range() @@ -717,7 +715,7 @@ impl<'a> Iterator for GlyphIterator<'a> { debug_assert!(i < self.store.char_len()); let entry = self.store.entry_buffer[i.to_usize()]; if entry.is_simple() { - Some((i, GlyphInfo::Simple(self.store, i))) + Some(GlyphInfo::Simple(self.store, i)) } else { // Fall back to the slow path. self.next_complex_glyph(&entry, i)