Bug 150394 - [Mac] Add support for the USE_TYPO_METRICS flag
Summary: [Mac] Add support for the USE_TYPO_METRICS flag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 150340 150396
Blocks: USE_TYPO_METRICS 150451
  Show dependency treegraph
 
Reported: 2015-10-21 09:43 PDT by Frédéric Wang (:fredw)
Modified: 2015-10-22 10:46 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.56 KB, patch)
2015-10-21 09:59 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (748.83 KB, application/zip)
2015-10-21 10:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-mavericks (775.35 KB, application/zip)
2015-10-21 10:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (777.76 KB, application/zip)
2015-10-21 10:49 PDT, Build Bot
no flags Details
Patch (11.45 KB, patch)
2015-10-21 11:01 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (13.92 KB, patch)
2015-10-21 13:57 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (778.24 KB, application/zip)
2015-10-21 14:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (980.39 KB, application/zip)
2015-10-21 14:17 PDT, Build Bot
no flags Details
Patch (14.11 KB, patch)
2015-10-21 14:19 PDT, Frédéric Wang (:fredw)
mmaxfield: review+
Details | Formatted Diff | Diff
Patch + review comment 21 (14.38 KB, patch)
2015-10-22 08:32 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch + review comment 21 minus kCTFontTableMATH (14.37 KB, patch)
2015-10-22 09:25 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2015-10-21 09:43:13 PDT
This is bug 131839 for Linux.
Comment 1 Frédéric Wang (:fredw) 2015-10-21 09:59:02 PDT
Created attachment 263689 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2015-10-21 10:01:52 PDT
There is an expected style-check failure, due to fact that the slash in the 'OS/2' tag is interpreted as a division by the style checker...
Comment 3 WebKit Commit Bot 2015-10-21 10:02:19 PDT
Attachment 263689 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:179:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Build Bot 2015-10-21 10:18:14 PDT
Comment on attachment 263689 [details]
Patch

Attachment 263689 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/316610

