[HarfBuzz] collect_glyphs() API

[Jonathan Kew]

On 31/12/12 17:33, Jonathan Kew wrote:
> On 4/12/12 22:21, Behdad Esfahbod wrote:
>> Hi Werner and Jonathan,
>>
>> Just wanted to follow up and bring the new
>> hb_ot_layout_lookup_collect_glyphs() to your attention.
>>
>> The implementation of this API is complete as far as I'm concerned.
>> HOWEVER,
>> I've done absolutely zero testing.

One more issue I've found, in addition to those mentioned earlier:

> (1) hb_ot_layout_collect_lookups(), called without a specific language
> tag, fails to take account of the DefaultLangSys in the script
> record(s)...

> (2) The API claims that you can pass NULL for any of the glyph sets.
> Internally, it handles this by using hb_set_get_empty() in place of any
> NULL parameters. However, this breaks...

(3) The collect_class callback expects to find the ClassDef in the data 
pointer of the lookup_context, but in a couple of places 
(ContextFormat2, ChainContextFormat2) the code just sets this field to 
NULL. This will crash when the callback is used. Patch:

diff --git a/gfx/harfbuzz/src/hb-ot-layout-gsubgpos-private.hh 
b/gfx/harfbuzz/src/hb-ot-layout-gsubgpos-private.hh
--- a/gfx/harfbuzz/src/hb-ot-layout-gsubgpos-private.hh
+++ b/gfx/harfbuzz/src/hb-ot-layout-gsubgpos-private.hh
@@ -1187,19 +1187,20 @@ struct ContextFormat2
        }
    }

    inline void collect_glyphs (hb_collect_glyphs_context_t *c) const
    {
      TRACE_COLLECT_GLYPHS (this);
      (this+coverage).add_coverage (c->input);

+    const ClassDef &class_def = this+classDef;
      struct ContextCollectGlyphsLookupContext lookup_context = {
        {collect_class},
-      NULL
+      &class_def
      };

      unsigned int count = ruleSet.len;
      for (unsigned int i = 0; i < count; i++)
        (this+ruleSet[i]).collect_glyphs (c, lookup_context);
    }

    inline bool would_apply (hb_would_apply_context_t *c) const
@@ -1746,19 +1747,22 @@ struct ChainContextFormat2
        }
    }

    inline void collect_glyphs (hb_collect_glyphs_context_t *c) const
    {
      TRACE_COLLECT_GLYPHS (this);
      (this+coverage).add_coverage (c->input);

+    const ClassDef &backtrack_class_def = this+backtrackClassDef;
+    const ClassDef &input_class_def = this+inputClassDef;
+    const ClassDef &lookahead_class_def = this+lookaheadClassDef;
      struct ChainContextCollectGlyphsLookupContext lookup_context = {
        {collect_class},
-      {NULL, NULL, NULL}
+      {&backtrack_class_def, &input_class_def, &lookahead_class_def}
      };

      unsigned int count = ruleSet.len;
      for (unsigned int i = 0; i < count; i++)
        (this+ruleSet[i]).collect_glyphs (c, lookup_context);
    }

    inline bool would_apply (hb_would_apply_context_t *c) const