ProcessExecutablePathEfl.cpp is required for EFL.
Created attachment 148924 [details] patch
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.
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.
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().
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.
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.
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.
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.
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.
Created attachment 153718 [details] Patch Fixed include order and removed remaining test code.
Retitling per our conventions of adding '[WK2]' to WebKit2-related patches.
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.
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. :(
Created attachment 153749 [details] Patch Modified ChangeLog: Added [WK2]
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.
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() ?
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.
Comment on attachment 153754 [details] Patch Attachment 153754 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13326028
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.
Created attachment 153762 [details] Patch 1. Fixed build break. I'm sorry about this. 2. Applied DEFINE_STATIC_LOCAL macro.
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.
Created attachment 153764 [details] Patch Fixed webkit coding style.. :(
Comment on attachment 153764 [details] Patch Clearing flags on attachment: 153764 Committed r123331: <http://trac.webkit.org/changeset/123331>
All reviewed patches have been landed. Closing bug.