[webkit-reviews] review granted: [Bug 173043] [Cocoa] Expand system-ui to include every item in the Core Text cascade list : [Attachment 312354] 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 173043: [Cocoa] Expand system-ui to include every item in the Core Text
cascade list
https://bugs.webkit.org/show_bug.cgi?id=173043

Attachment 312354: Patch

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




--- Comment #18 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 312354
  --> https://bugs.webkit.org/attachment.cgi?id=312354
Patch

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

> Source/WebCore/platform/graphics/FontDescription.h:41
> +#include "FontFamilySpecificationCoreText.h"
> +#else
> +#include "FontFamilyPlatformSpecification.h"

The name mismatch here is a bit jarring.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:732
> +    CFNotificationCenterAddObserver(center, this,
&fontCacheRegisteredFontsChangedNotificationCallback, notificationName,
nullptr, CFNotificationSuspensionBehaviorDeliverImmediately);

Do we need to call CFNotificationCenterRemove{Every}Observer at some point?

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:85
> +    Vector<RetainPtr<CTFontDescriptorRef>>
systemFontCascadeList(CoreTextCascadeListParameters parameters)

const CoreTextCascadeListParameters&

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:105
> +    RetainPtr<CTFontRef> applyWeightAndItalics(CTFontRef font, CGFloat
weight, bool italic, float size)

Can this be const or static? I don't see it changing local state.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:121
> +    Vector<RetainPtr<CTFontDescriptorRef>> computeCascadeList(CTFontRef
font, CFStringRef locale)

Can this be const or static? I don't see it changing local state.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:182
> +    auto weight = description.weight();
> +    if (weight < FontSelectionValue(150))
> +	   result.weight = kCTFontWeightUltraLight;
> +    else if (weight < FontSelectionValue(250))
> +	   result.weight = kCTFontWeightThin;
> +    else if (weight < FontSelectionValue(350))
> +	   result.weight = kCTFontWeightLight;
> +    else if (weight < FontSelectionValue(450))
> +	   result.weight = kCTFontWeightRegular;
> +    else if (weight < FontSelectionValue(550))
> +	   result.weight = kCTFontWeightMedium;
> +    else if (weight < FontSelectionValue(650))
> +	   result.weight = kCTFontWeightSemibold;
> +    else if (weight < FontSelectionValue(750))
> +	   result.weight = kCTFontWeightBold;
> +    else if (weight < FontSelectionValue(850))
> +	   result.weight = kCTFontWeightHeavy;
> +    else
> +	   result.weight = kCTFontWeightBlack;

This feels like the reverse mapping of code I reviewed a short time ago. Would
be nice to put both in the the same place.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:214
> +	       index -= cascadeList.size();

What guarantees that index doesn't underflow?

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:218
> +	       --index;

Ditto.