Bug 96281 – [EFL] Make DumpRenderTree/WebKitTestRunner smarter at finding the fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kenneth Rohde Christiansen]

Currently it doesn't respect WEBKITOUTPUTDIR and WEBKIT_TOP_LEVEL.

[Kenneth Rohde Christiansen]

Created attachment 163151 [details]
Patch

[Thiago Marcos P. Santos]

Comment on attachment 163151 [details]
Patch

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

> Tools/DumpRenderTree/efl/FontManagement.cpp:59
> +static CString joinPath(const char* base, const char* first, ...)

The "first" here is not needed.
It could be just joinPath(const char* base, ...)

> Tools/DumpRenderTree/efl/FontManagement.cpp:70
> +    do {
> +        result.append('/');
> +        result.append(current);
> +    } while ((current = va_arg(ap, const char*)));

By taking off the "first" argument, you can make this a while {}

> Tools/DumpRenderTree/efl/FontManagement.cpp:111
> +    static char readLinkBuffer[PATH_MAX];

EINA_PATH_MAX would move EFL-ish. It is smaller than PATH_MAX but I don't know how the eina/ecore functions behave with paths longer than that.

> Tools/DumpRenderTree/efl/FontManagement.cpp:112
> +    ssize_t result = readlink("/proc/self/exe", readLinkBuffer, PATH_MAX);

Ditto.

[Chris Dumez]

Comment on attachment 163151 [details]
Patch

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

> Tools/DumpRenderTree/efl/FontManagement.cpp:76
> +static void addFontDirectory(const CString& path, FcConfig* config)

Can't we use this instead?
http://freedesktop.org/software/fontconfig/fontconfig-devel/fcconfigappfontadddir.html

> Tools/DumpRenderTree/efl/FontManagement.cpp:78
> +    size_t pathLength = strlen(path.data());

Why use strlen() if we already have the size in the CString?

> Tools/DumpRenderTree/efl/FontManagement.cpp:94
> +    eina_iterator_free(it);

iirc correctly, eina_iterator_free() displays warnings if the iterator is NULL (happens if the path passed to eina_file_ls() does not exist for example). Maybe we should abort early if eina_file_ls() returns NULL?

> Tools/DumpRenderTree/efl/FontManagement.cpp:97
> +

extra line?

> Tools/DumpRenderTree/efl/FontManagement.cpp:136
> +    const char* parentPath = ecore_file_dir_get(getCurrentExecutablePath().data());

Why store the returned value in a const char* although the function returns a char* (which must be freed)? This is error-prone IMHO.

> Tools/DumpRenderTree/efl/FontManagement.cpp:137
> +    const char* realPath = ecore_file_realpath(joinPath(parentPath, "..", "..", 0).data());

ditto.

[Chris Dumez]

Note that this patch affects WebKitTestRunner in addition to DumpRenderTree so the bug title may be more accurate.

[Chris Dumez]

Comment on attachment 163151 [details]
Patch

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

>> Tools/DumpRenderTree/efl/FontManagement.cpp:136
>> +    const char* parentPath = ecore_file_dir_get(getCurrentExecutablePath().data());
> 
> Why store the returned value in a const char* although the function returns a char* (which must be freed)? This is error-prone IMHO.

What if getCurrentExecutablePath() returns a null CString?

> Tools/DumpRenderTree/efl/FontManagement.cpp:152
> +    CString webkitOutputDir = getOutputDir();

What if getOutputDir() returns a null CString?

[Kenneth Rohde Christiansen]

Thanks for the comments; this is what I was looking for. I will look at them tomorrow.

The patch is not 100% ready as such, but I added the r? flag to get some preliminary comments.

For instance I think I want to change the getOutputDir() to getBuildPath() etc.

> http://freedesktop.org/software/fontconfig/fontconfig-devel/fcconfigappfontadddir.html

Maybe, not opening. But I guess there is a reason why GTK+ are doing it this way, and only accepting those file types.

> > Tools/DumpRenderTree/efl/FontManagement.cpp:78
> > +    size_t pathLength = strlen(path.data());
> 
> Why use strlen() if we already have the size in the CString?

Well if it has that, then I should use it :-) Guess I use CString too infrequently.

> > Tools/DumpRenderTree/efl/FontManagement.cpp:94
> > +    eina_iterator_free(it);
> 
> iirc correctly, eina_iterator_free() displays warnings if the iterator is NULL (happens if the path passed to eina_file_ls() does not exist for example). Maybe we should abort early if eina_file_ls() returns NULL?

That is a good idea, I see that is done in FileSystemEfl.cpp as well.

> > Tools/DumpRenderTree/efl/FontManagement.cpp:136
> > +    const char* parentPath = ecore_file_dir_get(getCurrentExecutablePath().data());
> 
> Why store the returned value in a const char* although the function returns a char* (which must be freed)? This is error-prone IMHO.

Sorry I didn't notice it was returning char*. Will fix.

> What if getCurrentExecutablePath() returns a null CString?
> What if getOutputDir() returns a null CString?

I'll handle that.(In reply to comment #2)

> The "first" here is not needed.
> It could be just joinPath(const char* base, ...)

But in that case it is not must if a join :-) joinPath("hello"). Also this way I need to special casing for not adding a trailing /.

> EINA_PATH_MAX would move EFL-ish. It is smaller than PATH_MAX but I don't know how the eina/ecore functions behave with paths longer than that.

