Bug 81507

Summary:

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

Product:

WebKit

Reporter:

Dominik Röttsches (drott) <d-r>

Component:

Tools / Tests

Assignee:

Dominik Röttsches (drott) <d-r>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

gyuyoung.kim, mrobinson, rakuco, tonikitoo, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Linux

Attachments:

Description	Flags
Use downloaded fonts	none
Diff of results before and after font change applied, r111388	none
Use downloaded fonts, simplify FontManagement.cpp	mrobinson: review-
Use downloaded fonts, simplify FontManagement.cpp, CString	none

Dominik Röttsches (drott)

Reported 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.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Dominik Röttsches (drott)

Comment 1 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.

Raphael Kubo da Costa (:rakuco)

Comment 2 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.

Dominik Röttsches (drott)

Comment 3 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.

Dominik Röttsches (drott)

Comment 4 2012-03-21 01:55:58 PDT

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

Gyuyoung Kim

Comment 5 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.

Dominik Röttsches (drott)

Comment 6 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.

Raphael Kubo da Costa (:rakuco)

Comment 7 2012-03-21 06:36:03 PDT

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

Martin Robinson

Comment 8 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.

Martin Robinson

Comment 9 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.

Dominik Röttsches (drott)

Comment 10 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.

WebKit Review Bot

Comment 11 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>

WebKit Review Bot

Comment 12 2012-03-21 13:24:31 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.