[webkit-reviews] review granted: [Bug 199653] New York erroneously gets synthetic bold : [Attachment 373965] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 199653: New York erroneously gets synthetic bold
https://bugs.webkit.org/show_bug.cgi?id=199653

Attachment 373965: Patch

https://bugs.webkit.org/attachment.cgi?id=373965&action=review




--- Comment #15 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 373965
  --> https://bugs.webkit.org/attachment.cgi?id=373965
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