Bug 147009 – Web Inspector: Add a function to CSSCompletions to get a list of supported system fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

In preparation for a Fonts panel/picker in the future, there should be an easy way for the Inspector frontend to retrieve the list of fonts on the system.  Seeing as this is already available in the backend, a protocol should be added to make this list visible.

[Radar WebKit Bug Importer]

<rdar://problem/21861152>

[Devin Rousso]

Created attachment 256928 [details]
Patch

[Michael Catanzaro]

Note, we can't distribute Arial or Times for legal and ethical reasons, so unfortunately those are not as universal as you had assumed. If we implement this in bug #147018 we could simply remove the "PASS:" and "FAIL:" prefixes from hasFontFamily().

[Joseph Pecoraro]

(In reply to comment #3)
> Note, we can't distribute Arial or Times for legal and ethical reasons, so
> unfortunately those are not as universal as you had assumed. If we implement
> this in bug #147018 we could simply remove the "PASS:" and "FAIL:" prefixes
> from hasFontFamily().

Interesting! What fonts can we expect to be on your port? Can we make any assumptions? It would be nice if we can assume a non-empty list with at least 1 or 2 common fonts, but maybe that is not something we can rely on.

[Joseph Pecoraro]

Comment on attachment 256928 [details]
Patch

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

r- for a couple issues, but overall this approach looks good to me. Would be nice to get a nod from Font experts like Myles.

> Source/JavaScriptCore/inspector/protocol/CSS.json:351
> +            "name": "getSupportedCSSFontFamilyNames",

I wonder if "CSS" makes sense here.

  (1) we are already in the CSS domain
  (2) this is really asking for the System Font Family Names, and won't provide Web Font names

A better name may be:

  getSupportedFontFamilyNames
  getSupportedSystemFontFamilyNames

> Source/WebCore/inspector/InspectorCSSAgent.cpp:718
> +    Vector<String> systemFontFamilies = FontCache::singleton().systemFontFamilies();
> +
> +    for (String familyName : systemFontFamilies)

Style: Drop the empty line and keep these highly related lines together.

> Source/WebCore/platform/graphics/FontCache.h:113
>      // This method is implemented by the platform.

Update this comment, "These methods are implemented by the platform."

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:104
> +    Vector<String> fontFamilies;
> +    return fontFamilies;

You should point to the bug for implementing this:

    // FIXME: <https://webkit.org/b/147018> Web Inspector: [Freetype] Allow inspector to retrieve a list of system fonts

> Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:468
> +    CFArrayRef availableFontFamilies = CTFontManagerCopyAvailableFontFamilyNames();

We should use RetainPtr to help automate Retain/Release for us. To do that:

    RetainPtr<CFArrayRef> availableFontFamiles = adoptCF(CTFontManagerCopyAvailableFontFamilyNames());

Then you don't need the release at the bottom of the method.

Style: Same thing about the empty lines here. The top and bottom sure, but in the middle none of the newlines seem significant.

> Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:470
> +    for (CFIndex i = 0; i < CFArrayGetCount(availableFontFamilies); ++i) {

Call me crazy, but most of our code would extract the count out above the loop.

    CFIndex count = CFArrayGetCount(availableFontFamilies);
    for (CFIndex i = 0; i < count; ++i) {

> Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:473
> +        if (CFGetTypeID(CFStringRef) != CFStringGetTypeID()) {

This looks wrong. It should be CFGetTypeID(fontName). r- for this.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:534
> +

Same style nit here.

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:314
> +    Vector<String> fontFamilies;

Same FIXME comment. The bug for this one:

    // FIXME: <https://webkit.org/b/147017> Web Inspector: [Win] Allow inspector to retrieve a list of system fonts

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:171
> +            CSSAgent.getSupportedCSSFontFamilyNames(fontFamilyNamesCallback);

For iOS 8 backends, CSSAgent.getSupportedCSSFontFamilyNames is not available. You need to check if it is available:

    if (CSSAgent.getSupportedCSSFontFamilyNames)
        CSSAgent.getSupportedCSSFontFamilyNames(fontFamilyNamesCallback);

In which case, you might want to initialize WebInspector.CSSCompletions.cssFontFamilyNames to an empty list to behave reasonably on iOS 8 and earlier.

[Michael Catanzaro]

(In reply to comment #4) 
> Interesting! What fonts can we expect to be on your port? Can we make any
> assumptions? It would be nice if we can assume a non-empty list with at
> least 1 or 2 common fonts, but maybe that is not something we can rely on.

We install the following fonts to ensure they're present for layout tests: https://github.com/mrobinson/webkitgtk-test-fonts

I'm pretty sure none of those can be found on Windows or OS X. :( I'm not aware of any requirements we have on the fonts in that repo, except of course they should be legally redistributable (so none of the Microsoft fonts would be permitted, for example).

[Joseph Pecoraro]

> /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:468:40: error: call to unavailable function 'CTFontManagerCopyAvailableFontFamilyNames': not available on iOS
>     CFArrayRef availableFontFamilies = CTFontManagerCopyAvailableFontFamilyNames();

Sounds like a legit iOS build failure. Looks like this CoreText function is only available on Mac, not iOS based on the availability macro:

    CFArrayRef CTFontManagerCopyAvailableFontFamilyNames( void ) CT_AVAILABLE_MAC(10_6);

We will have to come up with another way to do this on iOS.

[Joseph Pecoraro]

> (In reply to comment #4) 
> > Interesting! What fonts can we expect to be on your port? Can we make any
> > assumptions? It would be nice if we can assume a non-empty list with at
> > least 1 or 2 common fonts, but maybe that is not something we can rely on.
> 
> We install the following fonts to ensure they're present for layout tests:
> https://github.com/mrobinson/webkitgtk-test-fonts
> 
> I'm pretty sure none of those can be found on Windows or OS X. :( I'm not
> aware of any requirements we have on the fonts in that repo, except of
> course they should be legally redistributable (so none of the Microsoft
> fonts would be permitted, for example).

We may just have to write this function expecting different cross platform results. As long as we write it in a way that doesn't need to continuously be updated that should be nice.

[Devin Rousso]

Created attachment 256958 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 256958 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCache.h:117
>      // Also implemented by the platform.

Please delete this comment

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:532
> +    Vector<String> fontFamilies;

I'm trying to remove all uses of NSFont* from WebKit. Please don't add more. Instead, use CoreText.

[Joseph Pecoraro]

Comment on attachment 256958 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:532
>> +    Vector<String> fontFamilies;
> 
> I'm trying to remove all uses of NSFont* from WebKit. Please don't add more. Instead, use CoreText.

Heh, the version you wrote for iOS here will probably work for Mac here then =P

[Myles C. Maxfield]

Comment on attachment 256958 [details]
Patch

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

>>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:532
>>> +    Vector<String> fontFamilies;
>> 
>> I'm trying to remove all uses of NSFont* from WebKit. Please don't add more. Instead, use CoreText.
> 
> Heh, the version you wrote for iOS here will probably work for Mac here then =P

Yes (though I don't see anything in the FontCacheIOS.mm implementation of FontCache::systemFontFamilies())

[Myles C. Maxfield]

Comment on attachment 256958 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:532
>>>> +    Vector<String> fontFamilies;
>>> 
>>> I'm trying to remove all uses of NSFont* from WebKit. Please don't add more. Instead, use CoreText.
>> 
>> Heh, the version you wrote for iOS here will probably work for Mac here then =P
> 
> Yes (though I don't see anything in the FontCacheIOS.mm implementation of FontCache::systemFontFamilies())

CTFontManagerCopyAvailableFontFamilyNames()

[Joseph Pecoraro]

Comment on attachment 256928 [details]
Patch

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

> Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:484
> +    Vector<String> fontFamilies;
> +    CFArrayRef availableFontFamilies = CTFontManagerCopyAvailableFontFamilyNames();
> +
> +    for (CFIndex i = 0; i < CFArrayGetCount(availableFontFamilies); ++i) {
> +        CFStringRef fontName = static_cast<CFStringRef>(CFArrayGetValueAtIndex(availableFontFamilies, i));
> +
> +        if (CFGetTypeID(CFStringRef) != CFStringGetTypeID()) {
> +            ASSERT_NOT_REACHED();
> +            continue;
> +        }
> +
> +        fontFamilies.append(fontName);
> +    }
> +
> +    CFRelease(availableFontFamilies);
> +
> +    return fontFamilies;
> +}

This one, with the original review comments.

[Devin Rousso]

Created attachment 257103 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 257103 [details]
Patch

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

Patch looks good, just a few minor things to cleanup.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:717
> +    for (String familyName : systemFontFamilies)

I think this can be:

    for (const auto& familyName : systemFontFamilies)

which is slightly simpler and more in line with WebKit style.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:533
> +    // CFStringRef period = CFSTR(".");

Leftover comment.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:542
> +        if (CFStringHasPrefix(fontName, CFSTR(".")))

Should have a comment here about why this is, because it may not be obvious to readers.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:76
> +        this.supportedSystemFontFamilyNames = [];

This is actually putting an empty list on an instance of CSSCompletions.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:168
> +            WebInspector.CSSCompletions.supportedSystemFontFamilyNames = fontFamilyNames;

And this is putting a list on the constructor.

We should start with empty lists on the constructor.

[Devin Rousso]

Created attachment 257211 [details]
Patch

[Myles C. Maxfield]

CoreText stuff looks fine to me.

[Joseph Pecoraro]

Comment on attachment 257211 [details]
Patch

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

r=me

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:541
> +        // We dont' want to make the hidden system fonts visible and since they

Typo: "dont'" => "don't"

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:166
> +            WebInspector.CSSKeywordCompletions.addPropertyCompletionValues("font-family", fontFamilyNames);

Nice! We may also want to add this to "font" which, being a shorthand for font-family, would want to include these completion values.

[Devin Rousso]

Created attachment 257356 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 257356 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 257356 [details]
Patch

Clearing flags on attachment: 257356

Committed r187249: <http://trac.webkit.org/changeset/187249>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brian Burg]

Comment on attachment 257356 [details]
Patch

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

> LayoutTests/inspector/css/get-system-fonts.html:6
> +// Testing that we can get the fonts on the system and that the fonts contain some universal fonts (arial, times, etc).

The test description should live in the <body> of the test.

> LayoutTests/inspector/css/get-system-fonts.html:9
> +        return (fontFamilies.includes(fontFamily) ? "PASS: Includes " : "FAIL: Missing ") + fontFamily;

These test assertions seem a bit unusual. I am trying to wean the inspector tests towards a more traditional testcase and assertion-based approach where possible, rather than relying completely on reference output.

Part of this is to ensure that the only thing that changes between a passing and failing test execution is the 'verdict'. The text following PASS or FAIL shouldn't change unless the test case has been aborted and no further assertions will run. For example, if there was a protocol error when getting font family names, the test should throw an exception and cause the test to abort.

InspectorTest.assert(fontFamilyNames.length >= 5, "At least 5 font family names should be returned.");
InspectorTest.assert(fontFamilyNames.includes("Arial"), "The font family names should always include Arial"); // not true for other platforms, btw.

> LayoutTests/inspector/css/get-system-fonts.html:23
> +    CSSAgent.getSupportedSystemFontFamilyNames(fontFamilyNamesCallback);

Since this test doesn't exercise any model classes, it would be better to use the simpler "protocol-test.js" harness instead and put it in inspector-protocol/css/.

That will also allow you to use Promises rather than callbacks and the TestSuite classes.