Bug 187723 – [Cocoa] Ask platform for generic font family mappings

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[Cocoa] Ask platform for generic font family mappings

[Myles C. Maxfield]

Created attachment 345137 [details]
WIP

[Myles C. Maxfield]

<rdar://problem/41892438>

[EWS Watchlist]

Attachment 345137 [details] did not pass style-queue:


ERROR: Source/WebCore/page/FrameView.cpp:4142:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Created attachment 345200 [details]
Patch

[EWS Watchlist]

Attachment 345200 [details] did not pass style-queue:


ERROR: Source/WebCore/page/FrameView.cpp:4142:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 345200 [details]
Patch

Attachment 345200 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8577754

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 345277 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 345200 [details]
Patch

Attachment 345200 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8580189

New failing tests:
http/tests/preload/onload_event.html
fast/css/beforeSelectorOnCodeElement.html

[EWS Watchlist]

Created attachment 345301 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Myles C. Maxfield]

The iOS failures are because of kCTFontUIFontDesignTrait.

I think this patch is the wrong approach, because the whole point of FontFamilySpecificationCoreText is that CoreText can give is exactly the right CTFontRef to use, and we shouldn't use the CSS font selection algorithm. This is right for system-ui, but it isn't right for the other generic font families. For those, we still need to run the font selection algorithm in order to find the font with the best weight/width/slope. Therefore, we should simply pull out the family name from the return of CTFontDescriptorCreateForCSSFamily() and continue down the normal path.

We still need to figure out how to determine which signal to listen to:
1) https://developer.apple.com/documentation/webkit/webpreferences/1536182-cursivefontfamily API clients can set the default font families
2) Core Text can tell us what the default families should be

[Myles C. Maxfield]

WebKit sets all of these generic font families to default values (see WebPreferencesDefaultValues.h)

[Myles C. Maxfield]

The current design is that the API changes the "default" value of the generic family, but SettingsBaseCocoa sets the script-specific value of the generic family, which overrides the default. Therefore, web content in CJK will see standard/fixed/serif/sans-serif from SettingsBaseCocoa, and never will see anything the API sets.

This is pretty unfortunate because, for scripts other than CJK, CoreText can give us better choices than these hardcoded font names inside WebPreferencesDefaultValues.h.

It seems like this API wasn't designed well.

[Myles C. Maxfield]

+[NSFont userFixedPitchFontOfSize] :(

[Myles C. Maxfield]

Looks like WebKit2 doesn't have API for changing these generic font families. (Though it does have some SPI: WKPreferencesSetSansSerifFontFamily)

[Myles C. Maxfield]

Created attachment 346259 [details]
Patch

[Myles C. Maxfield]

*** Bug 187895 has been marked as a duplicate of this bug. ***

[Myles C. Maxfield]

Created attachment 346260 [details]
Patch

[EWS Watchlist]

Comment on attachment 346260 [details]
Patch

Attachment 346260 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8720839

New failing tests:
fast/text/jp-sans-serif.html

[EWS Watchlist]

Created attachment 346264 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Myles C. Maxfield]

Created attachment 360532 [details]
Patch

[EWS Watchlist]

Comment on attachment 360532 [details]
Patch

Attachment 360532 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10946086

New failing tests:
fast/overflow/007.html
fast/text/hyphenate-avoid-orphaned-word.html
fast/text/international/002.html
imported/w3c/web-platform-tests/css/css-text/word-break/word-break-normal-lo-000.html
fast/css-generated-content/014.html
fast/text/international/lang-sensitive-fonts-xml.xhtml
fast/text/hangul-generic-font-families.html
fast/text/international/generic-font-family-language-traditional.html
fast/text/international/locale-sensitive-fonts.html
fast/ruby/nested-ruby.html
fast/text/hyphens.html
fast/text/international/lang-sensitive-fonts.html
tables/mozilla/bugs/bug139524-2.html
tables/mozilla/bugs/bug10296-1.html
svg/custom/object-sizing.xhtml
fast/css/line-height-font-order.html
fast/text/international/font-fallback-to-common-script.html
fast/text/hyphenate-character.html
tables/mozilla/bugs/bug10269-2.html
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360536 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360532 [details]
Patch