OK, didn't know that existed :-)

[Kenneth Rohde Christiansen]

Created attachment 163340 [details]
Patch

[Chris Dumez]

Comment on attachment 163340 [details]
Patch

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

> Tools/DumpRenderTree/efl/FontManagement.cpp:-61
> -        || !FcConfigAppFontAddDir(config, reinterpret_cast<const FcChar8*>(path))) {

Do we have a good reason to get rid of FcConfigAppFontAddDir() and do the listing manually? This makes the code more complex and font management has been working just fine using FcConfigAppFontAddDir() until now.

[Kenneth Rohde Christiansen]

Comment on attachment 163340 [details]
Patch

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

>> Tools/DumpRenderTree/efl/FontManagement.cpp:-61
>> -        || !FcConfigAppFontAddDir(config, reinterpret_cast<const FcChar8*>(path))) {
> 
> Do we have a good reason to get rid of FcConfigAppFontAddDir() and do the listing manually? This makes the code more complex and font management has been working just fine using FcConfigAppFontAddDir() until now.

If you insist, I can change it. This way it more of a white listing of files (we whitelist .otf and .tff right now). GTK is doing it this way but I couldnt get hold on anyone to explain if whether they had issues with *FontAddDir before. It might be slightly more complicated but not much really.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 163340 [details]
Patch

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

A few separate comments about the patch as a whole:

 o Trying to guess a build directory from the current executable might be too overzealous an approach, and the current code would only work on Linux.
 o We do not normally take the WEBKIT_TOP_LEVEL environment variable into account, so perhaps just trying to use WEBKITOUTPUTDIR should be just fine.
 o What if you fall back to the value of DOWNLOADED_FONTS_DIR as a last resort (if you don't, that definition can be removed from the build system).
 o Isn't it better to use ASCIILiteral wherever possible here?

> Tools/DumpRenderTree/efl/FontManagement.cpp:4
> + * Copyright (C) 2012 Intel Cooperation.

s/Cooperation/Corporation/?

[Kenneth Rohde Christiansen]

>  o Trying to guess a build directory from the current executable might be too overzealous an approach, and the current code would only work on Linux.

That is true, but it can easily be made to work at least on all POSIX platforms with little effort. Is the EFL port targeting that?

>  o We do not normally take the WEBKIT_TOP_LEVEL environment variable into account, so perhaps just trying to use WEBKITOUTPUTDIR should be just fine.

I saw that GTK are doing that, so I added it as well. I thought it might be set by the testing system.

>  o What if you fall back to the value of DOWNLOADED_FONTS_DIR as a last resort (if you don't, that definition can be removed from the build system).
>  o Isn't it better to use ASCIILiteral wherever possible here?

Only if we move totally away from CString and to String

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #11)
> >  o Trying to guess a build directory from the current executable might be too overzealous an approach, and the current code would only work on Linux.
> 
> That is true, but it can easily be made to work at least on all POSIX platforms with little effort. Is the EFL port targeting that?

Well, trying not to break things on !Linux as much as possible makes me happy, especially when I try to build the port at home on FreeBSD :-) More realistically speaking, while some effort could be made to make it work on all POSIX-compliant systems, I just don't see much value in this kind of heuristic here. As I see it, the whole problem we are trying to solve here is that WEBKITOUTPUTDIR should be respected just like it already is in other parts of the code.

[Thiago Marcos P. Santos]

Comment on attachment 163340 [details]
Patch

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

>> Tools/DumpRenderTree/efl/FontManagement.cpp:4
>> + * Copyright (C) 2012 Intel Cooperation.
> 
> s/Cooperation/Corporation/?

Copyright (C) 2012 Intel Corporation. All rights reserved.

> Tools/DumpRenderTree/efl/FontManagement.cpp:54
> +static CString buildPath(const char* base, const char* first, ...)
>  {
> -    Vector<String> fontFilePaths;
> +    va_list ap;
> +    StringBuilder result;
> +    result.append(base);
> +
> +    if (const char* current = first) {
> +        va_start(ap, first);
> +        do {
> +            result.append('/');
> +            result.append(current);
> +        } while ((current = va_arg(ap, const char*)));
> +        va_end(ap);
> +    }
> +
> +    return result.toString().utf8();
> +}

You could simplify this as something like:

static CString buildPath(const char* base, ...)
{
    StringBuilder result;
    result.append(base);

    va_list ap; 
    va_start(ap, base);

    const char* current;
    while ((current = va_arg(ap, const char*))) {
        result.append('/');
        result.append(current);
    }   

    va_end(ap);

    return result.toString().utf8();
}

[Kenneth Rohde Christiansen]

Created attachment 163369 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 163369 [details]
Patch

This looks fine to me. Yesterday I mentioned the build system side should be adjusted, but I take that back now -- the behavior in this patch is at least consistent with the way we determine the build dir in webkitdirs.pm, in that we first check if WEBKITOUTPUTDIR is set, and return $SOURCE_DIR/WebKitBuild otherwise.

What could help is changing the value of DOWNLOADED_FONTS_DIR from the source directory (ie. the directory we actually git clone'd) to the fonts' installation directory, so that we do not need to worry about updating the version number in the directory name.

[WebKit Review Bot]

Comment on attachment 163369 [details]
Patch

Clearing flags on attachment: 163369

Committed r128292: <http://trac.webkit.org/changeset/128292>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.