[Webkit-unassigned] [Bug 48511] [GTK] Implement Process/Thread Launcher classes and WebProcessGtkMain for WebKit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48511


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78237|review?                     |review-
               Flag|                            |




--- Comment #10 from Martin Robinson <mrobinson at webkit.org>  2011-01-07 10:26:45 PST ---
(From update of attachment 78237)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.