New failing tests:
mathml/opentype/opentype-stretchy-horizontal.html
mathml/opentype/opentype-stretchy.html
Comment 5 Build Bot 2015-10-21 10:18:19 PDT
Created attachment 263695 [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 6 Build Bot 2015-10-21 10:21:39 PDT
Comment on attachment 263689 [details]
Patch

Attachment 263689 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/316629

New failing tests:
mathml/opentype/opentype-stretchy-horizontal.html
mathml/opentype/opentype-stretchy.html
Comment 7 Build Bot 2015-10-21 10:21:52 PDT
Created attachment 263697 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 8 Build Bot 2015-10-21 10:49:45 PDT
Comment on attachment 263689 [details]
Patch

Attachment 263689 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/316689

New failing tests:
mathml/opentype/opentype-stretchy-horizontal.html
mathml/opentype/opentype-stretchy.html
Comment 9 Build Bot 2015-10-21 10:49:49 PDT
Created attachment 263701 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 10 Frédéric Wang (:fredw) 2015-10-21 11:01:33 PDT
Created attachment 263702 [details]
Patch
Comment 11 WebKit Commit Bot 2015-10-21 11:04:37 PDT
Attachment 263702 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:179:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Myles C. Maxfield 2015-10-21 12:19:08 PDT
Comment on attachment 263702 [details]
Patch

Is there a way we can limit these different metrics to only 'MATH' support? That is the intended use case, right? It seems that changing the rendering of all text shouldn't be necessary for just math stuff.
Comment 13 Frédéric Wang (:fredw) 2015-10-21 12:41:33 PDT
(In reply to comment #12)
> Comment on attachment 263702 [details]
> Patch
> 
> Is there a way we can limit these different metrics to only 'MATH' support?
> That is the intended use case, right? It seems that changing the rendering
> of all text shouldn't be necessary for just math stuff.

Maybe some type designers or other font people will be willing that Safari follows the Open Font Format standard (http://www.iso.org/iso/home/store/catalogue_ics/catalogue_detail_ics.htm?csnumber=66391 and http://mpeg.chiariglione.org/standards/mpeg-4/open-font-format/text-isoiec-dis-14496-22-3rd-edition):

"USE_TYPO_METRICS = If set, it is strongly recommended to use OS/2.sTypoAscender - OS/2.sTypoDescender+ OS/2.sTypoLineGap as a value for default line spacing for this font."

However, for the use case of MathML I'm interested in, this is indeed only necessary for fonts with a MATH table. If Apple reviewers want to keep the current metrics in general and only consider typo metrics for fonts with a 'MATH' table then I can easily add a CGFontCopyTableForTag(m_platformData.cgFont(), 'MATH') check before the new if statement (and modify the reftest to include a dummy MATH table). Please let me know what you prefer.
Comment 14 Myles C. Maxfield 2015-10-21 13:24:19 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 263702 [details]
> > Patch
> > 
> > Is there a way we can limit these different metrics to only 'MATH' support?
> > That is the intended use case, right? It seems that changing the rendering
> > of all text shouldn't be necessary for just math stuff.
> 
> Maybe some type designers or other font people will be willing that Safari
> follows the Open Font Format standard
> (http://www.iso.org/iso/home/store/catalogue_ics/catalogue_detail_ics.
> htm?csnumber=66391 and
> http://mpeg.chiariglione.org/standards/mpeg-4/open-font-format/text-isoiec-
> dis-14496-22-3rd-edition):
> 
> "USE_TYPO_METRICS = If set, it is strongly recommended to use
> OS/2.sTypoAscender - OS/2.sTypoDescender+ OS/2.sTypoLineGap as a value for
> default line spacing for this font."
> 
> However, for the use case of MathML I'm interested in, this is indeed only
> necessary for fonts with a MATH table. If Apple reviewers want to keep the
> current metrics in general and only consider typo metrics for fonts with a
> 'MATH' table then I can easily add a
> CGFontCopyTableForTag(m_platformData.cgFont(), 'MATH') check before the new
> if statement (and modify the reftest to include a dummy MATH table). Please
> let me know what you prefer.

Adding this check is a good idea.

Also, you can probably do a little better by using the enum forms for the table names. For example: CTFontCopyTable(…, kCTFontTableOS2)
Comment 15 Frédéric Wang (:fredw) 2015-10-21 13:57:18 PDT
Created attachment 263729 [details]
Patch
Comment 16 Build Bot 2015-10-21 14:11:14 PDT
Comment on attachment 263729 [details]
Patch

Attachment 263729 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/317395

New failing tests:
fonts/use-typo-metrics-1.html
Comment 17 Build Bot 2015-10-21 14:11:20 PDT
Created attachment 263732 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 18 Build Bot 2015-10-21 14:17:08 PDT
Comment on attachment 263729 [details]
Patch

Attachment 263729 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/317410

New failing tests:
fonts/use-typo-metrics-1.html
Comment 19 Build Bot 2015-10-21 14:17:23 PDT
Created attachment 263734 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 20 Frédéric Wang (:fredw) 2015-10-21 14:19:32 PDT
Created attachment 263736 [details]
Patch
Comment 21 Myles C. Maxfield 2015-10-21 16:12:08 PDT
Comment on attachment 263736 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263736&action=review

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:180
> +    if (CFDataRef mathTable = CGFontCopyTableForTag(m_platformData.cgFont(), 'MATH')) {

kCTFontTableMATH

Also, you can use CTFontCopyAvailableTables() if you just want to know if a table exists, rather than copying the whole table.
Comment 22 Frédéric Wang (:fredw) 2015-10-21 23:46:07 PDT
Committed r191440: <http://trac.webkit.org/changeset/191440>
Comment 23 Frédéric Wang (:fredw) 2015-10-22 00:19:43 PDT
rollout fow now: https://trac.webkit.org/changeset/191444
Comment 24 Frédéric Wang (:fredw) 2015-10-22 08:32:29 PDT
Created attachment 263827 [details]
Patch + review comment 21
Comment 25 Frédéric Wang (:fredw) 2015-10-22 09:25:36 PDT
Created attachment 263831 [details]
Patch + review comment 21 minus kCTFontTableMATH

kCTFontTableMATH does not seem to be defined on old systems.
Comment 26 Frédéric Wang (:fredw) 2015-10-22 10:18:44 PDT
Committed r191456: <http://trac.webkit.org/changeset/191456>