Attachment 360532 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10946096

New failing tests:
fast/overflow/007.html
fast/text/hyphenate-avoid-orphaned-word.html
fast/text/international/002.html
imported/w3c/web-platform-tests/css/css-text/word-break/word-break-normal-lo-000.html
fast/css-generated-content/014.html
fast/text/international/lang-sensitive-fonts-xml.xhtml
fast/text/hangul-generic-font-families.html
fast/text/international/generic-font-family-language-traditional.html
fast/text/international/locale-sensitive-fonts.html
fast/ruby/nested-ruby.html
fast/text/hyphens.html
fast/text/international/lang-sensitive-fonts.html
tables/mozilla/bugs/bug139524-2.html
tables/mozilla/bugs/bug10296-1.html
svg/custom/object-sizing.xhtml
fast/css/line-height-font-order.html
fast/text/international/font-fallback-to-common-script.html
fast/text/hyphenate-character.html
tables/mozilla/bugs/bug10269-2.html
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360538 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360532 [details]
Patch

Attachment 360532 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10946223

New failing tests:
fast/block/float/016.html
tables/mozilla_expected_failures/bugs/bug2479-5.html
fast/overflow/007.html
fast/text/hyphenate-avoid-orphaned-word.html
fast/text/jp-sans-serif.html
fast/css-generated-content/014.html
fast/text/international/lang-sensitive-fonts-xml.xhtml
fast/text/hangul-generic-font-families.html
fast/text/international/generic-font-family-language-traditional.html
fast/text/international/locale-sensitive-fonts.html
fast/text/international/lang-sensitive-fonts.html
tables/mozilla/bugs/bug139524-2.html
tables/mozilla/bugs/bug10296-1.html
svg/custom/object-sizing.xhtml
fast/css/line-height-font-order.html
fast/text/international/font-fallback-to-common-script.html
tables/mozilla/bugs/bug10269-2.html
fast/ruby/nested-ruby.html

[EWS Watchlist]

Created attachment 360539 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360532 [details]
Patch

Attachment 360532 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10946373

New failing tests:
fast/overflow/007.html
fast/text/hyphenate-avoid-orphaned-word.html
fast/text/international/002.html
imported/w3c/web-platform-tests/css/css-text/word-break/word-break-normal-lo-000.html
fast/css-generated-content/014.html
fast/text/international/lang-sensitive-fonts-xml.xhtml
fast/text/hangul-generic-font-families.html
fast/text/international/generic-font-family-language-traditional.html
fast/text/international/locale-sensitive-fonts.html
fast/ruby/nested-ruby.html
fast/text/hyphens.html
fast/text/international/lang-sensitive-fonts.html
tables/mozilla/bugs/bug139524-2.html
tables/mozilla/bugs/bug10296-1.html
svg/custom/object-sizing.xhtml
fast/css/line-height-font-order.html
fast/text/international/font-fallback-to-common-script.html
fast/text/hyphenate-character.html
tables/mozilla/bugs/bug10269-2.html
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360540 [details]
Archive of layout-test-results from ews113 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Myles C. Maxfield]

Created attachment 360610 [details]
Patch

[Jiang Jiang]

Comment on attachment 360610 [details]
Patch

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

> Source/WebCore/page/cocoa/SettingsBaseCocoa.mm:53
>      setStandardFontFamily("Songti TC", USCRIPT_TRADITIONAL_HAN);

This is interesting, why is standard family Songti for MAC platforms?

[Myles C. Maxfield]

Comment on attachment 360610 [details]
Patch

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

>> Source/WebCore/page/cocoa/SettingsBaseCocoa.mm:53
>>      setStandardFontFamily("Songti TC", USCRIPT_TRADITIONAL_HAN);
> 
> This is interesting, why is standard family Songti for MAC platforms?

