I think it would be useful to have a way to pass an arbitrary prefix to the command line when launching the web process in order to debug webkit more easily. Similar to what the EFL or the (former) Qt port do. I wrote a patch that uses the WEB_PROCESS_CMD_PREFIX environment variable for that purpose. Example: $ WEB_PROCESS_CMD_PREFIX='/usr/bin/gdbserver localhost:8080' WebKitBuild/Debug/Programs/MiniBrowser and in a different terminal: $ gdb WebKitBuild/Debug/Programs/.libs/WebKitWebProcess (gdb) target remote localhost:8080
Created attachment 214946 [details] Patch
Comment on attachment 214946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214946&action=review Think the idea is a great one, would prefer the impl to be simplified a bit. > Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:104 > + if (!m_launchOptions.processCmdPrefix.isEmpty()) { > + Vector<String> splitted; > + m_launchOptions.processCmdPrefix.split(' ', splitted); > + for (auto it = splitted.begin(); it != splitted.end(); it++) > + prefixArgs.append(it->utf8()); Can we do away with this splitted intermediate vector? Are you doing this to make sure the CString is kept alive? I think it would be better to use g_strdup if that is the case - not a lot of overhead, way simpler code. > Source/WebKit2/UIProcess/gtk/WebProcessProxyGtk.cpp:29 > > +#include <glib.h> This should not have an empty line. > Source/WebKit2/UIProcess/gtk/WebProcessProxyGtk.cpp:37 > + if (webProcessCmdPrefix && *webProcessCmdPrefix) Why do you do this check on the dereferenced pointer?
Comment on attachment 214946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214946&action=review > Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:123 > if (!g_spawn_async(0, argv, 0, G_SPAWN_LEAVE_DESCRIPTORS_OPEN, childSetupFunction, GINT_TO_POINTER(sockets[1]), &pid, &error.outPtr())) { Maybe we could add G_SPAWN_SEARCH_PATH to the spawn flags as well, so that we would be able to pass something like "gdbserver localhost:8080" to the WEB_PROCESS_CMD_PREFIX variable and find gdbserver on the PATH?
(In reply to comment #2) > > Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:104 > > + if (!m_launchOptions.processCmdPrefix.isEmpty()) { > > + Vector<String> splitted; > > + m_launchOptions.processCmdPrefix.split(' ', splitted); > > + for (auto it = splitted.begin(); it != splitted.end(); it++) > > + prefixArgs.append(it->utf8()); > > Can we do away with this splitted intermediate vector? String::split() returns Strings, but we cannot get the raw data directly from them. Using only one vector<String> and something like argv[i++] = it->utf8().data(); won't work because the intermediate CStrings which hold the data are immediately destroyed. The problem of g_strdup() is that we have to free that memory manually afterwards, but the rest of the contents of the argv vector are freed automatically. We can also duplicate all the strings in the argv vector, but I was looking for a solution that doesn't add overhead to release builds. > > Source/WebKit2/UIProcess/gtk/WebProcessProxyGtk.cpp:37 > > + if (webProcessCmdPrefix && *webProcessCmdPrefix) > > Why do you do this check on the dereferenced pointer? It just checks that the string is not empty (this part is actually copied from the EFL code).
(In reply to comment #3) > Maybe we could add G_SPAWN_SEARCH_PATH to the spawn flags as well, > so that we would be able to pass something like "gdbserver > localhost:8080" to the WEB_PROCESS_CMD_PREFIX variable and find > gdbserver on the PATH? Dunno, I guess we're always using the full path to spawn processes so it should be safe to do it, but I'm fine leaving it as it is now.
Created attachment 215001 [details] Patch I fixed the style problem in WebProcessProxy.cpp but otherwise the code is essentially the same. I rewrote the ProcessLauncher::launchProcess() part so it's hopefully a bit clearer now. As I explained earlier I don't think it's easy to do without the intermediate array. It's anyway necessary to split the string first in order to know how much memory we need to allocate for argv. In this patch I also put the prefixArgs vector inside the #ifndef NDEBUG block, so for release builds the code is almost identical.
Created attachment 215287 [details] Patch The previous patch no longer applies, here's the rebased version.
Comment on attachment 215287 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215287&action=review This looks good to me. Thanks. > Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:105 > + if (!m_launchOptions.processCmdPrefix.isEmpty()) { You are preventing this from being empty in WebProcessProxy::platformGetLaunchOptions, so we could probably check whether it's null instead. > Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:106 > + Vector<String> splitted; I'm not an English expert at all, but I think splitted doesn't exists. We could call this something like splitArgs or splitPrefixArgs
(In reply to comment #8) > You are preventing this from being empty in > WebProcessProxy::platformGetLaunchOptions, so we could probably > check whether it's null instead. > I'm not an English expert at all, but I think splitted doesn't > exists. We could call this something like splitArgs or > splitPrefixArgs You're right in both cases, I'll fix these and commit. Thanks!
Committed r158105: <http://trac.webkit.org/changeset/158105>