The next version of HarfBuzz will provide a new API to parse the OpenType MATH table: https://github.com/behdad/harfbuzz/blob/master/src/hb-ot-math.h We should use that API instead of our own parsing code in gfx/thebes/gfxMathTable.cpp

This is still work-in-progress, but I believe it's the minimal change needed to switch to HarfBuzz ot-math API. I guess we probably want to improve the caching for the MathVariant table (as that used to be done) and we can maybe simplify things a bit more.

Comment on attachment 8796498 [details] [diff] [review] Part 2 - Use HarfBuzz ot-math API to parse the OpenType MATH table (WIP) Review of attachment 8796498 [details] [diff] [review]: ----------------------------------------------------------------- I may be mis-reading this, but at first look it seems like there's a problem here: a gfxFontEntry is potentially shared by many different-sized gfxFont instances, but you're giving it a single size-specific hb_font_t. Doesn't that mean that the gfxMathTable will be correct for the first gfxFont that requests it, but subsequent requests by fonts with a different size will (incorrectly) get that same table using the same-sized hb_font_t?

(In reply to Jonathan Kew (:jfkthame) from comment #4) > I may be mis-reading this, but at first look it seems like there's a problem > here: a gfxFontEntry is potentially shared by many different-sized gfxFont > instances, but you're giving it a single size-specific hb_font_t. Doesn't > that mean that the gfxMathTable will be correct for the first gfxFont that > requests it, but subsequent requests by fonts with a different size will > (incorrectly) get that same table using the same-sized hb_font_t? Mmmh, I suspect you are right. So I guess we should now probably create a gfxMathTable per gfxFont, right?

Yes, that sounds right. In this new world, gfxMathTable will be a pretty lightweight wrapper around the hb_font, providing Gecko-friendly interfaces to the hb_ot_math_... accessors for the values we need. The sharing of the actual table data (previously held by gfxMathTable, for all instances of gfxFont using the same gfxFontEntry) will now happen in the underlying hb_face, and the gfxMathTable will be instance-specific.

Comment on attachment 8799218 [details] [diff] [review] Part 2 - Use HarfBuzz ot-math API to parse the OpenType MATH table Review of attachment 8799218 [details] [diff] [review]: ----------------------------------------------------------------- This looks basically fine, I think. (It actually makes gfxMathTable virtually redundant, to the extent that at this stage it'd make sense to remove it and store the hb_font directly in gfxFont itself, but I see that the next patch introduces enough additional content into the gfxMathTable wrapper that it's worth keeping after all.) ::: gfx/thebes/gfxFont.h @@ +1845,5 @@ > delete sDefaultFeatures; > } > > + > + enum MathConstant { Could we move this over to be defined in gfxMathTable instead, or would that make things significantly more awkward in places where they're used? It would mean an extra #include in a few places, I expect, but it seems like a more fitting home for the constants, and avoids making the already over-large gfxFont.h even longer. (And it'll allow you to drop the gfxFont:: prefix where they're used in gfxMathTable.cpp itself.) @@ +2193,5 @@ > mozilla::UniquePtr<const Metrics> mVerticalMetrics; > > + // OpenType MATH table, used for MathML layout. > + mozilla::UniquePtr<gfxMathTable> mMathTable; > + bool mMathInitialized : 1; Please group the bool together with other bool fields in the class (for potentially better packing). Also, it doesn't look like gfxFont currently uses single-bit bool fields (like gfxFontEntry did), so it's probably better not to declare this one as a bit-field either. ::: gfx/thebes/gfxMathTable.h @@ +10,1 @@ > struct MathConstants; This struct isn't needed any longer either, is it?

(In reply to Jonathan Kew (:jfkthame) from comment #9) > This looks basically fine, I think. (It actually makes gfxMathTable > virtually redundant, to the extent that at this stage it'd make sense to > remove it and store the hb_font directly in gfxFont itself, but I see that > the next patch introduces enough additional content into the gfxMathTable > wrapper that it's worth keeping after all.) Yes, that's what I meant by "we can maybe simplify things a bit more.". I believe it makes sense to keep math-specific stuff in its own class, though. Given your suggestion about the "enum MathConstant" I'm even tempted to move almost all the GetMath* functions from gfxFont to gfxMathTable and just keep a gfxMathTable* gfxFont::GetMathTable() that will return a non-null pointer iff a MATH table is available. Then instead of the current gfxFont::GetFontEntry()->GetMath*() calls we would do gfxFont::GetMathTable()->GetMath*(). What do you think? > ::: gfx/thebes/gfxMathTable.h > @@ +10,1 @@ > > struct MathConstants; > > This struct isn't needed any longer either, is it? Indeed.

(In reply to Frédéric Wang (:fredw) from comment #10) > I'm even tempted to move > almost all the GetMath* functions from gfxFont to gfxMathTable and just keep > a gfxMathTable* gfxFont::GetMathTable() that will return a non-null pointer > iff a MATH table is available. Then instead of the current > gfxFont::GetFontEntry()->GetMath*() calls we would do > gfxFont::GetMathTable()->GetMath*(). What do you think? Yes, that sounds reasonable. Actually, I think gfxFont::GetMathTable() should be an infallible method that can never return null. It should simply crash if there's no table, because that's a programming error; we should never call it unless TryGetMathTable() has returned true. (Use MOZ_RELEASE_ASSERT?) Maybe consider shortening some of these names, too? Instead of things like font->GetMathTable()->GetMathItalicsCorrection(glyphID) which seems like it has a bunch of redundancy, it might be nice to be able to write font->MathTable()->ItalicsCorrection(glyphID) which I think is equally clear, and a bit more concise. WDYT?

Comment on attachment 8796056 [details] [diff] [review] Part 1 - Upgrade HarfBuzz to fd7a245d3525905ffbce57472b52900fcb0e330b Asking review for that one, although we probably want to use the latest version (maybe wait that Bedhad makes a new release)

Comment on attachment 8799364 [details] [diff] [review] Part 2 - Use HarfBuzz ot-math API to parse the OpenType MATH table Review of attachment 8799364 [details] [diff] [review]: ----------------------------------------------------------------- Looks good; r=me, just a couple of minor adjustments suggested below. ::: gfx/thebes/gfxFont.h @@ +1845,5 @@ > delete sDefaultFeatures; > } > > + // Call TryGetMathTable() to try and load the Open Type MATH table. You can > + // then access the gfxMathTable data via the MathTable() function. Please make it really explicit that MathTable() may *only* be called if TryGetMathTable() has returned true. Something like // If (and ONLY if) TryGetMathTable() has returned true, the MathTable() method // may be called to access the gfxMathTable data. @@ +2087,5 @@ > bool mKerningSet; // kerning explicitly set? > bool mKerningEnabled; // if set, on or off? > > + bool mMathInitialized; // TryGetMathTable() called? > + nit: whitespace ::: gfx/thebes/gfxMathTable.cpp @@ +37,2 @@ > } > + return FixedToFloat(value); I'm not sure FixedToFloat is available everywhere... there are several definitions of it scattered around the codebase, but none of them necessarily included here. And I believe OS X provides it in a system header, but that's not standard elsewhere AFAIK. So probably safer to define a local version of it, unless you can confirm that it's more universal than I thought. ::: gfx/thebes/gfxMathTable.h @@ +19,2 @@ > */ > + explicit gfxMathTable(hb_face_t *aFace, gfxFont *aFont); No need for the 'explicit' any longer, now that the constructor takes two params. I'd prefer to simply pass a size parameter, rather than a gfxFont, given that the only thing it's used for is to look up its size. @@ +88,4 @@ > /** > * Returns the value of the specified constant from the MATH table. > */ > + gfxFloat Constant(MathConstant aConstant); Can we declare this and the following accessors as const methods, or will that create problems?

Comment on attachment 8796056 [details] [diff] [review] Part 1 - Upgrade HarfBuzz to fd7a245d3525905ffbce57472b52900fcb0e330b Review of attachment 8796056 [details] [diff] [review]: ----------------------------------------------------------------- r=me, in principle, but let's see if we can get an official release sometime soon.

Comment on attachment 8799220 [details] [diff] [review] Part 3 - gfxMathTable: Cache MathVariants data. Review of attachment 8799220 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine, changing feedback? to review+ on it. :) ::: gfx/thebes/gfxMathTable.cpp @@ +52,5 @@ > uint16_t aSize) > { > + UpdateMathVariantCache(aGlyphID, aVertical); > + if (aSize < kMaxCachedSizeCount) > + return mMathVariantCache.sizes[aSize]; braces, please ::: gfx/thebes/gfxMathTable.h @@ +68,5 @@ > > private: > // size-specific font object, owned by the gfxMathTable > hb_font_t *mHBFont; > + stray whitespace

(In reply to Jonathan Kew (:jfkthame) from comment #14) > Can we declare this and the following accessors as const methods, or will > that create problems? Thanks for the review, I guess all the feedback can be easily addressed. Regarding making const accessors, that won't probably work after caching is implemented in part 3. But maybe I can just make mMathVariantCache a mutable member.

(In reply to Frédéric Wang (:fredw) from comment #17) > (In reply to Jonathan Kew (:jfkthame) from comment #14) > > Can we declare this and the following accessors as const methods, or will > > that create problems? > > Thanks for the review, I guess all the feedback can be easily addressed. > Regarding making const accessors, that won't probably work after caching is > implemented in part 3. But maybe I can just make mMathVariantCache a mutable > member. Yeah, that's what I'd be inclined to do. The API is logically const, and the (mutable) cache is purely an internal implementation detail. So if it works to do that, and doesn't run into other complications, I'd say go for it.

(In reply to Jonathan Kew (:jfkthame) from comment #15) > Comment on attachment 8796056 [details] [diff] [review] > Part 1 - Upgrade HarfBuzz to fd7a245d3525905ffbce57472b52900fcb0e330b > > Review of attachment 8796056 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me, in principle, but let's see if we can get an official release sometime > soon. This no longer applies on ToT, so let's wait a bit to see if a new version of HarfBuzz is released before updating the source again.

Some test failures to check: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7d024358ab5

Comment on attachment 8799405 [details] [diff] [review] Part 1 - Use HarfBuzz ot-math API to parse the OpenType MATH table Review of attachment 8799405 [details] [diff] [review]: ----------------------------------------------------------------- I submitted a try build again now that HarfBuzz has been updated: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18032aeea46fd3bcff42c8a4fc975672a5e1e7d6 However, there will probably still be failures again (see below). ::: gfx/thebes/gfxFont.cpp @@ +4020,5 @@ > + if (!face) { > + return false; > + } > + > + mMathTable = MakeUnique<gfxMathTable>(face, GetAdjustedSize()); There is a subtle change here: TryGetMathTable used to return false when the font does not contain any MATH table. Now, it succeeds whenever the hb_face_t can be obtained. I believe we should add a check for hb_ot_math_has_data here.

Comment on attachment 8805778 [details] [diff] [review] Part 1bis - Make gfxFont::TryGetMathTable check the present of a MATH table Review of attachment 8805778 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFont.cpp @@ +4016,5 @@ > if (!mMathInitialized) { > mMathInitialized = true; > > hb_face_t *face = GetFontEntry()->GetHBFace(); > + if (!face || !hb_ot_math_has_data(face)) { Ah, right - good catch, that could mislead us into using the wrong font. However, it looks like this will leak the face if it has no math table. I'd suggest rearranging as: if (face) { if (hb_ot_math_has_data(face)) { mMathTable = ... } hb_face_destroy(face); } to ensure it's always released properly. r=me with an adjustment along these lines.

(In reply to Jonathan Kew (:jfkthame) from comment #27) > Ah, right - good catch, that could mislead us into using the wrong font. Yes, and the reftests result confirmed that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18032aeea46fd3bcff42c8a4fc975672a5e1e7d6 VS https://treeherder.mozilla.org/#/jobs?repo=try&revision=adcc21b52376b42599f58793159c051ca3127baf > However, it looks like this will leak the face if it has no math table. I'd > suggest rearranging as: OK, I'll do that and squash the patches before landing.

Squashing the patches into a single patch. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b60c9cd1db2add23dadec74ec55d43f442ae3ea

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/961a84574836 Use HarfBuzz ot-math API to parse the OpenType MATH table. r=jfkthame

Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/374fb57de7e2 Use HarfBuzz ot-math API to parse the OpenType MATH table: disable test multiscripts-1.html on Windows. r=developer-request on a CLOSED TREE

Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/d53a00dfd353 Use HarfBuzz ot-math API to parse the OpenType MATH table: really disable test multiscripts-1.html on Windows. r=developer-request

I had to back this out for windows reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=38550058&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/773aed2f801b27cbad3c031fc7aa747ea9b2c1ed

Pushed by kwierso@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3898797a6267 Use HarfBuzz ot-math API to parse the OpenType MATH table. r=jfkthame

Ugh, sorry, didn't catch that this had a followup land. Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/3898797a6267e6d0f84049258e261e207fafd4e5

https://hg.mozilla.org/mozilla-central/rev/374fb57de7e2 https://hg.mozilla.org/mozilla-central/rev/d53a00dfd353 https://hg.mozilla.org/mozilla-central/rev/3898797a6267