I still need to figure out how to map all the parameters with respect to what TeX / Gecko does.

This article http://www.ntg.nl/maps/38/03.pdf should help with MATH constants ↔ TeX fontdimens mapping, should not be too different for Gecko.

(In reply to Khaled Hosny from comment #2) > This article http://www.ntg.nl/maps/38/03.pdf should help with MATH > constants ↔ TeX fontdimens mapping, should not be too different for Gecko. Thanks, I've updated my patch queue to do the obvious remapping for fractions, stacks, radicals and scripts. https://github.com/fred-wang/MozillaCentralPatches

(In reply to Frédéric Wang (:fredw) from comment #3) > (In reply to Khaled Hosny from comment #2) > > This article http://www.ntg.nl/maps/38/03.pdf should help with MATH > > constants ↔ TeX fontdimens mapping, should not be too different for Gecko. > > Thanks, I've updated my patch queue to do the obvious remapping for > fractions, stacks, radicals and scripts. > > https://github.com/fred-wang/MozillaCentralPatches I am re-spinning my builds to pick up these changes.

Some thoughts: For many of these parameters, we'll need to convert from the MATH table value to nscoord. My concern is that nscoord is an integer (although there is a macro to migrate it to float in the future) so we may lose accuracy when rounding the value. That's probably not a problem for DisplayOperatorMinHeight (used in bug 407059) but that might be a problem for other spacing and size values. Currently, I do nscoordValue = NSToCoordRound(MATH_table_value * [aFontMetrics->GetMetrics().emHeight * aFontMetrics->AppUnitsPerDevPixel()] / mUnitsPerEm) In nsMathFrame there is actually a (very old) comment: https://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLFrame.h#281 that says to compute the emHeight by aFontMetrics->EmHeight() which is essentially a rounded value of the bracket factor above. What we currently use instead is aFontMetrics->Font().size. I'm wondering if we should really switch to this API as the comment suggest. Trying to debug and changing the zoom level, I didn't really see any improvement. However, I just gave a quick look at gfxFont::InitMetricsFromSfntTables again and emHeight is already a rounding of mStyle.size so perhaps we should directly use that latter value? (I'm not sure whether the "adjustments" are important here). aFontMetrics->Font().size is a nscoord but gfxFontStyle::size is a gfxFloat so that might give better accuracy. (In general, we may be willing to correctly round to pixel values e.g. https://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLmrootFrame.cpp#255. The "DeviceTable" of MathValues seems to be what we want here, although I'm not sure exactly how it works)

I'm asking review for this bug, since these are long-pending patches and will probably overlap with the changes in bug 1002526.

Comment on attachment 8412011 [details] [diff] [review] Part 1 - Add helper functions to access MATH constants and use them to get the AxisHeight. Hi Jonathan. The interwebs tell me you have experience in layout using the MATH table. Would you be willing to review this and the other patches here, please?

It looks like I forgot to upload the part 6...

Comment on attachment 8412011 [details] [diff] [review] Part 1 - Add helper functions to access MATH constants and use them to get the AxisHeight. Review of attachment 8412011 [details] [diff] [review]: ----------------------------------------------------------------- This looks like it should work OK, but I wonder whether it'd be more convenient and readable with some restructuring. See what you think... ::: layout/mathml/nsMathMLFrame.cpp @@ +179,5 @@ > + if (TryGetMathTable(aFontMetrics)) { > + // The font has a MATH table so we just used the specified axis height. > + GetMathConstantEm(aFontMetrics, gfxFontEntry::AxisHeight, aAxisHeight); > + return; > + } With the suggestion below, this could become something like if (aFontMetrics->GetThebesFontGroup()->GetFontAt(0)-> GetMathConstantAppUnits(gfxFontEntry::AxisHeight, aFontMetrics->AppUnitsPerDevPixel(), aAxisHeight)) { return; } although I wonder whether you also want a helper such as GetFirstMathFont() on gfxFontGroup, to find the first available font with a MATH table rather than just looking at the first in the group? ::: layout/mathml/nsMathMLFrame.h @@ +164,5 @@ > + static bool > + TryGetMathTable(nsFontMetrics* aFontMetrics) > + { > + gfxFont* mathFont = aFontMetrics->GetThebesFontGroup()->GetFontAt(0); > + return mathFont->GetFontEntry()->TryGetMathTable(mathFont); Hmm - why does gfxFontEntry::TryGetMathTable need to take a gfxFont* parameter? It's not used. Let's get rid of it! @@ +188,5 @@ > + gfxFont* mathFont = aFontMetrics->GetThebesFontGroup()->GetFontAt(0); > + aValue = NSToCoordRound(mathFont->GetFontEntry()-> > + GetMathConstant(aConstant) * > + mathFont->GetAdjustedSize() * > + aFontMetrics->AppUnitsPerDevPixel()); This seems awkward to me. Could we move most of the work here into a more convenient method on gfxFont? Pass the desired appUnitsPerDev, and return a bool indicating whether there was in fact a math table present. Something like: bool gfxFont::GetMathConstant(gfxFontEntry::MathConstant aConstant, uint32_t aAppUnitsPerDevPixel, nscoord& aValue) { if (!mFontEntry->TryGetMathTable()) { return false; } aValue = NSToCoordRound(mFontEntry->GetMathConstant(aConstant) * GetAdjustedSize() * aAppUnitsPerDevPixel); return true; } (assuming you get rid of the parameter to TryGetMathTable). And similarly for constants that are pure numbers rather than coordinates: bool gfxFont::GetMathConstant(gfxFontEntry::MathConstant aConstant, float& aValue) { if (!mFontEntry->TryGetMathTable()) { return false; } aValue = mFontEntry->GetMathConstant(aConstant); return true; } Code that has already confirmed there is a math table could ignore the bool return values of these, but it might sometimes be convenient to just go ahead and call GetMathConstant, and then proceed to do something fallback-ish to calculate a value if it returns false.

Comment on attachment 8422514 [details] [diff] [review] Part 1 - Add helper functions to access MATH constants and use them to get the AxisHeight. Review of attachment 8422514 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks much nicer, IMO. r=me, with minor tidying-up as per comments below. ::: gfx/thebes/gfxFont.h @@ +1900,5 @@ > > + bool GetMathConstant(gfxFontEntry::MathConstant aConstant, > + uint32_t aAppUnitsPerDevPixel, > + nscoord& aValue); > + bool GetMathConstant(gfxFontEntry::MathConstant aConstant, float& aValue); Simple comments to document these, please. E.g. for the first method: // Get a font dimension from the MATH table, scaled to appUnits; // returns true if it succeeds, false if no MATH table is available. And the second: // Get a dimensionless math constant (e.g. a percentage); returns... @@ +3489,5 @@ > > return mFonts[i].Font(); > } > > + gfxFont *GetFirstMathFont(); Please include a comment, something like // Returns the first font in the font-group that has an OpenType MATH table, // or null if no such font is available. ::: layout/mathml/nsMathMLChar.cpp @@ +1202,5 @@ > textRun = aGlyphTable->MakeTextRun(mThebesContext, oneDevPixel, > *aFontGroup, ch); > nsBoundingMetrics bm = MeasureTextRun(mThebesContext, textRun); > if (ch.IsGlyphID()) { > + gfxFont* mathFont = aFontGroup->get()->GetFirstMathFont(); I don't think you need the explicit ->get() here, do you? @@ +1203,5 @@ > *aFontGroup, ch); > nsBoundingMetrics bm = MeasureTextRun(mThebesContext, textRun); > if (ch.IsGlyphID()) { > + gfxFont* mathFont = aFontGroup->get()->GetFirstMathFont(); > + if (mathFont && mathFont->GetFontEntry()->TryGetMathTable()) { And here, you don't need TryGetMathTable() any longer, as GetFirstMathFont() will have already ensured that any font it returns has a MATH table ready for use.

Thanks. I'll update all the patches later and see if everything still works.

@Jonathan: As you can see, the MathML code has stuff to ensure that bars are at least 1px. I wonder how you get the issue you mentioned in bug 775060 comment 16. Is the onePixel = nsPresContext::CSSPixelsToAppUnits(1) incorrect or confused by the font-inflation perhaps?

Thanks very much Jonathan and Frédéric. This all sounds good.(In reply to Frédéric Wang (:fredw) from comment #23) > Is the onePixel = nsPresContext::CSSPixelsToAppUnits(1) > incorrect or confused by the font-inflation perhaps? CSSPixels is not necessarily device pixels. I guess it can be less than one device pixel at some zoom levels. I don't know how font-inflation affects these things.

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #24) > CSSPixels is not necessarily device pixels. I guess it can be less than one > device pixel at some zoom levels. I don't know how font-inflation affects > these things. OK, so I suppose we could use oneDevPixel = aFontMetrics->AppUnitsPerDevPixel() as the minimal thickness value instead. That sounds good, because this variable is introduced in some of these patches to get the math constants.

Comment on attachment 8422610 [details] [diff] [review] Part 1 - Add helper functions to access MATH constants and use them to get the AxisHeight. Review of attachment 8422610 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFont.cpp @@ +4743,5 @@ > +{ > + uint32_t count = mFonts.Length(); > + for (uint32_t i = 0; i < count; ++i) { > + gfxFont* font = mFonts[i].Font(); > + if (font->GetFontEntry()->TryGetMathTable()) { There are some crashes when MATH fonts are absent. count = 1 but the only gfxFont is null. Is it expected? (HasFont above does not check whether the gfxFont is non-null). https://tbpl.mozilla.org/?tree=Try&rev=89245d70d9f8

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #24) > Thanks very much Jonathan and Frédéric. This all sounds good.(In reply to > Frédéric Wang (:fredw) from comment #23) > > > Is the onePixel = nsPresContext::CSSPixelsToAppUnits(1) > > incorrect or confused by the font-inflation perhaps? > > CSSPixels is not necessarily device pixels. I guess it can be less than one > device pixel at some zoom levels. Right - I expect that's quite likely when zoomed-out on a mobile device, though it could also happen on desktop at <100% zoom levels. IMO, when zoomed out we really -should- allow the fraction bar (etc) to become less than one device pixel thick, otherwise at small sizes it'll be much too heavy in comparison to the glyphs. However, to avoid it simply disappearing, we'd need to render such a fractional-pixel-wide line with antialiasing, so that it'd still be present, just fainter than a 1-devpx solid line. But I don't know how readily we could achieve that... if we can't antialias it, then maintaining a minimum thickness of 1 dev px is our safest option.

(In reply to Frédéric Wang (:fredw) from comment #26) > Comment on attachment 8422610 [details] [diff] [review] > Part 1 - Add helper functions to access MATH constants and use them to get > the AxisHeight. > > Review of attachment 8422610 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/thebes/gfxFont.cpp > @@ +4743,5 @@ > > +{ > > + uint32_t count = mFonts.Length(); > > + for (uint32_t i = 0; i < count; ++i) { > > + gfxFont* font = mFonts[i].Font(); > > + if (font->GetFontEntry()->TryGetMathTable()) { > > There are some crashes when MATH fonts are absent. count = 1 but the only > gfxFont is null. Is it expected? (HasFont above does not check whether the > gfxFont is non-null). That seems bad; we shouldn't generally be putting null fonts into the gfxFontGroup's list. AFAICT, this can't happen on gfxPlatformFontList-based platforms; it must be a peculiarity of the fontconfig-based version (gfxPangoFontGroup, etc). Looking.... Indeed, I see that the gfxPangoFontGroup::gfxPangoFontGroup constructor puts a "dummy" FontFace record into the group, with a comment that it "will be replaced when actually needed". But if it has not actually been replaced when we try to use it, bad things will happen. The replacement of the dummy FontFace with a real one happens in gfxPangoFontGroup::GetBaseFont. And that's called by gfxPangoFontGroup::GetFontAt(). So I think that if you replace the direct access to mFonts[i].Font() with GetFontAt(i), you'll probably be fine.

Comment on attachment 8422612 [details] [diff] [review] Part 2 - Use MATH constants for radicals. Review of attachment 8422612 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/mathml/nsMathMLFrame.cpp @@ +373,5 @@ > + if (!mathFont || > + !mathFont->GetMathConstant(gfxFontEntry::RadicalRuleThickness, > + oneDevPixel, aRadicalRuleThickness)) { > + GetRuleThickness(aFontMetrics, aRadicalRuleThickness); > + } As I understand it, GetMathConstant() will never return false if a MATH table is present; and GetFirstMathFont() has already ensured that is true. Which means the successive tests here can all be consolidated into a simpler if (mathFont) { // call mathFont->GetMathConstant for each value you need } else { // do the fallback calculations }

(In reply to Jonathan Kew (:jfkthame) from comment #29) > if (mathFont) { > // call mathFont->GetMathConstant for each value you need > } else { > // do the fallback calculations > } Ah, it's a mistake. For some reason, I did everything with the idea that mathFont->GetMathConstant would return false if a constant is absent from the MATH table (but this never happens). So some of the patches can be simplified...

(In reply to Jonathan Kew (:jfkthame) from comment #28) > The replacement of the dummy FontFace with a real one happens in > gfxPangoFontGroup::GetBaseFont. And that's called by > gfxPangoFontGroup::GetFontAt(). So I think that if you replace the direct > access to mFonts[i].Font() with GetFontAt(i), you'll probably be fine. (In reply to Jonathan Kew (:jfkthame) from comment #29) > if (mathFont) { > // call mathFont->GetMathConstant for each value you need > } else { > // do the fallback calculations > } OK, all of these are fixed and seem to work locally. I've sent the patches again to try https://tbpl.mozilla.org/?tree=Try&rev=abae528d38c7

There are still crashes on Windows when Cambria Math is installed (I'm not able to reproduce it on Linux). I've executed some try tests and isolated a bit the problem, which is indeed due to the new GetMathConstant calls (it does not happen with the one of nsMathMLChar::StretchEnumContext::TryVariants, though) for example: > gfxFont* mathFont = aFontMetrics->GetThebesFontGroup()->GetFirstMathFont(); > if (mathFont && mathFont->GetMathConstant(gfxFontEntry::AxisHeight, > aFontMetrics->AppUnitsPerDevPixel(), > aAxisHeight)) { > return; > } https://tbpl.mozilla.org/?tree=Try&rev=54247093b236 (with the axis-height change) https://tbpl.mozilla.org/?tree=Try&rev=5f04f5ecd6f6 (without the axis-height change) The crash seems to happen in the DestroyBlobFunc function when the gfxMathTable is destroyed: https://tbpl.mozilla.org/php/getParsedLog.php?id=39756883&tree=Try&full=1#error4 Perhaps it's specific to Windows' gfxDWriteFontEntry::GetFontTable memory management. @Jonathan: any idea?

Ugh, that's frustrating. It's occuring during font destruction, when a gfxFont expires from the gfxFontCache and releases its reference(s) to the math table - but it's not immediately clear to me why. It may well be an error in the gfxDWriteFontEntry table management rather than an error in your patches, though it's surprising that we're seeing the problem here, when it doesn't occur with the SVG table, which is handled in a pretty similar way. I fear this is going to need some real debugging on Windows.... I've retriggered a couple of your try runs, to see how consistent this is; given that it happens when a font expires from the cache, which happens some time after last use, it may well be a bit unpredictable.

(In reply to Jonathan Kew (:jfkthame) from comment #39) > Ugh, that's frustrating. It's occuring during font destruction, when a > gfxFont expires from the gfxFontCache and releases its reference(s) to the > math table - but it's not immediately clear to me why. It may well be an > error in the gfxDWriteFontEntry table management rather than an error in > your patches, though it's surprising that we're seeing the problem here, > when it doesn't occur with the SVG table, which is handled in a pretty > similar way. I think the SVG table management is lightly different. I initially tried to mimic the SVG behavior, but you told me that it could be simplified in the Math case. See bug 407059 comment 39.

(In reply to Jonathan Kew (:jfkthame) from comment #39) > Ugh, that's frustrating. It's occuring during font destruction, when a > gfxFont expires from the gfxFontCache and releases its reference(s) to the > math table - but it's not immediately clear to me why. It may well be an > error in the gfxDWriteFontEntry table management rather than an error in > your patches, though it's surprising that we're seeing the problem here, > when it doesn't occur with the SVG table, which is handled in a pretty > similar way. > > I fear this is going to need some real debugging on Windows.... > I tried to follow the management of SVG fonts doc, but that didn't help: https://tbpl.mozilla.org/?tree=Try&rev=84e94506b440

Comment on attachment 8423301 [details] [diff] [review] Part 6 - Reftests. Review of attachment 8423301 [details] [diff] [review]: ----------------------------------------------------------------- I've been able to reproduce the crash. It is limited to Windows with DirectWrite/2d acceleration enabled. The crash happens within DirectWrite code, so I am unable to work out exactly what causes it, beyond an attempt to free/recover memory by deleting the font table. The crashes are caused by a subset of the fonts created for this patch. Good: axis-height limits radical Bad/crashing: fraction scripts stack (Each lot of fonts was tested individually but each grouping was tested together (eg all fraction fonts together, but not in combination with scripts fonts). I haven't tested to see if only a subset of the bad variants actually cause crashes) I've yet to determine what makes these fonts different, however from an initial observation all of the "good" fonts listed here call createSquareGlyph(), while none of the bad ones do. It may be a coincidence (stretchy, which is also a "good" font doesn't call it), but it might be worthwhile to do a try run with the bad fonts calling createSquareGlyph(), to help isolate what makes the fonts crash. ::: layout/reftests/mathml/opentype-stack.html @@ +56,5 @@ > + assert(almostEqual(getBox("d3").bottom - getBox("ref").bottom, ref*3), > + "Bad StackBottomShiftDown"); > + > + /* display style */ > + ref = getBox("ref").height; Should this be getBox("dref").height?

Also, the code path followed by the crashing fonts is also followed by the good fonts.

Comment on attachment 8423301 [details] [diff] [review] Part 6 - Reftests. Review of attachment 8423301 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/reftests/mathml/opentype-stack.html @@ +56,5 @@ > + assert(almostEqual(getBox("d3").bottom - getBox("ref").bottom, ref*3), > + "Bad StackBottomShiftDown"); > + > + /* display style */ > + ref = getBox("ref").height; Yes, probably (that's only used for the reference height, so that should be the same I guess).

@James: are you testing with all or only Part 1+6 applied?

All patches applied. The list of good/crashing fonts was generated by replacing all font references with stretchy.otf and restoring the originals on a file by file basis. (Assertions were also disabled for the tests with stretchy replacements). I also get a bit more of the call stack than the try run. DWrite.dll!FontFileReference::ReleaseFragment(void *) // Crashes within this DWrite.dll!DWriteFontFace::ReleaseFontTable(void *) [external code] (unsure what this is) xul.dll!DestroyBlobFunc

(In reply to James Kitchener (:jkitch) from comment #46) > All patches applied. > > The list of good/crashing fonts was generated by replacing all font > references with stretchy.otf and restoring the originals on a file by file > basis. (Assertions were also disabled for the tests with stretchy > replacements). I think it's ok to consider only part 1 + 6. The issue should be the same for the other patches. > > I also get a bit more of the call stack than the try run. > > DWrite.dll!FontFileReference::ReleaseFragment(void *) // Crashes within this > DWrite.dll!DWriteFontFace::ReleaseFontTable(void *) > [external code] (unsure what this is) > xul.dll!DestroyBlobFunc Are you able to debug a bit more e.g. placing a break point in gfxDWriteFontEntry::GetFontTable when aTag = TRUETYPE_TAG('M','A','T','H') or on gfxDWriteFontEntry::CopyFontTable?

Good font/bad font thing may have been a red herring. I can get a mostly similar crash without any of the patches applied, using the stretchy font already in the testsuite (there are minor differences that happen during the call to ReleaseFontTable, but I can't debug them.). This has been spun off into bug 1018234 as it affects MC. gfxDWriteFontEntry::CopyFontTable() is never called for a MATH argument. gfxDWriteFontEntry::GetFontTable doesn't show any abnormal behaviour. The same FontTableRecord is unchanged between creation and destruction (other than possible changes to the IDWriteFontFace struct, which I can't detect).

So here are the new results: https://tbpl.mozilla.org/?tree=Try&rev=b9cb64624d13 There seem to be failures on Windows: * On Windows 7/8, the reftests testing dynamic modification of the linethickness attribute seems to fail. I added a test opentype-fraction-dynamic-linethickness to test that on all platforms but it seems to pass. James, could you please check that? * On Windows XP, the dynamic menclose tests seem to be broken. Apparently, the painting is not correctly updated. Perhaps it's related to the issue on Windows 7/8. * On Windows XP, UpperLimitBaselineRiseMin, LowerLimitBaselineDropMin, RadicalDegreeBottomRaisePercent fails. It's possible that I need to increase the error tolerance.

(In reply to Frédéric Wang (:fredw) from comment #54) > > * On Windows 7/8, the reftests testing dynamic modification of the > linethickness attribute seems to fail. I added a test > opentype-fraction-dynamic-linethickness to test that on all platforms but it > seems to pass. James, could you please check that? I've looked into the mfrac tests and have been unable to determine the cause of it. Visually the test works. By adding a 5s delay before reftest-wait is cleared, I can see that the linethickness attribute is correctly modified. (I've also stepped through with a debugger to verify that lines of appropriate thickness are drawn in PlaceInternal() with aPlaceOrigin==true.) However, the screenshot taken by the reftest still manages to capture the unmodified line. It will be a few days before I can comment on the WinXP failures.

(In reply to Frédéric Wang (:fredw) from comment #54) > > * On Windows XP, UpperLimitBaselineRiseMin, LowerLimitBaselineDropMin, > RadicalDegreeBottomRaisePercent fails. It's possible that I need to increase > the error tolerance. Windows XP does not like the fonts used in the mochitests. The characters are rendered as solid black rectangles, wider than they are tall (although the square root character can still stretch). It seems that the failing tests are those that rely on the dimensions of the elements containing the problem characters. I have tested in a non-MathML context and with only part 6 applied and get the same rectangular rendering.

(In reply to James Kitchener (:jkitch) from comment #56) > (In reply to Frédéric Wang (:fredw) from comment #54) > > > > * On Windows XP, UpperLimitBaselineRiseMin, LowerLimitBaselineDropMin, > > RadicalDegreeBottomRaisePercent fails. It's possible that I need to increase > > the error tolerance. > > Windows XP does not like the fonts used in the mochitests. The characters > are rendered as solid black rectangles, wider than they are tall (although > the square root character can still stretch). It seems that the failing > tests are those that rely on the dimensions of the elements containing the > problem characters. > > I have tested in a non-MathML context and with only part 6 applied and get > the same rectangular rendering. I think the rectangular rendering is on purpose. The glyphs used for the radical in the font are all rectangular so that we can easily measure and more accurately measure them. It's possible that the metrics are different on Windows XP. Have you been able to find any problem with the menclose tests on XP?

(In reply to Frédéric Wang (:fredw) from comment #57) > (In reply to James Kitchener (:jkitch) from comment #56) > > (In reply to Frédéric Wang (:fredw) from comment #54) > > > > > > * On Windows XP, UpperLimitBaselineRiseMin, LowerLimitBaselineDropMin, > > > RadicalDegreeBottomRaisePercent fails. It's possible that I need to increase > > > the error tolerance. > > > > Windows XP does not like the fonts used in the mochitests. The characters > > are rendered as solid black rectangles, wider than they are tall (although > > the square root character can still stretch). It seems that the failing > > tests are those that rely on the dimensions of the elements containing the > > problem characters. > > > > I have tested in a non-MathML context and with only part 6 applied and get > > the same rectangular rendering. > > I think the rectangular rendering is on purpose. The glyphs used for the > radical in the font are all rectangular so that we can easily measure and > more accurately measure them. It's possible that the metrics are different > on Windows XP. > I get different behaviour in Windows 8.1. In the case of test_opentype-limits, sigma symbols render as a sigma, and the last <mo> elements render as a square, not as a rectangle. I have checked both with the font inspector and the limits-* fonts appear to be used. > Have you been able to find any problem with the menclose tests on XP? I'm still looking into this. Symptoms are different to the Windows 7 case - with the 5 second delay added there is a visible rendering failure.

(In reply to James Kitchener (:jkitch) from comment #58) > I get different behaviour in Windows 8.1. In the case of > test_opentype-limits, sigma symbols render as a sigma, and the last <mo> > elements render as a square, not as a rectangle. OK, a square is a rectangle for me but now I understand what you mean. So indeed that's probably a metrics or drawing difference on XP that we can ignore. To see the exact content of the custom web fonts, you can check layout/reftests/fonts/math/generate.py or open them with fontforge. > I'm still looking into this. Symptoms are different to the Windows 7 case - > with the 5 second delay added there is a visible rendering failure. OK, I wonder if that's something like an uninitialized variable in nsMathMLmenclose.cpp or something after part 2.

So I refreshed the patches, marking the tests mentioned by James as failing. I also tried to modify a bit nsMathMLmenclose.cpp, but that didn't help for the menclose failures: https://tbpl.mozilla.org/?tree=Try&rev=1a059a6c0e9e I'll try to do more experiments later...

Comment on attachment 8435538 [details] [diff] [review] Part 6 - Tests Review of attachment 8435538 [details] [diff] [review]: ----------------------------------------------------------------- Regarding the Windows XP Reftest failures: Uncommenting this line fixed the problem for me. Not entirely sure why it would mess up subsequent menclose rendering though. https://hg.mozilla.org/try/rev/dbba3004adb8 Try run: https://tbpl.mozilla.org/?tree=Try&rev=9b928587f063

(In reply to James Kitchener (:jkitch) from comment #64) > Regarding the Windows XP Reftest failures: > > Uncommenting this line fixed the problem for me. Not entirely sure why it > would mess up subsequent menclose rendering though. Thanks, that's definitely a mistake, the line delta should be computed or the rounding will depend on the totally unrelated delta = padding % onePixel above. I'll try to run the tests again without disabling them.

Comment on attachment 8435529 [details] [diff] [review] Part 1 - Add helper functions to access MATH constants and use them to get the AxisHeight. (I don't think there has been any change since the last review)

Comment on attachment 8435529 [details] [diff] [review] Part 1 - Add helper functions to access MATH constants and use them to get the AxisHeight. Review of attachment 8435529 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFont.cpp @@ +3816,5 @@ > + uint32_t aAppUnitsPerDevPixel, nscoord& aValue) > +{ > + if (!mFontEntry->TryGetMathTable()) { > + return false; > + } I think we can omit the TryGetMathTable check from here. GetMathConstant should only be called on a font that is already known to be a valid math font (see GetFirstMathFont()), and so the table has already been loaded. How about just replacing this with a (debug-only) assertion: NS_ASSERTION(mFontEntry->TryGetMathTable(), "invalid math font"); This also means there shouldn't be any need to return a bool and test it in all the various callers; used correctly, this will be an infallible method. This will result in changes at all the callsites; instead of if (mathfont && mathfont->GetMathConstant(gfxFontEntry::Whatever, appUnitsPerDevPixel, &value)) { ... } I'd expect us to have if (mathfont) { value = mathfont->GetMathConstant(gfxFontEntry::Whatever, appUnitsPerDevPixel); ... } etc. So I won't comment on the individual callsites. @@ +3827,5 @@ > +gfxFont::GetMathConstant(gfxFontEntry::MathConstant aConstant, float& aValue) > +{ > + if (!mFontEntry->TryGetMathTable()) { > + return false; > + } As above; this should never fail. And with these simplifications, I think the gfxFont::GetMathConstant methods could be defined inline in gfxFont.h, as all they really need to do is forward to the font entry (and apply the scaling factors, in the first version). Altogether, this should make all the math constant accesses a bit cheaper. ::: gfx/thebes/gfxFont.h @@ +1943,5 @@ > + // Get a font dimension from the MATH table, scaled to appUnits; > + // returns true if it succeeds, false if no MATH table is available. > + bool GetMathConstant(gfxFontEntry::MathConstant aConstant, > + uint32_t aAppUnitsPerDevPixel, > + nscoord& aValue); As above, I think we could clean this up and have: // Get a font dimension from the MATH table, scaled to appUnits; // may only be called if mFontEntry->TryGetMathTable has succeeded // (i.e. the font is known to be a valid OpenType math font). nscoord GetMathConstant(gfxFontEntry::MathConstant aConstant, uint32_t aAppUnitsPerDevPixel); @@ +1946,5 @@ > + uint32_t aAppUnitsPerDevPixel, > + nscoord& aValue); > + // Get a dimensionless math constant (e.g. a percentage); > + // returns true if it succeeds, false if no MATH table is available. > + bool GetMathConstant(gfxFontEntry::MathConstant aConstant, float& aValue); Similarly, float GetMathConstant(gfxFontEntry::MathConstant aConstant); @@ +3544,5 @@ > return mFonts[i].Font(); > } > > + // Returns the first font in the font-group that has an OpenType MATH table, > + // or null if no such font is available. Add to the comment, // The GetMathConstant methods may be called on the returned font. ::: layout/mathml/nsMathMLChar.cpp @@ +1160,5 @@ > NS_ASSERTION(isVertical, "Stretching should be in the vertical direction"); > ch = aGlyphTable->BigOf(mThebesContext, oneDevPixel, *aFontGroup, uchar, > isVertical, 0); > if (ch.IsGlyphID()) { > + gfxFont* mathFont = aFontGroup->get()->GetFirstMathFont(); The "->get()" is unnecessary here, AFAICS. @@ +1204,5 @@ > textRun = aGlyphTable->MakeTextRun(mThebesContext, oneDevPixel, > *aFontGroup, ch); > nsBoundingMetrics bm = MeasureTextRun(mThebesContext, textRun); > if (ch.IsGlyphID()) { > + gfxFont* mathFont = aFontGroup->get()->GetFirstMathFont(); ditto

(In reply to Jonathan Kew (:jfkthame) from comment #69) > ::: layout/mathml/nsMathMLChar.cpp > @@ +1160,5 @@ > > NS_ASSERTION(isVertical, "Stretching should be in the vertical direction"); > > ch = aGlyphTable->BigOf(mThebesContext, oneDevPixel, *aFontGroup, uchar, > > isVertical, 0); > > if (ch.IsGlyphID()) { > > + gfxFont* mathFont = aFontGroup->get()->GetFirstMathFont(); > > The "->get()" is unnecessary here, AFAICS. > mmh, I think I tried to remove that and got a build failure.

TryVariants receives a nsRefPtr<gfxFontGroup>* (because of how SetFontFamily works, but that might change later) so don't think I can directly call the members with ->get()

(In reply to Frédéric Wang (:fredw) from comment #71) > Created attachment 8441213 [details] [diff] [review] > Part 1 - Add helper functions to access MATH constants and use them to get > the AxisHeight. > > TryVariants receives a nsRefPtr<gfxFontGroup>* (because of how SetFontFamily > works, but that might change later) so don't think I can directly call the > members with ->get() Ah, I see, OK. (Well...actually, I think it makes the code more confusing than I like. We should probably try to re-think that. But not here and now; that's for another bug, another day.)

Comment on attachment 8441213 [details] [diff] [review] Part 1 - Add helper functions to access MATH constants and use them to get the AxisHeight. Review of attachment 8441213 [details] [diff] [review]: ----------------------------------------------------------------- This looks much nicer, IMO - thanks. ::: gfx/thebes/gfxFont.h @@ +1946,5 @@ > + // (i.e. the font is known to be a valid OpenType math font). > + nscoord GetMathConstant(gfxFontEntry::MathConstant aConstant, > + uint32_t aAppUnitsPerDevPixel) > + { > + NS_ASSERTION(mFontEntry->TryGetMathTable(), "invalid math font"); Actually, on second thoughts we don't even need the assertion here. If we ever misuse this and call it on a non-math font, the font entry's GetMathConstant will assert already. @@ +1956,5 @@ > + // may only be called if mFontEntry->TryGetMathTable has succeeded > + // (i.e. the font is known to be a valid OpenType math font). > + float GetMathConstant(gfxFontEntry::MathConstant aConstant) > + { > + NS_ASSERTION(mFontEntry->TryGetMathTable(), "invalid math font"); ditto

(In reply to Jonathan Kew (:jfkthame) from comment #78) > (In reply to Frédéric Wang (:fredw) from comment #71) > > Created attachment 8441213 [details] [diff] [review] > > Part 1 - Add helper functions to access MATH constants and use them to get > > the AxisHeight. > > > > TryVariants receives a nsRefPtr<gfxFontGroup>* (because of how SetFontFamily > > works, but that might change later) so don't think I can directly call the > > members with ->get() > > Ah, I see, OK. (Well...actually, I think it makes the code more confusing > than I like. We should probably try to re-think that. But not here and now; > that's for another bug, another day.) This will probably go away after bug 1009582 (but not sure when it will be fixed).

Comment on attachment 8441214 [details] [diff] [review] Part 2 - Use MATH constants for radicals. Review of attachment 8441214 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/mathml/nsMathMLFrame.cpp @@ +377,5 @@ > + oneDevPixel); > + } else { > + GetRuleThickness(aFontMetrics, aRadicalRuleThickness); > + } > + nit: trailing whitespace ::: layout/mathml/nsMathMLmencloseFrame.cpp @@ +339,5 @@ > nsLayoutUtils::GetFontMetricsForFrame(this, getter_AddRefs(fm)); > aRenderingContext.SetFont(fm); > GetRuleThickness(aRenderingContext, fm, mRuleThickness); > + if (mRuleThickness < onePixel) > + mRuleThickness = onePixel; style nit: braces even for single-line if. I realize there's a lot of existing code here that lacks them, but maybe we can start a trend towards following the style guide. :) @@ +362,4 @@ > > + // make sure that the rule appears on on screen > + if (mRadicalRuleThickness < onePixel) > + mRadicalRuleThickness = onePixel; ditto @@ +367,5 @@ > + // adjust clearance psi to get an exact number of pixels -- this > + // gives a nicer & uniform look on stacked radicals (bug 130282) > + delta = psi % onePixel; > + if (delta) > + psi += onePixel - delta; // round up and again! ::: layout/mathml/nsMathMLmrootFrame.cpp @@ +128,5 @@ > } > + > + if (!mathFont) { > + // avoid collision by leaving a minimum space between index and radical > + nscoord minimumClearance = aSqrWidth/2; nit: spaces around / operator, please. @@ +151,1 @@ > } Could you please reverse the condition and swap the branches here, so that we have if (mathFont) { ... } else { ... }? It's just easier to follow the code when conditions are kept simple and positive, IMO.

Comment on attachment 8441215 [details] [diff] [review] Part 3 - Use MATH constants for fractions and stacks. Review of attachment 8441215 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/mathml/nsMathMLmfracFrame.cpp @@ +283,5 @@ > GetDenominatorShifts(fm, denShift1, denShift2); > + > + if (0 == actualRuleThickness) { > + numShift = displayStyle ? numShift1 : numShift3; > + denShift = displayStyle ? numShift1 : numShift2; This looks odd, mixing denShift and numShift... typos? @@ +298,5 @@ > + gfxFontEntry::StackBottomShiftDown, > + oneDevPixel); > + } > + } else { > + numShift = displayStyle ? denShift1 : denShift2; And again? @@ +306,5 @@ > + mathFont->GetMathConstant(displayStyle ? > + gfxFontEntry:: > + FractionNumeratorDisplayStyleShiftUp : > + gfxFontEntry::FractionNumeratorShiftUp, > + oneDevPixel); These long constant names are a nuisance, aren't they? I find this rather awkward to read, with the break after the ::. IMO, it'd might look better wrapped something like if (mathFont) { numShift = mathFont-> GetMathConstant(...etc. which would probably give room to avoid the line-break in gfxFontEntry::FractionNumeratorDisplayStyleShiftUp. Or use a line-break after the open-paren of the function call, and then pull all the arguments back to a single extra indent on the next line? Same applies to a couple more examples below.

Comment on attachment 8441217 [details] [diff] [review] Part 4 - Use MATH constants for scripts. Review of attachment 8441217 [details] [diff] [review]: ----------------------------------------------------------------- A couple of the tiniest of style nits, but the actual code looks fine. ::: layout/mathml/nsMathMLmmultiscriptsFrame.cpp @@ +190,5 @@ > nscoord xHeight = fm->XHeight(); > > + nscoord oneDevPixel = fm->AppUnitsPerDevPixel(); > + gfxFont* mathFont = fm->GetThebesFontGroup()->GetFirstMathFont(); > + // scriptspace from TeX for extra spacing after sup/subscript nit: trailing space @@ +257,5 @@ > + // on the current style > + nscoord supScriptShift1, supScriptShift2, supScriptShift3; > + // Set supScriptShift{1,2,3} default from font > + GetSupScriptShifts (fm, supScriptShift1, supScriptShift2, supScriptShift3); > + nit: trailing space @@ +441,5 @@ > + oneDevPixel); > + } else { > + // get min supscript shift limit from x-height > + // = d(x) + 1/4 * sigma_5, Rule 18c, App. G, TeXbook > + superscriptBottomMin = NSToCoordRound((1.0f/4.0f) * xHeight); spaces around the / operator @@ +442,5 @@ > + } else { > + // get min supscript shift limit from x-height > + // = d(x) + 1/4 * sigma_5, Rule 18c, App. G, TeXbook > + superscriptBottomMin = NSToCoordRound((1.0f/4.0f) * xHeight); > + } nit: trailing space @@ +504,5 @@ > + if (mathFont) { > + superscriptBottomMaxWithSubscript = > + mathFont->GetMathConstant(gfxFontEntry:: > + SuperscriptBottomMaxWithSubscript, > + oneDevPixel); I'd prefer a line-wrap strategy that doesn't break within "gfxFontEntry::SuperscriptBottomMaxWithSubscript" here. @@ +507,5 @@ > + SuperscriptBottomMaxWithSubscript, > + oneDevPixel); > + } else { > + superscriptBottomMaxWithSubscript = > + NSToCoordRound((4.0f/5.0f) * xHeight); spaces around operator

Comment on attachment 8441219 [details] [diff] [review] Part 5 - Use MATH constants for limits. Review of attachment 8441219 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. I see you've left some XXX comments for future consideration, but I think that's OK - we can do this a step at a time.

(In reply to Jonathan Kew (:jfkthame) from comment #88) > LGTM. I see you've left some XXX comments for future consideration, but I > think that's OK - we can do this a step at a time. Yes, it was not really obvious how the constants should be used. Those handled in this bug are more or less those given in the "OpenType Math Illuminated" paper that compares with what is done in TeX (and so in our current MathML code). I actually had opened bug 963125 and bug 963131, so they should be mentioned in the comments.

Same with references to bug 963125 and 963131.

Just removing trailing whitespace...

unbitrotting the patch.

Comment on attachment 8441505 [details] [diff] [review] Part 6 - Add tests for OpenType MATH constants. Review of attachment 8441505 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay here. I haven't tried to double-check each individual test, but overall this looks systematic and reasonable. So if tryserver is happy, I'm happy. :)

Unbitrotting the patch + removing the B2G failure for opentype-fraction-dynamic-linethickness.html after bug 1000722.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a7ca4122b48 https://hg.mozilla.org/integration/mozilla-inbound/rev/245bb70cf580 https://hg.mozilla.org/integration/mozilla-inbound/rev/bd00eb78ed2f https://hg.mozilla.org/integration/mozilla-inbound/rev/d8861daf4825 https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3ca4925cb5 https://hg.mozilla.org/integration/mozilla-inbound/rev/a6a15c2d5349 Please cancel your old Try runs when you don't need them anymore. While investigating extremely large (6000+ jobs) backlog on Try this morning, I think I canceled 3 or 4 runs from this bug that still had pending jobs. Also, try to be more conscientious about which platforms and tests you run to try to make better use of the infra. The link below provides some more recommendations. Thanks! https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #98) > Please cancel your old Try runs when you don't need them anymore. When trying to edit the try runs, I get a prompt "Mozilla Contributors - LDAP Authentication". I'm not sure I got the credentials for that (I only have commit access level 1).

https://hg.mozilla.org/mozilla-central/rev/5a7ca4122b48 https://hg.mozilla.org/mozilla-central/rev/245bb70cf580 https://hg.mozilla.org/mozilla-central/rev/bd00eb78ed2f https://hg.mozilla.org/mozilla-central/rev/d8861daf4825 https://hg.mozilla.org/mozilla-central/rev/7f3ca4925cb5 https://hg.mozilla.org/mozilla-central/rev/a6a15c2d5349

https://developer.mozilla.org/en-US/Firefox/Releases/33#MathML https://wiki.mozilla.org/MathML:Open_Type_MATH_Table#Implementation_Status