Bug 81507 – [EFL] Use jhbuild downloaded fonts instead of hardcoded system font paths

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dominik Röttsches (drott)]

Now that we have jhbuild in place, I propose to use the fonts that we download using jhbuild, instead of system provided ones. 
This should further help to get stable test results across machines.

[Dominik Röttsches (drott)]

Created attachment 132569 [details]
Use downloaded fonts

Quick patch, feedback on how to determine the right path is welcome.
Also, if this change is accepted in general, I think we can get rid of the getFontDirectories() function.

[Raphael Kubo da Costa (:rakuco)]

Looks fine to me if it passes the EWS checks. We should pass the path to DOWNLOADED_FONTS_DIR in a more flexible way in 81475.

[Dominik Röttsches (drott)]

Created attachment 132834 [details]
Diff of results before and after font change applied, r111388

Fixes about 60 cases, makes three different ones fail, as it seems.
I think we should apply this. I will upload a new version of the patch that simplifies FontManagement.cpp.

[Dominik Röttsches (drott)]

Created attachment 132995 [details]
Use downloaded fonts, simplify FontManagement.cpp

[Gyuyoung Kim]

Comment on attachment 132995 [details]
Use downloaded fonts, simplify FontManagement.cpp

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

> Tools/DumpRenderTree/efl/FontManagement.cpp:56
> +static bool addFontDirectory(const String& fontDirectory, FcConfig* config)

In my opinion, we should not use WebKit utilities in order to avoid WebKit dependency. However, it seems this is ok for now.

[Dominik Röttsches (drott)]

(In reply to comment #5)
> (From update of attachment 132995 [details])
> In my opinion, we should not use WebKit utilities in order to avoid WebKit dependency. However, it seems this is ok for now.

Alright - thanks. Let's handle this concern separately in bug 80683 since I am not adding/changing any class usage dependency.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 132995 [details]
Use downloaded fonts, simplify FontManagement.cpp

Looks awesome, thanks.

[Martin Robinson]

Comment on attachment 132995 [details]
Use downloaded fonts, simplify FontManagement.cpp

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

Looks good, but I have a concern about using the system filesystem encoding.

> Tools/DumpRenderTree/efl/FontManagement.cpp:62
> +    if (!ecore_file_is_dir(path)
> +        || !FcConfigAppFontAddDir(config, reinterpret_cast<const FcChar8*>(path))) {

Do these functions accept paths in UTF-8 or in the system encoding? If it's in the system encoding you need to convert them from UTF-8 first.

[Martin Robinson]

Comment on attachment 132995 [details]
Use downloaded fonts, simplify FontManagement.cpp

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

> Tools/DumpRenderTree/efl/FontManagement.cpp:93
> -    if (!addFontDirectories(getFontDirectories(), config)) {
> +    if (!addFontDirectory(String(DOWNLOADED_FONTS_DIR), config)) {

You can avoid this problem entirely simply by making the argument to addFontDirectories a CString.

[Dominik Röttsches (drott)]

Created attachment 133099 [details]
Use downloaded fonts, simplify FontManagement.cpp, CString

Review comment about utf8 vs system locale addressed by using CString.

[WebKit Review Bot]

Comment on attachment 133099 [details]
Use downloaded fonts, simplify FontManagement.cpp, CString

Clearing flags on attachment: 133099

Committed r111598: <http://trac.webkit.org/changeset/111598>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.