Bug 131839 (USE_TYPO_METRICS)

I've reported that to STIX: https://sourceforge.net/p/stixfonts/tracking/53/ and sent a mail to the GUST e-foundry group.

Created attachment 263416 [details] Screenshot of the testcase on epiphany

Some update on this, now that the situation is clearer. 1) Sfnt fonts have several vertical metrics: a) The "typo" metrics from the OS/2 table b) The "Windows" metrics from the OS/2 table c) The metrics from the hhea table Since math fonts contain tall glyphs (e.g. integrals) the metrics from b) are often much larger than those from a). Depending on the fonts, the metrics from c) may also be larger than those from a). 2) The OS/2 table contains a USE_TYPO_METRICS flag to explicitly ask to use the metrics from a). At the time of writing, i) This flag is now taken into account on all platforms in Gecko. ii) STIX 1.1 does not set this flag. We'll have to wait version 2 which is currently in development. iii) All the other fonts with a MATH table sets the flag. We should probably do the same for WebKit for all the platforms supported. I can at least verify that the flag is not taken into account on Linux (using epiphany) with the following testcase: http://tests.mathml-association.org/mathml/relations/text-and-math/use-typo-metrics-1.html

Created attachment 263497 [details] Patch Freetype Here is a quick patch for Linux that seems to work for me with the testcase. We need something similar for WebCore/platform/graphics/cocoa/ and WebCore/platform/graphics/win/...

Comment on attachment 263497 [details] Patch Freetype Moved to bug 150340.

I opened bug 150340 for the FreeType backend. Other places to consider are: graphics/cocoa/FontCocoa.mm graphics/win/SimpleFontDataCairoWin.cpp graphics/win/SimpleFontDataCGWin.cpp graphics/win/SimpleFontDataWin.cpp

Created attachment 263596 [details] Patch Windows API Here is a patch for the Windows API. @Brent, Gurpreet: could you please test it on a Windows machine? The remaining files use the CGFont API and there does not seem to be any other ways than getting the raw data of the OS/2 table and extracting the fsSelection flags and typo metrics... :-(

Created attachment 263616 [details] Patch Apple API Untested patch for the Apple API.

Created attachment 263903 [details] Patch iOS

Created attachment 263907 [details] Patch iOS 2

Comment on attachment 263907 [details] Patch iOS 2 View in context: https://bugs.webkit.org/attachment.cgi?id=263907&action=review I’m saying review+, but please do *not* land this patch exactly as is. > Source/WebCore/platform/graphics/ios/FontServicesIOS.mm:38 > +static bool fontHasMathTable(CTFontRef ctFont) This function already exists in FontCocoa.mm. Please don’t write a second copy of it; instead lets find a way to share the existing function. > Source/WebCore/platform/graphics/ios/FontServicesIOS.mm:125 > + if (*(reinterpret_cast<const OpenType::UInt16*>(os2Data + fsSelectionOffset)) & useTypoMetricsMask) { I suggest writing a helper function to do this messy reinterpret_cast thing that is repeated four times. > Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:32 > +#if ENABLE(OPENTYPE_MATH) > +#include "Glyph.h" > +#endif I don’t understand why this include needs to be added to a header, when we aren’t touching the header otherwise. If this is needed in FontServicesIOS.mm, then please put it in there. Also, the header isn’t mentioned in the change log. Please don’t land this header change!

(In reply to comment #11) > > Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:32 > > +#if ENABLE(OPENTYPE_MATH) > > +#include "Glyph.h" > > +#endif > > I don’t understand why this include needs to be added to a header, when we > aren’t touching the header otherwise. If this is needed in > FontServicesIOS.mm, then please put it in there. This is to fix the build failure from attachment 263903 [details]: OpenTypeTypes.h needs to know about the Glyph type since it is used in TableWithCoverage::getCoverageIndex . That change is unrelated to the present bug, but so far all the files including OpenTypeTypes.h also included Glyph.h so the problem did not appear.

Created attachment 264672 [details] Patch iOS/Mac refactoring

Created attachment 264674 [details] Patch iOS/Mac refactoring

Created attachment 264678 [details] Patch iOS/Mac refactoring

Created attachment 264680 [details] Patch iOS/Mac refactoring

Created attachment 264682 [details] Patch iOS/Mac refactoring

Comment on attachment 264682 [details] Patch iOS/Mac refactoring Attachment 264682 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/376504 New failing tests: fonts/use-typo-metrics-1.html mathml/opentype/opentype-stretchy-horizontal.html mathml/opentype/opentype-stretchy.html

Created attachment 264686 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5

Comment on attachment 264682 [details] Patch iOS/Mac refactoring Attachment 264682 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/376505 New failing tests: fonts/use-typo-metrics-1.html mathml/opentype/opentype-stretchy-horizontal.html mathml/opentype/opentype-stretchy.html

Created attachment 264687 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Comment on attachment 264682 [details] Patch iOS/Mac refactoring Attachment 264682 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/376502 New failing tests: fonts/use-typo-metrics-1.html mathml/opentype/opentype-stretchy-horizontal.html mathml/opentype/opentype-stretchy.html

Created attachment 264688 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 264689 [details] Patch

@Darin: In order to address your review comments I had to refactor the code, so can you please review this new version again?

Comment on attachment 264689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264689&action=review OK to land like this. I don’t fully understand why these are in a directory named OpenType, but it’s OK. > Source/WebCore/platform/graphics/opentype/OpenTypeCoreFoundation.h:26 > +#ifndef OpenTypeCoreFoundation_h Not sure this file has the right name. These are CoreText and CoreGraphics helper functions; the only role CoreFoundation has is that these other frameworks happen to be based on that framework. I would probably have put these functions into FontCG.h/cpp/mm rather than creating new files. Generally speaking the right suffix would probably be CG. > Source/WebCore/platform/graphics/opentype/OpenTypeCoreFoundation.h:29 > +#include <CoreFoundation/CoreFoundation.h> Not sure this include is needed. > Source/WebCore/platform/graphics/opentype/OpenTypeCoreFoundation.h:34 > +namespace OpenType { I don’t see a strong reason to put these in an OpenType namespace. > Source/WebCore/platform/graphics/opentype/OpenTypeCoreFoundation.h:41 > +} // namespace WebCore > +#endif // OpenTypeCoreFoundation_h Seems like a missing blank line here. > Source/WebCore/platform/graphics/opentype/OpenTypeCoreFoundation.mm:27 > +#import "config.h" > +#import "OpenTypeCoreFoundation.h" I’m not sure why this file is .mm rather than .cpp.

@Darin: As I understand the opentype directory / namespace are used for functions to parse the TrueType & OpenType tables which is the case for OS/2 here. Indeed, it should probably be named with "CG" rather than CoreFoundation, sorry about that. Note that I'm developing on Linux so it's hard to me to know which headers are actually needed, whether I can use the .cpp suffix (and thus use these functions for the WinCG interface too) etc I'm happy to move these new functions into another shared namespace / directory / file. However, I'm not sure which file "FontCG.h/cpp/mm" you're talking about? I guess I can move this into graphics/Font.h/cpp under a #define.

Probably OK like this. Can always move these later if we have a better idea. It’s the sharing that matters.

OK. However, I'll first give a try to the mm => cpp change and see if that allows to use these functions for WinCG too.

Created attachment 264712 [details] Patch

Created attachment 264723 [details] Patch

Committed r192017: <http://trac.webkit.org/changeset/192017>