[webkit-reviews] review granted: [Bug 177041] [WPE][GTK] Merge ProcessLauncher[WPE, GTK] : [Attachment 321014] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Michael Catanzaro
<mcatanzaro at igalia.com>'s request for review:
Bug 177041: [WPE][GTK] Merge ProcessLauncher[WPE,GTK]
https://bugs.webkit.org/show_bug.cgi?id=177041

Attachment 321014: Patch

https://bugs.webkit.org/attachment.cgi?id=321014&action=review




--- Comment #2 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 321014
  --> https://bugs.webkit.org/attachment.cgi?id=321014
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321014&action=review

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:104
> +	   nargs = 5;

nargs++ here would be safer in case we add a new one

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:113
> +	   for (auto it = splitArgs.begin(); it != splitArgs.end(); it++)

I know this was existing code, but we could take advantage of this patch to
modernize it a bit.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:123
> +    for (auto it = prefixArgs.begin(); it != prefixArgs.end(); it++)

Ditto.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:132
> +    argv[i++] = const_cast<char*>(realPluginPath.data());

I think this need to be protected by a ENABLE(NETSCAPE_PLUGIN_API) ifdef.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:136
> +    if (!g_spawn_async(0, argv, 0, G_SPAWN_LEAVE_DESCRIPTORS_OPEN,
childSetupFunction, GINT_TO_POINTER(socketPair.server), &pid, &error.outPtr()))
{

0 -> nullptr

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:137
> +	   g_printerr("Unable to fork a new WebProcess: %s.\n",
error->message);

WTFLogAlways?

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:151
> +    RefPtr<ProcessLauncher> protectedThis(this);
> +    IPC::Connection::Identifier serverSocket = socketPair.server;
> +    RunLoop::main().dispatch([protectedThis, pid, serverSocket] {

RunLoop::main().dispatch([protectedThis = makeRef(*this), pid, serverSocket =
socketPair.server] {

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:152
> +	   protectedThis->didFinishLaunchingProcess(pid, serverSocket);

You can also capture this for convenience and then you don't need to capture
pid

didFinishLaunchingProcess(m_processIdentifier, serverSocket);

I know all this is just copied code, not yours, I don't like making
refactorings or other changes in behavior in this kind of patches, but coding
style and modernization is always welcome, I think :-)