https://bugs.webkit.org/show_bug.cgi?id=117568

[Jiang Jiang]

I guess what I'm trying to say is: why is standard font family for PLATFORM(MAC) serif instead of sans-serif like other platforms?

[Myles C. Maxfield]

(In reply to Jiang Jiang from comment #32)
> I guess what I'm trying to say is: why is standard font family for
> PLATFORM(MAC) serif instead of sans-serif like other platforms?

I don't know; that code is older than my tenure at Apple.

My guess is that, since the standard font family is serif for en, we assumed it should be serif for other languages too. But I don't know.

Do you have a suggestion for a better choice?

[Myles C. Maxfield]

See the comment in the next section: // There is no serif Chinese font in default iOS installation.

[EWS Watchlist]

Comment on attachment 360610 [details]
Patch

Attachment 360610 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10956926

New failing tests:
fast/overflow/007.html
fast/text/hyphenate-avoid-orphaned-word.html
imported/w3c/web-platform-tests/css/css-text/word-break/word-break-normal-lo-000.html
fast/css-generated-content/014.html
fast/text/international/lang-sensitive-fonts-xml.xhtml
fast/text/international/generic-font-family-language-traditional.html
fast/text/international/locale-sensitive-fonts.html
fast/text/hyphens.html
fast/text/international/lang-sensitive-fonts.html
tables/mozilla/bugs/bug139524-2.html
tables/mozilla/bugs/bug10296-1.html
svg/custom/object-sizing.xhtml
fast/css/line-height-font-order.html
fast/text/international/font-fallback-to-common-script.html
fast/text/hyphenate-character.html
tables/mozilla/bugs/bug10269-2.html
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360624 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360610 [details]
Patch

Attachment 360610 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10956972

New failing tests:
fast/overflow/007.html
fast/text/hyphenate-avoid-orphaned-word.html
fast/text/international/font-fallback-to-common-script.html
fast/css-generated-content/014.html
fast/text/international/lang-sensitive-fonts-xml.xhtml
fast/text/international/generic-font-family-language-traditional.html
fast/text/international/locale-sensitive-fonts.html
fast/text/hyphens.html
fast/text/international/lang-sensitive-fonts.html
tables/mozilla/bugs/bug139524-2.html
tables/mozilla/bugs/bug10296-1.html
svg/custom/object-sizing.xhtml
fast/css/line-height-font-order.html
imported/w3c/web-platform-tests/css/css-text/word-break/word-break-normal-lo-000.html
fast/text/hyphenate-character.html
tables/mozilla/bugs/bug10269-2.html
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360628 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360610 [details]
Patch

Attachment 360610 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10957015

New failing tests:
fast/overflow/007.html
fast/text/hyphenate-avoid-orphaned-word.html
fast/text/international/font-fallback-to-common-script.html
fast/css-generated-content/014.html
fast/text/international/lang-sensitive-fonts-xml.xhtml
fast/text/international/generic-font-family-language-traditional.html
fast/text/international/locale-sensitive-fonts.html
fast/text/hyphens.html
fast/text/international/lang-sensitive-fonts.html
tables/mozilla/bugs/bug139524-2.html
tables/mozilla/bugs/bug10296-1.html
svg/custom/object-sizing.xhtml
fast/css/line-height-font-order.html
imported/w3c/web-platform-tests/css/css-text/word-break/word-break-normal-lo-000.html
fast/text/hyphenate-character.html
tables/mozilla/bugs/bug10269-2.html
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360631 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360610 [details]
Patch

Attachment 360610 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10957036

New failing tests:
fast/block/float/016.html
fast/overflow/007.html
fast/text/hyphenate-avoid-orphaned-word.html
fast/text/jp-sans-serif.html
fast/css-generated-content/014.html
fast/text/international/lang-sensitive-fonts-xml.xhtml
fast/text/international/generic-font-family-language-traditional.html
fast/text/international/locale-sensitive-fonts.html
fast/text/international/lang-sensitive-fonts.html
tables/mozilla/bugs/bug139524-2.html
tables/mozilla/bugs/bug10296-1.html
svg/custom/object-sizing.xhtml
fast/css/line-height-font-order.html
fast/text/international/font-fallback-to-common-script.html
tables/mozilla/bugs/bug10269-2.html
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360633 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Myles C. Maxfield]

