Bug 86565 – [chromium] run-webkit-tests can't work on ubuntu 12.04

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Li Yin]

In ubuntu 12.04, the run-webkit-tests in chromium can't work normally.
The error message:
You are missing /usr/share/fonts/truetype/thai/Garuda.ttf

But in ubuntu12.04, the package ttf-thai-tlwg install the Garuda.ttf into /usr/share/fonts/truetype/tlwg/Garuda.ttf

[Li Yin]

Created attachment 142136 [details]
Patch

[Kent Tamura]

Comment on attachment 142136 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestShellLinux.cpp:174
> +            if (!strcmp(fonts[i], "/usr/share/fonts/truetype/thai/Garuda.ttf")) {
> +                // ttf-thai-tlwg package uses different path in Ubuntu 12.04.
> +                static const char* tlwg = "/usr/share/fonts/truetype/tlwg/Garuda.ttf";
> +                if (access(tlwg, R_OK)) {
> +                    fprintf(stderr, "You are missing %s or %s. Try re-running build/install-build-deps.sh. Also see "
> +                                    "http://code.google.com/p/chromium/wiki/LayoutTestsLinux",
> +                                    fonts[i], tlwg);
> +                    exit(1);
> +                }
> +            } else {
> +                fprintf(stderr, "You are missing %s. Try re-running build/install-build-deps.sh. Also see "
>                              "http://code.google.com/p/chromium/wiki/LayoutTestsLinux",
>                              fonts[i]);
> -            exit(1);
> +                exit(1);
> +            }
>          }

I'd like:
 - removing Garuda.ttf from 'fonts' array
 - introduce new function to take two arguments for paths (tahi/Garuda.ttf and tlwg/Garuda.ttf). The function checks their existence and calls FcConfigAppFontAddFile().

The following check for lohit_pa.ttf might use the function too.

[Li Yin]

(In reply to comment #2)
> I'd like:
>  - removing Garuda.ttf from 'fonts' array
>  - introduce new function to take two arguments for paths (tahi/Garuda.ttf and tlwg/Garuda.ttf). The function checks their existence and calls FcConfigAppFontAddFile().
> 
> The following check for lohit_pa.ttf might use the function too.

Thanks for your review.

Yeah, I should add the "Garuda.tff" into optionalFonts.
And the "Garuda.tff" is not required for all the test, some tests don't need it, such as fast/worker etc. I think it should be optional.

Add the "Garuda.tff" into optionalFonts, optionFounts array should be the interface for handling the exception. It is very convenient to fix this issue. So I don't introduce a new function.

[Li Yin]

Created attachment 142175 [details]
Patch

[Kent Tamura]

Comment on attachment 142175 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestShellLinux.cpp:173
>      static const char* const optionalFonts[] = {
>          "/usr/share/fonts/truetype/ttf-indic-fonts-core/lohit_pa.ttf",
> +        "/usr/share/fonts/truetype/thai/Garuda.ttf",

I don't want to make Garuda.ttf optional.

[Li Yin]

(In reply to comment #5)
> (From update of attachment 142175 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142175&action=review
> 
> > Tools/DumpRenderTree/chromium/TestShellLinux.cpp:173
> >      static const char* const optionalFonts[] = {
> >          "/usr/share/fonts/truetype/ttf-indic-fonts-core/lohit_pa.ttf",
> > +        "/usr/share/fonts/truetype/thai/Garuda.ttf",
> 
> I don't want to make Garuda.ttf optional.

Okay, I have updated the patch already, please have a review again. 
Thanks.

[Li Yin]

Created attachment 142460 [details]
Patch

[WebKit Review Bot]

Attachment 142460 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/c..." exit_code: 1
Tools/DumpRenderTree/chromium/TestShellLinux.cpp:86:  Missing space before ( in if(  [whitespace/parens] [5]
Tools/DumpRenderTree/chromium/TestShellLinux.cpp:88:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Li Yin]

Created attachment 142461 [details]
Patch

[Kent Tamura]

Comment on attachment 142461 [details]
Patch

Looks good.

[WebKit Review Bot]

Comment on attachment 142461 [details]
Patch

Clearing flags on attachment: 142461

Committed r117446: <http://trac.webkit.org/changeset/117446>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.