[webkit-reviews] review granted: [Bug 180062] [Cocoa] Add SPI to disallow user-installed fonts : [Attachment 328650] 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 180062: [Cocoa] Add SPI to disallow user-installed fonts
https://bugs.webkit.org/show_bug.cgi?id=180062

Attachment 328650: Patch

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




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

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

> Source/WebCore/platform/graphics/FontCache.h:205
> +    Vector<FontSelectionCapabilities>
getFontSelectionCapabilitiesInFamily(const AtomicString&, bool
shouldDisallowUserInstalledFonts);

why not use the AllowUserInstalledFonts enum?

> Source/WebCore/platform/graphics/FontDescription.h:117
> +    bool mayRepresentUserInstalledFont() const { return
m_mayRepresentUserInstalledFont; }

AllowUserInstalledFonts?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:989
> +    AllowUserInstalledFonts allowUserInstalledFonts;

Why not m_

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1179
> +Vector<FontSelectionCapabilities>
FontCache::getFontSelectionCapabilitiesInFamily(const AtomicString& familyName,
bool shouldDisallowUserInstalledFonts)

AllowUserInstalledFonts?

> Source/WebCore/testing/InternalSettings.cpp:735
> +ExceptionOr<void> InternalSettings::setShouldDisallowUserInstalledFonts(bool
shouldDisallow)
> +{
> +    if (!m_page)
> +	   return Exception { InvalidAccessError };
> +    settings().setShouldDisallowUserInstalledFonts(shouldDisallow);
> +    return { };
> +}
> +

Not sure why you need this. Settings get exposed to tests already without any
code.

> Source/WebCore/testing/InternalSettings.idl:105
> +    [MayThrowException] void setShouldDisallowUserInstalledFonts(boolean
shouldDisallow);

Don't think you need this.

> Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:118
> + at property (nonatomic, setter=_setShouldDisallowUserInstalledFonts:) BOOL
_shouldDisallowUserInstalledFonts WK_API_AVAILABLE(macosx(WK_MAC_TBA),
ios(WK_IOS_TBA));

"shouldDisallow" or just "disallows"?

Is it a question of "allowing" them, or just using them? Maybe
"setUsesUserInstalledFonts"?

> Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm:139
> +    auto bufferSize = WKStringGetMaximumUTF8CStringSize(configuration);
> +    char characterBuffer[bufferSize];
> +    WKStringGetUTF8CString(configuration, characterBuffer, bufferSize);

Don't we have WKStringRef -> NSString helpers?

> Tools/WebKitTestRunner/InjectedBundle/cocoa/ActivateFontsCocoa.mm:153
> +    NSMutableArray *fontsToRemove = [NSMutableArray arrayWithCapacity:0];

[NSMutableArray array];