695857 - update harfbuzz to latest code from upstream

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - harfbuzz code from upstream repo (89d89646e8163b6c0874b9a3c14d4da974ea8219)

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - update thebes code to account for harfbuzz api changes

There have been quite a few API changes in harfbuzz since the older release we're currently using, so we can't just drop in a new snapshot of the code - we also need to update our code to match the revised API.

Comment 2

[Jonathan Kew [:jfkthame]]

BTW, I also included "generated" copies of hb-version.h and hb-ot-shape-complex-indic-machine.hh here, as that seemed much less painful than hacking our build system to generate them - in the latter case, that would also introduce a build-time dependency on the ragel processor. Simpler to check in the generated file, then every developer doesn't need to install an extra tool.

(For "real" harfbuzz releases, as opposed to git snapshots, perhaps we can persuade Behdad to include the generated state-machine file in the release package; in the meantime, we'll need to remember to regenerate it manually when we take future code updates.)

Comment 3

[John Daggett (:jtd)]

Comment on attachment 568193 [details] [diff] [review]
part 2 - update thebes code to account for harfbuzz api changes

> +    // FIXME!!!! aRunScript is now a script TAG
>      // Pango, GLib, and HarfBuzz all happen to use the same script codes.
>      const PangoScript script = static_cast<PangoScript>(aRunScript);
>      // Might be nice to call pango_language_includes_script only once for the
>  
> +    // FIXME!!!!!!!!!!!!!
>      const PangoScript script = static_cast<PangoScript>(aRunScript);

Either fix these or file a bug and include a comment in place of FIXME.

Comment 4

[Jonathan Kew [:jfkthame]]

Huh, I thought I'd dealt with that! Maybe I posted a stale version. Update coming....

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - update thebes code to account for harfbuzz api changes

Removed obsolete "FIXME" comments.

So the code was actually OK; the "FIXME" comments were residue from an earlier approach, when I was going to switch gfx over to follow the new HarfBuzz API using tags for script identification throughout, in place of the existing sequentially-allocated codes. But I backed off on that, as it involved extra work and complexity for no benefit, particularly for the Linux implementation; maybe we could reconsider it when we're ready to drop Pango altogether and use HB exclusively. For now, though, we're better off staying with script codes, and only mapping them to tags (a trivial lookup) when we actually call into harfbuzz.

As a bonus, I've added a handful of static assertions (for the Pango backend only) to verify that the different script code enumerations do in fact match, as the comments claim. That way, if we do decide to switch our script codes to be HB-style tags some day, we'll get a heads-up that these casts are no longer correct.

Comment 6

[Jonathan Kew [:jfkthame]]

This should fix bug 680692, btw.

Comment 7

[Behdad Esfahbod]

Jonathan: yes, for tarball releases the state machine will be in the tarball.  Any other changes upstream that would make your life easier?

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: harfbuzz library code from upstream, rev 3d0ddd12801689b4093ffca97da4dd9ca669b64a (16-01-2012)

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - update thebes code to account for harfbuzz api changes

Updated for changes in thebes font code in the past few months.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: remove trailing commas in enums

Harfbuzz patch needed to avoid build problems with older/stricter compilers; I expect this to be fixed upstream soon, so this patch will become obsolete.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: fix indexing in mark-skipping loops

This is the patch from bug 701637, plus correction from bug 714067, ported to current harfbuzz trunk; I expect this to be committed upstream shortly, so the separate patch here will then become obsolete.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: harfbuzz library code from upstream (25-01-2012)

Refreshed to pick up latest fixes/enhancements from upstream.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: update thebes code to account for harfbuzz api changes

Un-bitrotted the gfxHarfBuzzShaper changes.

Comment 14

[John Daggett (:jtd)]

Did we push a harfbuzz change recently?  The library patch doesn't apply cleanly to latest trunk with a failure in hb-ot-layout.cc.  No biggy, just thought I would note in case it's important.

Comment 15

[Jonathan Kew [:jfkthame]]

That'll be the patch from bug 719366 (which has already been pushed upstream as well).

Comment 16

[Jonathan Kew [:jfkthame]]

jdaggett: r? ping...

Comment 17

[John Daggett (:jtd)]

I still need to do some testing but I'm on the road this week for meetings.  I'll finish testing it next Monday.

Comment 18

[John Daggett (:jtd)]

There are a bunch of compiler warnings generated by or related to HarfBuzz code that I think we should clean up:

> /builds/harfbuzz-update/gfx/thebes/gfxHarfBuzzShaper.cpp: In member function ‘nsresult gfxHarfBuzzShaper::SetGlyphsFromRun(gfxContext*, gfxShapedWord*, hb_buffer_t*)’:
> /builds/harfbuzz-update/gfx/thebes/gfxHarfBuzzShaper.cpp:1045: warning: comparison between signed and unsigned integer expressions

Shouldn't glyphStart and charStart be PRUint32 instead of PRInt32?

> In file included from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-unicode.cc:31:
> /builds/harfbuzz-update/gfx/harfbuzz/src/hb-private.hh:489:1: warning: "DEBUG" redefined

Undefine DEBUG for HarfBuzz in the makefile, since it's used as a macro function within this code.

Would it make sense to enable HB_DEBUG for debug builds?

> In file included from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-object-private.hh:37,
>                  from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-unicode-private.hh:37,
>                  from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-unicode.cc:33:
> /builds/harfbuzz-update/gfx/harfbuzz/src/hb-mutex-private.hh:69:2: warning: #warning "Could not find any system to define platform macros, library will NOT be thread-safe"
> In file included from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-unicode-private.hh:37,
>                  from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-unicode.cc:33:
> /builds/harfbuzz-update/gfx/harfbuzz/src/hb-object-private.hh:81:2: warning: #warning "Could not find any system to define atomic_int macros, library will NOT be thread-safe"

Not much point to this since our code is not using this code across threads.

Bracket the #warning with a mozilla-specific #ifdef.

Comment 19

[John Daggett (:jtd)]

It looks like the data in gfxUnicodePropertyData.cpp was generated using recent-ish data but there's no way of knowing this.  How about including the header date from say the Scripts.txt file in the data?

Something like the comment in hb-ot-shape-complex-arabic-table.hh:

/*
 * The following table is generated by running:
 *
 *   ./gen-arabic-table.py ArabicShaping.txt
 *
 * on files with these headers:
 *
 * # ArabicShaping-6.1.0.txt
 * # Date: 2011-04-15, 23:16:00 GMT [KW]
 */

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: harfbuzz library code from upstream (2012-02-13)

Comment 21

[Jonathan Kew [:jfkthame]]

Attachment: update thebes code to account for harfbuzz api changes, refreshed

Comment 22

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #18)
> Shouldn't glyphStart and charStart be PRUint32 instead of PRInt32?

They're signed because we use -1 as an "invalid" indicator (e.g. the NO_GLYPH value used to initialize charToGlyphArray), so I've just added a typecast here.

> > In file included from /builds/harfbuzz-update/gfx/harfbuzz/src/hb-unicode.cc:31:
> > /builds/harfbuzz-update/gfx/harfbuzz/src/hb-private.hh:489:1: warning: "DEBUG" redefined
> 
> Undefine DEBUG for HarfBuzz in the makefile, since it's used as a macro
> function within this code.

OK, done.

> Would it make sense to enable HB_DEBUG for debug builds?

I don't think so, that's designed to offer very detailed tracing - suitable for debugging harfbuzz itself, but not really appropriate or useful within our context.

> > /builds/harfbuzz-update/gfx/harfbuzz/src/hb-mutex-private.hh:69:2: warning: #warning "Could not find any system to define platform macros, library will NOT be thread-safe"
> > /builds/harfbuzz-update/gfx/harfbuzz/src/hb-object-private.hh:81:2: warning: #warning "Could not find any system to define atomic_int macros, library will NOT be thread-safe"
> 
> Not much point to this since our code is not using this code across threads.

True, but I'd prefer not to maintain a local modification just to suppress the messages - let's ask Behdad to consider providing a HB_SINGLE_THREADED symbol or something like that, which we can then set from our makefile.

Comment 23

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #22)
> True, but I'd prefer not to maintain a local modification just to suppress
> the messages - let's ask Behdad to consider providing a HB_SINGLE_THREADED
> symbol or something like that, which we can then set from our makefile.

That sounds reasonable.

Comment 24

[Jonathan Kew [:jfkthame]]

Pushed both patches together, as neither one can build without the other:
https://hg.mozilla.org/integration/mozilla-inbound/rev/392319d8c1fa

Comment 25

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/392319d8c1fa

Comment 26

[Behdad Esfahbod]

I've added support for HB_NO_MT upstream, also made the warning happen once only, also implemented native Apple MT macros.  Pick your preferred solution :).