[IFT] address review comments.

incremental-font-transfer/src/glyph_keyed.rs

@@ -1,25 +1,28 @@
+//! Implementation of Glyph Keyed patch application.
+//!
+//! Glyph Keyed patches are a type of incremental font patch which stores opaque data blobs
+//! keyed by glyph id. Patch application places the data blobs into the appropriate place
+//! in the base font based on the associated glyph id.
+//!
+//! Glyph Keyed patches are specified here:
+//! <https://w3c.github.io/IFT/Overview.html#glyph-keyed>
 use crate::font_patch::PatchingError;
-/// Implementation of Glyph Keyed patch application.
-///
-/// Glyph Keyed patches are a type of incremental font patch which stores opaque data blobs
-/// keyed by glyph id. Patch application places the data blobs into the appropriate place
-/// in the base font based on the associated glyph id.
-///
-/// Glyph Keyed patches are specified here:
-/// <https://w3c.github.io/IFT/Overview.html#glyph-keyed>
 use crate::table_keyed::copy_unprocessed_tables;
 
 use font_types::Scalar;
-use read_fonts::collections::IntSet;
-use read_fonts::tables::ift::{GlyphKeyedPatch, GlyphPatches};
-use read_fonts::tables::loca::Loca;
-use read_fonts::types::Tag;
-use read_fonts::ReadError;
-use read_fonts::{FontData, FontRef, TableProvider};
+use read_fonts::{
+    collections::IntSet,
+    tables::{
+        ift::{GlyphKeyedPatch, GlyphPatches},
+        loca::Loca,
+    },
+    types::Tag,
+    FontData, FontRef, ReadError, TableProvider,
+};
+
 use shared_brotli_patch_decoder::shared_brotli_decode;
 use skrifa::GlyphId;
-use std::collections::BTreeSet;
-use std::collections::HashMap;
+use std::collections::{BTreeSet, HashMap};
 use std::ops::RangeInclusive;
 
 use write_fonts::FontBuilder;

incremental-font-transfer/src/patch_group.rs

@@ -1,3 +1,8 @@
+//! API for selecting and applying a group of IFT patches.
+//!
+//! This provides methods for selecting a maximal group of patches that are compatible with each other and
+//! additionally methods for applying that group of patches.
+
 use read_fonts::{tables::ift::CompatibilityId, FontRef, ReadError, TableProvider};
 use std::collections::{BTreeMap, HashMap};
 
@@ -6,17 +11,20 @@ use crate::{
     patchmap::{intersecting_patches, IftTableTag, PatchEncoding, PatchUri, SubsetDefinition},
 };
 
-/// A group of patches derived from a single IFT font which can be applied simultaneously
-/// to that font. Patches are initially missing data which must be fetched and supplied to
-/// the group before it can be applied to the font.
+/// A group of patches derived from a single IFT font.
+///
+/// This is a group which can be applied simultaneously to that font. Patches are
+/// initially missing data which must be fetched and supplied to patch application
+/// method.
 pub struct PatchGroup<'a> {
     font: FontRef<'a>,
     patches: Option<CompatibleGroup>,
 }
 
 impl<'a> PatchGroup<'a> {
-    /// Intersect the available and unapplied patches in ift_font against subset_definition and return a group
-    /// of patches which would be applied next.
+    /// Intersect the available and unapplied patches in ift_font against subset_definition
+    ///
+    /// Returns a group of patches which would be applied next.
     pub fn select_next_patches<'b>(
         ift_font: FontRef<'b>,
         subset_definition: &SubsetDefinition,
@@ -41,13 +49,14 @@ impl<'a> PatchGroup<'a> {
         })
     }
 
-    /// Returns the list of URIs in this group.
+    /// Returns an iterator over URIs in this group.
     pub fn uris(&self) -> impl Iterator<Item = &str> {
         self.invalidating_patch_iter()
             .chain(self.non_invalidating_patch_iter())
             .map(|info| info.uri.as_str())
     }
 
+    /// Returns true if there is at least one uri associated with this group.
     pub fn has_uris(&self) -> bool {
         let Some(patches) = &self.patches else {
             return false;
@@ -1250,4 +1259,6 @@ mod tests {
             ])
         );
     }
+
+    // TODO(garretrieger): test that previously applied patches are ignored.
 }

incremental-font-transfer/src/table_keyed.rs

@@ -22,14 +22,15 @@ pub(crate) fn apply_table_keyed_patch(
     let mut font_builder = FontBuilder::new();
     let mut processed_tables = BTreeSet::<Tag>::new();
     // TODO(garretrieger): enforce a max combined size of all decoded tables? say something in the spec about this?
-    for i in 0..patch.patches_count() {
-        let i = i as usize;
+    for (i, table_patch) in patch
+        .patches()
+        .iter()
+        .take(patch.patches_count() as usize)
+        .enumerate()
+    {
         let next = i + 1;
 
-        let table_patch = patch
-            .patches()
-            .get(i)
-            .map_err(PatchingError::PatchParsingFailed)?;
+        let table_patch = table_patch.map_err(PatchingError::PatchParsingFailed)?;
         let (Some(offset), Some(next_offset)) = (
             patch.patch_offsets().get(i),
             patch.patch_offsets().get(next),
@@ -131,7 +132,7 @@ mod tests {
     use read_fonts::ReadError;
     use write_fonts::FontBuilder;
 
-    const IFT_TABLE: &[u8] = "IFT PATCH MAP".as_bytes();
+    const IFT_TABLE: &[u8] = b"IFT PATCH MAP";
     const TABLE_1_FINAL_STATE: &[u8] = "hijkabcdeflmnohijkabcdeflmno\n".as_bytes();
     const TABLE_2_FINAL_STATE: &[u8] = "foobarbaz foobarbaz foobarbaz\n".as_bytes();
     const TABLE_3_FINAL_STATE: &[u8] = "foobaz\n".as_bytes();