Created attachment 360654 [details]
Patch

[EWS Watchlist]

Comment on attachment 360654 [details]
Patch

Attachment 360654 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10960833

New failing tests:
fast/text/han-generic-font-families.html
imported/w3c/web-platform-tests/css/css-text/word-break/word-break-normal-lo-000.html
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360664 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360654 [details]
Patch

Attachment 360654 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10961188

New failing tests:
fast/text/han-generic-font-families.html
imported/w3c/web-platform-tests/css/css-text/word-break/word-break-normal-lo-000.html
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360665 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360654 [details]
Patch

Attachment 360654 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10961183

New failing tests:
fast/text/han-generic-font-families.html
imported/w3c/web-platform-tests/css/css-text/word-break/word-break-normal-lo-000.html
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360671 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360654 [details]
Patch

Attachment 360654 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10961916

New failing tests:
fast/block/float/016.html
fast/overflow/007.html
fast/text/hyphenate-avoid-orphaned-word.html
fast/css-generated-content/014.html
tables/mozilla/bugs/bug139524-2.html
tables/mozilla/bugs/bug10296-1.html
svg/custom/object-sizing.xhtml
fast/css/line-height-font-order.html
fast/text/jp-sans-serif.html
tables/mozilla/bugs/bug10269-2.html
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360679 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Myles C. Maxfield]

*** Bug 167907 has been marked as a duplicate of this bug. ***

[Myles C. Maxfield]

Created attachment 360887 [details]
Patch

[EWS Watchlist]

Comment on attachment 360887 [details]
Patch

Attachment 360887 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10998253

New failing tests:
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360901 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360887 [details]
Patch

Attachment 360887 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10998294

New failing tests:
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360905 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360887 [details]
Patch

Attachment 360887 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10998386

New failing tests:
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360909 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360887 [details]
Patch

Attachment 360887 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10998435

New failing tests:
fast/block/float/016.html

[EWS Watchlist]

Created attachment 360911 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Myles C. Maxfield]

Created attachment 360917 [details]
No exceptions

[EWS Watchlist]

Comment on attachment 360917 [details]
No exceptions

Attachment 360917 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11000874

New failing tests:
fast/text/ja-sans-serif.html

[EWS Watchlist]

Created attachment 360924 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360917 [details]
No exceptions

Attachment 360917 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11000970

New failing tests:
fast/text/ja-sans-serif.html

[EWS Watchlist]

Created attachment 360932 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360917 [details]
No exceptions

Attachment 360917 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11001124

New failing tests:
fast/text/ja-sans-serif.html

[EWS Watchlist]

Created attachment 360935 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Myles C. Maxfield]

Created attachment 360968 [details]
No exceptions

[Myles C. Maxfield]

Created attachment 360982 [details]
Patch

[EWS Watchlist]

Comment on attachment 360982 [details]
Patch

Attachment 360982 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11010047

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 360985 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360982 [details]
Patch

Attachment 360982 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11010106

New failing tests:
fast/block/float/016.html
fast/overflow/007.html
fast/text/hyphenate-avoid-orphaned-word.html
fast/css-generated-content/014.html
tables/mozilla/bugs/bug139524-2.html
tables/mozilla/bugs/bug10296-1.html
svg/custom/object-sizing.xhtml
fast/css/line-height-font-order.html
tables/mozilla/bugs/bug10269-2.html
tables/mozilla_expected_failures/bugs/bug2479-5.html

[EWS Watchlist]

