[webkit-reviews] review denied: [Bug 48511] [GTK] Implement Process/Thread Launcher classes and WebProcessGtkMain for WebKit2 : [Attachment 78237] ProcessLauncher and ThreadLauncher implementation for WebKit2 GTK port

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Ravi Phaneendra Kasibhatla
<ravi.kasibhatla at motorola.com>'s request for review:
Bug 48511: [GTK] Implement Process/Thread Launcher classes and
WebProcessGtkMain for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=48511

Attachment 78237: ProcessLauncher and ThreadLauncher implementation for WebKit2
GTK port
https://bugs.webkit.org/attachment.cgi?id=78237&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78237&action=review

Look good, but we need to be a bit careful about path encoding.

> WebCore/platform/gtk/FileSystemGtk.cpp:190
> +#if OS(UNIX)
> +#if OS(LINUX)

I think it makes more sense to use #if OS(LINUX) for the first block and then
do #elif OS(UNIX) for the second block.

> WebCore/platform/gtk/FileSystemGtk.cpp:192
> +    char pathFromProc[128] = {0};

This should probably be sized PATH_MAX, but 128 is definitely too small. :)

> WebCore/platform/gtk/FileSystemGtk.cpp:196
> +    return directoryName(String::fromUTF8(pathFromProc));

You really want to use filenameToString here, since Linux paths aren't
necessarily UTF-8, but since you're just going just use it with GLib later, I
recommend just returning a CString from this function.

> WebCore/platform/gtk/FileSystemGtk.cpp:207
> +    return directoryName(String::fromUTF8(currentExePath.get()));
> +#else
> +    return String();
> +#endif

Same comment here. Just return a CString. See below at the callsite for why.

> WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:45
> +    if (getenv("WEBKIT2_DEBUG_MODE"))
> +	   sleep(30);

I would prefer to use the same environment variable as Qt,
WEBKIT2_PAUSE_WEB_PROCESS_ON_LAUNCH and to also surround this with an #ifndef
NDEBUG.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:43
> +#define WEBKIT_WEB_PROCESS_NAME "WebKitWebProcess" 

This should either be:
1. a const char* gWebKitWebProcessName = "WebKitWebProcess";
2. Passed in by the build system.
3. Used as a constant below.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:51
> +    if (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) < 0) 
> +	   return;

Better print an error and ASSERT_NOT_REACHED here too.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:57
> +	   String binaryPath = applicationDirectoryPath() + String("/") +
String(WEBKIT_WEB_PROCESS_NAME);

You should use g_build_filename here, as and just work with raw char arrays.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:58
> +	   execl(binaryPath.utf8().data(), WEBKIT_WEB_PROCESS_NAME,
socket.utf8().data(), (char*)0);

If you end up sticking with execl, please just use NULL here instead of casting
0.