[webkit-reviews] review denied: [Bug 48511] [GTK] Implement Process/Thread Launcher classes and WebProcessGtkMain for WebKit2 : [Attachment 77753] 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 77753: ProcessLauncher and ThreadLauncher implementation for WebKit2
GTK port
https://bugs.webkit.org/attachment.cgi?id=77753&action=review

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

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:60
> +    char currentExePath[128] = {0};
> +    ssize_t numOfCharsRead = readlink("/proc/self/exe", currentExePath,
127);
> +    if (numOfCharsRead == -1)
> +	   applicationPath = "/usr/local/bin/"
> +    else
> +	   applicationPath = currentExePath;

One major issue here is that this will only work on systems with procfs. I'm
not sure that's even true of OS X.  If possible we need to have some platform
independent way to get the binary path. Maybe it should just be some #ifdefs.
I'm not sure what the best solution is here. It's unfortunate that this never
made it into GLib.

Another issue is that I believe this code makes the assumption that the path is
in the latin1 character set. That obviously won't work when the filesystem uses
UTF-8. I think should should use GLib functions which can convert paths into
UTF-8 strings.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:61
> +    applicationPath = applicationPath.substring(0,
applicationPath.reverseFind("/")+1) + WEBKIT_WEB_PROCESS_NAME;

I think instead of searching through the string, it's better to use GLib
functions for munging paths, such as g_path_get_dirname. Of course, the path
will need to be a raw string, since it expects it to be in the native format.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:74
> +	   // FIXME:: What else can we do in this case?

One extra : here. :)