Closed Bug 695857 Opened 8 years ago Closed 8 years ago

update harfbuzz to latest code from upstream

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(2 files, 9 obsolete files)

903.13 KB, patch
jtd
: review+
Details | Diff | Splinter Review
105.30 KB, patch
jtd
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #568191 - Flags: review?(jdaggett)
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.
Attachment #568193 - Flags: review?(jdaggett)
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 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.
Huh, I thought I'd dealt with that! Maybe I posted a stale version. Update coming....
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.
Attachment #568193 - Attachment is obsolete: true
Attachment #568193 - Flags: review?(jdaggett)
Attachment #569327 - Flags: review?(jdaggett)
This should fix bug 680692, btw.
Blocks: 680692
Blocks: 665352
Jonathan: yes, for tarball releases the state machine will be in the tarball.  Any other changes upstream that would make your life easier?
Attachment #568191 - Attachment is obsolete: true
Attachment #568191 - Flags: review?(jdaggett)
Attachment #588955 - Flags: review?(jdaggett)
Updated for changes in thebes font code in the past few months.
Attachment #569327 - Attachment is obsolete: true
Attachment #569327 - Flags: review?(jdaggett)
Attachment #588956 - Flags: review?(jdaggett)
Attached patch remove trailing commas in enums (obsolete) — Splinter Review
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.
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.
Refreshed to pick up latest fixes/enhancements from upstream.
Attachment #588955 - Attachment is obsolete: true
Attachment #588958 - Attachment is obsolete: true
Attachment #588960 - Attachment is obsolete: true
Attachment #588955 - Flags: review?(jdaggett)
Attachment #591508 - Flags: review?(jdaggett)
Un-bitrotted the gfxHarfBuzzShaper changes.
Attachment #588956 - Attachment is obsolete: true
Attachment #588956 - Flags: review?(jdaggett)
Attachment #591512 - Flags: review?(jdaggett)
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.
That'll be the patch from bug 719366 (which has already been pushed upstream as well).
Blocks: 662055
No longer blocks: 662055
Blocks: 662055
jdaggett: r? ping...
I still need to do some testing but I'm on the road this week for meetings.  I'll finish testing it next Monday.
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.
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]
 */
Attachment #591508 - Attachment is obsolete: true
Attachment #591508 - Flags: review?(jdaggett)
Attachment #596636 - Flags: review?(jdaggett)
Attachment #591512 - Attachment is obsolete: true
Attachment #591512 - Flags: review?(jdaggett)
Attachment #596644 - Flags: review?(jdaggett)
(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.
(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.
Attachment #596636 - Flags: review?(jdaggett) → review+
Attachment #596644 - Flags: review?(jdaggett) → review+
Pushed both patches together, as neither one can build without the other:
https://hg.mozilla.org/integration/mozilla-inbound/rev/392319d8c1fa
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/392319d8c1fa
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 727736
Depends on: 729626
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 :).
Depends on: 986802
You need to log in before you can comment on or make changes to this bug.