Refactor GlyphStore::iter_glyphs_for_byte_range without recursion (#33074)

* Implement DoubleEndedIterator for EachIndex

Signed-off-by: crbrz <cristianb@gmail.com>

* Refactor GlyphStore::iter_glyphs_for_byte_range without recursion

Signed-off-by: crbrz <cristianb@gmail.com>

* Update WPT result

Signed-off-by: crbrz <cristianb@gmail.com>

* Update WPT legacy result

Signed-off-by: crbrz <cristianb@gmail.com>

---------

Signed-off-by: crbrz <cristianb@gmail.com>

Cargo.lock

@@ -1870,6 +1870,7 @@ dependencies = [
  "freetype-sys",
  "harfbuzz-sys",
  "ipc-channel",
+ "itertools 0.13.0",
  "libc",
  "log",
  "malloc_size_of",

components/fonts/Cargo.toml

@@ -25,6 +25,7 @@ fontsan = { git = "https://github.com/servo/fontsan" }
 fonts_traits = { workspace = true }
 harfbuzz-sys = "0.6.1"
 ipc-channel = { workspace = true }
+itertools = { workspace = true }
 libc = { workspace = true }
 log = { workspace = true }
 malloc_size_of = { workspace = true }

components/fonts/glyph.rs

@@ -3,6 +3,7 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 use std::cmp::{Ordering, PartialOrd};
+use std::iter::once;
 use std::sync::Arc;
 use std::vec::Vec;
 use std::{fmt, mem};
@@ -10,9 +11,10 @@ use std::{fmt, mem};
 use app_units::Au;
 use euclid::default::Point2D;
 pub use fonts_traits::ByteIndex;
+use itertools::Either;
 use log::debug;
 use malloc_size_of_derive::MallocSizeOf;
-use range::{self, EachIndex, Range, RangeIndex};
+use range::{self, Range, RangeIndex};
 use serde::{Deserialize, Serialize};
 
 /// GlyphEntry is a port of Gecko's CompressedGlyph scheme for storing glyph data compactly.
@@ -588,7 +590,10 @@ impl<'a> GlyphStore {
     }
 
     #[inline]
-    pub fn iter_glyphs_for_byte_range(&'a self, range: &Range<ByteIndex>) -> GlyphIterator<'a> {
+    pub fn iter_glyphs_for_byte_range(
+        &self,
+        range: &Range<ByteIndex>,
+    ) -> impl Iterator<Item = GlyphInfo> {
         if range.begin() >= self.len() {
             panic!("iter_glyphs_for_range: range.begin beyond length!");
         }
@@ -596,16 +601,31 @@ impl<'a> GlyphStore {
             panic!("iter_glyphs_for_range: range.end beyond length!");
         }
 
-        GlyphIterator {
+        let range_it = if self.is_rtl {
-            store: self,
+            Either::Left(range.each_index().rev())
-            byte_index: if self.is_rtl {
+        } else {
-                range.end()
+            Either::Right(range.each_index())
+        };
+        range_it.into_iter().flat_map(move |range_idx| {
+            let entry = self.entry_buffer[range_idx.to_usize()];
+            let result = if entry.is_simple() {
+                Either::Left(once(GlyphInfo::Simple(self, range_idx)))
             } else {
-                range.begin() - ByteIndex(1)
+                // Slow path for complex glyphs
-            },
+                let glyphs = self.detail_store.detailed_glyphs_for_entry(
-            byte_range: *range,
+                    range_idx,
-            glyph_range: None,
+                    self.entry_buffer[range_idx.to_usize()].glyph_count(),
-        }
+                );
+
+                let complex_glyph_range =
+                    range::each_index(ByteIndex(0), ByteIndex(glyphs.len() as isize));
+                Either::Right(complex_glyph_range.map(move |i| {
+                    GlyphInfo::Detail(self, range_idx, i.get() as u16 /* ??? */)
+                }))
+            };
+
+            result.into_iter()
+        })
     }
 
     // Scan the glyphs for a given range until we reach a given advance. Returns the index
@@ -707,87 +727,6 @@ impl fmt::Debug for GlyphStore {
     }
 }
 
-/// An iterator over the glyphs in a byte range in a `GlyphStore`.
-pub struct GlyphIterator<'a> {
-    store: &'a GlyphStore,
-    byte_index: ByteIndex,
-    byte_range: Range<ByteIndex>,
-    glyph_range: Option<EachIndex<ByteIndex>>,
-}
-
-impl<'a> GlyphIterator<'a> {
-    // Slow path when there is a glyph range.
-    #[inline(never)]
-    fn next_glyph_range(&mut self) -> Option<GlyphInfo<'a>> {
-        match self.glyph_range.as_mut().unwrap().next() {
-            Some(j) => {
-                Some(GlyphInfo::Detail(
-                    self.store,
-                    self.byte_index,
-                    j.get() as u16, /* ??? */
-                ))
-            },
-            None => {
-                // No more glyphs for current character.  Try to get another.
-                self.glyph_range = None;
-                self.next()
-            },
-        }
-    }
-
-    // Slow path when there is a complex glyph.
-    #[inline(never)]
-    fn next_complex_glyph(&mut self, entry: &GlyphEntry, i: ByteIndex) -> Option<GlyphInfo<'a>> {
-        let glyphs = self
-            .store
-            .detail_store
-            .detailed_glyphs_for_entry(i, entry.glyph_count());
-        self.glyph_range = Some(range::each_index(
-            ByteIndex(0),
-            ByteIndex(glyphs.len() as isize),
-        ));
-        self.next()
-    }
-}
-
-impl<'a> Iterator for GlyphIterator<'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.
-    //
-    // This function consists of the fast path and is designed to be inlined into its caller. The
-    // slow paths, which should not be inlined, are `next_glyph_range()` and
-    // `next_complex_glyph()`.
-    #[inline(always)]
-    fn next(&mut self) -> Option<GlyphInfo<'a>> {
-        // 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();
-        }
-
-        // No glyph range. Look at next byte.
-        self.byte_index = self.byte_index +
-            if self.store.is_rtl {
-                ByteIndex(-1)
-            } else {
-                ByteIndex(1)
-            };
-        let i = self.byte_index;
-        if !self.byte_range.contains(i) {
-            return None;
-        }
-        debug_assert!(i < self.store.len());
-        let entry = self.store.entry_buffer[i.to_usize()];
-        if entry.is_simple() {
-            Some(GlyphInfo::Simple(self.store, i))
-        } else {
-            // Fall back to the slow path.
-            self.next_complex_glyph(&entry, i)
-        }
-    }
-}
-
 /// A single series of glyphs within a text run.
 #[derive(Clone, Debug, Deserialize, Serialize)]
 pub struct GlyphRun {

components/range/lib.rs

@@ -193,6 +193,19 @@ impl<I: RangeIndex> Iterator for EachIndex<I> {
     }
 }
 
+impl<I: RangeIndex> DoubleEndedIterator for EachIndex<I> {
+    #[inline]
+    fn next_back(&mut self) -> Option<Self::Item> {
+        if self.start < self.stop {
+            let next = self.stop - I::one();
+            self.stop = next;
+            Some(next)
+        } else {
+            None
+        }
+    }
+}
+
 impl<I: RangeIndex> Range<I> {
     /// Create a new range from beginning and length offsets. This could be
     /// denoted as `[begin, begin + length)`.

tests/wpt/meta-legacy-layout/css/css-fonts/crash-large-grapheme-cluster.html.ini

@@ -1,4 +0,0 @@
-[crash-large-grapheme-cluster.html]
-  expected: CRASH
-  [Should not crash when there is an exceedingly large grapheme cluster]
-    expected: FAIL

tests/wpt/meta/css/css-fonts/crash-large-grapheme-cluster.html.ini

@@ -1,4 +0,0 @@
-[crash-large-grapheme-cluster.html]
-  expected: CRASH
-  [Should not crash when there is an exceedingly large grapheme cluster]
-    expected: FAIL