New York erroneously gets synthetic bold
Created attachment 373813 [details] WIP
Created attachment 373863 [details] WIP
Created attachment 373883 [details] Needs tests
One thing I shouldn't forget to do is to add code to catch the dot-prefixed name to make sure it explicitly doesn't work.
Attachment 373883 [details] did not pass style-queue: 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: 1 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.
<rdar://problem/51692592>
Comment on attachment 373883 [details] Needs tests View in context: https://bugs.webkit.org/attachment.cgi?id=373883&action=review > Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:60 > + auto result = adoptCF(CTFontCreateUIFontForLanguage(kCTFontUIFontSystem, parameters.size, locale)); Shouldn't need to start with this. > Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:85 > + // FIXME: Use applyWeightItalicsAndFallbackBehavior() once <rdar://problem/33046041> is fixed. Isn't this fixed long long time ago? > Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:141 > + CFTypeRef traitsValues[] = { weightNumber.get(), italicsNumber.get(), design ? static_cast<CFTypeRef>(design) : static_cast<CFTypeRef>(kCFBooleanTrue) }; We shouldn't use kCFBooleanTrue as the value in any case, either left design out, or use kCTFontUIFontDesignDefault.
Comment on attachment 373883 [details] Needs tests View in context: https://bugs.webkit.org/attachment.cgi?id=373883&action=review >> Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:85 >> + // FIXME: Use applyWeightItalicsAndFallbackBehavior() once <rdar://problem/33046041> is fixed. > > Isn't this fixed long long time ago? I'll remove this in a subsequent patch.
Created attachment 373941 [details] Patch
Running fast/text/design-system-ui-{11,14}.html causes #14 to fail
file:///Users/mmaxfield/src/WebKit/OpenSource/LayoutTests/fast/text/design-system-ui-11.html file:///Users/mmaxfield/src/WebKit/OpenSource/LayoutTests/fast/text/design-system-ui-14.html
window.internals.invalidateFontCache(); in the test appears to fix this.
Created attachment 373964 [details] Patch
Created attachment 373965 [details] Patch
Comment on attachment 373965 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373965&action=review > Source/WTF/ChangeLog:3 > + New York erroneously gets synthetic bold The state, or the city? Oh, you mean the font? Please say so. > Source/WebCore/ChangeLog:15 > + WKPreferencesSetShouldAllowDesignSystemUIFonts(true) Nope > Source/WebCore/ChangeLog:16 > + or -[WKPreferences _shouldAllowDesignSystemUIFonts] = YES. OK > Source/WTF/wtf/Platform.h:1598 > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) || (PLATFORM(WATCHOS) && __WATCH_OS_VERSION_MIN_REQUIRED >= 60000) || (PLATFORM(APPLETV) && __TV_OS_VERSION_MIN_REQUIRED >= 130000) I have no idea. > Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:57 > +#if HAVE(DESIGN_SYSTEM_UI_FONTS) Blank line above please. > Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:112 > + if (auto use = systemFontUse(cssFamily, shouldAllowDesignSystemUIFonts())) So hard to distinguish between 'use' noun, and 'use' verb. > Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:131 > + if (auto use = systemFontUse(cssFamily, shouldAllowDesignSystemUIFonts())) { Ditto. > Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:50 > +RetainPtr<CTFontRef> SystemFontDatabaseCoreText::createSystemUI(const CascadeListParameters& parameters, CFStringRef locale) createSystemUIFont? > Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:58 > +RetainPtr<CTFontRef> SystemFontDatabaseCoreText::createDesignSystemUI(ClientUse clientUse, const CascadeListParameters& parameters) createSystemUIFont? > Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:78 > +RetainPtr<CTFontRef> SystemFontDatabaseCoreText::createTextStyle(const CascadeListParameters& parameters) It makes a font, not a text style. > Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:132 > +RetainPtr<CTFontRef> SystemFontDatabaseCoreText::applyWeightItalicsAndFallbackBehavior(CTFontRef font, CGFloat weight, bool italic, float size, AllowUserInstalledFonts allowUserInstalledFonts, CFStringRef design) This should be called createFontByApplying... > Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:214 > + static NeverDestroyed<AtomString> systemUI = AtomString("system-ui-serif", AtomString::ConstructFromLiteral); systemUI -> systemUISerif > Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:219 > + static NeverDestroyed<AtomString> systemUI = AtomString("system-ui-monospaced", AtomString::ConstructFromLiteral); systemUI -> systemUIMonospaced > Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.cpp:224 > + static NeverDestroyed<AtomString> systemUI = AtomString("system-ui-rounded", AtomString::ConstructFromLiteral); systemUI -> systemUIRounded > Source/WebCore/platform/graphics/cocoa/SystemFontDatabaseCoreText.h:99 > + enum class ClientUse { : uint8_t if anyone stores it. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:111 > +#if HAVE(DESIGN_SYSTEM_UI_FONTS) > + if (fontDescription.shouldAllowDesignSystemUIFonts()) { > + Optional<SystemFontDatabaseCoreText::ClientUse> designSystemUI; > + if (equalLettersIgnoringASCIICase(family, "-apple-system-ui-serif")) > + designSystemUI = SystemFontDatabaseCoreText::ClientUse::ForSystemUISerif; > + else if (equalLettersIgnoringASCIICase(family, "-apple-system-ui-monospaced")) > + designSystemUI = SystemFontDatabaseCoreText::ClientUse::ForSystemUIMonospaced; > + else if (equalLettersIgnoringASCIICase(family, "-apple-system-ui-rounded")) > + designSystemUI = SystemFontDatabaseCoreText::ClientUse::ForSystemUIRounded; > + > + if (designSystemUI) { > + auto cascadeList = SystemFontDatabaseCoreText::singleton().cascadeList(fontDescription, family, *designSystemUI, allowUserInstalledFonts); > + if (!cascadeList.isEmpty()) > + return createFontForInstalledFonts(cascadeList[0].get(), size, allowUserInstalledFonts); > + } > + } > +#endif Need to share more of this code between macOS and iOS. > Source/WebCore/svg/graphics/SVGImage.cpp:474 > + m_page->settings().setShouldAllowDesignSystemUIFonts(false); It's false by default so no need to do anything. > Source/WebCore/testing/InternalSettings.cpp:88 > + , m_shouldAllowDesignSystemUIFonts(settings.shouldAllowDesignSystemUIFonts()) Remove > Source/WebCore/testing/InternalSettings.cpp:188 > + settings.setShouldAllowDesignSystemUIFonts(m_shouldAllowDesignSystemUIFonts); Remove > Source/WebCore/testing/InternalSettings.cpp:704 > +ExceptionOr<void> InternalSettings::setShouldAllowDesignSystemUIFonts(bool allow) > +{ > + if (!m_page) > + return Exception { InvalidAccessError }; > + settings().setShouldAllowDesignSystemUIFonts(allow); > + return { }; > +} Remove > Source/WebCore/testing/InternalSettings.h:89 > + ExceptionOr<void> setShouldAllowDesignSystemUIFonts(bool); Remove > Source/WebCore/testing/InternalSettings.h:189 > + bool m_shouldAllowDesignSystemUIFonts; Remove > Source/WebCore/testing/InternalSettings.idl:49 > + [MayThrowException] void setShouldAllowDesignSystemUIFonts(boolean ignore); This shouldn't be necessary. All Settings can be changed from tests already. > Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:576 > +WK_EXPORT void WKPreferencesSetShouldAllowDesignSystemUIFonts(WKPreferencesRef, bool flag); > +WK_EXPORT bool WKPreferencesGetShouldAllowDesignSystemUIFonts(WKPreferencesRef); I don't know why we're still adding to the C SPI
Committed r247377: <https://trac.webkit.org/changeset/247377>
<rdar://problem/52982969>
This seems to break the build. e.g.: failed r247377: https://build.webkit.org/builders/Apple%20iOS%2012%20Release%20%28Build%29/builds/7431 passed r247375: https://build.webkit.org/builders/Apple%20iOS%2012%20Release%20%28Build%29/builds/7430
FontCacheCoreText.cpp:1250:73: error: unused parameter 'family' [-Werror,-Wunused-parameter] FontCacheCoreText.cpp:1250:104: error: unused parameter 'fontDescription' [-Werror,-Wunused-parameter] FontCacheCoreText.cpp:1250:127: error: unused parameter 'size' [-Werror,-Wunused-parameter] FontCacheCoreText.cpp:1250:157: error: unused parameter 'allowUserInstalledFonts' [-Werror,-Wunused-parameter] In file included from /Volumes/Data/slave/ios-12-release/build/WebKitBuild/Release-iphoneos/DerivedSources/WebCore/unified-sources/UnifiedSource360.cpp:8: ./platform/graphics/cocoa/FontCacheCoreText.cpp:1250:73: error: unused parameter 'family' [-Werror,-Wunused-parameter] static RetainPtr<CTFontRef> fontWithFamilySpecialCase(const AtomString& family, const FontDescription& fontDescription, float size, AllowUserInstalledFonts allowUserInstalledFonts) ^ ./platform/graphics/cocoa/FontCacheCoreText.cpp:1250:104: error: unused parameter 'fontDescription' [-Werror,-Wunused-parameter] static RetainPtr<CTFontRef> fontWithFamilySpecialCase(const AtomString& family, const FontDescription& fontDescription, float size, AllowUserInstalledFonts allowUserInstalledFonts) ^ ./platform/graphics/cocoa/FontCacheCoreText.cpp:1250:127: error: unused parameter 'size' [-Werror,-Wunused-parameter] static RetainPtr<CTFontRef> fontWithFamilySpecialCase(const AtomString& family, const FontDescription& fontDescription, float size, AllowUserInstalledFonts allowUserInstalledFonts) ^ ./platform/graphics/cocoa/FontCacheCoreText.cpp:1250:157: error: unused parameter 'allowUserInstalledFonts' [-Werror,-Wunused-parameter] static RetainPtr<CTFontRef> fontWithFamilySpecialCase(const AtomString& family, const FontDescription& fontDescription, float size, AllowUserInstalledFonts allowUserInstalledFonts) ^ 4 errors generated.
Broke mac os build as well.
Broke macos build as well: https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20%28Build%29/builds/15784
Why was EWS green? Fixing.
The commit in r247377 seems substantially different from the last uploaded patch (373965). Might be good idea to atleast run the patch through EWS before landing (if not using commit-queue).
Build fix in https://trac.webkit.org/changeset/247379/webkit
Committed r247462: <https://trac.webkit.org/changeset/247462>
Re-opened since this is blocked by bug 199816
Committed r247465: <https://trac.webkit.org/changeset/247465>
Committed r247482: <https://trac.webkit.org/changeset/247482>