Bug 89719 – [EFL][WK2] ProcessExecutablePath is required

• Component: WebKit EFL
• Status: RESOLVED FIXED

[KwangYong Choi]

ProcessExecutablePathEfl.cpp is required for EFL.

[KwangYong Choi]

Created attachment 148924 [details]
patch

[Gyuyoung Kim]

Comment on attachment 148924 [details]
patch

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

> Source/WebKit2/ChangeLog:8
> +        Added ProcessExecutablePathEfl.cpp for EFL.

Meaningless patch description. Please write more meaningful description.

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:23
> +#include <libgen.h>

This functions only can be used on linux platform. So, use #if OS(LINUX)

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:28
> +const char* gWebKitWebProcessName = "WebProcess";

I prefer to use "static const char*" for constant variable.

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:37
> +    ssize_t result = readlink("/proc/self/exe", readLinkBuffer, PATH_MAX);

ditto. By the way, personally, I think EFL port also need to implement utility functions like GTK port in future.

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:40
> +        executablePath = String("/usr/bin");

It is good to make constant variable for default path. See also, GTK port use LIBEXECDIR macro.

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:46
> +    String fullPath = executablePath + "/" + processName;

If possible, use makeString(), pathByAppendingComponent() or WebKit utilities functions.

[Gyuyoung Kim]

Comment on attachment 148924 [details]
patch

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

>> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:28
>> +const char* gWebKitWebProcessName = "WebProcess";
> 
> I prefer to use "static const char*" for constant variable.

One more thing, gXXX is gtk style. Just use webkitWebProcessName.

[Chris Dumez]

Comment on attachment 148924 [details]
patch

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

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:34
> +    char readLinkBuffer[PATH_MAX];

PATH_MAX is not guaranteed to be defined. It is better to use lstat() (see st_size member) to get the size and then use malloc to dynamically allocate the memory.

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:36
> +    memset(readLinkBuffer, 0, PATH_MAX);

It would be less expensive to put a '\0' at the end after the readlink().

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:42
> +        char* executablePathPtr = dirname(readLinkBuffer);

For clarity, I think it would be nice to use const char* here.

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:43
> +        executablePath = String(executablePathPtr);

You should probably use String::fromUTF8().

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 148924 [details]
patch

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

I don't really understand the need for this patch. The functions being implemented here are only used by the Qt and GTK+ WebKit2 ports, so assuming we're using them for the same purpose, the UIProcess/Launcher/efl/ProcessLauncherEfl.cpp side should be changed as well. And in this case, I don't understand why these low-level calls and Linux-only semantics are being used instead of ecore_exe_run(), both here and in ProcessLauncherEfl.cpp itself.

>>> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:28
>>> +const char* gWebKitWebProcessName = "WebProcess";
>> 
>> I prefer to use "static const char*" for constant variable.
> 
> One more thing, gXXX is gtk style. Just use webkitWebProcessName.