Created attachment 360987 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Myles C. Maxfield]

Created attachment 361078 [details]
Patch

[EWS Watchlist]

Attachment 361078 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:217:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:222:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiang Jiang]

Comment on attachment 361078 [details]
Patch

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

> Source/WebCore/page/cocoa/SettingsBaseCocoa.mm:53
>      setStandardFontFamily("Songti TC", USCRIPT_TRADITIONAL_HAN);

Do we still need this? Can it be from CTFontDescriptorCreateWithCSSFamily()?

> Source/WebCore/page/cocoa/SettingsBaseCocoa.mm:54
>      setStandardFontFamily("Songti SC", USCRIPT_SIMPLIFIED_HAN);

Do we still need this? Can it be from CTFontDescriptorCreateWithCSSFamily()?

> Source/WebCore/page/cocoa/SettingsBaseCocoa.mm:55
>      setStandardFontFamily("Hiragino Mincho ProN", USCRIPT_KATAKANA_OR_HIRAGANA);

Do we still need this? Can it be from CTFontDescriptorCreateWithCSSFamily()?

> Source/WebCore/page/cocoa/SettingsBaseCocoa.mm:56
>      setStandardFontFamily("AppleMyungjo", USCRIPT_HANGUL);

Do we still need this? Can it be from CTFontDescriptorCreateWithCSSFamily()?

[Myles C. Maxfield]

Comment on attachment 361078 [details]
Patch

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

>> Source/WebCore/page/cocoa/SettingsBaseCocoa.mm:53
>>      setStandardFontFamily("Songti TC", USCRIPT_TRADITIONAL_HAN);
> 
> Do we still need this? Can it be from CTFontDescriptorCreateWithCSSFamily()?

CTFontDescriptorCreateWithCSSFamily() doesn't have a "standard family" concept.

[Myles C. Maxfield]

Comment on attachment 361078 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:217
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500

This is supposed to be in platform.h

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:222
> +#elif PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED < 130000

Ditto

[Jiang Jiang]

