The default bug view has changed. See this FAQ.

update harfbuzz to latest code from upstream

RESOLVED FIXED in mozilla13

Status

()

Core
Layout: Text
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 9 obsolete attachments)

903.13 KB, patch
jtd
: review+
Details | Diff | Splinter Review
105.30 KB, patch
jtd
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Created attachment 568191 [details] [diff] [review]
part 1 - harfbuzz code from upstream repo (89d89646e8163b6c0874b9a3c14d4da974ea8219)
Attachment #568191 - Flags: review?(jdaggett)
(Assignee)

Comment 1

6 years ago
Created attachment 568193 [details] [diff] [review]
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.
Attachment #568193 - Flags: review?(jdaggett)
(Assignee)

Comment 2

6 years ago
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

6 years ago
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.
(Assignee)

Comment 4

6 years ago
Huh, I thought I'd dealt with that! Maybe I posted a stale version. Update coming....
(Assignee)

Comment 5

6 years ago
Created attachment 569327 [details] [diff] [review]
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.
Attachment #568193 - Attachment is obsolete: true
Attachment #568193 - Flags: review?(jdaggett)
Attachment #569327 - Flags: review?(jdaggett)
(Assignee)

Comment 6

6 years ago
This should fix bug 680692, btw.
Blocks: 680692

Updated

5 years ago
Blocks: 665352

Comment 7

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

Comment 8

5 years ago
Created attachment 588955 [details] [diff] [review]
harfbuzz library code from upstream, rev 3d0ddd12801689b4093ffca97da4dd9ca669b64a (16-01-2012)
Attachment #568191 - Attachment is obsolete: true
Attachment #568191 - Flags: review?(jdaggett)
Attachment #588955 - Flags: review?(jdaggett)
(Assignee)

Comment 9

5 years ago
Created attachment 588956 [details] [diff] [review]
part 2 - update thebes code to account for harfbuzz api changes

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)
(Assignee)

Comment 10

5 years ago
Created attachment 588958 [details] [diff] [review]
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.
(Assignee)

Comment 11

5 years ago
Created attachment 588960 [details] [diff] [review]
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.
(Assignee)

Comment 12

5 years ago
Created attachment 591508 [details] [diff] [review]
harfbuzz library code from upstream (25-01-2012)

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)
(Assignee)

Comment 13

5 years ago
Created attachment 591512 [details] [diff] [review]
update thebes code to account for harfbuzz api changes

Un-bitrotted the gfxHarfBuzzShaper changes.
Attachment #588956 - Attachment is obsolete: true
Attachment #588956 - Flags: review?(jdaggett)
Attachment #591512 - Flags: review?(jdaggett)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

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

Updated

5 years ago
Blocks: 662055
No longer blocks: 662055
Blocks: 662055
(Assignee)

Comment 16

5 years ago
jdaggett: r? ping...

Comment 17

5 years ago
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

5 years ago
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

5 years ago
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]
 */
(Assignee)

Comment 20

5 years ago
Created attachment 596636 [details] [diff] [review]
harfbuzz library code from upstream (2012-02-13)
Attachment #591508 - Attachment is obsolete: true
Attachment #591508 - Flags: review?(jdaggett)
Attachment #596636 - Flags: review?(jdaggett)
(Assignee)

Comment 21

5 years ago
Created attachment 596644 [details] [diff] [review]
update thebes code to account for harfbuzz api changes, refreshed
Attachment #591512 - Attachment is obsolete: true
Attachment #591512 - Flags: review?(jdaggett)
Attachment #596644 - Flags: review?(jdaggett)
(Assignee)

Comment 22

5 years ago
(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

5 years ago
(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.

Updated

5 years ago
Attachment #596636 - Flags: review?(jdaggett) → review+

Updated

5 years ago
Attachment #596644 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 24

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 727736
Depends on: 729626

Comment 26

5 years ago
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 :).
Blocks: 832486

Updated

3 years ago
Depends on: 986802
Blocks: 745780
Depends on: 603879
You need to log in before you can comment on or make changes to this bug.