Bug 177041 – [WPE][GTK] Merge ProcessLauncher[WPE,GTK]

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Michael Catanzaro]

ProcessLauncherWPE.cpp and ProcessLauncherGTK.cpp contain largely duplicated code. Let's merge them together as ProcessLauncherGLib.cpp.

[Michael Catanzaro]

Created attachment 321014 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 321014 [details]
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 :-)

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #2)
> > 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.

Yes, I broke this by moving realPluginPath inside the ifdef. I'll do this:

#if ENABLE(NETSCAPE_PLUGIN_API)
    argv[i++] = const_cast<char*>(realPluginPath.data());
#else
    argv[i++] = nullptr;
#endif
    argv[i++] = nullptr;

This will avoid leaving uninitialized stack memory in that index. It's awkward. This messiness should all be replaced with a Vector, but that will be a follow-up patch.

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

This should be a fatal error, so I'll change it to g_error(). I'll also change the error message to say "child process" rather than "WebProcess" since that has been on my TODO list for at least two years now. Also, "g_printerr() should not be used from within libraries" according to its documentation.

[Michael Catanzaro]

Committed r222133: <http://trac.webkit.org/changeset/222133>

[Michael Catanzaro]

Committed r222134: <http://trac.webkit.org/changeset/222134>