Bug 81507 - [EFL] Use jhbuild downloaded fonts instead of hardcoded system font paths
Summary: [EFL] Use jhbuild downloaded fonts instead of hardcoded system font paths
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Dominik Röttsches (drott)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-19 06:49 PDT by Dominik Röttsches (drott)
Modified: 2012-03-21 13:24 PDT (History)
5 users (show)

See Also:


Attachments
Use downloaded fonts (2.20 KB, patch)
2012-03-19 06:54 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Diff of results before and after font change applied, r111388 (8.77 KB, text/plain)
2012-03-20 08:41 PDT, Dominik Röttsches (drott)
no flags Details
Use downloaded fonts, simplify FontManagement.cpp (4.03 KB, patch)
2012-03-21 01:55 PDT, Dominik Röttsches (drott)
mrobinson: review-
Details | Formatted Diff | Diff
Use downloaded fonts, simplify FontManagement.cpp, CString (3.99 KB, patch)
2012-03-21 12:48 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2012-03-19 06:49:21 PDT
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.
Comment 1 Dominik Röttsches (drott) 2012-03-19 06:54:23 PDT
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.
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-03-20 06:40:02 PDT
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.
Comment 3 Dominik Röttsches (drott) 2012-03-20 08:41:44 PDT
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.
Comment 4 Dominik Röttsches (drott) 2012-03-21 01:55:58 PDT
Created attachment 132995 [details]
Use downloaded fonts, simplify FontManagement.cpp
Comment 5 Gyuyoung Kim 2012-03-21 02:13:53 PDT
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.
Comment 6 Dominik Röttsches (drott) 2012-03-21 03:14:18 PDT
(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.
Comment 7 Raphael Kubo da Costa (:rakuco) 2012-03-21 06:36:03 PDT
Comment on attachment 132995 [details]
Use downloaded fonts, simplify FontManagement.cpp

Looks awesome, thanks.
Comment 8 Martin Robinson 2012-03-21 12:02:34 PDT
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.
Comment 9 Martin Robinson 2012-03-21 12:06:53 PDT
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.
Comment 10 Dominik Röttsches (drott) 2012-03-21 12:48:15 PDT
Created attachment 133099 [details]
Use downloaded fonts, simplify FontManagement.cpp, CString

Review comment about utf8 vs system locale addressed by using CString.
Comment 11 WebKit Review Bot 2012-03-21 13:24:25 PDT
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>
Comment 12 WebKit Review Bot 2012-03-21 13:24:31 PDT
All reviewed patches have been landed.  Closing bug.