Bug 147018 - Web Inspector: [Freetype] Allow inspector to retrieve a list of system fonts
Summary: Web Inspector: [Freetype] Allow inspector to retrieve a list of system fonts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-07-16 14:34 PDT by Devin Rousso
Modified: 2015-07-30 07:51 PDT (History)
13 users (show)

See Also:


Attachments
Patch (7.82 KB, patch)
2015-07-28 19:49 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (7.67 KB, patch)
2015-07-28 19:55 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (7.69 KB, patch)
2015-07-29 08:37 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (8.24 KB, patch)
2015-07-29 18:49 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2015-07-16 14:34:32 PDT
This will be really useful for a font picker and for autocompletion.  See https://bugs.webkit.org/show_bug.cgi?id=147009 for more details.
Comment 1 Radar WebKit Bug Importer 2015-07-16 14:34:55 PDT
<rdar://problem/21863018>
Comment 2 Michael Catanzaro 2015-07-16 15:39:30 PDT
I think we'll need to use FcFontList:

"""
FcFontSet * FcFontList(FcConfig *config, FcPattern *p, FcObjectSet *os);

Selects fonts matching p, creates patterns from those fonts containing only the objects in os and returns the set of unique such patterns. If config is NULL, the default configuration is checked to be up to date, and used.
"""

Maybe someone with more endurance for fontconfig could translate that to English....
Comment 3 Michael Catanzaro 2015-07-16 15:39:52 PDT
Forgot the link: http://www.freedesktop.org/software/fontconfig/fontconfig-devel/fcfontlist.html
Comment 4 Michael Catanzaro 2015-07-28 19:49:10 PDT
Created attachment 257724 [details]
Patch
Comment 5 Michael Catanzaro 2015-07-28 19:55:31 PDT
Created attachment 257725 [details]
Patch
Comment 6 Carlos Garcia Campos 2015-07-28 22:58:38 PDT
Comment on attachment 257725 [details]
Patch

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

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:105
> +static Vector<String> patternToFamilies(const FcPattern& pattern)
> +{
> +    char* patternChars = reinterpret_cast<char*>(FcPatternFormat(
> +        const_cast<FcPattern*>(&pattern),
> +        reinterpret_cast<const FcChar8*>("%{family}")));

I think it's more convenient to pass the pointer here, since it's what you need and you don't need more ugly casts. This is a private static function after all.
Comment 7 Michael Catanzaro 2015-07-29 07:42:43 PDT
(In reply to comment #6)
> I think it's more convenient to pass the pointer here, since it's what you
> need and you don't need more ugly casts. This is a private static function
> after all.

Yeah... I will just remove the const though, then no cast is needed.
Comment 8 Michael Catanzaro 2015-07-29 08:37:40 PDT
Created attachment 257748 [details]
Patch
Comment 9 Brian Burg 2015-07-29 13:12:43 PDT
Comment on attachment 257748 [details]
Patch

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

The LayoutTests changes look good to me, with a small suggestion.

> LayoutTests/inspector/css/get-system-fonts.html:6
> +// Testing that we can get the fonts on the system...

Please move the test description to inside <body>. You might want to improve the wording, i.e., "Tests that the Web Inspector can enumerate system fonts, and checks for the existence of common fonts."

> LayoutTests/inspector/css/get-system-fonts.html:16
>              InspectorTest.log("Has at least 5 fonts: " + (fontFamilyNames.length >= 5));

This should really be an assertion: InspectorTest.assert(cond, message). But you'd have to rebaseline the text for other platforms.
Comment 10 Michael Catanzaro 2015-07-29 17:45:27 PDT
(In reply to comment #9)
> > LayoutTests/inspector/css/get-system-fonts.html:6
> > +// Testing that we can get the fonts on the system...
> 
> Please move the test description to inside <body>. You might want to improve
> the wording, i.e., "Tests that the Web Inspector can enumerate system fonts,
> and checks for the existence of common fonts."

OK

> > LayoutTests/inspector/css/get-system-fonts.html:16
> >              InspectorTest.log("Has at least 5 fonts: " + (fontFamilyNames.length >= 5));
> 
> This should really be an assertion: InspectorTest.assert(cond, message). But
> you'd have to rebaseline the text for other platforms.

OK
Comment 11 Michael Catanzaro 2015-07-29 18:49:56 PDT
Created attachment 257798 [details]
Patch
Comment 12 Carlos Garcia Campos 2015-07-29 23:09:08 PDT
Comment on attachment 257798 [details]
Patch

Ok.
Comment 13 WebKit Commit Bot 2015-07-30 07:51:16 PDT
Comment on attachment 257798 [details]
Patch

Clearing flags on attachment: 257798

Committed r187583: <http://trac.webkit.org/changeset/187583>
Comment 14 WebKit Commit Bot 2015-07-30 07:51:21 PDT
All reviewed patches have been landed.  Closing bug.