`static const char foo[]' is even better, since it is placed in the .rodata section. Ideally, this should be defined at configure-time in CMake and put into cmakeconfig.h.in.

[KwangYong Choi]

Created attachment 149253 [details]
patch

I modified the functions to get the path from cmake script.
And I also changed ProcessLauncher::launchProcess() method to use these functions.

The reason why I add this patch is to use scan plugin scheme that uses executablePathOfPluginProcess().
Scan plugin feature is already implemented on GTK/QT. It is also required on EFL.

[Ryuan Choi]

Comment on attachment 149253 [details]
patch

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

I want to fix this bug for applications to be able to execute WebProcess after installed, but the patch is wrong.

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:33
> +static String findWebKitProcess(const char* processName)
> +{
> +    return WebCore::pathByAppendingComponent(String(LIBEXECDIR), processName);
> +}

In this function, we should check not only LIBEXECDIR but also  current executable path to run MiniBrowser and other applications.

If not, MiniBrowser can not execute WebProcess which is in WebKitBuild/Release/bin.

[KwangYong Choi]

Created attachment 153717 [details]
Patch

I modified to search current path first. The process path is set to the current path if the process is exist on the current path. Otherwise, it is set to default installed path.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 4 files


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

[KwangYong Choi]

Created attachment 153718 [details]
Patch

Fixed include order and removed remaining test code.

[Raphael Kubo da Costa (:rakuco)]

Retitling per our conventions of adding '[WK2]' to WebKit2-related patches.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 153718 [details]
Patch

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

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:33
> +static const char webKitWebProcessName[] = "WebProcess";
> +static const char webKitPluginProcessName[] = "PluginProcess";

The binary names are set in CMake, so hardcoding them here generates duplication and they may become out of sync in the future. These values should be passed directly from the build system.

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:35
> +static String findWebKitProcess(const char* processName)

It probably makes sense to cache the return value so we do not need to perform the lookup every time this function is called.

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:48
> +    ssize_t result = readlink("/proc/self/exe", readLinkBuffer, PATH_MAX);
> +    if (result > 0) {
> +        char* executablePathPtr = dirname(readLinkBuffer);
> +        String executablePath = WebCore::pathByAppendingComponent(String(executablePathPtr), processName);
> +
> +        // check whether process exist on the current path
> +        struct stat fileStat;
> +        if (!stat(executablePath.utf8().data(), &fileStat))
> +            return executablePath;
> +    }

With portability in mind, this implementation still does not look good, as it is very Linux-specific. GTK's own implementation in WTF at least performs a distinction between Linux and other Unices and has different code paths for them. Ideally, you should put this implementation within a #if OS(LINUX) check, have an #elif for OS(UNIX) and add a more generic implementation there which first checks if /proc/curproc/file exists and use that if it does (similar to what GTK does), and use argv[0]'s path otherwise. An #else might just include a notImplemented() call for Windows and the rest.

[KwangYong Choi]

Created attachment 153746 [details]
Patch

1. Added cmake script for getting process names.

2. Added static string for each process executable path.

3. Added #if statement for UNIX/LINUX.

I tried to get argv from ecore_app_args_get(). But it did not work. So, I tried to add ecore_app_args_set() to MiniBrowser. It did not work, either. I think it's hard to get argv value from here. :(

[KwangYong Choi]

Created attachment 153749 [details]
Patch

Modified ChangeLog: Added [WK2]

[Chris Dumez]

Comment on attachment 153749 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:66
>              String cmd = makeString(m_launchOptions.processCmdPrefix, ' ', fullPath, ' ', socket);

This patch breaks build in debug mode. I think you should replace "fullPath" by "executablePath" on this line.

[Kenneth Rohde Christiansen]

Comment on attachment 153749 [details]
Patch

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

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:7
> +/*
> +    Copyright (C) 2012 Samsung Electronics
> +
> +    This library is free software; you can redistribute it and/or
> +    modify it under the terms of the GNU Library General Public
> +    License as published by the Free Software Foundation; either
> +    version 2 of the License, or (at your option) any later version.

Any reason to not use the default BSD license? It makes copying code around in WebKit and doing refactorings easier without having to worry about the license.

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:32
> +static String findWebKitProcess(const char* processName)

WebKitProcess is kind of confusing. I would just call it findProcessPath()

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:46
> +        // check whether process exist on the current path

Start with capital, end with punctuation mark. Someone needs to make the style check check for this

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:56
> +String executablePathOfWebProcess()

findWebProcessPath() ?

[KwangYong Choi]

Created attachment 153754 [details]
Patch

1. Fixed debug build break.

2. Applied BSD license.

3. Use findProcessPath() for finding web process and plugin process.

4. Modified comment string.

5. The name of executablePathOfWebProcess and executablePathOfPluginProcess is defined in ProcessExecutablePath.h So, I'm not changed these function names.

[Gyuyoung Kim]

Comment on attachment 153754 [details]
Patch

Attachment 153754 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13326028

[Ryuan Choi]

Comment on attachment 153754 [details]
Patch

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

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:67
> +    static String webKitWebProcessName;
> +
> +    if (webKitWebProcessName.isNull())
> +        webKitWebProcessName = findProcessPath(WEBPROCESSNAME);

Can we use `DEFINE_STATIC_LOCAL(String, webkitWebProcessName, findProcessPath(WEBPROCESSNAME));` for this ?

> Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:77
> +    static String webKitPluginProcessName;
> +
> +    if (webKitPluginProcessName.isNull())
> +        webKitPluginProcessName = findProcessPath(PLUGINPROCESSNAME);

Ditto.

[KwangYong Choi]

Created attachment 153762 [details]
Patch

1. Fixed build break. I'm sorry about this.

2. Applied DEFINE_STATIC_LOCAL macro.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/Shared/efl/ProcessExecutablePathEfl.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


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

[KwangYong Choi]

Created attachment 153764 [details]
Patch

Fixed webkit coding style.. :(

[WebKit Review Bot]

Comment on attachment 153764 [details]
Patch

Clearing flags on attachment: 153764

Committed r123331: <http://trac.webkit.org/changeset/123331>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.