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.
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.
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
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
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
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.
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
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
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
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
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
(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?
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
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()?
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.
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
(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.
(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.
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?
> 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.
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.
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.
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.
(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.
2018-07-16 19:36 PDT, Myles C. Maxfield
2018-07-17 15:47 PDT, Myles C. Maxfield
2018-07-18 13:37 PDT, EWS Watchlist
2018-07-18 16:11 PDT, EWS Watchlist
2018-07-31 22:03 PDT, Myles C. Maxfield
2018-07-31 22:29 PDT, Myles C. Maxfield
2018-08-01 00:23 PDT, EWS Watchlist
2019-01-29 17:54 PST, Myles C. Maxfield
2019-01-29 18:48 PST, EWS Watchlist
2019-01-29 19:01 PST, EWS Watchlist
2019-01-29 19:46 PST, EWS Watchlist
2019-01-29 20:08 PST, EWS Watchlist
2019-01-30 13:33 PST, Myles C. Maxfield
2019-01-30 14:41 PST, EWS Watchlist
2019-01-30 14:54 PST, EWS Watchlist
2019-01-30 15:23 PST, EWS Watchlist
2019-01-30 15:36 PST, EWS Watchlist
2019-01-30 17:09 PST, Myles C. Maxfield
2019-01-30 18:10 PST, EWS Watchlist
2019-01-30 18:32 PST, EWS Watchlist
2019-01-30 18:53 PST, EWS Watchlist
2019-01-30 19:47 PST, EWS Watchlist
2019-02-01 13:16 PST, Myles C. Maxfield
2019-02-01 14:25 PST, EWS Watchlist
2019-02-01 14:38 PST, EWS Watchlist
2019-02-01 15:09 PST, EWS Watchlist
2019-02-01 15:19 PST, EWS Watchlist
2019-02-01 16:03 PST, Myles C. Maxfield
2019-02-01 16:59 PST, EWS Watchlist
2019-02-01 17:44 PST, EWS Watchlist
2019-02-01 17:47 PST, EWS Watchlist
2019-02-02 00:58 PST, Myles C. Maxfield
2019-02-02 15:12 PST, Myles C. Maxfield
2019-02-02 16:34 PST, EWS Watchlist
2019-02-02 17:20 PST, EWS Watchlist
2019-02-04 11:39 PST, Myles C. Maxfield
2019-02-08 19:03 PST, Myles C. Maxfield
2019-02-11 17:17 PST, Myles C. Maxfield
2019-02-11 17:19 PST, Myles C. Maxfield