Closed Bug 758236 Opened 8 years ago Closed 8 years ago

update harfbuzz to pick up (in-progress) Indic shaper

Categories

(Core :: Graphics: Text, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 4 obsolete files)

333.19 KB, patch
jtd
: review+
Details | Diff | Splinter Review
4.48 KB, patch
jtd
: review+
Details | Diff | Splinter Review
52.79 KB, patch
jtd
: review+
Details | Diff | Splinter Review
96.09 KB, patch
jtd
: review+
Details | Diff | Splinter Review
57.56 KB, patch
jtd
: review+
Details | Diff | Splinter Review
2.79 KB, patch
jtd
: review+
Details | Diff | Splinter Review
904 bytes, patch
Details | Diff | Splinter Review
Time for another harfbuzz update; in particular, the Indic shaper has made great strides, along with miscellaneous other fixes.
Attachment #626831 - Flags: review?(jdaggett)
Minor fix needed for harfbuzz with the HB_NO_MT option (as reported to the mailing list).
Attachment #626832 - Flags: review?(jdaggett)
Fix for fonts with non-zerowidth mark glyphs; carrying forward r+ from bug 727736, just posting it here so I don't forget that it needs to be re-applied on top of the HB update.
Attachment #626833 - Flags: review+
Blocks: 758241
Comment on attachment 626832 [details] [diff] [review]
fix for HB non-multithreaded build breakage

I'm assuming this is going upstream also?
Attachment #626832 - Flags: review?(jdaggett) → review+
It just did, as commit cde1c0114ba66a45d907e81a49bf625e0dc946b0.
Refreshed harfbuzz patch, now at upstream commit a3547330fa88e30a138f6f17e60d9c7d1e316622, incorporates the HB_NO_MT build fix.
Attachment #626831 - Attachment is obsolete: true
Attachment #626832 - Attachment is obsolete: true
Attachment #626831 - Flags: review?(jdaggett)
Attachment #627573 - Flags: review?(jdaggett)
This adds a few reftests designed to check that specific features of Indic shaping are behaving as expected. Extensive testing of the Indic shaper itself belongs upstream in harfbuzz (and Behdad is collecting a large regression-testing suite), but this checks that it is being properly activated within Gecko.

Whether the tests would pass or fail with harfbuzz disabled is of course dependent on the capabilities of the platform shapers we'd be using, but with harfbuzz enabled (using the reftest pref() annotation) and an in-tree font, results should be consistent across platforms.

This first set of tests use Devanagari; I'll put together a few similar examples with other Indic scripts as separate patches.
Attachment #627575 - Flags: review?(jdaggett)
Attachment #627592 - Flags: review?(jdaggett)
Attachment #627663 - Flags: review?(jdaggett)
Oops, the devanagari-2 test made a bad assumption about glyphs that I thought would have the same width, but in Lohit they're slightly different, so it failed on tryserver. This version should work properly.
Attachment #627575 - Attachment is obsolete: true
Attachment #627575 - Flags: review?(jdaggett)
Attachment #627678 - Flags: review?(jdaggett)
Another issue is that a number of the Gujarati and Bengali tests fail on OS X 10.6 because our masking of "unsupported" Indic ranges from the cmap on that OS version means that the intended reftest font isn't actually used, and the platform lacks font support for these scripts so they don't fall back to working with an available AAT font.

Until we're ready to switch on harfbuzz for Indic on OS X (which we probably shouldn't do until it's been in the tree for a while and had some testing by knowledgeable people willing to explicitly enable it and check the resulting behavior), we probably just need to mark those tests as failing.
This annotates the tests that fail on OS X prior to 10.7, due to the masking of the Indic-script codepoints in the cmap. (We'll want to remove that when we're ready to switch on harfbuzz for those scripts, so this is a temporary situation.)
Attachment #627869 - Flags: review?(jdaggett)
Attachment #627574 - Flags: review?(jdaggett) → review+
Comment on attachment 627869 [details] [diff] [review]
mark hb-indic tests that fail on OS X < 10.7

Please log a follow-on bug for these and mark the tests with that.
Attachment #627869 - Flags: review?(jdaggett) → review+
Hmm, the harfbuzz.scripts pref seems to have become a confusing mess.  Trying to enable Indic means doing a bit of spelunking through various parts of the code.

So setting gfx.font_rendering.harfbuzz.scripts to 255 should mean "enable for all scripts", right?

The code in all.js is out of date, it points the viewer to gfx/thebes/gfxUnicodeProperties.h:

http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#194

But the constants were moved here:

http://mxr.mozilla.org/mozilla-central/source/intl/unicharutil/util/nsUnicodeProperties.h#122
Attachment #628614 - Flags: review?(jfkthame)
Yes, 255 would enable for all (so far, anyhow, unless we subdivide further and allocate more bits). Simplest suggestion, though, would be to use -1 for "all scripts"; that's what I generally set in about:config when I want to test.
Actually, the patch in bug 758241 already includes a revision of the out-of-date comment in all.js, though perhaps mentioning the mxr identifier search would be good too.
Comment on attachment 628614 [details] [diff] [review]
update the comment for the harfbuzz.scripts pref

Covered by changes in bug 758241.
Attachment #628614 - Flags: review?(jfkthame)
Attachment #627573 - Flags: review?(jdaggett) → review+
Attachment #627592 - Flags: review?(jdaggett) → review+
Attachment #627663 - Flags: review?(jdaggett) → review+
Attachment #627678 - Flags: review?(jdaggett) → review+
Blocks: 761019
gcc4.3.2 complains about trailing commas in enumerator lists...
(In reply to neil@parkwaycc.co.uk from comment #20)
> gcc4.3.2 complains about trailing commas in enumerator lists...

See bug 761019
Depends on: 767249
You need to log in before you can comment on or make changes to this bug.