(In reply to Myles C. Maxfield from comment #78)
> Comment on attachment 361078 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361078&action=review
> 
> >> Source/WebCore/page/cocoa/SettingsBaseCocoa.mm:53
> >>      setStandardFontFamily("Songti TC", USCRIPT_TRADITIONAL_HAN);
> > 
> > Do we still need this? Can it be from CTFontDescriptorCreateWithCSSFamily()?
> 
> CTFontDescriptorCreateWithCSSFamily() doesn't have a "standard family"
> concept.

But even if we do, it won't be helpful. The fact that Safari/WebKit picks serif fonts as standard family is purely artificial, the system won't pick that anyway.

[Shawn Roberts]

Test is a flaky failure and crash on Mac WK2

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fmediarecorder%2FMediaRecorder-AV-audio-video-dataavailable.html

Reproducible locally with:

run-webkit-tests d240977 http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html --iterations 500 -f --debug --exit-after-n-failures=1 --no-retry-failures

Crash log : Assertion failure
https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r240972%20(1465)/http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable-crash-log.txt

ASSERTION FAILED: dlsym(0x7fcdf7532a50, AVEncoderBitRateKey): symbol not found
constant
./platform/mediarecorder/cocoa/MediaRecorderPrivateWriterCocoa.mm(55) : NSString *initAVEncoderBitRateKey()
1   0x11b590e09 WTFCrash
2   0x10bca0aba initAVEncoderBitRateKey()
3   0x10bc9a887 WebCore::MediaRecorderPrivateWriter::setAudioInput()
4   0x10bc9a434 WebCore::MediaRecorderPrivateWriter::create(WebCore::MediaStreamTrackPrivate const*, WebCore::MediaStreamTrackPrivate const*)
5   0x10e51f33a WebCore::MediaRecorderPrivateAVFImpl::create(WebCore::MediaStreamPrivate const&)

[Myles C. Maxfield]

(In reply to Shawn Roberts from comment #81)
> Test is a flaky failure and crash on Mac WK2
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Fwpt%2Fmediarecorder%2FMediaRecorder-AV-
> audio-video-dataavailable.html

This patch has nothing to do with MediaRecorder.

[Brent Fulgham]

Comment on attachment 361078 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:-1723
> -

Doesn't seem like we need this change.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:1070
> +        if (RefPtr<CSSValue> parsedValue = consumeGenericFamily(range))

Could this be auto?

>> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:217
>> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500
> 
> This is supposed to be in platform.h

I don't understand this statement. What is? The PLATFORM macro?

[Alexey Proskuryakov]

> I don't understand this statement. What is? The PLATFORM macro?

As style checker said, "Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file."

> 2   0x10bca0aba initAVEncoderBitRateKey()

Bug 193724. Crazy stuff.

[Myles C. Maxfield]

Created attachment 361580 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 361580 [details]
Patch for committing

Clearing flags on attachment: 361580

Committed r241229: <https://trac.webkit.org/changeset/241229>

[Darin Adler]

Comment on attachment 361078 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:217
>>> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500
>> 
>> This is supposed to be in platform.h
> 
> I don't understand this statement. What is? The PLATFORM macro?

Alexey recently established a WebKit coding style rule that we don’t put specific iOS or macOS version checks into source files. Instead, all such checks are named HAVE() or ENABLE() definitions at the bottom of Platform.h.

We still have a couple hundred old call sites to clean up, but I think we’re pretty solid on this being the new strategy for many reasons. I don’t know if we have done a good enough job announcing and documenting this.

[Darin Adler]

Comment on attachment 361078 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:217
>>>> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101500
>>> 
>>> This is supposed to be in platform.h
>> 
>> I don't understand this statement. What is? The PLATFORM macro?
> 
> Alexey recently established a WebKit coding style rule that we don’t put specific iOS or macOS version checks into source files. Instead, all such checks are named HAVE() or ENABLE() definitions at the bottom of Platform.h.
> 
> We still have a couple hundred old call sites to clean up, but I think we’re pretty solid on this being the new strategy for many reasons. I don’t know if we have done a good enough job announcing and documenting this.

Oh, I see Alexey already replied about this, but I didn’t see it since it was a bug comment rather than a patch review comment response.

[Darin Adler]

Comment on attachment 361580 [details]
Patch for committing

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

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:171
> +        static int dummy;

Maybe char instead of int will save a tiny bit of memory?

[Darin Adler]

Comment on attachment 361580 [details]
Patch for committing

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

> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:185
> +    return m_serifFamilies.ensure(locale, [&] {
> +        auto descriptor = adoptCF(CTFontDescriptorCreateForCSSFamily(kCTFontCSSFamilySerif, locale.createCFString().get()));
> +        return adoptCF(static_cast<CFStringRef>(CTFontDescriptorCopyAttribute(descriptor.get(), kCTFontFamilyNameAttribute))).get();
> +    }).iterator->value;

Seems like we could write a helper so these 4 almost identical functions weren’t so repetitive.

[Truitt Savell]

Reverted r241229 for reason:

Revision broke internal builds for watchOS.

Committed r241268: <https://trac.webkit.org/changeset/241268>

[Myles C. Maxfield]

Created attachment 361737 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 361738 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 361738 [details]
Patch for committing

Clearing flags on attachment: 361738

Committed r241288: <https://trac.webkit.org/changeset/241288>

[Shawn Roberts]

(In reply to Myles C. Maxfield from comment #82)
> (In reply to Shawn Roberts from comment #81)
> > Test is a flaky failure and crash on Mac WK2
> > 
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=http%2Fwpt%2Fmediarecorder%2FMediaRecorder-AV-
> > audio-video-dataavailable.html
> 
> This patch has nothing to do with MediaRecorder.

Sorry, I filed on the wrong bug.

[Myles C. Maxfield]

*** Bug 152322 has been marked as a duplicate of this bug. ***